Closed Bug 1597508 Opened 1 year ago Closed 7 months ago

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

Categories

(Core :: Widget: Cocoa, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla77
Fission Milestone M6
Tracking Status
firefox77 --- fixed

People

(Reporter: djvj, Assigned: spohl)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file)

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:

This is either dead code, or very likely to be chrome-only code.

Note: Ping someone on MacOS dev about this.

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

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

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

Component: DOM: Navigation → Widget: Cocoa
Priority: P3 → --
Summary: Fix uses of GetNativeWindowPointerFromDOMWindow in toolkit/xre/nsNativeAppSupportCocoa.mm → Audit nsIDocShellTreeItem usage in GetNativeWindowPointerFromDOMWindow in toolkit/xre/nsNativeAppSupportCocoa.mm

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

This can be called by the OS[1]. Does this help?

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

Flags: needinfo?(kvijayan)

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

(In reply to Stephen A Pohl [:spohl] from comment #3)

Called from Cocoa code here:

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.

Flags: needinfo?(spohl.mozilla.bugs)

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

Flags: needinfo?(kvijayan)

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

(In reply to Stephen A Pohl [:spohl] from comment #3)

Called from Cocoa code here:

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.

Flags: needinfo?(spohl.mozilla.bugs)

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

Depends on: 1597509
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Pushed by spohl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fc341103b84a
Use WidgetUtils::DOMWindowToWidget to determine native window during reopen of Firefox on macOS. r=farre
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77
You need to log in before you can comment on or make changes to this bug.