1637785 - Remove use of Ci.nsIDocShellTreeItem in JS

Status: NEW

(DevTools :: General, task)

Description

[Chris Peterson [:cpeterson]]

https://searchfox.org/mozilla-central/search?q=nsIDocShellTreeItem&case=false&regexp=false&path=.js

such as:

QueryInterface(Ci.nsIDocShellTreeItem)
Ci.nsIDocShellTreeItem.typeAll
Ci.nsIDocShellTreeItem.typeChrome
Ci.nsIDocShellTreeItem.typeContent

Tracking for Fission Future because we don't need to remove all uses of Ci.nsIDocShellTreeItem for Fission MVP. Uses that aren't broken with Fission can wait to be removed until rmrf-docshell-tree-item bug 1607591.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::DOM: Navigation' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[:Gijs (he/him)]

I'm removing some dead-ish code in 1860046. That leaves the devtools consumer and a docshell self-test.

For the devtools consumer, I'm a bit confused - https://searchfox.org/mozilla-central/rev/e37662380b7b5507732a4f37a17da1a2f7802c04/devtools/server/actors/resources/utils/content-process-storage.js#136-137 suggests that the intent is for it to iterate over the entire document tree, but it no longer does that in fission as the children of a given frame are not necessarily in the same process after fission. Alexandre, is this a known storage actor issue on the devtools side? And/or is that code less alive than it looks at first glance? (this is the code actually using nsIDocShellTreeItem)

Comment 3

[:Gijs (he/him)]

Oh, and with the advent of .mjs, slightly better query than comment 0:

https://searchfox.org/mozilla-central/search?q=nsIDocShellTreeItem&path=js%24&case=false&regexp=false

Comment 4

[Alexandre Poirot [:ochameau]]

(In reply to :Gijs (he/him) from comment #2)

I'm removing some dead-ish code in 1860046. That leaves the devtools consumer and a docshell self-test.

For the devtools consumer, I'm a bit confused - https://searchfox.org/mozilla-central/rev/e37662380b7b5507732a4f37a17da1a2f7802c04/devtools/server/actors/resources/utils/content-process-storage.js#136-137 suggests that the intent is for it to iterate over the entire document tree, but it no longer does that in fission as the children of a given frame are not necessarily in the same process after fission. Alexandre, is this a known storage actor issue on the devtools side? And/or is that code less alive than it looks at first glance? (this is the code actually using nsIDocShellTreeItem)

I agree that's confusing.

In theory, regular devtools, debugging tabs should not involve this code because of the ignoreSubFrames flag which is set to true.
But we still do query the docshell tree. This is isIncludedInTopLevelWindow which will ultimately ignore all the children tree items.
Because target actor's windows, refers to actor's docShells, which returns only the top docShell (i.e. this.targetActor.docShell)
So ultimately: this.fetchChildWindows(this.targetActor.docShell); only register the targetActor top's docshell's window.

Now... the browser toolbox is still having this ignoreSubFrames flag set to false.
I would need to investigate just a bit more to see if we can clean this up before bug 1785266 (which toggles ignoreSubFrames to true for the browser toolbox and is a quite complex long overdue cleanup).

Comment 5

[:Gijs (he/him)]

Moving to DevTools as that's the only remaining JS consumer at this point, AFAICT.

Comment 6

[Alexandre Poirot [:ochameau]]

If there is no urgency in removing this docshell API, it would be nice to wait for bug 1785266.
This would effectively make DevTools debug each WindowGlobal independently of each others and stop using the DocShell child-iteration APIs.