1593937 - Watch for additional remote frame targets in the case of the content toolbox

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Alexandre Poirot [:ochameau]]

Bug 1578745 introduced the devtools.contenttoolbox.fission preference, but TargetList landed in bug 1471754 doesn't support it.
We should make it so that the TargetList tries to iterate over remote frames for the case of the content toolbox when the pref is turned on.

Landing such change might be blocked on bug 1565200 and bug 1593764 as frame.getTarget(); in LegacyImplementationFrames.listen will return null and we will log a warning message.

Comment 1

[Alexandre Poirot [:ochameau]]

TargetList tests should be updated carefuly based on this comment:
https://phabricator.services.mozilla.com/D51783#inline-313810

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1593937 - Implement watchTargets for remote frames.

Comment 3

[Chris Peterson [:cpeterson]]

Tracking for Fission Nightly (M6)

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1593937 - Move frame and process descriptor pools in a dictionary in order to ease sharing them.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1593937 - Implement polymorphism in protocol.js

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1593937 - Simplify the watcher thanks to polymorphism.

Comment 7

[Phabricator Automation]

Comment on attachment 9119751 [details]
Bug 1593937 - Implement polymorphism in protocol.js

Revision D59333 was moved to bug 1609128. Setting attachment 9119751 [details] to obsolete.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1593937 - Fetch BrowsingContextTargets via the watcher instead of the RootFront.

Now that the BrowsingContextTarget are created by the watcher,
we should redirect existing code in the frontend which was using
RootFront.getBrowsingContextDescriptor in order to directly fetch the target from the watcher
.
We don't really need descriptors for additional targets.
I think that descriptors are mostly useful for the top level target and for about:debugging, to describe a target without debugging it yet.

This patch is a bit complex because we can't fetch the "parent BrowsingContext ID" from actors/browsing-context.js:form().
browsingContext.parent and browsingContext.embedderWindowGlobal are both null from the content process.
I have not found any way/API to get the parent browsing context ID from the content process, i.e. the ID of browser.xhtml from a tab content process.
So, I end up with this helper method on Watcher actor to get the parent ID on-demand.
We could also inject the parent ID into the browsingContextTarget's form in WindowGlobalWatcher._createTarget,
but that sounds very hacky and hard to follow.

Note that this patch address the issue of duplicated targets.
WatcherFront.getBrowsingContextTarget ensures fetching the ParentProcessTarget for browser.xhtml
instead of recreating a duplicated FrameDescriptor and BrowsingContextTarget for this document, as we do in the existing codebase.

I should probably update the node picker test, as the node picker in the browser toolbox was broken without this patch

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1593937 - Implement watchTargets for remote frames.

This would allow creating the WindowGlobalTarget as soon as the WindowGlobal is created,
before any JS of the page starts executing.
Doing this in the parent is too late and the page already started running.

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1593937 - Make the BrowsingContextTargetActor optionally follow the WindowGlobal lifecycle.

The new followWindowGlobalLifeCycle argument and field makes the actor behaves like a WindowGlobalTargetActor and only care about the current global (and all its inner iframes, still).
But it ignores the previous/next document. We still uses the DebuggerProgressListener, but mostly for iframes.
will-navigate and navigate are irrelevant for this actor now.
The plan would be to eventually switch all codepaths to this WindowGlobalTargetActor and rename it.
For now, this would only be used for remoted iframes, only additional frame targets.
But not for top level document, nor top level target switching.

Comment 12

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=291816974&revision=e2d11a3f372d5c2637b371e973ee429f6b130113

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1593937 - Load the DevToolsFrameChild from BrowsingContext, when the parent toggles watchedByDevTools attribute.

Toggling the watchedByDevTools attribute will:

• force loading the DevToolsFrameChild JS Window actor in the content process
• emit DevToolsWatching DOM event, which will be listened to create the target actor

We have to stop setting the watchedByDevTools flag from DevTools code in the content
process as it is already set an spread by native code of BrowsingContext.

If we don't, and do not have the if (mFields.Get<IDX_WatchedByDevTools>()) return; check
the code would actually infinite loop as setting a BrowsingContext field,
even to the exact same value, would actually still call ::DidSet() methods.

Comment 14

[Chris Peterson [:cpeterson]]

Tentatively moving P1 Fission M6 bugs to M6a.

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1593937 - Prevent fetching NodeFront for already destroyed AccessibleFront.

When navigating between process, we happen to destroy the AccessibleFront
in middle of this call to Target.getFront("inspector").
Before calling updateDetails, we already check if AccessibleFront is already destroyed.
This code also checks after the call to getFront.

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1593937 - Remove/Deprecate listRemoteFrames.

The previous changeset removed all the usages of listRemoteFrames from
the frontend code. We can start removing this method from the actor codebase,
and keep backward compat code in the client.

Comment 18

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9cea2bd8cba1370e6307597aafae5eadbb716c6

Comment 19

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1593937 - Disable browser_accessibility_tree_nagivation_oop.js as it fails intermittenly on Fission.

I also disable browser_accessibility_tree_nagivation_oop.js as it fails intermittenly on Fission
and triggers a C++ assertion.

Comment 20

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdefae4d686a928d03abe9d75fe0b61a5a5d017e

Comment 21

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0138dc60d21035c2898ec33a76211458e2d6bff

Comment 22

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47f92c74c17f52e931f08024907157ec3563a033

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/481eb7022f30
Make the BrowsingContextTargetActor optionally follow the WindowGlobal lifecycle. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/8f0492701940
Implement watchTargets for remote frames. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/04eee1b240f8
Fetch BrowsingContextTargets via the watcher instead of the RootFront. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/8dddeb7310ca
Prevent fetching NodeFront for already destroyed AccessibleFront. r=yzen
https://hg.mozilla.org/integration/autoland/rev/7acfcfdcbfd9
Remove/Deprecate listRemoteFrames. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/fc77f6e49447
Disable browser_accessibility_tree_nagivation_oop.js as it fails intermittenly on Fission. r=yzen,jdescottes

Comment 24

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/481eb7022f30
https://hg.mozilla.org/mozilla-central/rev/8f0492701940
https://hg.mozilla.org/mozilla-central/rev/04eee1b240f8
https://hg.mozilla.org/mozilla-central/rev/8dddeb7310ca
https://hg.mozilla.org/mozilla-central/rev/7acfcfdcbfd9
https://hg.mozilla.org/mozilla-central/rev/fc77f6e49447