GNOME Bugzilla – Bug 710227
ClutterGestureAction memory corruption
Last modified: 2013-12-05 15:24:23 UTC
I started to get crashes coming from the ClutterGestureAction : ==11263== Invalid read of size 4 ==11263== at 0x678D099: clutter_event_free (clutter-event.c:1277) ==11263== by 0x6791296: gesture_update_motion_point (clutter-gesture-action.c:235) ==11263== by 0x67918B1: stage_captured_event_cb (clutter-gesture-action.c:395) ==11263== by 0x67FA104: _clutter_marshal_BOOLEAN__BOXED (clutter-marshal.c:85) ==11263== by 0xA98AC97: g_closure_invoke (gclosure.c:777) ==11263== by 0xA99D2E0: signal_emit_unlocked_R (gsignal.c:3656) ==11263== by 0xA9A4716: g_signal_emit_valist (gsignal.c:3340) ==11263== by 0xA9A5401: g_signal_emit (gsignal.c:3386) ==11263== by 0x6762007: clutter_actor_event (clutter-actor.c:13611) ==11263== by 0x6768E2D: _clutter_actor_handle_event (clutter-actor.c:20162) ==11263== by 0x67A03AE: _clutter_process_event (clutter-main.c:2266) ==11263== by 0x67B99EF: _clutter_stage_process_queued_events (clutter-stage.c:1070) ==11263== Address 0x25cd8880 is 0 bytes inside a block of size 136 free'd ==11263== at 0x4C2A68C: free (vg_replace_malloc.c:446) ==11263== by 0xABEA042: g_array_remove_range (garray.c:683) ==11263== by 0xABEA1DB: g_array_set_size (garray.c:567) ==11263== by 0x6791474: begin_gesture (clutter-gesture-action.c:340) ==11263== by 0x6791703: stage_captured_event_cb (clutter-gesture-action.c:399) ==11263== by 0x67FA104: _clutter_marshal_BOOLEAN__BOXED (clutter-marshal.c:85) ==11263== by 0xA98AC97: g_closure_invoke (gclosure.c:777) ==11263== by 0xA99D2E0: signal_emit_unlocked_R (gsignal.c:3656) ==11263== by 0xA9A4716: g_signal_emit_valist (gsignal.c:3340) ==11263== by 0xA9A5401: g_signal_emit (gsignal.c:3386) ==11263== by 0x6762007: clutter_actor_event (clutter-actor.c:13611) ==11263== by 0x6768E2D: _clutter_actor_handle_event (clutter-actor.c:20162) Seemingly introduced by abcf1d589f29ba7914d5648bb9814ad26c13cd83.
Created attachment 257394 [details] [review] gesture-action: fix memory corruption
*** Bug 710229 has been marked as a duplicate of this bug. ***
Created attachment 263575 [details] [review] gesture-action: fix memory corruption
Created attachment 263576 [details] [review] gesture-action: set default edge value to NONE to restore initial behavior
Review of attachment 263576 [details] [review]: instead of continuously switch back and forth, the correct thing to do is to set the edge trigger in each GestureAction subclass. changing the default behaviour is going to break something else, and then we're back to square one. ZoomAction and RotateAction should both use the NONE trigger edge. TapAction is already using the BEFORE trigger edge, so it should not be changed. PanAction and SwipeAction are a bit of a toss up; I can see them both taking into account the drag threshold, so they could use the AFTER trigger edge like they currently do. *then* we can change the default trigger edge value in GestureAction to be NONE - even if, to be fair, the intended default when I wrote it was to take into account the drag threshold unless explicitly overridden, which would make the AFTER trigger edge the correct default.
Review of attachment 263575 [details] [review]: okay.
Created attachment 263580 [details] [review] gesture: Clean up trigger edge accessors Use G_GNUC_INTERNAL instead of the leading underscore, as we may make the accessor functions public at some point. Also, clean up the documentation.
Review of attachment 263576 [details] [review]: I understand that the change might break something, somewhere. But the breaking has already happen, when abcf1d589f29ba7914d5648bb9814ad26c13cd83 was landed it changed the default behavior. And on top of that it introduces a API that is private so only GestureAction subclasses within Clutter can modify it.
Created attachment 263582 [details] [review] Explicitly set the trigger edge in GestureAction subclasses Each GestureAction subclass has its own trigger edge handling, so we want to be resilient in case of changes in the super-class.
(In reply to comment #8) > Review of attachment 263576 [details] [review]: > > I understand that the change might break something, somewhere. But the breaking > has already happen, when abcf1d589f29ba7914d5648bb9814ad26c13cd83 was landed it > changed the default behavior. that's incorrect, as you can clearly see from the commit you linked: the original default behaviour was to use the dnd-threshold setting and only emit the ::gesture-begin signal if the threshold was cleared, which is precisely the behaviour of the AFTER edge trigger. it had bugs, but that default was intentional. > And on top of that it introduces a API that is private so only GestureAction > subclasses within Clutter can modify it. the API was introduced late in the cycle, so I elected to keep it private at the time, given that nobody was implementing gesture actions outside of Clutter anyway. I can definitely accept a patch to make it public - in fact, attachment 263580 [details] [review] facilitates it. again, we can discuss changing the default to NONE before making the threshold-edge-trigger property public; but what I don't want is a change in defaults that breaks some action with respect to others, just because ZoomAction and RotateAction are the ones you care about.
Created attachment 263590 [details] [review] gesture: Make threshold-trigger-edge public When the threshold-trigger-edge property was introduced in GestureAction, it was late in the cycle and I elected to keep it private, given the fact that nobody was subclassing GestureAction outside of Clutter itself. These days, people are experimenting more with the GestureAction API, so they will need access to the various knobs that control the class default behaviour.
Review of attachment 263580 [details] [review]: Looks good.
Review of attachment 263582 [details] [review]: Looks good.
Review of attachment 263590 [details] [review]: Looks good.
Attachment 263580 [details] pushed as 154ca6e - gesture: Clean up trigger edge accessors Attachment 263582 [details] pushed as 8cb326d - Explicitly set the trigger edge in GestureAction subclasses Attachment 263590 [details] pushed as ed2fdf8 - gesture: Make threshold-trigger-edge public