Open Bug 1979032 Opened 17 days ago Updated 2 days ago

sync-about-blank: Skip SetupInheritingPrincipal / ignore null principals to inherit?

Categories

(Core :: DOM: Navigation, task)

Product:
Core
Component:
DOM: Navigation
Type:
task

Tracking

()

People

(Reporter: vhilla, Unassigned, NeedInfo)

References

(Blocks 1 open bug)

Details

To my knowledge, the last open question for bug 543435 is how we handle principals, especially null principals. Tests are currently all passing.

The tl;dr is that in DoURILoad for about:blank, we early return by staying on the already eagerly created document. But we fail to assert aLoadState->PrincipalToInherit()->Equals(GetDocument()->GetPrincipal()) in the case where both are null principals. SetupInheritingPrincipal will naturally create a non-equal null principal to the ones we create in e.g. ContentParent::CreateBrowser or nsFrameLoader::MaybeCreateDocShell. MaybeHandleSubframeHistory might also retreive a null principal from SH that doesn't equal the one on the eagerly created document.

For some background, consider what happens if we create a new docshell and load about:blank. Right now on central, the docshell starts out without document. If one tries to access it, a transient about:blank will be generated lazily while the actual about:blank loads asynchronous. When this async load finishes, it'll replace the transient document. Afaik this async load does some special things to take only one tick, but it largely follows the normal load path.

Our approach with bug 543435 is to create the about:blank eagerly when a docshell is initialized. And if we then reach DoURILoad for about:blank, we don't start an async load but instead commit to the already existing document. This has largely two consequences

  • DoURILoad returns early by calling CompleteInitialAboutBlankLoad. We pulled all kinds of things that would happen during the load path into this method, like firing a load event.
  • Certain state needs to be known at DocShell initialization and this first about:blank document isn't transient anymore, it's what we stay on. We use nsOpenWindowInfo for that.

This was mostly the work of :hsivonen, apologies for my vague explanation.

This state that needs to be known at DocShell initialization includes the principal to be inherited. My understanding of the related code is not sufficient to determine whether we handle that correctly and what might need changing. I think the most relevant places where nsOpenWindowInfo::mPrincipalToInheritForAboutBlank is set are ContentParent::CreateBrowser and nsFrameLoader::MaybeCreateDocShell. I believe before our changes, the principal would've come from nsDocShellLoadState::SetupInheritingPrincipal, or via MaybeHandleSubframeHistory from session history, or maybe other places.

The question is whether the null principals assigned to mPrincipalToInheritForAboutBlank are good enough and we can live with the following assertion in https://phabricator.services.mozilla.com/D155376 nsDocShell.cpp. I see SetupInheritingPrincipals sets some precursor and I don't know the implications of not doing that.

 MOZ_ASSERT_IF(
      isAboutBlankLoadOntoInitialAboutBlank && inheritPrincipal,
      aLoadState->PrincipalToInherit()->Equals(GetDocument()->GetPrincipal()) ||
          (aLoadState->PrincipalToInherit()->GetIsNullPrincipal() &&
           GetDocument()->GetPrincipal()->GetIsNullPrincipal()));

I suppose the risk is some documents being restricted from access due to having different null principals. Maybe this is unlikely to occur on the web?

The only web-observable change I'm aware of is navigating back to data:text/html,<body onunload=''><iframe></iframe></body>. On central, the parent will get a new null principal while the child gets it's principal restored from session history. With bug 1955324, we don't restore null principals and therefore both get the same new null principal. This seems like a good thing.

Blocks: 1955324
Severity: -- → N/A

Leaving a ni? for myself so I can look into the situation here a bit more.

Flags: needinfo?(nika)

Ugh, the much more common case than data URIs might be sandboxed iframes.

ifr = document.createElement("iframe")
ifr.sandbox = ""
ifr.onload = console.log
document.body.appendChild(ifr)

This will hit the (edit: more strict) assert. Interesting that no test does that. If we skip the initial about:blank handling (as discussed) for non-matching principals, sandboxed iframes will have the old async about:blank behavior.

In the sandboxed case, the conflicting principals come from nsDocShellLoadState::SetupInheritingPrincipal vs nsDocShell::CreateAboutBlankDocumentViewer ln6741.

You need to log in before you can comment on or make changes to this bug.