Remove nsIDocShellTreeItem usage in nsMenuPopupFrame::CreateWidgetForView in layout/xul/nsMenuPopupFrame.cpp
Categories
(Core :: XUL, defect, P3)
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.
Comment 1•5 years ago
|
||
Kannan says replacing nsIDocShellTreeItem calls should block enabling Fission in Nightly (M6).
Updated•5 years ago
|
Comment 2•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
(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?
Updated•5 years ago
|
(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.
Updated•5 years ago
|
Updated•5 years ago
|
Comment 6•5 years ago
|
||
The severity field is not set for this bug.
:enndeakin, could you have a look please?
For more information, please visit auto_nag documentation.
Updated•5 years ago
|
Description
•