2021375 - Crash in [@ VerifyCientPrincipalInfosMatch ] - Inherit controller for top level initial about:blank

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P2)

Description

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

Larger crash volume this signature had been fixed in bug 2006265 but some crashes are still reported for 148 and 149, e.g. bp-8b89d647-f2d9-44e6-a3d0-276d00260305.

Crash report: https://crash-stats.mozilla.org/report/index/8b89d647-f2d9-44e6-a3d0-276d00260305

MOZ_CRASH Reason:

MOZ_RELEASE_ASSERT(leftContent.attrs() == rightContent.attrs() && leftContent.originNoSuffix() == rightContent.originNoSuffix())

Top 10 frames:

0  xul.dll  VerifyCientPrincipalInfosMatch(mozilla::ipc::PrincipalInfo const&, mozilla::i...  docshell/base/nsDocShell.cpp:2498
1  xul.dll  nsDocShell::MaybeInheritController(mozilla::dom::ClientSource*, nsIPrincipal*)  docshell/base/nsDocShell.cpp:2535
2  xul.dll  nsDocShell::CompleteInitialAboutBlankLoad(nsDocShellLoadState*, nsILoadInfo*)  docshell/base/nsDocShell.cpp:11418
2  xul.dll  nsDocShell::DoURILoad(nsDocShellLoadState*, mozilla::Maybe<unsigned int>, nsI...  docshell/base/nsDocShell.cpp:11176
2  xul.dll  nsDocShell::InternalLoad(nsDocShellLoadState*, mozilla::Maybe<unsigned int>)  docshell/base/nsDocShell.cpp:10159
3  xul.dll  nsDocShell::LoadURI(nsDocShellLoadState*, bool, bool)  docshell/base/nsDocShell.cpp:891
4  xul.dll  nsDocShell::LoadURI(nsDocShellLoadState*, bool)  docshell/base/nsDocShell.cpp:721
4  xul.dll  nsFrameLoader::ReallyStartLoadingInternal()  dom/base/nsFrameLoader.cpp:772
4  xul.dll  nsFrameLoader::ReallyStartLoading()  dom/base/nsFrameLoader.cpp:638
4  xul.dll  mozilla::dom::Document::MaybeInitializeFinalizeFrameLoaders()  dom/base/Document.cpp:9986

Comment 1

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 543435

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

I believe the problem I've identified in bug 2013379 and have a fix coming for imminently should ideally address this.

Comment 3

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 543435

Comment 4

[BugBot [:suhaib / :marco]]

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 10 AArch64 and ARM crashes on release

For more information, please visit BugBot documentation.

Comment 6

[Vincent Hilla [:vhilla]]

I spent some time trying to create a minimal test case for bug 2013379 in hopes this might help me find one for about:blank. But I haven't succeeded yet - with service workers, anti tracking and redirects, this code is quite unknown to me. I hope bug 2052798 will help us make a tiny bit of progress. But from what I remember from bug 2006265, this crash is quite hard without a minimal reproducible example.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

I wonder if Hackbot might be able to help.

Comment 8

[Hackbot [:hackbot]]

Attachment: Bug 2021375 - Don't inherit a ServiceWorker controller into an initial about:blank whose principal doesn't match. r?asuth

nsDocShell::MaybeInheritController release-asserted (via VerifyClientPrincipalInfosMatch)
that the ClientSource it is about to control has the same principal as the parent's
controller, and then unconditionally inherited it. Crash-stats shows that assumption is
simply not true:

• 457 of 459 reports come from the CompleteInitialAboutBlankLoad call site, only 2 from
  MaybeCreateInitialClientSource.
• 354 of them go through nsDocShell::MaybeHandleSubframeHistory ->
  LoadHistoryEntry, i.e. a subframe session history entry restored onto the freshly
  created initial about:blank (see also bug 2037346). All 126 originNoSuffix()
  mismatches are in this bucket: the restored entry carries its own principal to inherit,
  CompleteInitialAboutBlankLoad clobbers the document with it, and the new client then
  gets the embedder's controller, which is a different origin.
• The remaining 97 come from a plain nsFrameLoader::ReallyStartLoading and are all
  attrs() mismatches, i.e. the long standing partitioned-principal divergence tracked in
  bugs 2010596 / 2013379 / 2003449.
• The leftForeignBit == rightForeignBit assert added in bug 2052798 never fires, so this
  is not the bug 2013379 foreign-bit problem the code comment guessed at.

In every one of those cases inheriting is the wrong thing to do: the client would either
end up controlled cross-origin or in a different storage partition than its controller,
which is exactly why ClientSource::SetController release-asserts on it. So check the
condition instead of asserting it and leave the client uncontrolled when it does not hold.
This is the single choke point for both call sites and both mismatch classes, and it uses
the same ClientMatchPrincipalInfo() predicate that ClientSource::SetController and
ClientHandle::Control enforce, so the guard cannot drift from the assert it protects.
VerifyClientPrincipalInfosMatch is removed (bug 2052801).

This does not fix the underlying partitioned-principal computation; bugs 2010596, 2013379
and 2003449 continue to track that. It stops it from being a topcrash.

Testing: new browser test browser_initial_about_blank_subframe_history.js. A controlled
https://example.com page statically embeds a https://test1.example.com subframe; the
subframe navigates itself to about:blank and the top level page is reloaded. Without the
patch this crashes the content process with
[@ VerifyClientPrincipalInfosMatch] /
MOZ_RELEASE_ASSERT(leftContent.originNoSuffix() == rightContent.originNoSuffix()), in
both the default and the dFPI (browser-dFPI.toml) configurations. With the patch the
restored about:blank keeps its own origin and is simply not controlled.

browser_partitionedABBredirectToABAServiceWorkers.js (which asserts that an inheriting
about:blank nested in a controlled ABA frame is controlled),
browser_antitracking_subiframes.js, browser_antitracking.js and WPT
service-workers/service-worker/about-blank-replacement.https.html all still pass.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Does Hackbot's analysis make any sense?

Comment 10

[Vincent Hilla [:vhilla]]

This looks great, the testcase reproduces. I'll have a look but need to find the time and familiarize myself with the code.

Comment 11

[Vincent Hilla [:vhilla]]

The ones going through MaybeHandleSubframeHistory are due to bug 2037346. As the bot says, the others are due to the long standing partitioned-principal divergence. I don't immediately agree with the suggested fix, but the diagnosis is correct. Instead of silently failing and just not inheriting the controller, maybe we can fix the MaybeHandleSubframeHistory case and leave the rest as a pre-existing issue.

For the MaybeHandleSubframeHistory, we have an xorigin non-initial about:blank and then reload. This can occur if an xorigin page navigates itself to about:blank. On reload, we create a new iframe and docshell that doesn't yet have mHasStartedLoadingOtherThanInitialBlankURI set and contains a transient about:blank document. The first navigation that arrives is the xorigin about:blank being restored, and as it's the first navigation for a new docshell, we treat it as a synchronous about:blank. Though it arguably shouldn't get this initial treatment, as the about:blank that was there before the reload wasn't initial. This xorigin initial about:blank also isn't something I thought about previously, the code expects an initial about:blank to be same origin with it's embedder.

Two possible fixes

• We could track in session history whether an about:blank is initial. Though bug 2007894 is about reloading a page with an initial about:blank, which already has the issue that we asynchronously get to the synchronous load in CompleteInitialAboutBlankLoad. So we wouldn't regress anything by always rejecting ShouldDoInitialAboutBlankSyncLoad for loads from session history. One related issue is whether an xorigin iframe could somehow embed an xorigin initial about:blank in the parent document. That might be possible with bug 2046096, so we'd have to fix that one too.
• Alternatively, we can fix bug 2037346, which is mostly an issue with keeping the session history state consistent. Edit: that's not entirely enough, a back traversal might be able to trigger the same crash.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)

Does Hackbot's analysis make any sense?

Agreed with :vhilla that the suggested fix of "turn the violation of an important structural invariant into a log message we can more easily ignore" is not a good one but analyzing the crashes and providing a testcase that triggers one of the cases of the crash is great, as he also conveys. It's also good to have more information on the breakdown of the crashes (although it would always be nice to have specific artifacts that can be double-checked); I do have a fix for bug 2013379 which is a specific partitioning problem (and the key issue has been some difficulty in reproducing for test coverage combined with higher priority sec or sec-adjacent bugs), but clearly I should try and see if the LLMs can help with that given the recent model and tooling upgrades.

Leaving my needinfo for now so I can make sure I understand these non-partitioning cases.

Comment 13

[Vincent Hilla [:vhilla]]

Attachment: Bug 2021375 - Skip initial about:blank for session history. r=smaug,asuth,#dom-core

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

(Switching to assignee to :vhilla since this bug has now thankfully been specialized to a specific problem case and :vhilla is fixing; bug 2013379 remains something I hope to finish up shortly.)

Comment 15

[BugBot [:suhaib / :marco]]

Based on the topcrash criteria, the crash signatures linked to this bug are not in the topcrash signatures anymore.

For more information, please visit BugBot documentation.

Comment 16

[Donal Meehan [:dmeehan]]

Clearing stale need-info

Comment 17

[Vincent Hilla [:vhilla]]

Attachment: Bug 2021375 - Add WPTs for iframe history restore on container navigation. r=#dom-core

Comment 18

[Vincent Hilla [:vhilla]]

Try is green https://treeherder.mozilla.org/jobs?repo=try&revision=38ac103454f58d75fe8cdc50960435b047c3cc38

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/fad7c139f14e
https://hg.mozilla.org/integration/autoland/rev/60f0aa5efade
Skip initial about:blank for session history. r=smaug,asuth,dom-core-reviewers,dom-worker-reviewers
https://github.com/mozilla-firefox/firefox/commit/166c15341bec
https://hg.mozilla.org/integration/autoland/rev/6c9f7b10cd28
Add WPTs for iframe history restore on container navigation. r=dom-core-reviewers,smaug

Comment 20

[Andrew Sutherland [:asuth] (he/him)]

Does this still want to be leave-open?

Comment 21

[Vincent Hilla [:vhilla]]

I guess not.

Though we'll need to see if the remaining crashes are from the pre-existing principal divergence issues.

Comment 22

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/60f0aa5efade
https://hg.mozilla.org/mozilla-central/rev/6c9f7b10cd28

Comment 23

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/62397 for changes under testing/web-platform/tests

Comment 24

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Please add an ESR153 uplift request to this when you get a chance.

Comment 26

[Phabricator Automation]

firefox-esr153 Uplift Approval Request

• User impact if declined/Reason for urgency: This avoids a crash which mostly occurred on Android. It could also occur in ESR, but there haven't been reports yet.
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: This affects the load of an xorigin about:blank, which is an edge case. The fix has been in the tree for some time already.
• String changes made/needed?: None
• Is Android affected?: yes

Comment 27

[Vincent Hilla [:vhilla]]

Attachment: Bug 2021375 - Skip initial about:blank for session history.

Original Revision: https://phabricator.services.mozilla.com/D320395

Comment 28

[Vincent Hilla [:vhilla]]

Done. While there were cashes on Windows Release, most happened on Android and there aren't reports from ESR. In theory, the bug should reproduce in ESR too, so I put up the request.

Comment 29

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr153/rev/fee99e029c10