1597496 - Remove nsIDocShellTreeItem usage in nsSecureBrowserUIImpl::PrepareForContentChecks in security/manager/ssl/nsSecureBrowserUIImpl.cpp

Status: RESOLVED DUPLICATE of bug 1631405

(Core :: Security: PSM, defect)

Description

[Kannan Vijayan [:djvj]]

In file security/manager/ssl/nsSecureBrowserUIImpl.cpp

Checks ItemType of existing DocShell, and if it is content, returns the SameType root DocShell’s Document.

Related to mixed content BrowserContext state bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1580678

Returned documents are used here (see method calls on doc):

• https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/security/manager/ssl/nsSecureBrowserUIImpl.cpp#155
• https://searchfox.org/mozilla-central/source/security/manager/ssl/nsSecureBrowserUIImpl.cpp#184

Much of the state seems to be sensitive state that can be used to fingerprint out-of-process documents, and may not be easy to sync up.

Most uses seem to not be that perf sensitive, content-blocking checks on web-pages are rare.

Either change this to replicate state in BrowsingContext and use it directly (ask nika or someone else whether the state being accessed here is syncable), or change implementation to use BrowsingContext as is and change users to use IPC when root is out of process.

Comment 1

[Chris Peterson [:cpeterson]]

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

Comment 2

[Chris Peterson [:cpeterson]]

Dana, can someone on your team please audit this use of the nsIDocShellTreeItem interface in nsSecureBrowserUIImpl?

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 and your team should prioritize (or delegate) the fix accordingly.

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 3

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Ehsan, do we even need to use nsIDocShellTreeItem in nsSecureBrowserUIImpl::PrepareForContentChecks? It doesn't make sense to me that we would need to check the mixed content state of the root docshell if the current document isn't the root (also, removing that code doesn't seem to cause tests to fail: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d58a6c9a6bca48d4b5823391ebc2c22ec5088df)

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I don't think this blocks Fission in Nightly - from my reading the current implementation will safely no-op.

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #4)

I don't think this blocks Fission in Nightly - from my reading the current implementation will safely no-op.

Thanks for checking! In that case, we can postpone this bug until Fission Future (clean up work after we ship Fission MVP).

Comment 6

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

It looks like this might have been fixed as part of bug 1631405.

:mattwoodrow, can we dup?