Re: gtkdnd.c race condition?

[Owen Taylor <otaylor redhat com>]

Alexander Larsson <alla lysator liu se> writes:

> I've just implemented drag and drop on linux-fb, and I have a problem with
> the generic gtkdnd code. Specifically this code in gtk_drag_update:
> 
>   if (gdk_drag_motion (info->context, dest_window, protocol,
> 		       x_root, y_root, action,
> 		       possible_actions,
> 		       time))
>     {
>       if (info->last_event)
> 	gdk_event_free ((GdkEvent *)info->last_event);
> 
>       info->last_event = gdk_event_copy ((GdkEvent *)event);
>     }
> 
> I get an assert in gdk_event_copy due to the fact that info->last_event ==
> event.
> 
> Here is what i think happens:
> 1) User moves mouse, leading to a call to gtk_drag_update().
> 2) gdk_drag_motion() sets src_context->drag_state to
>    GDK_DRAG_STATUS_MOTION_WAIT and sends a GDK_DRAG_MOTION event.
> Before the destination gets to run gdk_drag_status() to set drag_status
> back to GDK_DRAG_STATUS_DRAG the user moves the mouse again.
> 3) gdk_drag_motion() is called again, but returns TRUE because it will not
> send anything until it gets gdk_drag_status(). Since it returns true, the
> event will be stored in info->last_event.
> 4) now we get the GDK_DRAG_STATUS event, which leads
> gtk_drag_source_handle_event() calls gtk_drag_update() with the
> info->last_event as parameter.

First, I'd say that code gtkdnd.c is, at least implicitely, quite
specific to the X drag-and-drop protocols and if it works for you
doing a local drag-and-drop protocol not via X, that is at to some
extent coincidence. :-)

There is no reason why your code for GdkFB needs to use the GDK
DND events at all if it isn't convenient, or the same gdkdnd.h
interface, since these aren't public interfaces. The only public
interface is that in gtkdnd.h.


Anyways, yes you are right about the race condition, although
it will almost never happen for X.

The basic problem is you are running into stuff being done
between the point where the context->drag_state is reset
to GDK_DRAG_STATUS_DRAG and the point where the accompanying
GDK_DRAG_STATUS event is received and gdk_drag_motion()
is called again.

In the X port, these two events are tightly bound together - one
occurs when the event is translated into a GdkEvent prior to
dispatching and the second occurs when the event is actually
dispatched. These events, in some circumnstance, be separated,
but almost never are, at least by any significant amount.

For the FB port, these events are quite separated since the
first occurs immediately when gdk_drag_status() is called.
(There is no event translation step for the FB port.)


Your patch is probably fundamentally OK - it certainly is somewhat
cleaner if this case is handled, but I don't think, even with the
framebuffer port, it should ever happen in practice.

The very fact that gdk_drag_motion() returns TRUE implies
that gdk_drag_motion() must have been called and returned
FALSE after the point that the state was reset to 
GDK_DRAG_STATUS_DRAG. So there is no point in trying to pass
in the position from context->last_event, because that 
event is _older_ then the one we passed to gdk_drag_motion().

The way we should be handling this situation is that every
time we call gdk_drag_motion() we should either clear ->last_event
or store the new event. The return value of gdk_drag_motion()
determines which we should do.

 if (gdk_drag_motion (...)) 
   {
      if (info->last_event != event) /* Paranoia, should not happen */
        {
          gdk_event_free ((GdkEvent *)info->last_event);
          info->last_event = gdk_event_copy ((GdkEvent *)event);
        }
   }
 else
   {
      if (info->last_event)
        {
          gdk_event_free ((GdkEvent *)info->last_event);
          info->last_event = NULL;
        }
   }

If you want to check a patch to this effect in, please go ahead.
This should fix the current mem leak we have in the code:

	    if (info->last_event)
	      {
		gtk_drag_update (info,
				 info->cur_x, info->cur_y,
				 info->last_event);
		info->last_event = NULL;
	      }

But it looks like there is also a mem leak if the drop is
aborted before info->last_event is used, so we need to make
sure info->last_event always gets freed one way or the other.

Regards,
                                        Owen