Closed Bug 1585074 Opened 1 year ago Closed 3 months ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
82 Branch
Fission Milestone M6b
Tracking Status
firefox82 --- fixed

People

(Reporter: djvj, Assigned: smacleod)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file)

Fission Milestone: --- → M5
Priority: -- → P2
Whiteboard: [rm-docshell-tree-item:sync-state]
Whiteboard: [rm-docshell-tree-item:sync-state] → [rm-docshell-tree-item:validate]
Fission Milestone: M5 → Future

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

Fission Milestone: Future → M6

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

Priority: P2 → P3
Summary: Fix usage of nsIDocShellTreeItem in PendingFullscreenChangeList::Iterator::Iterator → Audit usage of nsIDocShellTreeItem in PendingFullscreenChangeList::Iterator::Iterator

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

Fission Milestone: M6 → M6b

Alphan, what functionality does it affect?

Assignee: nobody → alchen
Status: NEW → ASSIGNED

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.

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

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.

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

Assignee: alchen → smacleod
Pushed by smacleod@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b3e160c2f110
Audit usage of nsIDocShellTreeItem in PendingFullscreenChangeList::Iterator::Iterator r=smaug,nika,xidorn
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch
You need to log in before you can comment on or make changes to this bug.