1950418 - sync-about-blank: browser_modifiedclick_inherit_principal.js

Status: RESOLVED DUPLICATE of bug 543435

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

Description

[Vincent Hilla [:vhilla]]

The test browser_modifiedclick_inherit_principal.js fails. It was disabled by the patch, probably to unmask other failures. I don't think it ever passed but can't be sure.

The main problem are the changes to tabbrowser.js within the patch. Instead of calling createAboutBlankDocumentViewer(originPrincipal, we set b.openWindowInfo (createBrowser) on the XULFrameElement. This seems to have two problems:

• The originPrincipal isn't propagated to the content document
• We don't call transmitPermissionsForPrincipal from createAboutBlankDocumentViewer link

What is failing?

The test doesn't resolve the newTabPromise as the js URI isn't loaded. nsDocShell::LoadURI aborts due to NS_ERROR_DOM_BAD_CROSS_ORIGIN_URI in CheckDisallowedJavascriptLoad. The targetPrincipal is a null principal, the triggering principal and the targetPrincipal on central are both for example.com. I suppose the incorrect (?) targetPrincipal comes from mPrincipalToInheritForAboutBlank.

I assume CheckDisallowedJavascriptLoad is correct and the initial about:blank opened from a link click should have a content principal. But maybe this isn't the case? E.g. CheckDisallowedJavascriptLoad should always allow when the targetPrincipal is for the initial about:blank?

Propagating the principal

We set b.openWindowInfo. Binding the browser to the DOM leads to XULFrameElement::LoadSrc, where we create an nsFrameLoader and pass openWindowInfo on to it. From the connectedCallback of the browser, we'll call into nsFrameLoader::TryRemoteBrowserInternal and to ContentParent::CreateBrowser, but openWindowInfo is not propagated. Instead, CreateBrowser always creates a null principal for mPrincipalToInheritForAboutBlank.

I can add aOpenWindowInfo as an argument to CreateBrowser and only if null, create a null principal. We then hit an assertion in IsAboutBlankLoadOntoInitialAboutBlank, due to aPrincipalToInherit being null but the document not having a null principal. This is because of another change from the patch: if openWindowInfo wasn't set on XULFrameElement, we'll create one based on OwnerDoc in LoadSrc. I don't know what led to this change being made, I disabled it for now.

With these two changes, we'll get the right principal to the content document, but a new failure occurs.

transmitPermissionsForPrincipal is missing

Once the principal is propagated to the content document, we'll fail this assert for PermissionAvailable. Without the changes, the initial about:blank will be loaded with system principal and the permission manager always defaults to ALLOW_ACTION. But now we got a content principal and the call to transmitPermissionsForPrincipal via BrowserHost::CreateAboutBlankDocumentViewer is missing.

As transmitPermissionsForPrincipal is exposed via IDL, I tried adding it back in tabbrowser.js, e.g. to where we now create the openWindowInfo in _createBrowserForTab. This would make sense, as the patch moved the createAboutBlankDocumentViewer parts there. But we won't have a MozBrowser.frameLoader.remoteTab till binding the browser, so this doesn't work.

This leaves us with two other options:

• I'm unsure at which point we do have a frameLoader, maybe there is some place in browser-custom-element.js where we could add the call. E.g. somewhere during the connectedCallback that will call into CreateBrowser.
• Or maybe we can transmitPermissionsForPrincipal somewhere in CreateBrowser? I don't see how we could access ContentParent from there or if it already exists. And createAboutBlankDocumentViewer would've also called principalWithMatchingOA(principal, contentPrincipal) but I guess the content principal would've been created during the initial about:blank load. If we only have one about:blank load, we won't yet have a content principal.

Comment 1

[Vincent Hilla [:vhilla]]

:hsivonen can you give some advice on this failure? E.g. why did you create openWindowInfo in LoadSrc? Should the initial about:blank opened from a link click have a content principal? Where could we call transmitPermissionsForPrincipal?

Comment 2

[Henri Sivonen (:hsivonen)]

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

:hsivonen can you give some advice on this failure? E.g. why did you create openWindowInfo in LoadSrc?

I generally tried to create it as early as needed to pass whatever test I was looking at or as early as logically possible. In comment 0 you say "but openWindowInfo is not propagated". If that's new non-propagation, it's most likely an oversight on my part. If that's an old non-propagation, it's most likely an artifact of things just happening to work out that way with the duality of the docshell-generated about:blank and the parser-created about:blank.

Should the initial about:blank opened from a link click have a content principal? Where could we call transmitPermissionsForPrincipal?

Redirecting the question to Gijs.

(I don't recall the name transmitPermissionsForPrincipal from when I was working on the patch, so it's quite possible that I've just missed something important.)

Comment 3

[:Gijs (he/him)]

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

Should the initial about:blank opened from a link click have a content principal?

I would expect it to, yes. I haven't checked but expect this is web-observable today, with something like:

<iframe id="myframe" name="myframe" src="about:blank"></iframe>
<script>
function doSomethingWithFrame() {
  let f = document.getElementById("myframe");
  f.contentDocument.body.append("Hello");
}
</script>

and I imagine the same should work for the case where window.open or <a target="_blank"> opens a popup window or new tab with about:blank (or even with some other URL, but while that's loading the content will be about:blank and will be same origin with the opener, for which it needs the same principal. The opener gets a reference in the window.open case but even in the "human link click" case it may be able to find such a reference with service workers' window clients or somesuch).

Where could we call transmitPermissionsForPrincipal?

https://searchfox.org/mozilla-central/rev/fa11d06fd84c487cb0a135c08af796daa6a56e44/dom/ipc/ContentParent.cpp#3034,3067 looks relevant, off-hand? But someone like Nika would have a better answer. Note that I think the default permissions part here is related to the principal but separate - for the permissions to work properly the document will need the right principal, as well as the permissions. Judging by the name I'd expect the function to deal with making sure the child process has access to the right list of permissions, but not in and of itself do anything to change the principal of a particular document.

Hopefully this helps? My time with about:blank optimizations was a very long time ago so I'm a bit fuzzy on some details...

Comment 4

[Vincent Hilla [:vhilla]]

Thanks, that was indeed helpful.

I didn't fully realize that transmitPermissionsForPrincipal is done by ContentParent with BrowserHost::TransmitPermissionsForPrincipal only forwarding the call to it. ContentParent::InitInternal happens long before CreateBrowser and we don't seem to have any blob urls at that point? But we also call into ContentParent::TransmitPermissionsForPrincipal from WindowGlobalParent::Init, which is called in CreateBrowser right after SendConstructBrowser. I guess we just need to init WindowGlobalParent before constructing the browser. At least this fixes the test and didn't cause regressions.