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

Status: NEW

(Core :: XUL, defect, P3)

Description

[Kannan Vijayan [:djvj]]

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.

Comment 1

[Chris Peterson [:cpeterson]]

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

Comment 2

[Chris Peterson [:cpeterson]]

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

Comment 3

[Neil Deakin]

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.

Comment 4

[Chris Peterson [:cpeterson]]

(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?

Comment 5

[Henri Sivonen (:hsivonen)]

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

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

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

For more information, please visit auto_nag documentation.