1580619 - Audit usage of nsIDocShellTreeItem in nsDocShell::GetInheritedPrincipal

Status: NEW

(Core :: DOM: Navigation, task, P2)

Description

[Kannan Vijayan [:djvj]]

https://searchfox.org/mozilla-central/rev/a777ff11b6d700a698c61e5bd17e73b044304494/docshell/base/nsDocShell.cpp#9694

I spent a while on this - mostly digging into what the semantics of principal inheritance are and when it happens.

So this method might reach into the parent docshell via the nsIDocShellTreeItem api and get the internal document, and obtain a principal from that document to return.

This object (nsIPrincipal) obviously cannot cross process boundaries. This cannot be handled by some IPC as the principal object itself is returned, saved, and used directly by the caller (along certain codepaths).

Further reading into the semantics of principals seems to indicate that inheriting principals should only happen between documents which should already been in the same process due to sharing a security context (implied by the ability to inherit principals).

If that's true, then this code should be basically correct already: we don't want to consider out-of-process parents.

Comment 1

[Kannan Vijayan [:djvj]]

Nika: Is the above assessment accurate?

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Yup - sounds about correct. We should probably change it to explicitly use BrowsingContext, but principal inheritance for OOP iframes is handled through a separate mechanism.

Comment 3

[Chris Peterson [:cpeterson]]

Kannan says replacing nsIDocShellTreeItem calls should block enabling Fission in Nightly (M6).

Comment 4

[Chris Peterson [:cpeterson]]

We need to audit this use of the nsIDocShellTreeItem interface. With Fission enabled, Documents and nsDocShells for related frames, such as subframes and parent documents, may not be available within the current process and the corresponding nsIDocShellTreeItem methods will return null.

If this code is broken with Fission, fixing it blocks enabling Fission is Nightly.

If this code works as-is with Fission, we don't need to remove this usage of nsIDocShellTreeItem until when we remove nsIDocShellTreeItem entirely (bug 1607591) after we ship Fission MVP.

Fission documentation about replacing nsIDocShellTree Item:
https://wiki.mozilla.org/Project_Fission/DocShell_Tree_Replace

:farre's presentation with examples of replacing nsIDocShellTreeItem with BrowsingContext, WindowContext, SyncedContexts, and BrowsingContextGroup:
https://docs.google.com/presentation/d/1K4j6ngty64TZjJNS5qH-MBoOm3TI2dJedVsbH8jUhKE/edit#slide=id.g6e35225e5d_1_264

Comment 5

[Chris Peterson [:cpeterson]]

Auditing whether this use of nsIDocShellTreeItem breaks when Fission is enabled blocks Fission Nightly.

Comment 6

[Neha Kochar [:neha]]

This is already not broken for Fission.