Closed Bug 741666 Opened 8 years ago Closed 8 years ago

touches for touchLists are not refcounted correctly

Categories

(Core :: User events and focus handling, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14
Tracking Status
blocking-fennec1.0 --- +

People

(Reporter: wesj, Assigned: wesj)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Patch (obsolete) — Splinter Review
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 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+
Attached patch Patch v2Splinter Review
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 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+
blocking-fennec1.0: --- → ?
https://hg.mozilla.org/mozilla-central/rev/507c719614ec
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
blocking-fennec1.0: ? → +
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.