1948216 - sync-about-blank: initial about:blank from CreateBrowser does not have a FirstPartyDomain

Status: ASSIGNED

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

Description

[Vincent Hilla [:vhilla]]

The patch from Bug 543435 failed the subtest test_remote_window_open_aboutBlank in test browser_firstPartyIsolation_aboutPages.js and several related ones. Paraphrasing the log:

PASS should be a remote browser
INFO origin moz-nullprincipal:{9865c259-3a40-44b0-99d3-7994f00bee66}
PASS The principal of remote about:blank should be a NullPrincipal.
FAIL remote about:blank should have firstPartyDomain set to ... - got "", expected "9865c259-3a40-44b0-99d3-7994f00bee66.mozilla

From pernosco, we can see that we call BasePrincipal::GetOrigin and mOriginNoSuffix is empty. On central (pernosco), we would've created the right principal from nsDocShell::LoadURI in nsDocShellLoadState::SetupInheritingPrincipal and propagated it to the Document during the channel load from CreateDocument, StartDocumentLoad, Reset, ResetToURI.

With the patch, we still do LoadURI and SetupInheritingPrincipal, but skip the channel load and rather commit to the already existing initial about:blank in DoURILoad. From :smaug comment 188 we want to avoid changing the document principal and would prefer to have the right principal from the beginning.

So this leaves us with setting firstPartyDomain during CreateBrowser. Within the patch, :hsivonen added the mPrincipalToInheritForAboutBlank to nsOpenWindowInfo, which is set in ContentParent::CreateBrowser and then propagated to the document in the content process.

1. mPrincipalToInheritForAboutBlank is largely const, but we could create a new principal based on it somewhere in the child.
2. ContentParent::CreateBrowser could copy aBrowsingContext->OriginAttributesRef() and SetFirstPartyDomain when creating mPrincipalToInheritForAboutBlank. This will trigger this assert, but I didn't see test failures when ignoring that assert.
3. When OriginAttributesRef is initialized during nsFrameLoader::EnsureBrowsingContextAttached, or somewhere else during browsing context creation, we could initialize the firstPartyDomain. Though we won't have mPrincipalToInheritForAboutBlank jet whose origin is used to generate the suffix string. We could use ABOUT_URI_FIRST_PARTY_DOMAIN see c194, but this doesn't align with the test.

I favor the second option, but the question remains what to do about the assertion. Remove? Exclude about:blank? Or should we update the bc origin attributes?

Comment 1

[Vincent Hilla [:vhilla]]

:baku you wanted to take a look at the assert, I hope I gave a good explanation of the context. You introduced the assert in Bug 1271516.

Comment 2

[Andrea Marchesini [:baku]]

Redirect the NI. Tim, in case, redirect this NI back to me.

Comment 3

[Tim Huang[:timhuang]]

Sorry for the late response.

I think the assertion is correct because we want to ensure the originAttributes align between the nodePrincipal of the document and the browsing context. Given that the remote about:blank page is loaded with a NullPrincipal, we should call SetFirstPartyDomain() to properly initiate the firstPartyDomain when creating the principal for the about:blank page. A similar example is https://searchfox.org/mozilla-central/rev/5c3577d8865aa2d61fa89fd453d803b7c4f8b4ed/docshell/base/nsDocShellLoadState.cpp#1082-1089

Comment 4

[Vincent Hilla [:vhilla]]

That seems like what I suggest as option 2, my code in ContentParent::CreateBrowser looks very similar:

 if (!openWindowInfo) {
    nsCOMPtr<nsIURI> nullPrincipalURI = NullPrincipal::CreateURI(nullptr);
    OriginAttributes attrs(aBrowsingContext->OriginAttributesRef());
    attrs.SetFirstPartyDomain(true, nullPrincipalURI);
    nsCOMPtr<nsIPrincipal> initialPrincipal =
        NullPrincipal::Create(attrs, nullPrincipalURI);

but this triggers the assert. I don't understand how setting nsDocShellLoadState::mPrincipalToInherit in your example causes BrowsingContext::mOriginAttributes to be updated, or if it does at all. I cannot aBrowsingContext->SetOriginAttributes as it is disallowed here for content browsing contexts.

Comment 5

[Vincent Hilla [:vhilla]]

I looked at an older pernosco trace for firstPartyDomain on central. I don't see calls to BrowsingContext::SetOriginAttributes for the newly created origin attributes with firstPartyDomain from nsDocShellLoadState::SetupInheritingPrincipal.

On central, we'll RecvCreateBrowser and CreateAboutBlankDocumentViewer without firstPartyDomain, thus passing the assert. Then we learn that about:blank is our navigation destination and go through the regular load path to load a new about:blank document. We'll SetupInheritingPrincipal, setup firstPartyDomain and propagate that principal to the document. These origin attributes are not propagated to the browsing context (or I'm missing something?), but the load path also doesn't call into CreateAboutBlankDocumentViewer, so the assert is not hit.

With the patch, we'll go through RecvCreateBrowser the same way. But once we know about:blank is our navigation destination, we commit synchronously to the initial document that doesn't have a firstPartyDomain yet, but also don't update the document principal. So now we are back at the initial problem: We want the document to have the right principal from the beginning, so have to set firstPartyDomain already during CreateBrowser. But then we'll have diverging origin attributes already in CreateAboutBlankDocumentViewer and not only later during the async load.

Comment 6

[Vincent Hilla [:vhilla]]

From discussion with Tim.

We might be able to create the principal and set the first party domain on the BC OAs when they are created in EnsureBrowsingContextAttached. Propagating that principal to CreateBrowser and creating it under the right circumstances might not be too clean. It might work to set mOpenWindowInfo there.

After normal loads, the first party domain will be out of sync between top-level BC and document, as we cannot know the actual first party domain before the load finishes. If this is now also the case for the initial about:blank, we could change the assertion to use EqualsIgnoringFPD when top level.

Comment 7

[Vincent Hilla [:vhilla]]

From further discussion and testing, for now we don't want a FPD for this top-level about:blank.

While it is possible to use mOpenWindowInfo and set the FPD on the BC when creating the OAs, consecutive navigations away from about:blank won't cause the FPD to be updated on the BC. This causes e.g. browser_firstPartyDomain.js to fail, as addTab(example.com) will first load an initial about:blank before loading example.com. So we end up with a document that has example.com as FPD while the BC will have the FPD from the null principal from the initial about:blank load. It might make sense to have the BC FPD be non-const and keep it in sync.