After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 710227 - ClutterGestureAction memory corruption
ClutterGestureAction memory corruption
Status: RESOLVED FIXED
Product: clutter
Classification: Platform
Component: ClutterAction
1.16.x
Other Linux
: Normal normal
: ---
Assigned To: clutter-maint
clutter-maint
: 710229 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-10-15 23:40 UTC by Lionel Landwerlin
Modified: 2013-12-05 15:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gesture-action: fix memory corruption (1.12 KB, patch)
2013-10-15 23:41 UTC, Lionel Landwerlin
none Details | Review
gesture-action: fix memory corruption (1.45 KB, patch)
2013-12-05 11:15 UTC, Lionel Landwerlin
accepted-commit_now Details | Review
gesture-action: set default edge value to NONE to restore initial behavior (928 bytes, patch)
2013-12-05 11:15 UTC, Lionel Landwerlin
needs-work Details | Review
gesture: Clean up trigger edge accessors (4.85 KB, patch)
2013-12-05 11:54 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
Explicitly set the trigger edge in GestureAction subclasses (4.14 KB, patch)
2013-12-05 11:55 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review
gesture: Make threshold-trigger-edge public (10.68 KB, patch)
2013-12-05 14:08 UTC, Emmanuele Bassi (:ebassi)
committed Details | Review

Description Lionel Landwerlin 2013-10-15 23:40:58 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.
Comment 1 Lionel Landwerlin 2013-10-15 23:41:57 UTC
Created attachment 257394 [details] [review]
gesture-action: fix memory corruption
Comment 2 Lionel Landwerlin 2013-12-05 11:13:51 UTC
*** Bug 710229 has been marked as a duplicate of this bug. ***
Comment 3 Lionel Landwerlin 2013-12-05 11:15:09 UTC
Created attachment 263575 [details] [review]
gesture-action: fix memory corruption
Comment 4 Lionel Landwerlin 2013-12-05 11:15:37 UTC
Created attachment 263576 [details] [review]
gesture-action: set default edge value to NONE to restore initial behavior
Comment 5 Emmanuele Bassi (:ebassi) 2013-12-05 11:40:27 UTC
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.
Comment 6 Emmanuele Bassi (:ebassi) 2013-12-05 11:41:18 UTC
Review of attachment 263575 [details] [review]:

okay.
Comment 7 Emmanuele Bassi (:ebassi) 2013-12-05 11:54:10 UTC
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.
Comment 8 Lionel Landwerlin 2013-12-05 11:54:45 UTC
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.
Comment 9 Emmanuele Bassi (:ebassi) 2013-12-05 11:55:12 UTC
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.
Comment 10 Emmanuele Bassi (:ebassi) 2013-12-05 12:02:04 UTC
(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.
Comment 11 Emmanuele Bassi (:ebassi) 2013-12-05 14:08:25 UTC
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.
Comment 12 Lionel Landwerlin 2013-12-05 14:34:28 UTC
Review of attachment 263580 [details] [review]:

Looks good.
Comment 13 Lionel Landwerlin 2013-12-05 14:37:14 UTC
Review of attachment 263582 [details] [review]:

Looks good.
Comment 14 Lionel Landwerlin 2013-12-05 14:40:07 UTC
Review of attachment 263590 [details] [review]:

Looks good.
Comment 15 Emmanuele Bassi (:ebassi) 2013-12-05 15:23:55 UTC
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