Closed Bug 1258804 Opened 6 years ago Closed 6 years ago

FireFox crashes on a pointerevent capturing scenario

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

44 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: nzolghadr, Assigned: stone)

References

Details

(Whiteboard: [tw-dom] btpp-active)

Attachments

(3 files, 14 obsolete files)

7.71 KB, patch
stone
: review+
Details | Diff | Splinter Review
20.28 KB, patch
stone
: review+
Details | Diff | Splinter Review
3.88 KB, patch
stone
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.87 Safari/537.36

Steps to reproduce:

1. Enable pointer_event feature in about:config.
2. Goto http://output.jsbin.com/yiwose and left click on the green div



Actual results:

Firefox crashes or just gets into some infinite loop.


Expected results:

It should have continued working with no problem.

Looking at pointer event spec FireFox should have postponed sending gotpointercapture until the next pointer event happening. I believe FireFox tries to fires gotpointercapture for green/blue div forever and gets into some infinite loop.

Related links:
https://w3c.github.io/pointerevents/#process-pending-pointer-capture
https://github.com/w3c/pointerevents/issues/32
Component: Untriaged → Event Handling
("fixlater" just means "sometime soon-ish but not at the level of urgency of a major crasher or regression")
Whiteboard: [tw-dom] btpp-fixlater
Blocks: 1166347
Hi Stone, per our chat, you are investigating this issue so assign this bug to you. Please feel free to re-assign if I got something wrong.
Assignee: nobody → sshih
Whiteboard: [tw-dom] btpp-fixlater → [tw-dom] btpp-active
Like Navid's descriptions, this is caused by a while loop to check the pending pointer state .
Blocks: 1284185
Removing the loop will introduce failures of layout/base/tests/test_bug977003.html

These failures are all related to the timing to process pending capture states. According to [1][2][3], user agent should process pending pointer capture states when firing pointer events. 

[1]https://github.com/w3c/pointerevents/issues/32
[2]https://www.w3.org/TR/pointerevents/#process-pending-pointer-capture
[3]https://www.w3.org/TR/pointerevents/#releasing-pointer-capture

The failure in bug977003_inner_1.html is because
1. pointerdown trigger setPointerCapture
2. pointerup trigger processing pending capture state and send event gotpointercapture
3. releasePointerCapture when received gotpointercapture event
But no further pointer events are sent to trigger processing pending capture state. So I add a new pointermove event to trigger lostpointercapture

The failure in bug977003_inner_5.html is because
1. pointerdown trigger setPointerCapture
2. pointermove trigger processing pending capture state and send event gotpointercapture
3. gotpointercapture event trigger releasePointerCapture
4. pointerup trigger processing pending capture state and send event lostpointercapture
This test case expect there are no pointer events between gotpointercapture and lostpointercapture. It's not true since we process pending capture state when firing pointer events. So I refine this test case to accept pointermove between gotpointercapture and lostpointercapture

The failure in bug977003_inner_6.html is because it ping-pongs the pointer capture node and expects the capture state is updated immediately. That's the cause of this crash. So I changed the expectations in this test case.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Plan to follow the conclusions of PE hackathon, change the timing to process pending pointer capture as what https://github.com/w3c/pointerevents/issues/32#issuecomment-235751955 said.
Attachment #8776428 - Flags: feedback?(btseng)
Attachment #8776428 - Flags: feedback?(btseng)
Attachment #8776428 - Attachment is obsolete: true
Attachment #8776854 - Flags: feedback?(btseng)
Attachment #8776856 - Flags: feedback?(btseng)
Attachment #8776857 - Flags: feedback?(btseng)
Comment on attachment 8776856 [details] [diff] [review]
Part2: Attributes of gotpointercapture and lostpointercapture should be the same as the pointer event that causes them

Review of attachment 8776856 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsPresShell.cpp
@@ +6731,1 @@
>  {

MOZ_ASSERT(aPointerEvent);
Attachment #8776856 - Flags: feedback?(btseng) → feedback+
Blocks: 1292437
Attachment #8776854 - Attachment is obsolete: true
Attachment #8776854 - Flags: feedback?(btseng)
Attachment #8778773 - Flags: feedback?(btseng)
Attachment #8776857 - Attachment is obsolete: true
Attachment #8776857 - Flags: feedback?(btseng)
Attachment #8778776 - Flags: feedback?(btseng)
Comment on attachment 8778773 [details] [diff] [review]
Part1: Process pending pointer capture when firing pointer events

Review of attachment 8778773 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, thanks!
Attachment #8778773 - Flags: feedback?(btseng) → feedback+
Comment on attachment 8778775 [details] [diff] [review]
Part2: Attributes of gotpointercapture and lostpointercapture should be the same as the pointer event that causes them

Review of attachment 8778775 [details] [diff] [review]:
-----------------------------------------------------------------

f+ with the following issue addressed.

::: layout/base/nsPresShell.cpp
@@ +6734,5 @@
>    PointerCaptureInfo* pointerCaptureInfo = nullptr;
>    if (gPointerCaptureList->Get(aPointerId, &pointerCaptureInfo) && pointerCaptureInfo) {
>      // If pendingContent exist or anybody calls element.releasePointerCapture
>      // we should dispatch lostpointercapture event to overrideContent if it exist
>      if (pointerCaptureInfo->mPendingContent || pointerCaptureInfo->mReleaseContent) {

This is not your fault but it seems that we didn't check if the mPendingContent is the same to the mOverrideContent (which means the element.setPointerCapture() is called twice on the same element and the "lostpointercapture" event will be fired unexpectedly.
Please have a follow-up bug to address this, thanks!
Attachment #8778775 - Flags: feedback?(btseng) → feedback+
Comment on attachment 8778776 [details] [diff] [review]
Part3: Update related test case for PE spec changes

Review of attachment 8778776 [details] [diff] [review]:
-----------------------------------------------------------------

Please see my comments below, thanks!

::: dom/events/test/pointerevents/mochitest.ini
@@ +23,5 @@
>  [test_pointerevent_lostpointercapture_for_disconnected_node-manual.html]
>    support-files = pointerevent_lostpointercapture_for_disconnected_node-manual.html
>  [test_pointerevent_lostpointercapture_is_first-manual.html]
>    support-files = pointerevent_lostpointercapture_is_first-manual.html
> +  disabled = PE level 2 spec change its behavior. This tc will be refined soon

Can we have a bug for tracking this? Then, we can add the bug number here for tracking.

::: layout/base/tests/bug977003_inner_1.html
@@ +69,5 @@
>        target.addEventListener("lostpointercapture", LostPCHandler, false);
>        var rect = target.getBoundingClientRect();
>        synthesizePointer(target, rect.width/2, rect.height/2, {type: "pointerdown"});
>        synthesizePointer(target, rect.width/2, rect.height/2, {type: "pointerup"});
> +

nit: no need for this empty line.

::: layout/base/tests/bug977003_inner_6.html
@@ +34,5 @@
>        if("gotpointercapture" == event.type) {
>          logger("Send releasePointerCapture from listener");
>          listener.releasePointerCapture(event.pointerId);
>        } else if(event.type == "lostpointercapture") {
>          if(++async_counter < 100) {

The "async_counter" seems meaningless now but the order of lostpointercapture and pointmove should be identified in this test case instead.

@@ +74,5 @@
>        finishTest();
>      }
>      function finishTest() {
>        parent.is(test_target,    true,   "Part 6: pointerdown event should be received by target");
> +

nit: remove empty line.

@@ +79,5 @@
> +      // PE level 2 defines that the pending pointer capture is processed when firing next pointer events.
> +      // In this test case, pointer capture release is processed when firing pointermove
> +      parent.is(test_move,      true,   "Part 6: gotpointercapture should be triggered by pointermove");
> +      parent.is(test_listener,  false,  "Part 6: no other pointerevents should be fired before gotpointercapture");
> +

ditto
Attachment #8778776 - Flags: feedback?(btseng)
The modifications include
1) enable test_pointerevent_lostpointercapture_is_first-manual.html in the mochitest.ini
2) fix nit
3) remove async_counter and check if receive lostpointercapture more than once
> The "async_counter" seems meaningless now but the order of
> lostpointercapture and pointmove should be identified in this test case
> instead.
The order of lostpointercapture is tested in test_pointerevent_lostpointercapture_is_first-manual.html so I think we don't need to check it here.
Attachment #8778776 - Attachment is obsolete: true
Attachment #8779149 - Attachment is obsolete: true
Attachment #8779150 - Attachment is obsolete: true
Attachment #8779149 - Flags: feedback?(btseng)
Comment on attachment 8779149 [details] [diff] [review]
Part3: Update related test case for PE spec changes

Review of attachment 8779149 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thank you!

::: dom/events/test/pointerevents/test_pointerevent_releasepointercapture_onpointerup_mouse-manual.html
@@ +19,5 @@
>        }
>        function executeTest(int_win) {
>          sendPointerEvent(int_win, "btnCapture", "pointerdown", MouseEvent.MOZ_SOURCE_MOUSE);
>          sendPointerEvent(int_win, "btnCapture", "pointerup",   MouseEvent.MOZ_SOURCE_MOUSE);
> +

nit: remove this empty line.
Sorry for not capturing this in previous review. :(
Attachment #8779149 - Flags: feedback?(btseng) → feedback+
Comment on attachment 8779151 [details] [diff] [review]
Part3: Update related test case for PE spec changes

Review of attachment 8779151 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/test/pointerevents/test_pointerevent_releasepointercapture_onpointerup_mouse-manual.html
@@ +19,5 @@
>        }
>        function executeTest(int_win) {
>          sendPointerEvent(int_win, "btnCapture", "pointerdown", MouseEvent.MOZ_SOURCE_MOUSE);
>          sendPointerEvent(int_win, "btnCapture", "pointerup",   MouseEvent.MOZ_SOURCE_MOUSE);
> +

nit: remove this empty line.
Sorry for not capturing this in previous review. :(
Attachment #8779151 - Flags: feedback+
Comment on attachment 8778773 [details] [diff] [review]
Part1: Process pending pointer capture when firing pointer events

Followed pointer event spec to twist current implementation as
1) Stop using while loop to check pending pointer capture. The pending pointer capture should be triggered by the next pointer event
2) ReleasePointerCapturingContent should not trigger checking pending pointer capture
3) Implicit release pointer capture should trigger checking pending pointer capture
Attachment #8778773 - Flags: review?(bugs)
Comment on attachment 8778775 [details] [diff] [review]
Part2: Attributes of gotpointercapture and lostpointercapture should be the same as the pointer event that causes them

When triggering checking pending pointer capture, we need the attributes pointerId, pointerType and isPrimary to fire gotpointercapture or lostpointercapture. Original implementation caches them of pointerdown in gActivePointersIds but those attributes are no longer available when pointerup removes the pointer info from gActivePointersIds and triggers implicit release pointer capture. Cached pointerup's attributes in ReleasePointerCaptureCaller and used them when triggering lostpointercapture to solve this problem.
Attachment #8778775 - Flags: review?(bugs)
Updated web-platform-test pointerevent_lostpointercapture_is_first-manual.html:
Spec says UA runs the steps of processing pending pointer capture when firing a pointer event that is not gotpointercapture or lostpointercapture. This test case does not follow it. It expects the setPointerCapture is processed immediately and pointerup triggers lostpointercapture. The behavior follows spec should be the pointerup triggers gotpointercapture, then UA fire pointerup, then UA implicit release pointer capture and fire lostpointercapture.

Updated bug977003_inner_5.html and bug977003_inner_6.html
These tests setPointerCapture when pointerdown. Then the gotpointercapture should be triggered by next pointer event which is pointermove. Updated them to follow current behavior.
Attachment #8779151 - Attachment is obsolete: true
Attachment #8779223 - Attachment is obsolete: true
Attachment #8779224 - Flags: review?(bugs)
Attachment #8778773 - Flags: review?(bugs) → review+
Attachment #8778775 - Flags: review?(bugs) → review+
Attachment #8779224 - Flags: review?(bugs) → review+
Updated the patch summary
Attachment #8778773 - Attachment is obsolete: true
Attachment #8779582 - Flags: review+
Updated the patch summary
Attachment #8779224 - Attachment is obsolete: true
Attachment #8779584 - Flags: review+
has problems to apply :

adding 1258804 to series file
renamed 1258804 -> bug-1258804-fix-part1-v7.patch
applying bug-1258804-fix-part1-v7.patch
patching file layout/base/nsPresShell.cpp
Hunk #1 FAILED at 6676
1 out of 4 hunks FAILED -- saving rejects to file layout/base/nsPresShell.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug-1258804-fix-part1-v7.patch
Flags: needinfo?(sshih)
Keywords: checkin-needed
Fixed conflicts
Attachment #8779582 - Attachment is obsolete: true
Flags: needinfo?(sshih)
Attachment #8779940 - Flags: review+
Keywords: checkin-needed
Blocks: 1294335
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78208f8f73fb
Part 1: Process pending pointer capture when firing pointer events. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7fda047e483
Part 2: Attributes of gotpointercapture and lostpointercapture should be the same as the pointer event that causes them. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7ff51cfbdd9
Part 3: Update related test case for PE spec changes. r=smaug
Keywords: checkin-needed
Duplicate of this bug: 1290805
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.