1597508 - Audit nsIDocShellTreeItem usage in GetNativeWindowPointerFromDOMWindow in toolkit/xre/nsNativeAppSupportCocoa.mm

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Kannan Vijayan [:djvj]]

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

[Chris Peterson [:cpeterson]]

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

Comment 2

[Chris Peterson [:cpeterson]]

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

Comment 3

[Stephen A Pohl [:spohl]]

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

[1] https://developer.apple.com/documentation/appkit/nsapplicationdelegate/1428638-applicationshouldhandlereopen?language=objc

Comment 4

[Kannan Vijayan [:djvj]]

Certainly! Good to know where that was getting invoked from.

Comment 5

[Chris Peterson [:cpeterson]]

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

Comment 6

[Kannan Vijayan [:djvj]]

Removing needinfo as I'm not working on the Fission project, and am back to JS engine work.

Comment 7

[Stephen A Pohl [:spohl]]

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

Comment 8

[Stephen A Pohl [:spohl]]

Attachment: Bug 1597508: Use WidgetUtils::DOMWindowToWidget to determine native window during reopen of Firefox on macOS.

Comment 9

[Chris Peterson [:cpeterson]]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc341103b84a
Use WidgetUtils::DOMWindowToWidget to determine native window during reopen of Firefox on macOS. r=farre

Comment 11

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/fc341103b84a