GNOME Bugzilla – Bug 681814
ClutterDragAction causes crashes when drag actor is destroyed at drag-end time
Last modified: 2014-12-13 14:15:44 UTC
This seems to be a recent (a month or 2 ago ?) regression. Actually as the bug reads, I can't think of a better or more appropriate time to destroy the drag actor then at drag-end time ;-) I have this currently setup in a complex code base and would rather not setup a test case for this, I did briefly give an attempt at fixing it but some problems and uncertainties cropped up, let me explain what happens because it's very obvious while reading clutter-drag-action.c: o First, the "drag-end" signal is emitted normally o In the drag-end callback, the drag actor is discarded o Then the clutter-drag-action.c:on_drag_handle_destroy() callback is called, since the "in_drag" flag is only set to false at the end of emit_drag_end(), the flag is still true so emit_drag_end() is called again with a NULL event. o During the second round of emit_drag_end(), bad things happen: - after the clause 'if (priv->last_motion_device != NULL)' an event is expected however at this point it is NULL. - worse still, if the actual drag actor is the same as the actor which holds the drag action as one of it's actions (i.e., the most probable/natural case IMHO), then while the drag actor is discarded in "drag-end", the actor's actions are disposed, including the drag action emitting the signal. In this case the priv->stage pointer is NULL in the second round of emit_drag_end() (and if you read emit_drag_end() it's plain to see that priv->stage is expected to exist at this point). I've tried at least moving the 'priv->in_drag = FALSE' statement to *before* the actual emission/notification of the "drag-end" signal, however this did not help things. At this point I've inserted an ugly g_idle_add() of a hack into my code instead of investing further hours on fixing this.
Created attachment 221868 [details] [review] drag-action: Ensure that we can destroy the drag handle If the DragAction has a drag handle that gets destroyed inside the ::drag-end signal handler, the destruction sequence will trigger a callback we have in place to check if the handle is being destroyed mid-drag, e.g. from a ::drag-motion event. The callback on the drag handle destruction will check if we are still in the middle of a drag and emit the ::drag-end signal to allow cleaning up; the callback erroneously uses the drag handle as the argument for the emit_drag_end() function — instead of the actor to which the drag action has been attached. Also, by the time we emit the ::drag-end, we are not dragging the actor any more, so we shouldn't be emitted the ::drag-end signal twice. The fix is, thus, made of two parts: - reset the in_drag boolean before emitting the ::drag-end signal so that destroying the drag handle will not result in a double signal emission; - use the correct actor when calling emit_drag_end().
Attachment 221868 [details] pushed as 06ea2cf - drag-action: Ensure that we can destroy the drag handle
Unfortunately the crash remains. What is happening, is because there is only one drag actor, it is the same actor which owns the drag action, and it is destroyed in the drag-end callback: o drag-end is emitted o the drag actor is destroyed o the dispose of the drag actor causes an explicit dispose on all of it's actions, this causes the drag action emitting the signal to be disposed (however it should not be finalized afaik) o then in the next line from emit_drag_end(), after emitting the signal, the disposed drag action attempts to disconnect the capture_id, which is somehow valid at this point, and in my case it crashes while trying to disconnect to the stage, priv->stage at this point is 0xaaaaaaaa :-/
Thankfully I found a very easy way to reproduce this. It's a one liner so let me explain instead of attaching a patch a.) Open up clutter/examples/drag-action.c b.) In the on_drag_end() callback... before the line: t = clutter_actor_get_transition (actor, "disable"); insert the line clutter_actor_destroy (actor); What I did was just comment out that animated transition code in on_drag_end() and replace with clutter_actor_destroy() When doing a simple drag (a move drag, not a copy, I didnt test copy)... then I receive the same problem with the same stack trace as I do in my application.
Created attachment 223885 [details] [review] drag-action: Allow destroying the dragged actor inside ::drag-end Tested with examples/drag-action, by destroying the actor inside the ::drag-end signal handler.
Thanks for trying to fix this. Unfortunately... it still doesn't let me remove my destroy_in_idle approach, so whether it's really fixed is questionable :-/ To elaborate, what happens now is that the first drag finishes and I dont get a segfault at drag-end time... but as soon as I start to drag another actor in the same container... it crashes presumably while the first motion events are delivered.... however it crashes in the drop_action code now. Note that the setup I have runs basically like this: DragAreaActor { CustomLayoutManager (this one is like a flow-layout with special capabilities); CustomDropAction (this one talks to the layout manager, adding animated placeholder actors where the drop target should be) Child Actors [... about 20 of them ...] { CustomDragAction (only custom because I need to communicate some drag context and share that data, adds nothing really to ClutterDragAction) } } So when drag begins on one of the actors, I pop it out of the wrappy flow like parent and push it into a fixed layout actor (higher up in the hierarchy)... when drag ends I destroy that actor and create a new one back in the layout at the new position. I'm sure I dont need such a complex setup to reproduce the crash though, so long as there is a drop action involved it should reproduce... I'll try get something simple reproducing.. in the meantime here is the stack trace from the new segfault which occurs at the beginning of the second drag: Program received signal SIGSEGV, Segmentation fault. 0x00007ffff6f267d6 in clutter_actor_get_reactive (actor=0x18fb140) at ./clutter-actor.c:13569 13569 g_return_val_if_fail (CLUTTER_IS_ACTOR (actor), FALSE); (gdb) where
+ Trace 230841
Created attachment 242588 [details] [review] [PATCH] fix crash when destroying an actor during a drag-action with a drag_handle I also have crashes when an actor is destroyed during a drag-action. I'm not sure your crash is the same as mine: my actor is destroyed asynchronously before drag-end as a result of a received D-Bus message.
Review of attachment 242588 [details] [review]: Oops. Except for a trivial nitpick the patch seems totally good to me, thanks! ::: clutter/clutter-drag-action.c @@ +1144,3 @@ + priv->transformed_press_x = priv->press_x; + priv->transformed_press_y = priv->press_y; + clutter_actor_transform_stage_point (handle, priv->press_x, Maybe using priv->handle here would look more consistent since the test is on priv->handle.
Created attachment 242790 [details] [review] [PATCH] fix crash when destroying an actor during a drag-action with a drag_handle patch updated with the following changes: - mention the bug number in commit log - use priv->handle as asked by Emanuele in Commit #8
Created attachment 242802 [details] [review] [PATCH] ClutterDragAction: do not dereference a NULL priv->stage
Review of attachment 242802 [details] [review]: Looks fine. Emmanuele, are these ok to push?
Review of attachment 242790 [details] [review]: looks okay.
Review of attachment 242802 [details] [review]: ::: clutter/clutter-drag-action.c @@ +658,3 @@ * need to reset the state we are currently holding */ + if (priv->last_motion_device != NULL && priv->stage != NULL) I would move the if (priv->stage != NULL) check outside, since you're replicating it for every single condition. also, you need to nullify the pointers in the case where the stage is NULL but the pointers are not. in short: if (priv->stage != NULL) { if (priv->last_motion_device != NULL) ... if (priv->sequence != NULL) ... if (priv->capture_id != 0) ... } else { priv->last_motion_device = NULL; priv->sequence = NULL; priv->capture_id = 0; }
Comment on attachment 242790 [details] [review] [PATCH] fix crash when destroying an actor during a drag-action with a drag_handle Committed on git master: a0d9eaf15da5d2227bc95c6946fca5ed356fdfdb
Created attachment 242825 [details] [review] [PATCH] ClutterDragAction: do not dereference a NULL priv->stage patch updated thanks to ebassi's review.
Review of attachment 242825 [details] [review]: looks good.
Comment on attachment 242825 [details] [review] [PATCH] ClutterDragAction: do not dereference a NULL priv->stage Thanks! Committed on git master as 2be42c333ae621df085005e08b7c5c08c02dcbe5
Comment on attachment 223885 [details] [review] drag-action: Allow destroying the dragged actor inside ::drag-end This patch is already on git and is included in tag 1.11.16.