1633471 - Crash in [@ IPCError-browser | RecvUpdateDocumentPrincipal Trying to reuse WindowGlobalParent but the principal of the new document]

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[Gabriele Svelto [:gsvelto]]

Attachment: 6ef347da-94d8-49f8-bbe3-49b200200420-upload_file_minidump_browser.log

This bug is for crash report bp-6ef347da-94d8-49f8-bbe3-49b200200420.

Top 10 frames of crashing thread:

0 mozglue.dll replace_realloc memory/replace/phc/PHC.cpp:1265
1  @0x96e377 
2 xul.dll static mozilla::dom::FeaturePolicyUtils::ForEachFeature dom/security/featurepolicy/FeaturePolicyUtils.cpp:92
3 xul.dll mozilla::dom::FeaturePolicy::InheritPolicy dom/security/featurepolicy/FeaturePolicy.cpp:42
4 xul.dll mozilla::dom::Document::InitFeaturePolicy dom/base/Document.cpp:3460
5 xul.dll mozilla::dom::Document::StartDocumentLoad dom/base/Document.cpp:3173
6 xul.dll nsHTMLDocument::StartDocumentLoad dom/html/nsHTMLDocument.cpp:463
7 xul.dll nsContentDLF::CreateInstance layout/build/nsContentDLF.cpp
8 xul.dll nsDocShell::NewContentViewerObj docshell/base/nsDocShell.cpp:7787
9 xul.dll nsDocShell::CreateContentViewer docshell/base/nsDocShell.cpp:7518

This looks like a regression that started with buildid 20200416094846. I've attached a full stack trace for the main process.

Comment 1

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Something landed in bug 1594529 2020-04-15.

Comment 2

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

hmm, is this Fission only? I think not.

[Tracking Requested - why for this release]:
crashes are bad

Comment 3

[Anny Gakhokidze [:annyG]]

The reason for this crash is the code I added in my patch here https://searchfox.org/mozilla-central/rev/7fd1c1c34923ece7ad8c822bee062dd0491d64dc/dom/ipc/WindowGlobalParent.cpp#260-262 [1]

When I’m stepping through this crash in a debugger, the assertion here also fails https://searchfox.org/mozilla-central/rev/7fd1c1c34923ece7ad8c822bee062dd0491d64dc/dom/base/nsGlobalWindowOuter.cpp#2187 [2]

This usually happens when we are trying to print certain webpages (link omitted here). In the specific case I am debugging, inside of nsGlobalWindowOuter::SetNewDocument when we call nsGlobalWindowOuter::WouldReuseInnerWindow we end up returning the result of FastEqualsConsideringDomain and since both principals have set explicit domains we end up taking this path https://searchfox.org/mozilla-central/rev/7fd1c1c34923ece7ad8c822bee062dd0491d64dc/caps/BasePrincipal.h#367-368. Both principles subsume each other and WouldReuseInnerWindow returns true which leads to us reusing the inner window. However, later in the function SetNewDocument when we check if the principals are Equals to each other (whether in [1] or [2]) , the code takes a different path - it compares origins and returns false here https://searchfox.org/mozilla-central/rev/7fd1c1c34923ece7ad8c822bee062dd0491d64dc/caps/BasePrincipal.h#349-350

For a brief moment I thought maybe we should not use EqualsConsideringDomain, but I see here that Boris did this on purpose in Bug 1552541. I'm not sure what the right approach here is...

Comment 4

[Anny Gakhokidze [:annyG]]

Should WouldReuseInnerWindow use both FastEqualsConsideringDomain and Equals and make it so we only reuse the inner window if both of those return true? I'm getting an assertion failure here[1] if I go that route.. Going to ask Nika to maybe shed some light on this.
[1] https://searchfox.org/mozilla-central/rev/7fd1c1c34923ece7ad8c822bee062dd0491d64dc/dom/base/Element.cpp#1134..

Comment 5

[Chris Peterson [:cpeterson]]

Tracking for Fission M5b because this is a crash and Anny is already investigating.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

We discussed this a bit on matrix yesterday, and talked about a few possible solutions here. The assertion failure which :annyG is running into in comment 4 is probably bug 1570804, which is a separate issue.

The reason for this problem is related to a small oversight in bug 1552541 around the interaction with printing. Usually, the principal for the first non-about:blank document in an iframe does not have document.domain set on it, as it is a fresh load, and has not had the chance to have its principal changed. However, in the print-preview case, our static-cloned document copies the principal of the document it is a clone of. This means that EqualsConsideringDomain can consider two cross-origin principals to be equal, as both will have set document.domain.

We don't want to be preserving the inner window in that situation. The easiest way to do this is probably to check Equals() && EqualsConsideringDomain(), like Anny was suggesting earlier, although that could certainly be optimized.

Comment 7

[Anny Gakhokidze [:annyG]]

Attachment: Bug 1633471 - Do not reuse inner window when printing, r?nika!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8197bf1a1fed
Do not reuse inner window when printing, r=nika

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8197bf1a1fed

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:annyG, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 11

[Anny Gakhokidze [:annyG]]

Attachment: beta uplift approval request

Approval Request Comment
[Feature/Bug causing the regression]: This is not a regression, but an existing issue that is exposed because of the stricter check introduced in bug 1594529.
[User impact if declined]: Sometimes when the user tries to print certain pages, the tab crashes.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: Not sure, I did a manual test myself. But here are steps to reproduce just in case:
Go to one of the URLs in the related crash reports and try to print the page. We should not crash when printing.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: it is low risk
[Why is the change risky/not risky?]:
The change is low risk because its a 3 line C++ change making a decision about when to reuse a window for a new document more strict.
[String changes made/needed]: none

Comment 12

[Pascal Chevrel:pascalc]

Anny, you didn't attach attach a patch to your uplift request (example in another bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1636568#c4)

Comment 13

[Anny Gakhokidze [:annyG]]

Sorry about that. You can use the same patch (it applies cleanly on top of mozilla-beta) - https://phabricator.services.mozilla.com/D74261

Comment 14

[Pascal Chevrel:pascalc]

Comment on attachment 9147355 [details]
beta uplift approval request

Patch is minimal and add tests, , uplift approved for beta 6, thanks.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/0dd5a33c957c

Comment 16

[Raluca Popovici, Desktop QA]

Reproduced the initial issue with an old Beta build 77.0b5 using Windows 10.
Verified - Fixed in Beta 77.0b6, Beta 77.0b7, and latest Nightly 78.0a1 using Windows 10, Ubuntu 18.04, and Mac 10.15.