Closed Bug 1375940 Opened 3 years ago Closed 3 years ago

It is possible to dispatch events to a dead/dying PresShell

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: nika, Assigned: nika)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

I'll attach a test case for this bug. When dispatching a double-click event, we don't seem to correctly verify that the PresShell didn't die while running the "click" event listeners, and proceed to dispatch the "dblclick" event despite the PresShell being dead.

This manifests in debug builds as an assertion firing. I'm not sure if this is exploitable, I didn't produce any crashes while testing, but I marked the bug as a security bug just in case.

This is the cause of the test failure blocking landing of bug 1343728.
I had to change some linux-only platform code to make it possible for me to correctly simulate this double-click on windows.

The original test failure which led to this was a Windows-only one, because whether or not 2 synthesized clicks close together look like a double-click is platform specific. 

I've made this test linux-only because of how platform specific triggering the assertion is, while the code actually being tested is very non-platform-specific.

MozReview-Commit-ID: BdyWjvG9q8m
Attachment #8880895 - Attachment is obsolete: true
MozReview-Commit-ID: 4hRjlgbLDA8
Attachment #8880916 - Flags: review?(bugs)
Any thoughts on a sec-rating and affected branches?
Flags: needinfo?(michael)
I think this affects all branches, and as far as I know it is not exploitable (or potentially even a sec bug), but I marked it as one to be safe. Olli would probably have a better idea of whether or not this is exploitable.
Flags: needinfo?(michael)
Component: DOM: Events → Event Handling
Attachment #8880915 - Flags: review?(bugs) → review+
Comment on attachment 8880916 [details] [diff] [review]
Part 2: Don't dispatch dblclick or auxclick if our PresShell died dispatching click

I think we should just fix the assertion. Allow document's without shell there.
NS_ENSURE_STATE(!aContent || aContent->GetComposedDoc() == mDocument); is the key thing there after all.
Attachment #8880916 - Flags: review?(bugs)
Group: core-security → dom-core-security
MozReview-Commit-ID: 4hRjlgbLDA8
Attachment #8881423 - Flags: review?(bugs)
Attachment #8880916 - Attachment is obsolete: true
Group: dom-core-security
Attachment #8881423 - Flags: review?(bugs) → review+
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e744a3029de2
Part 1: Add a test for destroying a window during a click event while double-clicking, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/991cb2487be8
Part 2: Fix an assertion in PresShell::HandleEventWithTarget, r=smaug
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/852f7fddf9a7
Part 3: Fix test check to match updated behaviour, a=bustage
Backed out for failing dom/events/test/test_dblclick_kill_window.html:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc1b771539f8439839d5329b74bc7d666397cba5
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1603f9734b63311f1e79dcfff8729686112827f
https://hg.mozilla.org/integration/mozilla-inbound/rev/a249359c7dddfabadc6ef9e9131f55b3f24975f0

A push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=167c7d1eaee18b2f18682669858a09272a9cbdde&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=110189705&repo=mozilla-inbound

[task 2017-06-27T23:37:10.890430Z] 23:37:10     INFO - TEST-START | dom/events/test/test_dblclick_kill_window.html
[task 2017-06-27T23:37:11.189602Z] 23:37:11     INFO - TEST-INFO | started process screentopng
[task 2017-06-27T23:37:11.816997Z] 23:37:11     INFO - TEST-INFO | screentopng: exit 0
[task 2017-06-27T23:37:11.820127Z] 23:37:11     INFO - Buffered messages logged at 23:37:11
[task 2017-06-27T23:37:11.820394Z] 23:37:11     INFO - SpawnTask.js | Entering test 
[task 2017-06-27T23:37:11.822093Z] 23:37:11     INFO - TEST-PASS | dom/events/test/test_dblclick_kill_window.html | Sent event 4 
[task 2017-06-27T23:37:11.825328Z] 23:37:11     INFO - TEST-PASS | dom/events/test/test_dblclick_kill_window.html | Sent event 7 
[task 2017-06-27T23:37:11.827221Z] 23:37:11     INFO - TEST-PASS | dom/events/test/test_dblclick_kill_window.html | Sent event 4 
[task 2017-06-27T23:37:11.834345Z] 23:37:11     INFO - TEST-PASS | dom/events/test/test_dblclick_kill_window.html | Sent event 5 
[task 2017-06-27T23:37:11.836386Z] 23:37:11     INFO - TEST-PASS | dom/events/test/test_dblclick_kill_window.html | Sent event 7 
[task 2017-06-27T23:37:11.838275Z] 23:37:11     INFO - TEST-PASS | dom/events/test/test_dblclick_kill_window.html | Waiting for clicks 
[task 2017-06-27T23:37:11.840227Z] 23:37:11     INFO - TEST-PASS | dom/events/test/test_dblclick_kill_window.html | Got a click 
[task 2017-06-27T23:37:11.842149Z] 23:37:11     INFO - Buffered messages finished
[task 2017-06-27T23:37:11.844044Z] 23:37:11     INFO - TEST-UNEXPECTED-FAIL | dom/events/test/test_dblclick_kill_window.html | Should have already seen 2 clicks 
[task 2017-06-27T23:37:11.845736Z] 23:37:11     INFO -     gotClicks</<@dom/events/test/test_dblclick_kill_window.html:64:9
[task 2017-06-27T23:37:11.847456Z] 23:37:11     INFO -     EventListener.handleEvent*gotClicks<@dom/events/test/test_dblclick_kill_window.html:63:7
[task 2017-06-27T23:37:11.849170Z] 23:37:11     INFO -     @dom/events/test/test_dblclick_kill_window.html:44:21
[task 2017-06-27T23:37:11.850783Z] 23:37:11     INFO -     async*add_task/</<@SimpleTest/SpawnTask.js:277:21
[task 2017-06-27T23:37:11.852530Z] 23:37:11     INFO -     onFulfilled@SimpleTest/SpawnTask.js:69:15
[task 2017-06-27T23:37:11.854177Z] 23:37:11     INFO -     co/<@SimpleTest/SpawnTask.js:58:5
[task 2017-06-27T23:37:11.855870Z] 23:37:11     INFO -     co@SimpleTest/SpawnTask.js:54:10
[task 2017-06-27T23:37:11.857533Z] 23:37:11     INFO -     setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:676:12
[task 2017-06-27T23:37:11.863333Z] 23:37:11     INFO -     add_task@SimpleTest/SpawnTask.js:269:7
[task 2017-06-27T23:37:11.866446Z] 23:37:11     INFO -     @dom/events/test/test_dblclick_kill_window.html:37:3
Flags: needinfo?(michael)
The test was a bit sketchy, as it was dispatching GTK native events asynchronously to the parent process. 

Smaug, would you mind if I re-landed this patch without the test?
Flags: needinfo?(michael) → needinfo?(bugs)
I think so.
Flags: needinfo?(bugs)
it seems the backout broke another test and resulted in https://treeherder.mozilla.org/logviewer.html#?job_id=110314247&repo=mozilla-inbound 

Michael, smaug: could you take a look at this ?
Flags: needinfo?(michael)
Flags: needinfo?(bugs)
The three patches in this bug landed in two separate pushes. First push is here:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a1e773337202d436865cbdd1fa375277efada840

Second push is here:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=852f7fddf9a7177bad142ad2fdec5fb34807ce98.

It's pretty clear that in the first push the patches were part of a larger multi-bug push, and the second push was a follow-up. And yet when the patches were backed out (comment 11) all three patches were backed out, without backing out the rest of the patches in the multi-bug push. So it's not surprising that caused problems. Either we need to reland the first two patches on this bug or back out the rest of the bugs as well.
The failure in dom/events/test/test_dblclick_kill_window.html noted in comment 11 also occurred on cset acd2949f5415, which was in between the "first push" and the "second push" from my previous comment. So it's not at all clear to me why :Aryx backed out just this bug without backing out the rest of the bugs from the "first push" or even what evidence pointed to this bug being the culprit at all.

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&group_state=expanded&tochange=201b30adaf89493e1fdd200a4ce9172bb905a33d&fromchange=a1e773337202d436865cbdd1fa375277efada840&filter-searchStr=linux%20x64%20opt%20mochitests%20executed%20by%20taskcluster%20with%20e10s%20test-linux64%2Fopt-mochitest-e10s-3%20tc-m-e10s(3)&selectedJob=110139107
Flags: needinfo?(aryx.bugmail)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #15)
> The three patches in this bug landed in two separate pushes. First push is
> here:
> 
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=a1e773337202d436865cbdd1fa375277efada840
> 
> Second push is here:
> 
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=852f7fddf9a7177bad142ad2fdec5fb34807ce98.
> 
> It's pretty clear that in the first push the patches were part of a larger
> multi-bug push, and the second push was a follow-up. And yet when the
> patches were backed out (comment 11) all three patches were backed out,
> without backing out the rest of the patches in the multi-bug push. So it's
> not surprising that caused problems. Either we need to reland the first two
> patches on this bug or back out the rest of the bugs as well.

thanks kats! will go and backout the csets that where missed in the first backout
I retriggered some more linux64/opt-mochitest-e10s-3 jobs to backfill the relevant range so we can confirm what really caused the test_dblclick_kill_window.html regression.
The retriggers confirmed the range.

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&group_state=expanded&tochange=201b30adaf89493e1fdd200a4ce9172bb905a33d&fromchange=d516c35eabf14738e03728f9be602b732e63078c&filter-searchStr=linux%20x64%20opt%20mochitests%20executed%20by%20taskcluster%20with%20e10s%20test-linux64%2Fopt-mochitest-e10s-3%20tc-m-e10s(3)

The push that included the first two patches of this bug as well as a bunch of other bugs caused the test_dblclick_kill_window.html regression. The backout from comment 11 was incomplete as it only backed out *this* bug and not the rest of the bugs, and that incomplete backout resulted in the regression in bug 1375293.

The fix here is to back out the rest of the bugs, and pray that nothing else crept in in the meantime.
Flags: needinfo?(aryx.bugmail)
ok the complete backout landed now in https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=cc903e3f61894e60c3b0efaf05e9a446d1d85888 after talking to kats - also merged this back to integration
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31f3e20868fb
Fix an assertion in PresShell::HandleEventWithTarget, r=smaug
Flags: needinfo?(michael)
Flags: needinfo?(bugs)
https://hg.mozilla.org/mozilla-central/rev/31f3e20868fb
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1404059
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.