Closed Bug 1597469 Opened 1 year ago Closed 3 months ago

Audit nsIDocShellTreeItem usage in nsDocumentViewer::PermitUnloadInternal in layout/base/nsDocumentViewer.cpp

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME
Fission Milestone M6c

People

(Reporter: djvj, Unassigned)

References

(Blocks 1 open bug)

Details

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

In file layout/base/nsDocumentViewer.cpp

Called from nsIContentViewer::permitUnload XPCOM API

Used for firing beforeUnload handlers for unloading docs.

Iterates over all children of existing DocShell, finding child ContentViewers for each child doc.

Calls PermitUnloadInternal on them.

PermitUnload is a pretty involved method that can run script, dispatch events, etc. on the child document.

beforeUnload handling is special and does not care too much about performance.

IPC should be fine here when children cross process boundaries.

Sync IPC is easiest to do because this is conceptually a synchronous action (traverse entire tree and fire any necessary beforeUnload events).

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

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

Moving to Layout.

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 → Layout
Priority: P3 → --
Summary: Fix uses of nsDocumentViewer::PermitUnloadInternal in layout/base/nsDocumentViewer.cpp → Audit nsIDocShellTreeItem usage in nsDocumentViewer::PermitUnloadInternal in layout/base/nsDocumentViewer.cpp
Whiteboard: [rm-docshell-tree-item:hard] → [rm-docshell-tree-item:hard] [layout:triage-discuss]

The priority flag is not set for this bug.
:jwatt, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(jwatt)

Auditing whether this use of nsIDocShellTreeItem breaks when Fission is enabled blocks Fission Nightly.

Fission Milestone: M6 → M6c
Whiteboard: [rm-docshell-tree-item:hard] [layout:triage-discuss] → [rm-docshell-tree-item:hard]

I've confirmed that beforeunload event (which is the only one purpose of nsDocumentViewer::PermitUnloadInternal) is properly handled in OOP iframe (I believe it's been handled properly since bug 1533949, thanks to mconley!).

I found an issue that the event is not fired when the document is reloaded (bug 1655866), but as far as I can tell the case should also be handled in the parent process, so nothing to be changed in nsDocumentViewer::PermitUnloadInternal, I think.

Closing WORKSFORME.

Status: NEW → RESOLVED
Closed: 3 months ago
Flags: needinfo?(jwatt)
Resolution: --- → WORKSFORME
See Also: → 1533949
You need to log in before you can comment on or make changes to this bug.