1594421 - [meta] Delete nsDocShell::GetAllDocShellsInSubtree() and fix its callers

Status: RESOLVED FIXED

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

Description

[Kannan Vijayan [:djvj]]

The short-term change to this is just to move tree traversal entirely over to using BrowsingContext, and have the iterator skip over any out-of-process nodes when iterating children.

Long-term, this whole iterator is probably worthwhile to replace with some iterator over BrowsingContext that doesn't skip over any children at all, and leaves it up to the user to determine what to do when process boundaries are crossed.

As it stands, this iterator is very limited in functionality: it will act as if the tree terminates at process boundaries, and force the caller to use a side-path to check that property. At that point, it's better design just to have the caller use an enumerator protocol that walks the entire tree (in-process or not) via BrowsingContext, and handle the process-boundary switches in-line with the iteration.

Comment 1

[Chris Peterson [:cpeterson]]

Tentatively moving all bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to the "?" triage milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732

Comment 2

[Chris Peterson [:cpeterson]]

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

[Chris Peterson [:cpeterson]]

GetAllDocShellsInSubtree() uses nsDocShellEnumerator internally.

Callers should use BrowsingContexts instead.

Comment 5

[Neha Kochar [:neha]]

Kashav, please audit the callers and file sub-bugs for them.

Comment 6

[Neha Kochar [:neha]]

Moving to M7 because of dependencies targeted for M7.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

It appears as though the remaining uses of this API are largely in extensions (https://searchfox.org/mozilla-central/rev/69babd862de70cabfa1d0a369d38e4881bd41e4d/toolkit/components/extensions/ExtensionContent.jsm#1208, https://searchfox.org/mozilla-central/rev/69babd862de70cabfa1d0a369d38e4881bd41e4d/toolkit/components/extensions/WebNavigationFrames.jsm#30, and https://searchfox.org/mozilla-central/rev/69babd862de70cabfa1d0a369d38e4881bd41e4d/toolkit/components/extensions/ExtensionPolicyService.cpp#282), devtools (https://searchfox.org/mozilla-central/rev/69babd862de70cabfa1d0a369d38e4881bd41e4d/devtools/server/actors/targets/browsing-context.js#96, https://searchfox.org/mozilla-central/rev/69babd862de70cabfa1d0a369d38e4881bd41e4d/devtools/server/performance/timeline.js#104) and find (https://searchfox.org/mozilla-central/rev/69babd862de70cabfa1d0a369d38e4881bd41e4d/toolkit/components/find/nsWebBrowserFind.cpp#130,180, https://searchfox.org/mozilla-central/rev/69babd862de70cabfa1d0a369d38e4881bd41e4d/toolkit/components/typeaheadfind/nsTypeAheadFind.cpp#391,636)

I'm not sure if these specific callers are likely to be broken or covered by other bugs (I imagine the WebNavigationFrames use in extensions is related to bug 1581859, for example), but it might be worth getting someone from each component to take a look.

Comment 8

[Neha Kochar [:neha]]

The remaining usages of this are already handled now.

Extensions - Bug 1655528
Find - Bug 1570965 (broken for subframes even w/o Fission)
Devtools - Bug 1657435 (alex pushed a fix today)

Closing this meta as Fixed.