1620248 - Create ContentProcessTargets as soon as the content processes start, by using watchTargets API on the actor side

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P2)

Description

[Alexandre Poirot [:ochameau]]

Similarly to bug 1593937 and bug 1620248, which will help creating the BrowsingContextTargetActor before the process or document starts loading, we should do the same with ContentProcessTargetActor.

We should introduce a "(content) process target helper" over here:
https://searchfox.org/mozilla-central/source/devtools/server/actors/descriptors/watcher/target-helpers/
Which exposes 4 methods:

• createTargets
• destroyTargets
• watchResources
• unwatchResources
  The createTargets and destroyTargets should create and destroy the Content Process Targets for the processes that already exist.
  watchResources and unwatchResources should communicate the new watched/unwatched resources for all existing content process targets.

In parallel to that, we should have some code, which would watch all:

• new content process being created and call WatcherActor.notifyTargetCreated
• content process that have been destroyed and call WatcherActor.notifyTargetDestroyed
  We might have to hook up into WatcherRegistry in order to register/unregister this "content process watching" code. Similarly to how we register/unregister the DevToolsFrame JSWindow Actor. (Or see my other approach maybeRegisterMessageListeners in the current prototype patch)

While working on this, we might want to use the existing TargetList test in order to ensure it keeps working the same way as LegacyListener:
https://searchfox.org/mozilla-central/source/devtools/shared/resources/tests/browser_target_list_processes.js
The test looks quite complete but we might improve its coverage.

We might want to first start by migrating the existing codebase to JSProcessActor, but I'm not sure that is a good call.
I may be wrong, see comment 4 about that.

Otherwise, the main change here, will be the same as bug 1593937. It will be about reversing "connectFromParent" to "connectFromContent".
Instead of creating the target on-demand, via ProcessDescriptor.getTarget and initiate the creation from the parent process, we will create it when the process start, from the content process.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1620248 - [devtools] Create ContentProcessTarget from the server side via the Watcher Actor.

Comment 3

[Chris Peterson [:cpeterson]]

Tracking dt-fission-m2 bugs for Fission Nightly (M6c)

Comment 4

[Alexandre Poirot [:ochameau]]

These last two weeks I was trying to see if we could move Content Process targets from MessageManager to JSprocessActor, via bug 1648499.
I started looking as it sounds appealing to try to reuse DevToolsFrame JSWindowActor either as-is, forked, or by sharing some common class between frames and processes.
But I end up in a chicken egg situation here:

• Reusing DevToolsFrame code for content processes would help support targets being created from the watcher actor. DevToolsFrame already implements "instantiate-already-available", "watchResources", "unwatchResources" and "destroy" queries. Which are all the 4 primitives we would have to implement in a new "process-target" helper in devtools/server/actors/descriptors/watcher/target-helpers/process-helper.js.
• DevToolsFrame is designed to spawn the target from the target's process. So it depends on the new setup, where the target is created via the watcher actor, instead of being created via the descriptor, on-demand, from the parent process. So DevToolsFrames depends on "connectFromContent" pattern, while the existing code depends on "connectFromParent".

So, it sounds like if we want to migrate to JSProcessActor, if we do it first, we would be tempted to migrate from MessageManager to JSProcessActor, while, at the same time, instantiating the targets from the content process. Because the JSProcessActor would reuse part of DevToolsFrames, which is designed to instantiate the target from the target's process.
I tend to think that it is best to focus on one thing at a time. So try to finish the WIP patch I have here, which aims to making the existing code, still using message managers, to spawn the targets via watcher actor and from the content processes. And later on, followup with bug 1648499 in order to focus on just the MessageManager to JSProcessActor migration.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1620248 - Gross hack to make things work with artifact builds.

But avoid having this changeset applied when testing a full build
as it would call the JSM twice!!

Comment 6

[Alexandre Poirot [:ochameau]]

I'm making some progress, but try may still be orange with crashes around shared_data, which I haven't been able to reproduce locally (yet):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=77a183cf9bdb59c9fe2eb6d4c8ac0ce3264cd20a

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1620248 - Allow `sharedData` to be null very early during process startup.

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1620248 - [devtools] Ensure destroying ContentProcess target when the target is destroyed before process shutdown.

For now, we were emitting the "tabDetached" event on message-manager-close (=process shutdown),
or when the DevToolsServerConnection closes (=client closes the connection).
But not when the target is being manually destroyed by the frontend,
which is done during toolbox shutsdown, before the client closes the connection.
This was introducing test failure with never resolving RDP requests.

Comment 9

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=677e7b1edf0eb42d3153f76a5c1136247bbde1c5
The crashes are now gone, but there is now leaks reported on debug builds, unfortunately they seem to only reproduce on Mac and Windows :/

Comment 10

[Phabricator Automation]

Comment on attachment 9178160 [details]
Bug 1620248 - Allow sharedData to be null very early during process startup.

Revision D91558 was moved to bug 1667790. Setting attachment 9178160 [details] to obsolete.

Comment 11

[Neha Kochar [:neha]]

Bulk move of all m2-mvp devtools bugs to Fission M7.

Comment 12

[Alexandre Poirot [:ochameau]]

It looks like I'm getting some platform leaks, but only on Mac and Windows:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b90be0e485a51706dc6972004adc0ef1c858fed7

Comment 13

[Alexandre Poirot [:ochameau]]

No particular impact on DAMP:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&newProject=try&newRevision=b90be0e485a51706dc6972004adc0ef1c858fed7&originalSignature=1910071&newSignature=1910071&framework=12&originalRevision=d9f3d6eb54827d3217af9b5a51552b79459c6a7a
I imagine, we aren't involving process target actors in DAMP tests?

Comment 14

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1620248 - [devtools] Make hooks.onClosed optional for all Transport classes.

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1620248 - [devtools] Test content process console messages in a resource watcher test.

For now, resource watcher tests were only testing resources from a single tab.
This test targets the main process, like the browser toolbox and so will listen to all targets,
including content process targets.

Comment 16

[Alexandre Poirot [:ochameau]]

Try looks green enough:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ee3a88022a6d4c3f96bd2a9a136c7ba34a54b93

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1620248 - [devtools] Rename content-process.jsm init method to initContentProcessTarget.

Comment 18

[Alexandre Poirot [:ochameau]]

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc538aba0b5e
[devtools] Ensure destroying ContentProcess target when the target is destroyed before process shutdown. r=nchevobbe,jdescottes
https://hg.mozilla.org/integration/autoland/rev/696737f76f69
[devtools] Make hooks.onClosed optional for all Transport classes. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/1e6f907ccf91
[devtools] Create ContentProcessTarget from the server side via the Watcher Actor. r=nchevobbe,jdescottes
https://hg.mozilla.org/integration/autoland/rev/e08c452d0fa2
[devtools] Test content process console messages in a resource watcher test. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/e09fe2a6602b
[devtools] Rename content-process.jsm init method to initContentProcessTarget. r=jdescottes

Comment 20

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/cc538aba0b5e
https://hg.mozilla.org/mozilla-central/rev/696737f76f69
https://hg.mozilla.org/mozilla-central/rev/1e6f907ccf91
https://hg.mozilla.org/mozilla-central/rev/e08c452d0fa2
https://hg.mozilla.org/mozilla-central/rev/e09fe2a6602b