1585074 - Audit usage of nsIDocShellTreeItem in PendingFullscreenChangeList::Iterator::Iterator

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P3)

Description

[Kannan Vijayan [:djvj]]

https://searchfox.org/mozilla-central/rev/45f30e1d19bde27bf07e47a0a5dd0962dd27ba18/dom/base/Document.cpp#12958

This is related to full-screen code, and is being fixed in bug https://bugzilla.mozilla.org/show_bug.cgi?id=1505916

Comment 1

[Chris Peterson [:cpeterson]]

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

Comment 2

[Chris Peterson [:cpeterson]]

Fission fullscreen bug 1505916 was fixed in Firefox 71 (October 2019), but PendingFullscreenChangeList still uses nsIDocShellTreeItem.

We need to 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 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

[Chris Peterson [:cpeterson]]

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

Comment 4

[Neha Kochar [:neha]]

Alphan, what functionality does it affect?

Comment 5

[Alphan Chen [:alchen]]

Here is another usage of GetInProcessRootTreeItem in fullscreen code.
https://searchfox.org/mozilla-central/rev/03794edd6edcc3fc1e222de966cb27256ce08998/dom/base/Document.cpp#13765,13854
I will also try to rewrite it in this bug.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Alphan Chen [:alchen] from comment #5)

Here is another usage of GetInProcessRootTreeItem in fullscreen code.
https://searchfox.org/mozilla-central/rev/03794edd6edcc3fc1e222de966cb27256ce08998/dom/base/Document.cpp#13765,13854
I will also try to also it in this bug.

FYI, the function GetRootWindow in Document.cpp gets called only in the parent process since bug 1585078.

Comment 7

[Alphan Chen [:alchen]]

Attachment: Bug 1585074 - Audit usage of nsIDocShellTreeItem in PendingFullscreenChangeList::Iterator::Iterator

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

The goal of PendingFullscreenChangeList is to get a list of fullscreen changes which are initiated under the same top level window. It's iterated when we get informed that widget change has finished, and process all the changes within the same window.

So as far as it still goes through all fullscreen changes within the same top level window in the fission process, it would be enough. There is no point to exactly preserve what it does.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

i.e. it should go through all fullscreen changes which share the same widget fullscreen notifications from the parent process.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3e160c2f110
Audit usage of nsIDocShellTreeItem in PendingFullscreenChangeList::Iterator::Iterator r=smaug,nika,xidorn

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

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