Open Bug 1597487 Opened 5 years ago Updated 5 years ago

Remove nsIDocShellTreeItem usage in nsMenuPopupFrame::CreateWidgetForView in layout/xul/nsMenuPopupFrame.cpp

Categories

(Core :: XUL, defect, P3)

defect

Tracking

()

Fission Milestone Future

People

(Reporter: djvj, Unassigned)

References

(Blocks 1 open bug)

Details

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

In file layout/xul/nsMenuPopupFrame.cpp

Gets PresContext()->DocShell()->TreeOwner(), QueryInterfaces it to nsIBaseWindow, and gets the main widget for the window, to use as a parent widget.

I’m pretty sure this is all supposed to be chrome code anyway, and it’s guaranteed that everything is in-process.

Fix to use BrowsingContext to obtain the WindowContext somehow, and assert that it’s for an in-process window.

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

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

Neil, can you (or someone else on your team) please audit this use of the nsIDocShellTreeItem interface in nsMenuPopupFrame?

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

Component: DOM: Navigation → XUL
Flags: needinfo?(enndeakin)
Priority: P3 → --
Summary: Fix uses of nsMenuPopupFrame::CreateWidgetForView in layout/xul/nsMenuPopupFrame.cpp → Audit nsIDocShellTreeItem usage in nsMenuPopupFrame::CreateWidgetForView in layout/xul/nsMenuPopupFrame.cpp

This code is used to get the parent native widget and is only relevant in a parent process. In a content process, it should just use null.

Content processes shouldn't be able to create popups at all, but I recall hsivonen I think finding some case where they were during his testing, but I don't know what cases those are.

Flags: needinfo?(enndeakin)

(In reply to Neil Deakin from comment #3)

This code is used to get the parent native widget and is only relevant in a parent process. In a content process, it should just use null.

Content processes shouldn't be able to create popups at all, but I recall hsivonen I think finding some case where they were during his testing, but I don't know what cases those are.

Henri, do you recall in which cases content processes might be able to create popups? If so, does fixing those cases need to block Fission MVP?

Flags: needinfo?(hsivonen)
Priority: -- → P3

(In reply to Chris Peterson [:cpeterson] from comment #4)

(In reply to Neil Deakin from comment #3)

This code is used to get the parent native widget and is only relevant in a parent process. In a content process, it should just use null.

Content processes shouldn't be able to create popups at all, but I recall hsivonen I think finding some case where they were during his testing, but I don't know what cases those are.

Henri, do you recall in which cases content processes might be able to create popups?

It's dom/xul/test/test_bug486990.xhtml (filed as bug 1619240).

If so, does fixing those cases need to block Fission MVP?

The case I saw doesn't need to block, because I added a BrowsingContext code path. Not sure about the code path pointed to by this bug.

Flags: needinfo?(hsivonen)
Fission Milestone: M6 → Future
Summary: Audit nsIDocShellTreeItem usage in nsMenuPopupFrame::CreateWidgetForView in layout/xul/nsMenuPopupFrame.cpp → Remove nsIDocShellTreeItem usage in nsMenuPopupFrame::CreateWidgetForView in layout/xul/nsMenuPopupFrame.cpp

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.