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

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Vincent Hilla [:vhilla]]

https://phabricator.services.mozilla.com/D276230 separated this crash out of bug 1880012

Comment 1

[Vincent Hilla [:vhilla]]

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

There haven't been many crashes yet and most seem to be from one installation. But all I looked at had this stack:

VerifyCientPrincipalInfosMatch(mozilla::ipc::PrincipalInfo const&, mozilla::ipc::PrincipalInfo const&)
nsDocShell::MaybeInheritController(mozilla::dom::ClientSource*, nsIPrincipal*)
nsDocShell::CompleteInitialAboutBlankLoad(nsDocShellLoadState*, nsILoadInfo*)
nsDocShell::DoURILoad(nsDocShellLoadState*, mozilla::Maybe<unsigned int>, nsIRequest**)
nsDocShell::InternalLoad(nsDocShellLoadState*, mozilla::Maybe<unsigned int>)
nsDocShell::LoadURI(nsDocShellLoadState*, bool, bool)
mozilla::dom::BrowsingContext::LoadURI(nsDocShellLoadState*, bool)
nsWindowWatcher::OpenWindowInternal(mozIDOMWindowProxy*, nsIURI*, nsTSubstring<char> const&, nsTSubstring<char> const&, mozilla::dom::UserActivation::Modifiers const&, bool, bool, bool, nsIArray*, bool, bool, bool, nsPIWindowWatcher::PrintKind, nsDocShellLoadState*, mozilla::dom::BrowsingContext**)
nsWindowWatcher::OpenWindow2(mozIDOMWindowProxy*, nsIURI*, nsTSubstring<char> const&, nsTSubstring<char> const&, mozilla::dom::UserActivation::Modifiers const&, bool, bool, bool, nsIArray*, bool, bool, bool, nsPIWindowWatcher::PrintKind, nsDocShellLoadState*, mozilla::dom::BrowsingContext**)
nsGlobalWindowOuter::OpenInternal(nsTSubstring<char> const&, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, bool, bool, bool, bool, nsIArray*, nsDocShellLoadState*, bool, nsGlobalWindowOuter::PrintKind, mozilla::dom::BrowsingContext**)
nsGlobalWindowOuter::OpenJS(nsTSubstring<char> const&, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, mozilla::dom::BrowsingContext**)
nsGlobalWindowOuter::OpenOuter(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, mozilla::ErrorResult&)
nsGlobalWindowInner::Open(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, mozilla::ErrorResult&)
mozilla::dom::Window_Binding::open(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&)
[Javascript]
[event dispatching]
mozilla::dom::BrowserChild::DispatchWidgetEventViaAPZ(mozilla::WidgetGUIEvent&)
mozilla::dom::BrowserChild::HandleRealMouseButtonEvent(mozilla::WidgetMouseEvent const&, mozilla::layers::ScrollableLayerGuid const&, unsigned long long const&)
mozilla::dom::BrowserChild::ProcessPendingCoalescedMouseDataAndDispatchEvents()
mozilla::dom::BrowserChild::RecvRealMouseButtonEvent(mozilla::WidgetMouseEvent const&, mozilla::layers::ScrollableLayerGuid const&, unsigned long long const&)

Comment 2

[Vincent Hilla [:vhilla]]

Bug 2003449 is another storage principal origin-no-suffix mismatch.

Comment 3

[BugBot [:suhaib / :marco]]

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

Comment 4

[Vincent Hilla [:vhilla]]

Something opens a popup for about:blank. While the popup is opened, it's inner window obtains a client source without controller. Towards the end of opening the window when finishing the load, we call MaybeInheritController and crash because the opener controller has a principal that doesn't match with the client source.

A simple window.open("about:blank") from a controlled window isn't enough to reproduce, but does lead to executing VerifyClientPrincipalInfosMatch.

From this try job, no test is failing without this MaybeInheritController call. But :asuth commented on the patch in bug 543435 and indicated that this call does make sense: https://phabricator.services.mozilla.com/D155376#inline-1462397

Comment 5

[Vincent Hilla [:vhilla]]

From the following example, the MaybeInheritController call causes the popup to be controlled. This doesn't happen in Chrome.

sw.js: self.addEventListener('activate', evt => evt.waitUntil(self.clients.claim()) );

index.html:

<script>                                                                        
function setup() {                                                              
  navigator.serviceWorker.register('/sw.js');                                   
}                                                                               
function test() {                                                               
  w = window.open("about:blank");                                               
  console.log(w.navigator.serviceWorker.controller);                            
  w.close()                                                                     
}                                                                               
</script>                                                                       
<button onclick=setup()>setup</button>                                          
<button onclick=test()>test</button>

Comment 6

[Vincent Hilla [:vhilla]]

:asuth can you take a look?

Specifically, should CompleteInitialAboutBlankLoad call MaybeInheritController? For a new about:blank popup, how could the client source of the inner window and the opener controller mismatch?

Comment 7

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

(In reply to Vincent Hilla [:vhilla] from comment #5)

From the following example, the MaybeInheritController call causes the popup to be controlled. This doesn't happen in Chrome.

I think chrome is wrong here? https://w3c.github.io/ServiceWorker/#control-and-use-window-client is the relevant spec text and in my previous evaluation I believe my determination was that we should be inheriting the controller in that case. That said, if they have some kind of intervention such that they are acting like "noopener" has been passed, then it could make sense (although your example should then also throw on w.navigator I think). I suppose they could have also arbitrarily decided to do something different for w.open from what they would do for an iframe in the same page? Or maybe they have a different interpretation for what's going on in the spec text?

That said, I believe the crashes can and do happen with iframes, or at least that's what's happening in the pernosco trace case.

(In reply to Vincent Hilla [:vhilla] from comment #6)

:asuth can you take a look?

A relevant (but massive) pernosco trace is currently rebuilding, I'm about to time out for the night, but it's my hope it should be ready for me first thing tomorrow morning.

Specifically, should CompleteInitialAboutBlankLoad call MaybeInheritController? For a new about:blank popup, how could the client source of the inner window and the opener controller mismatch?

The family of problems we've had so far relates to races related to changing the partition key that is used by CookieJarSettings to mint storage/partitioned principals. The original variation of this problem was that CookieJarSettings was not being snapshotted in the parent process when it was being inherited, but instead a live reference was being propagated into child docshells and this meant that multiple documents' loads could interfere with each other and cause CookieJarSettings to mint inconsistent principals. My most recent investigation to the massive pernosco trace had suggested there is a variation with content processes where the LoadInfo bouncing around results in a similar problem. Ideally I should be able to finish out the analysis tomorrow assuming the pernosco rebuild process completes in a timely fashion.

Comment 8

[BugBot [:suhaib / :marco]]

The bug is marked as tracked for firefox148 (nightly). However, the bug still isn't assigned.

:slloyd, could you please find an assignee for this tracked bug? Given that it is a regression and we know the cause, we could also simply backout the regressor. If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 9

[Sam Lloyd [:slloyd]]

Assigned to :Asuth in line with above comments

Comment 10

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

The trace rebuilding had to be manually kicked for https://bugzilla.mozilla.org/show_bug.cgi?id=1910011#c22 which added some delays but I think I've identified an inconsistency in the maintenance of the anti-tracking third-party bits for that clients bug that could potentially explain the situation if Clients.claim() was used although I'll need to validate that theory a bit more as I confirm my current theory in that bug. (The idea being that it's basically the same situation but we didn't control the page at navigation time, instead only starting to control it afterwards, making it then possible when inheriting that we'd see the problem.)

Comment 11

[Andrew McCreight [:mccr8]]

This is tracked so I'll mark it S2.

Comment 12

[BugBot [:suhaib / :marco]]

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

• Top 10 desktop browser crashes on nightly
• Top 10 AArch64 and ARM crashes on nightly

For more information, please visit BugBot documentation.

Comment 13

[Vincent Hilla [:vhilla]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #7)

The family of problems we've had so far relates to races related to changing the partition key that is used by CookieJarSettings to mint storage/partitioned principals.

My understanding of storage principals is very basic. Perhaps bug 543435 creates the initial about:blank with the wrong partitioned principal? Bug 2003449 already indicates that being the case for iframes.

Comment 14

[Vincent Hilla [:vhilla]]

(deleted)

Comment 15

[Donal Meehan [:dmeehan]]

Next week is the final week of nightly for Fx148.

:asuth, what is the current status of the investigation/priority here? Can we expect a fix in time for Fx148?

Comment 16

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

I'm back and actively working the family of issues (there are a few related bugs here stemming from anti-tracking partition key inconsistencies); I'm working on a test reproduction now based on pernosco traces to re-create the iframe hierarchy we see in the wild but which we don't have test coverage for. It is my hope to have the test and fix in the next few days; I think/hope my cold I've been fighting for the past week is waning.

Comment 17

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

I believe I've gotten the fidelity needed from the pernosco traces (ABB to ABA transitions with redirects and process swaps and having filtered out the other noisy iframes) and am now focused on the browser mochitest.

Comment 18

[BugBot [:suhaib / :marco]]

Based on the topcrash criteria, the crash signature linked to this bug is not a topcrash signature anymore.

For more information, please visit BugBot documentation.

Comment 19

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

Attachment: WIP: Bug 2006265 - Reproducing test case

Comment 20

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

I have the test (WIP PR) reproducing both the assertion crash signature experienced here and in bug 1910011 (for this bug, we use window.open() after making sure we clients.claim after an ABB redirect to ABA so the nested ABA is controlled; for that bug we perform a fresh new ABB to ABA redirect which will be controlled). My initial concern about the mIsThirdPartyContext being stale at least was insufficient; currently working a fix to ensure we recompute the client info in the redirect case where we are transitioning the foreign bit.

Comment 21

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

(confusing bugzilla collision on ctrl-r with the new AJAX comment submission resulted in a dupe; obsoleting this)

Comment 22

[Gian-Carlo Pascutto [:gcp]]

STR

1. https://mozilla-hub.atlassian.net/wiki/spaces/RelEng/pages/26607774/Release+Flow
2. Fullscreen the diagram
3. Click Export
4. Click the diagram that appears

Comment 23

[Vincent Hilla [:vhilla]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #22)

STR

1. https://mozilla-hub.atlassian.net/wiki/spaces/RelEng/pages/26607774/Release+Flow
2. Fullscreen the diagram
3. Click Export
4. Click the diagram that appears

Thanks!

• Top level origin A https://mozilla-hub.atlassian.net
• draw.io iframe for the appearing diagram has origin B https://ac.draw.io
• Clicking on that smaller diagram opens a new window
• From a first look, I don't see a third context ABA as in the WIP test

The partition keys are mismatching, see c37 and bug 2010596. Setting network.cookie.cookieBehavior=4 avoids the crash.

I'll create a MRE and compare it to the WIP test.

Comment 24

[Vincent Hilla [:vhilla]]

Ok, I think the STR have the same root cause as crashing with winABAInheritingBC in the WIP test.

But we can simplify this part of the test as it's enough if we register a service worker in AB and open a window from it. The ABA frame is not needed to reproduce this crash.

Comment 25

[Vincent Hilla [:vhilla]]

Interestingly, even if we'd clobber the document whenever the partitioned principal mismatches in CompleteInitialAboutBlankLoad the crash would still occur. So clobbering the document when the inner window is reused won't update the partitioned principal of the window's client source. This is another pre-existing issue. Maybe there is something wrong in how nsGlobalWindowInner::EnsureClientSource verifies the client source matches the new document?

Comment 26

[Vincent Hilla [:vhilla]]

https://pernos.co/debug/p98KrZ1AEQT5jixPqhtXeA/index.html

I think it should be possible to come up with bandaid fixes, if we need something before fx 148 release.

Option A is for nsWindowWatcher::OpenWindowInternal to inherit mPartitionedPrincipalToInheritForAboutBlank from the opener under some conditions. (Edit) Then the new document has a partitioned principal that matches the controller we're trying to inherit. But nsGlobalWindowInner::EnsureClientSource still uses the regular principal for the client source as the new window isn't third-party. That seems unexpected to me.

Option B is to clobber the document also if the partitioned principal mismatches and make sure the client source is updated. See bug 2010596, this try run indicates this would be very common.

Edit: Option C, see comment 5. We could remove the relevant code from CompleteInitialAboutBlankLoad so that a new about:blank window won't be controlled by the opener's SW. That did seem to match Chrome, though goes against the spec (issue).

Comment 27

[Vincent Hilla [:vhilla]]

(In reply to Vincent Hilla [:vhilla] from comment #26)

Edit: Option C, see comment 5. We could remove the relevant code from CompleteInitialAboutBlankLoad so that a new window won't be controlled by the opener's SW. That did seem to match Chrome, though goes against the spec (issue).

https://pernos.co/debug/7JPgcKFPS4QZO3ox6AWF9A/index.html

Oh, I tested the code from C5 in Firefox before bug 543435 and the window.opened about:blank also wasn't service-worker-controlled.

There appears to be code explicitly for this: https://searchfox.org/firefox-main/rev/ba7a7a320649794a9948b32156f5b438fef5ce7a/docshell/base/nsDocShell.cpp#2430-2431

Comment 28

[Vincent Hilla [:vhilla]]

Attachment: Bug 2006265 - Don't inherit ServiceWorker to window.opened about:blank. r=asuth,hsivonen

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/3e79569b7ffe
https://hg.mozilla.org/integration/autoland/rev/02697f5b003f
Don't inherit ServiceWorker to window.opened about:blank. r=asuth,timhuang

Comment 30

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/02697f5b003f

Comment 31

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:asuth, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox148 to wontfix.

For more information, please visit BugBot documentation.

Comment 32

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

(In reply to BugBot [:suhaib / :marco/ :calixte] from comment #31)

The patch landed in nightly and beta is affected.
:asuth, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox148 to wontfix.

For more information, please visit BugBot documentation.

Forwarding the NI to Vincent.

Comment 33

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: Avoid a frequent content crash in beta/release.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: We remove a small amount of new code to restore previous behavior.
• String changes made/needed: None
• Is Android affected?: yes

Comment 34

[Vincent Hilla [:vhilla]]

Attachment: Bug 2006265 - Don't inherit ServiceWorker to window.opened about:blank.

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

Comment 35

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/c7525ad2103b
https://hg.mozilla.org/releases/mozilla-beta/rev/ddc8740efc25