1258804 - FireFox crashes on a pointerevent capturing scenario

Status: RESOLVED FIXED

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

Description

[Navid Zolghadr]

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

Comment 1

[Andrew Overholt [:overholt]]

("fixlater" just means "sometime soon-ish but not at the level of urgency of a major crasher or regression")

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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.

Comment 3

[Ming-Chou Shih [:stone]]

Like Navid's descriptions, this is caused by a while loop to check the pending pointer state .

Comment 4

[Ming-Chou Shih [:stone]]

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.

Comment 5

[Ming-Chou Shih [:stone]]

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.

Comment 6

[Ming-Chou Shih [:stone]]

Attachment: Process pending pointer capture when firing pointer events

Comment 7

[Ming-Chou Shih [:stone]]

Attachment: Part1: Process pending pointer capture when firing pointer events

Comment 8

[Ming-Chou Shih [:stone]]

Attachment: Part2: Attributes of gotpointercapture and lostpointercapture should be the same as the pointer event that causes them

Comment 9

[Ming-Chou Shih [:stone]]

Attachment: Part3: Update related test case for PE spec changes

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

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);

Comment 11

[Ming-Chou Shih [:stone]]

Attachment: Part1: Process pending pointer capture when firing pointer events

Comment 12

[Ming-Chou Shih [:stone]]

Attachment: Part2: Attributes of gotpointercapture and lostpointercapture should be the same as the pointer event that causes them

Comment 13

[Ming-Chou Shih [:stone]]

Attachment: Part3: Update related test case for PE spec changes

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

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!

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

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!

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 17

[Ming-Chou Shih [:stone]]

Attachment: Part3: Update related test case for PE spec changes

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.

Comment 18

[Ming-Chou Shih [:stone]]

Attachment: Part3: Update related test case for PE spec changes

Comment 19

[Ming-Chou Shih [:stone]]

Attachment: Part3: Update related test case for PE spec changes

Comment 20

[Bevis Tseng[:bevis][:btseng](Exited)]

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. :(

Comment 21

[Bevis Tseng[:bevis][:btseng](Exited)]

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. :(

Comment 22

[Ming-Chou Shih [:stone]]

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

Comment 23

[Ming-Chou Shih [:stone]]

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.

Comment 24

[Ming-Chou Shih [:stone]]

Attachment: Part3: Update related test case for PE spec changes

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.

Comment 25

[Ming-Chou Shih [:stone]]

Attachment: Part3: Update related test case for PE spec changes

Comment 26

[Ming-Chou Shih [:stone]]

Attachment: Bug 1258804 Part1: Process pending pointer capture when firing pointer events. r=smaug

Updated the patch summary

Comment 27

[Ming-Chou Shih [:stone]]

Attachment: Bug 1258804 Part2: Attributes of gotpointercapture and lostpointercapture should be the same as the pointer event that causes them.

Updated the patch summary

Comment 28

[Ming-Chou Shih [:stone]]

Attachment: Bug 1258804 Part3: Update related test case for PE spec changes. r=smaug

Updated the patch summary

Comment 29

[Ming-Chou Shih [:stone]]

Attachment: Bug 1258804 Part3: Update related test case for PE spec changes. r=smaug

Comment 30

[Ming-Chou Shih [:stone]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ac6a2ecc4c27eebc9c6906bd0e8ac47c58c464b

Comment 31

[Carsten Book [:Tomcat]]

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

Comment 32

[Ming-Chou Shih [:stone]]

Attachment: Bug 1258804 Part1: Process pending pointer capture when firing pointer events. r=smaug

Fixed conflicts

Comment 33

[Pulsebot]

Pushed by [e-mail]:
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

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/78208f8f73fb
https://hg.mozilla.org/mozilla-central/rev/a7fda047e483
https://hg.mozilla.org/mozilla-central/rev/e7ff51cfbdd9