touches for touchLists are not refcounted correctly

RESOLVED FIXED in mozilla14

Status

()

Core
Event Handling
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(blocking-fennec1.0 +)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Assignee: nobody → wjohnston
Attachment #612034 - Flags: review?(bugs)

Comment 2

5 years ago
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
Attachment #612034 - Flags: review?(bugs) → review+
(Assignee)

Comment 3

5 years ago
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?
Attachment #612034 - Attachment is obsolete: true
Attachment #612073 - Flags: review?(bugs)

Comment 4

5 years ago
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)
Attachment #612073 - Flags: review?(bugs) → review+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/507c719614ec

Updated

5 years ago
blocking-fennec1.0: --- → ?
https://hg.mozilla.org/mozilla-central/rev/507c719614ec
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
blocking-fennec1.0: ? → +
Depends on: 964153
You need to log in before you can comment on or make changes to this bug.