Re: gtkdnd.c race condition?

[Alexander Larsson <alla lysator liu se>]

On 16 Jan 2001, Owen Taylor wrote:

> 
> 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.

I must say that given my complete ignorance of how drag and drop works in
Gtk+ i was pretty amazed when my first try at converting gdkdnd-x11.c to
gdkdnd-fb.c worked at all. :)

But it might be better to conditionally compile gtkdnd.c to use a better
protocol for local dnd. I might look into this when there are no other
pressing issues.
 
> 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.
In practice, it happens quite often. Looking at my logs, i can see it
only ever happens after i drag from one window to
another. gdk_drag_motion() does gdk_drag_do_leave() and then resets
drag_state to GDK_DRAG_STATUS_DRAG, and then sends the GDK_DRAG_STATUS 
event. Then it sends the motion event, and immediately sends the status to
MOTION_WAIT.
 
> 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;
>         }
>    }

I will try it, and check it in if it works.
 
/ Alex