Closed
Bug 1179123
Opened 9 years ago
Closed 9 years ago
Close window in fullscreenchange event handler crash Firefox
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox38 | --- | unaffected |
firefox38.0.5 | --- | unaffected |
firefox39 | --- | unaffected |
firefox40 | + | fixed |
firefox41 | + | fixed |
firefox42 | + | fixed |
People
(Reporter: xidorn, Assigned: xidorn)
References
(Blocks 1 open bug)
Details
(Keywords: crash, regression)
Crash Data
Attachments
(4 files)
1.24 KB,
text/html
|
Details | |
1.79 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
5.17 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
1.80 KB,
patch
|
lmandel
:
approval-mozilla-aurora+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
In non-e10s mode, if a page request fullscreen and call window.exit() in the fullscreenchange handler, Firefox would crash. Steps to reproduce: 1. open a non-e10s window if in e10s mode 2. open the attachment page 3. click the page, a new tab will be open 4. click the newly opened page Expected result: the new page is closed properly Actual result: Firefox crashes I cannot reproduce this in release channel, hence likely a regression.
Assignee | ||
Comment 1•9 years ago
|
||
Since it causes crash in the coming Beta, mark it as security sensitive bug.
Comment 2•9 years ago
|
||
Is this just on OS X? Do you have a crashreport ID or stack? Am I right in thinking 39 is not affected?
Flags: needinfo?(quanxunzhen)
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #2) > Is this just on OS X? No. Actually I initially met this crash on Windows, and I can reproduce this on OS X as well. > Do you have a crashreport ID or stack? I just reproduced it and submit one: bp-e2b8091e-7f3d-4167-8bf1-121152150701 > Am I right in thinking 39 is not affected? I'm not sure. I haven't tested Firefox 39. But I guess it shouldn't be affected.
status-firefox39:
--- → ?
Flags: needinfo?(quanxunzhen)
Comment 4•9 years ago
|
||
This is null pointer crash, so shouldn't need to be security sensitive.
Updated•9 years ago
|
Group: core-security
Crash Signature: [ @ ExitFullscreenInDocTree ]
QA Whiteboard: [@ ExitFullscreenInDocTree ]
Updated•9 years ago
|
Comment 5•9 years ago
|
||
I can repro on windows on 42, but not on 39.
Assignee | ||
Comment 7•9 years ago
|
||
It seems this bug is fixed by some other bug (likely bug 1168705), but we do need some more safeguard as well as patch for uplift.
Assignee | ||
Comment 8•9 years ago
|
||
This would still trigger assertion in existing versions, but won't crash. I tried to thoroughly fix it, but failed to figure out how. This bug happened at the end of nsINode::doRemoveChildAt(). That method calls nsXULElement::UnbindFromTree() there, which calls nsFrameLoader::Destroy() -> nsDocument::SetSubDocumentFor() to remove the child document from the XUL document, then calls nsElement::UnbindFromTree() -> nsDocument::ExitFullscreen() to exit fullscreen on the root document. Since the child document has been detached at this point, we won't be able to visit that document to exit fullscreen there. Then the method calls the destructor of mozAutoDocUpdate, which eventually calls nsDocument::OnPageHide() which again tries to exit fullscreen, but finds the root has exited fullscreen. Not sure how this is fixed in the current m-i. I thought about replacing FullscreenRoots with FullscreenLeaves to track the leaf documents instead of roots. That could work now, but once the Fullscreen API spec is updated so that the fullscreen element is no longer necessarily in the same containing chain, maintaining the list of leaf documents could be more tricky.
Assignee: nobody → quanxunzhen
Attachment #8629165 -
Flags: review?(bugs)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8629169 -
Flags: review?(bugs)
Assignee | ||
Comment 10•9 years ago
|
||
Hmm, my patches for bug 1160014 reintroduce this bug again. If I land the mochitest before that, I would probably have hard time then. :(
Updated•9 years ago
|
Attachment #8629165 -
Flags: review?(bugs) → review+
Comment 11•9 years ago
|
||
Actually, what is the test trying to test? Making sure we don't crash here anymore? Well, why do we want to execute NS_NOTREACHED("Fullscreen root should be a fullscreen doc..."); while running mochitets?
Comment 12•9 years ago
|
||
Comment on attachment 8629169 [details] [diff] [review] mochitest patch So I don't know how we could then land this.
Attachment #8629169 -
Flags: review?(bugs)
Comment 13•9 years ago
|
||
Comment on attachment 8629165 [details] [diff] [review] safeguard patch And in fact, this feels like a valid case, so I don't see need for NS_NOTREACHED. Just return. With that, r+
Comment 14•9 years ago
|
||
Comment on attachment 8629169 [details] [diff] [review] mochitest patch One day I'll learn why people like Promises so much. This took about 10x more time to read than if this hadn't been so Promise heavy ;) I can perhaps blame myself, since I write so little JS. rs+
Attachment #8629169 -
Flags: review+
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/aba278798298 https://hg.mozilla.org/integration/mozilla-inbound/rev/1d669992e0da
Comment 17•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/aba278798298 https://hg.mozilla.org/mozilla-central/rev/1d669992e0da https://hg.mozilla.org/mozilla-central/rev/688ccfafb3fb
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment 18•9 years ago
|
||
Is this something that you plan to uplift to 40? Tracking in 40 because regression.
Flags: needinfo?(quanxunzhen)
Comment 19•9 years ago
|
||
Tracking in other 41, 42 as well because regression.
tracking-firefox41:
--- → +
tracking-firefox42:
--- → +
Assignee | ||
Comment 20•9 years ago
|
||
Approval Request Comment [Feature/regressing bug #]: bug 947854 [User impact if declined]: may crash in some cases [Describe test coverage new/current, TreeHerder]: test added on m-c [Risks and why]: no risk, just a safeguard to ensure we won't crash [String/UUID change made/needed]: n/a
Flags: needinfo?(quanxunzhen)
Attachment #8630195 -
Flags: approval-mozilla-beta?
Attachment #8630195 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 21•9 years ago
|
||
(In reply to Kate Glazko from comment #18) > Is this something that you plan to uplift to 40? Tracking in 40 because > regression. Sorry I forgot that...
Comment 22•9 years ago
|
||
Comment on attachment 8630195 [details] [diff] [review] safeguard patch for uplift, r=smaug Seems like a pretty safe fix for a crash and has been on m-c for a few days. Let's get this into beta3. Beta+ Aurora+
Attachment #8630195 -
Flags: approval-mozilla-beta?
Attachment #8630195 -
Flags: approval-mozilla-beta+
Attachment #8630195 -
Flags: approval-mozilla-aurora?
Attachment #8630195 -
Flags: approval-mozilla-aurora+
Comment 23•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/4d659b6e1c67
Flags: in-testsuite+
Updated•9 years ago
|
QA Whiteboard: [good first verify]
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•