Closed Bug 1597496 Opened 1 year ago Closed 5 months ago

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

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1631405
Fission Milestone Future

People

(Reporter: djvj, Unassigned)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [rm-docshell-tree-item:sync-state])

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):

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.

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

Fission Milestone: --- → M6
Priority: -- → P3

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

Component: DOM: Navigation → Security: PSM
Flags: needinfo?(dkeeler)
Priority: P3 → --
Summary: Fix uses of nsSecureBrowserUIImpl::PrepareForContentChecks in security/manager/ssl/nsSecureBrowserUIImpl.cpp → Audit nsIDocShellTreeItem usage in nsSecureBrowserUIImpl::PrepareForContentChecks in security/manager/ssl/nsSecureBrowserUIImpl.cpp

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)

Flags: needinfo?(dkeeler) → needinfo?(ehsan)

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

Fission Milestone: M6 → M7

(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).

Fission Milestone: M7 → Future
Summary: Audit nsIDocShellTreeItem usage in nsSecureBrowserUIImpl::PrepareForContentChecks in security/manager/ssl/nsSecureBrowserUIImpl.cpp → Remove nsIDocShellTreeItem usage in nsSecureBrowserUIImpl::PrepareForContentChecks in security/manager/ssl/nsSecureBrowserUIImpl.cpp

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

:mattwoodrow, can we dup?

Flags: needinfo?(matt.woodrow)
Status: NEW → RESOLVED
Closed: 5 months ago
Flags: needinfo?(matt.woodrow)
Resolution: --- → DUPLICATE
Duplicate of bug: 1631405
Flags: needinfo?(ehsan)
You need to log in before you can comment on or make changes to this bug.