Audit nsIDocShellTreeItem usage in GetNativeWindowPointerFromDOMWindow in toolkit/xre/nsNativeAppSupportCocoa.mm
Categories
(Core :: Widget: Cocoa, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox77 | --- | fixed |
People
(Reporter: djvj, Assigned: spohl)
References
(Blocks 1 open bug)
Details
(Whiteboard: [rm-docshell-tree-item:validate])
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
In file toolkit/xre/nsNativeAppSupportCocoa.mm
Gets the DocShellTreeItem from a window by QueryInterfacing it into a nsIWebNavigation and from there into an nsIDocShellTreeItem.
Gets the tree-owner from there and gets the base-window for the tree owner, and gets its main widget to obtain the native window.
Called from Cocoa code here:
- https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/toolkit/xre/MacApplicationDelegate.mm#174
-
- (BOOL) applicationShouldHandleReopen: (NSApplication*)theApp hasVisibleWindows: (BOOL)flag
- Can’t find any callers to that though.
This is either dead code, or very likely to be chrome-only code.
Note: Ping someone on MacOS dev about this.
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
|
||
Moving to Widget: Cocoa.
Please audit this use of the nsIDocShellTreeItem interface. 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 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
Assignee | ||
Comment 3•5 years ago
|
||
(In reply to Kannan Vijayan [:djvj] from comment #0)
In file toolkit/xre/nsNativeAppSupportCocoa.mm
Gets the DocShellTreeItem from a window by QueryInterfacing it into a nsIWebNavigation and from there into an nsIDocShellTreeItem.
Gets the tree-owner from there and gets the base-window for the tree owner, and gets its main widget to obtain the native window.
Called from Cocoa code here:
- https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/toolkit/xre/MacApplicationDelegate.mm#174
- (BOOL) applicationShouldHandleReopen: (NSApplication*)theApp hasVisibleWindows: (BOOL)flag
- Can’t find any callers to that though.
This can be called by the OS[1]. Does this help?
Reporter | ||
Comment 4•5 years ago
|
||
Certainly! Good to know where that was getting invoked from.
Comment 5•5 years ago
|
||
(In reply to Stephen A Pohl [:spohl] from comment #3)
Called from Cocoa code here:
- https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/toolkit/xre/MacApplicationDelegate.mm#174
- (BOOL) applicationShouldHandleReopen: (NSApplication*)theApp hasVisibleWindows: (BOOL)flag
- Can’t find any callers to that though.
This can be called by the OS[1]. Does this help?
Stephen, does this GetNativeWindowPointerFromDOMWindow code break with Fission enabled?
If this code is broken with Fission, fixing it blocks enabling Fission is Nightly and a Cocoa widget engineer 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.
Reporter | ||
Comment 6•5 years ago
|
||
Removing needinfo as I'm not working on the Fission project, and am back to JS engine work.
Assignee | ||
Comment 7•5 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #5)
(In reply to Stephen A Pohl [:spohl] from comment #3)
Called from Cocoa code here:
- https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/toolkit/xre/MacApplicationDelegate.mm#174
- (BOOL) applicationShouldHandleReopen: (NSApplication*)theApp hasVisibleWindows: (BOOL)flag
- Can’t find any callers to that though.
This can be called by the OS[1]. Does this help?
Stephen, does this GetNativeWindowPointerFromDOMWindow code break with Fission enabled?
I wouldn't know off-hand.
If this code is broken with Fission, fixing it blocks enabling Fission is Nightly and a Cocoa widget engineer should prioritize (or delegate) the fix accordingly.
I question the approach here. Cocoa widget engineers with spare cycles are hard to come by. Shouldn't we designate one or two experts in nsIDocShellTreeItem
replacements and have them go through the code base to make these replacements across the board? The code here isn't doing anything that isn't being done elsewhere in the code base. If it helps move things along, I'm going to post a patch that uses WidgetUtils::DOMWindowToWidget
. Once someone fixes the use of nsIDocShellTreeItem
there, this bug will disappear too.
Assignee | ||
Comment 8•5 years ago
|
||
Comment 9•5 years ago
|
||
(In reply to Stephen A Pohl [:spohl] from comment #7)
I question the approach here. Cocoa widget engineers with spare cycles are hard to come by. Shouldn't we designate one or two experts in
nsIDocShellTreeItem
replacements and have them go through the code base to make these replacements across the board? The code here isn't doing anything that isn't being done elsewhere in the code base.
I see what you mean. We have a couple DOM Fission engineers looking at these bugs, but we have over 150 functions (and bugs) using nsIDocShellTreeItem and some of them require experience with the surrounding code that the DOM engineers don't have. Non-DOM engineers will eventually need to become familiar with the new BrowsingContext/WindowContext APIs used in their code. Delegating these nsIDocShellTreeItem bugs to module owners, where feasible, is one way we're doing that.
If it helps move things along, I'm going to post a patch that uses
WidgetUtils::DOMWindowToWidget
. Once someone fixes the use ofnsIDocShellTreeItem
there, this bug will disappear too.
Thanks! With that patch, we can resolve this bug as fixed. DOMWindowToWidget will get fixed (by someone TBD) in widget bug 1597509.
Updated•5 years ago
|
Comment 10•5 years ago
|
||
Comment 11•5 years ago
|
||
bugherder |
Description
•