Open Bug 1580619 Opened 2 years ago Updated 1 year ago

Audit usage of nsIDocShellTreeItem in nsDocShell::GetInheritedPrincipal

Categories

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

task

Tracking

()

Fission Milestone Future

People

(Reporter: djvj, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [rm-docshell-tree-item:validate])

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.

Nika: Is the above assessment accurate?

Flags: needinfo?(nika)

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.

Flags: needinfo?(nika)
Fission Milestone: --- → M5
Priority: -- → P2
Whiteboard: [rm-docshell-tree-item:validate]
Fission Milestone: M5 → Future

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

Fission Milestone: Future → M6

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

Summary: Fix usage of nsIDocShellTreeItem in nsDocShell::GetInheritedPrincipal → Audit usage of nsIDocShellTreeItem in nsDocShell::GetInheritedPrincipal

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

Fission Milestone: M6 → M6b

This is already not broken for Fission.

Type: defect → task
Fission Milestone: M6b → Future
You need to log in before you can comment on or make changes to this bug.