Open Bug 1597490 Opened 1 year ago Updated 7 months ago

Replace nsIDocShellTreeItem usage in nsXULPopupManager::IsChildOfDocShell, HidePopupsInDocShell in layout/xul/nsXULPopupManager.cpp

Categories

(Core :: XUL, defect, P5)

defect

Tracking

()

People

(Reporter: djvj, Unassigned)

References

(Blocks 1 open bug)

Details

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

In file layout/xul/nsXULPopupManager.cpp

HidePopupsInDocShell is only user of IsChildOfDocShell, and its only use of nsIDocShellTreeItem is to accept a pointer to one as an argument and pass it to IsChildOfDocShell.

Checks if a Document is a child of a given nsIDocShellTreeItem.

Gets the Document’s DocShell, then walks up the DocShell tree, comparing against the given tree-item.

Can be converted entirely to BrowsingContext.

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

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

Neil, can someone on your team please audit this use of the nsIDocShellTreeItem interface in nsXULPopupManager.cpp?

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 → XUL
Flags: needinfo?(enndeakin)
Priority: P3 → --
Summary: Fix uses of nsXULPopupManager::IsChildOfDocShell, HidePopupsInDocShell in layout/xul/nsXULPopupManager.cpp → Audit nsIDocShellTreeItem usage in nsXULPopupManager::IsChildOfDocShell, HidePopupsInDocShell in layout/xul/nsXULPopupManager.cpp

This code is ok for now. It is called during page unload to hide popups for that document. If will be called within subdocuments in other processes when they do page unloading.

So, it only needs access to the parent docshell if it is in the same process.

Flags: needinfo?(enndeakin)

(In reply to Neil Deakin from comment #3)

This code is ok for now. It is called during page unload to hide popups for that document. If will be called within subdocuments in other processes when they do page unloading.

So, it only needs access to the parent docshell if it is in the same process.

Thanks. In that case, it sounds like this bug doesn't need to block Fission MVP.

Fission Milestone: M6 → ---
Summary: Audit nsIDocShellTreeItem usage in nsXULPopupManager::IsChildOfDocShell, HidePopupsInDocShell in layout/xul/nsXULPopupManager.cpp → Replace nsIDocShellTreeItem usage in nsXULPopupManager::IsChildOfDocShell, HidePopupsInDocShell in layout/xul/nsXULPopupManager.cpp
Priority: -- → P5

The severity field is not set for this bug.
:enndeakin, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(enndeakin)
Severity: normal → S4
Flags: needinfo?(enndeakin)
You need to log in before you can comment on or make changes to this bug.