Closed Bug 1323983 Opened 8 years ago Closed 7 years ago

A pointer lock request paired with a window.close() from an opened popup window crashes the browser

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Version:
50 Branch
Platform:
x86
Windows 10
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: jujjyl, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Attachments

(4 files)

testcase
289 bytes, text/html
Details
Bug 1323983 part 1 - Ensure to release the pointer even if the document has been detached.
58 bytes, text/x-review-board-request
smaug
: review+
jcristau
: approval-mozilla-beta+
Details
Bug 1323983 part 2 - Avoid sending response for native synthesis if there is no observer.
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1323983 part 3 - Add test for closing window while holding pointerlock.
58 bytes, text/x-review-board-request
smaug
: review+
Details 
Why is this a security bug?
Flags: needinfo?(jujjyl)
I err on making crash bugs security bugs, better than the alternative. If it is concluded otherwise, feel free to drop the label.
Flags: needinfo?(jujjyl)
Group: core-security
Keywords: crash
Flags: needinfo?(xidorn+moz)
Hmm... It actually affects both e10s and non-e10s. On non-e10s, although it would not crash, the window would still lock the pointer and user has no way to release it.
Blocks: pointer-lock
Flags: needinfo?(xidorn+moz)
Attached file testcase — 
A simplified testcase.

Steps to reproduce:
1. click the "Open New Windows" button
2. in the newly opened page, click "Request PointerLock and Close"

Expected result:
The page is closed, and everything else works as normal.

Actual restul:
On e10s window, the content process would crash.
On non-e10s window, the window would continue locking the pointer, and there is no way to escape from that until closing that window.


Note: if "window.close()" is called synchronously after "requestPointerLock()", e10s window would still crash, but non-e10s window would work just fine.
Component: DOM: Content Processes → DOM
The issue for non-e10s is that, when the window is closed via script, the document has already detached from the presshell, and thus nsDocument::SetPointerLock fails to find the event state manager and corresponding widget to unlock the pointer. Part 1 changes EventStateManager::SetPointerLock() to be a static method, so that we can reset the state in ESM even if we cannot get an ESM instance.

The issue for e10s is that, when we unlock the pointer, ESM asks the widget to move the mouse back to its pre-lock position. It does so via SynthesizeNativeMouseMove, which PuppetWidget redirects to the chrome process. These all works fine. The issue is that after the chrome process completes this mouse move, it sends a response to the child process, but at that point the window has died, and thus the content process doesn't find a receiver of the response, and thus crashes.

The reason why chrome process needs to send a response back is that, the API of synthesizing native mouse move supports attaching an observer, which would be notified when the move is done. However, in the case of unlocking pointer, we don't actually need to know that, so no observer is attached, so the response isn't really necessary. So in part 2, I make the chrome process not send the response if there is no observer at all.
Assignee: nobody → xidorn+moz
And, there could still be some... minor issues remaining, e.g. the warning popup. But those are not as bad as the current situation. To completely fix those issues, we may want to fix bug 1255338 which is non-trivial...
Comment on attachment 8819765 [details]
Bug 1323983 part 1 - Ensure to release the pointer even if the document has been detached.

https://reviewboard.mozilla.org/r/99410/#review99826
Attachment #8819765 - Flags: review?(bugs) → review+
Comment on attachment 8819766 [details]
Bug 1323983 part 2 - Avoid sending response for native synthesis if there is no observer.

https://reviewboard.mozilla.org/r/99412/#review99828

The crash is about the assertion MOZ_ASSERT(aObserverId == 0, "We should always find a saved observer for nonzero IDs"); ?
So, not a crash in debug builds, right?
Attachment #8819766 - Flags: review?(bugs) → review+
Comment on attachment 8819767 [details]
Bug 1323983 part 3 - Add test for closing window while holding pointerlock.

https://reviewboard.mozilla.org/r/99414/#review99830

::: dom/tests/mochitest/pointerlock/test_pointerlock-autoclose.html:13
(Diff revision 1)
> +  <link rel="stylesheet" href="/tests/SimpleTest/test.css">
> +</head>
> +<body>
> +  <button onclick="document.body.requestPointerLock()">Lock Pointer</button>
> +  <script>
> +    SimpleTest.waitForExplicitFinish();

Don't you want to call SimpleTest.waitForExplicitFinish(); only when you don't have opener.

::: dom/tests/mochitest/pointerlock/test_pointerlock-autoclose.html:23
(Diff revision 1)
> +      } else {
> +        document.addEventListener("pointerlockchange", function() {
> +          opener.is(document.pointerLockElement, document.body,
> +                    "Check we have locked the pointer");
> +          window.close();
> +          setTimeout(function() {

Trying to remember if this is racy. What if the timeouts for the closed window are suspended already before the timeout would run.
Perhaps call opener.setTimeout("SimpleTest.finish()", 0);
Attachment #8819767 - Flags: review?(bugs) → review+
Comment on attachment 8819766 [details]
Bug 1323983 part 2 - Avoid sending response for native synthesis if there is no observer.

https://reviewboard.mozilla.org/r/99412/#review99828

No, it is not about that. This assertion is unrelated. The crash is "Route error: message sent to unknown actor ID". See bp-5cf9cd2c-154c-4f45-a73b-fe2312161216.
Comment on attachment 8819767 [details]
Bug 1323983 part 3 - Add test for closing window while holding pointerlock.

https://reviewboard.mozilla.org/r/99414/#review99830

> Don't you want to call SimpleTest.waitForExplicitFinish(); only when you don't have opener.

Good catch.

> Trying to remember if this is racy. What if the timeouts for the closed window are suspended already before the timeout would run.
> Perhaps call opener.setTimeout("SimpleTest.finish()", 0);

Probably just add a new function and a global variable so that the opener can close the window.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #13)

> No, it is not about that. This assertion is unrelated. The crash is "Route
> error: message sent to unknown actor ID". See
> bp-5cf9cd2c-154c-4f45-a73b-fe2312161216.
Hmm, now I'm lost. What actor is not there?
(In reply to Olli Pettay [:smaug] from comment #15)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #13)
> 
> > No, it is not about that. This assertion is unrelated. The crash is "Route
> > error: message sent to unknown actor ID". See
> > bp-5cf9cd2c-154c-4f45-a73b-fe2312161216.
> Hmm, now I'm lost. What actor is not there?

I'm not familiar with e10s's IPC... My guess is the target document?
Documents don't have ipc actors.
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8f01cf080e69
part 1 - Ensure to release the pointer even if the document has been detached. r=smaug
https://hg.mozilla.org/integration/autoland/rev/f3290b0f20cc
part 2 - Avoid sending response for native synthesis if there is no observer. r=smaug
https://hg.mozilla.org/integration/autoland/rev/8f9298b5935f
part 3 - Add test for closing window while holding pointerlock. r=smaug
Sorry had to back this out for test_pointerlock-autoclose.html test timed out, i.e., https://treeherder.mozilla.org/logviewer.html#?job_id=8161735&repo=autoland#L46849
Flags: needinfo?(xidorn+moz)
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4dd995379ca7
Backed out changeset 8f9298b5935f for test_pointerlock-autoclose.html timed out
https://hg.mozilla.org/integration/autoland/rev/4e18a255b586
Backed out changeset f3290b0f20cc 
https://hg.mozilla.org/integration/autoland/rev/dd43bc709e8d
Backed out changeset 8f01cf080e69
Ah, Win8 only, which is not enabled on try by default... ok
Ah, this time it's Linux opt... It seems the real click is tricky after a whole of pointerlock tests...

I guess things would be better if the new test can be run before the old tests. I tried to do that in mochitest.ini but it seems the order in that file is not respected. Probably the test run by the order of their names?
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c818000827de
part 1 - Ensure to release the pointer even if the document has been detached. r=smaug
https://hg.mozilla.org/integration/autoland/rev/d80ab1b76771
part 2 - Avoid sending response for native synthesis if there is no observer. r=smaug
https://hg.mozilla.org/integration/autoland/rev/b67517ef132a
part 3 - Add test for closing window while holding pointerlock. r=smaug
Should we consider taking this on 52 as well? If so, please nominate accordingly :)
status-firefox50: --- → wontfix
status-firefox51: --- → wontfix
status-firefox52: --- → affected
Flags: needinfo?(xidorn+moz)
Flags: in-testsuite+
Comment on attachment 8819765 [details]
Bug 1323983 part 1 - Ensure to release the pointer even if the document has been detached.

Approval Request Comment
[Feature/Bug causing the regression]: PointerLock API, may not be a regression
[User impact if declined]: Browser may crash by malicious page
[Is this code covered by automated tests?]: A test was added with the patch.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No, automated test should be enough.
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: Should not be risky.
[Why is the change risky/not risky?]: Only affects PointerLock API which has been well covered by automated tests.
[String changes made/needed]: n/a

This is for all three patches to Firefox 52.
Flags: needinfo?(xidorn+moz)
Attachment #8819765 - Flags: approval-mozilla-aurora?
Comment on attachment 8819765 [details]
Bug 1323983 part 1 - Ensure to release the pointer even if the document has been detached.

pointer lock fixes, beta52+
Attachment #8819765 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: