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)

40 Branch
defect
Not set
critical

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)

Attached file testcase
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.
Since it causes crash in the coming Beta, mark it as security sensitive bug.
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)
(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.
Flags: needinfo?(quanxunzhen)
This is null pointer crash, so shouldn't need to be security sensitive.
Group: core-security
Crash Signature: [ @ ExitFullscreenInDocTree ]
QA Whiteboard: [@ ExitFullscreenInDocTree ]
Severity: normal → critical
QA Whiteboard: [@ ExitFullscreenInDocTree ]
Keywords: crash
I can repro on windows on 42, but not on 39.
Confirmed it is a regression since bug 947854.
Blocks: 947854
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.
Attached patch safeguard patchSplinter Review
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)
Attached patch mochitest patchSplinter Review
Attachment #8629169 - Flags: review?(bugs)
Hmm, my patches for bug 1160014 reintroduce this bug again. If I land the mochitest before that, I would probably have hard time then. :(
Attachment #8629165 - Flags: review?(bugs) → review+
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 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 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 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+
Depends on: 1180574
Is this something that you plan to uplift to 40? Tracking in 40 because regression.
Flags: needinfo?(quanxunzhen)
Tracking in other 41, 42 as well because regression.
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?
(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...
Blocks: 1181212
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+
QA Whiteboard: [good first verify]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.