Last Comment Bug 741666 - touches for touchLists are not refcounted correctly
: touches for touchLists are not refcounted correctly
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Wesley Johnston (:wesj)
:
Mentors:
Depends on: 964153
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-02 18:29 PDT by Wesley Johnston (:wesj)
Modified: 2014-01-26 22:23 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Patch (7.35 KB, patch)
2012-04-03 17:24 PDT, Wesley Johnston (:wesj)
bugs: review+
Details | Diff | Splinter Review
Patch v2 (9.13 KB, patch)
2012-04-03 19:42 PDT, Wesley Johnston (:wesj)
bugs: review+
Details | Diff | Splinter Review

Description Wesley Johnston (:wesj) 2012-04-02 18:29:09 PDT
If I do the following:

addEventListener("touchstart", function(aEvent) {
  // console.log(aEvent.touches.length); // If I include this line,
                                         // the second log will return 1, otherwise 0
  setTimeout(function() {
    console.log(aEvent.touches.length);
  }, 0);
}, false);

I find that all the touch lists are length 0 inside the setTimeout loop. We lazily build and cache the touchlist when you access it. If we don't create the cache outside the loop, the nsTArray of touches held by our nsTouchEvent is being destroyed, and we build a touch list of length 0.
Comment 1 Wesley Johnston (:wesj) 2012-04-03 17:24:39 PDT
Created attachment 612034 [details] [diff] [review]
Patch

We aren't duplicating these correctly.

Fix + test. Still building to run the test here, and will push to try as well.
Comment 2 Olli Pettay [:smaug] 2012-04-03 17:32:25 PDT
Comment on attachment 612034 [details] [diff] [review]
Patch


>     case NS_TOUCH_EVENT:
>     {
>-      newEvent = new nsTouchEvent(false, msg, nsnull);
>+      nsTouchEvent *oldTouchEvent = static_cast<nsTouchEvent*>(mEvent);
>+      newEvent = new nsTouchEvent(oldTouchEvent);
>       NS_ENSURE_TRUE(newEvent, NS_ERROR_OUT_OF_MEMORY);
>       isInputEvent = true;
>       break;
To be consistent with other events, make sure newEvent is not trusted, and add a test for that.
(your code makes it now trusted, if oldTouchEvent is trusted)

with that, r=me
Comment 3 Wesley Johnston (:wesj) 2012-04-03 19:42:41 PDT
Created attachment 612073 [details] [diff] [review]
Patch v2

This modifies our constructor so that it takes an isTruested parameter and passes in false like everyone else, but in (all?) cases, that gets overridden down here:


http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsDOMEvent.cpp#925


For now I left that in (but the test checks that isTrusted is always true now). Do we want to change that behavior?
Comment 4 Olli Pettay [:smaug] 2012-04-03 19:46:40 PDT
Comment on attachment 612073 [details] [diff] [review]
Patch v2


> 
>-      nsTouchEvent newEvent(touchEvent);
>+      nsTouchEvent newEvent(touchEvent->flags & NS_EVENT_FLAG_TRUSTED ?
>+                              true : false,
NS_IS_TRUSTED_EVENT(touchEvent)

Note You need to log in before you can comment on or make changes to this bug.