Crash in [@ IPCError-browser | RecvUpdateDocumentPrincipal Trying to reuse WindowGlobalParent but the principal of the new document]
Categories
(Core :: DOM: Navigation, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr68 | --- | unaffected |
firefox75 | --- | unaffected |
firefox76 | --- | unaffected |
firefox77 | --- | verified |
firefox78 | --- | verified |
People
(Reporter: gsvelto, Assigned: annyG)
References
(Regression)
Details
(Keywords: crash, regression)
Crash Data
Attachments
(3 files)
(deleted),
text/plain
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/plain
|
pascalc
:
approval-mozilla-beta+
|
Details |
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•5 years ago
|
||
Something landed in bug 1594529 2020-04-15.
Updated•5 years ago
|
Comment 2•5 years ago
|
||
hmm, is this Fission only? I think not.
[Tracking Requested - why for this release]:
crashes are bad
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 3•5 years ago
|
||
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...
Assignee | ||
Comment 4•5 years ago
|
||
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•5 years ago
|
||
Tracking for Fission M5b because this is a crash and Anny is already investigating.
Comment 6•5 years ago
|
||
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.
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 7•5 years ago
|
||
Updated•5 years ago
|
Comment 9•5 years ago
|
||
bugherder |
Comment 10•5 years ago
|
||
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.
Updated•5 years ago
|
Assignee | ||
Comment 11•5 years ago
|
||
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•5 years ago
|
||
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)
Assignee | ||
Comment 13•5 years ago
|
||
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•5 years ago
|
||
Comment 15•5 years ago
|
||
bugherder uplift |
Updated•5 years ago
|
Updated•5 years ago
|
Comment 16•5 years ago
|
||
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.
Description
•