Closed Bug 1620248 Opened 5 years ago Closed 4 years ago

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

Categories

(DevTools :: Framework, enhancement, P2)

enhancement

Tracking

(Fission Milestone:M7, firefox84 fixed)

RESOLVED FIXED
84 Branch
Fission Milestone M7
Tracking Status
firefox84 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 3 open bugs)

Details

(Whiteboard: dt-fission-m2-mvp)

Attachments

(5 files, 2 obsolete files)

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:

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.

Blocks: 1608848
Summary: Create ContntProcessTargets as soon as the content processes start, by using watchTargets API on the actor side → Create ContentProcessTargets as soon as the content processes start, by using watchTargets API on the actor side
Priority: -- → P2
Blocks: 1642599
Whiteboard: dt-fission-m2-mvp

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

Fission Milestone: --- → M6c

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.

Blocks: 1648499
Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED

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

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

Attachment #9131123 - Attachment description: Bug 1620248 - Prototype early creation of process targets → Bug 1620248 - [devtools] Prototype early creation of process targets.

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.

Depends on: 1667790

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

Attachment #9178160 - Attachment is obsolete: true

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

Fission Milestone: M6c → M7
Attachment #9131123 - Attachment description: Bug 1620248 - [devtools] Prototype early creation of process targets. → Bug 1620248 - [devtools] Create ContentProcessTarget from the server side via the Watcher Actor.
Depends on: 1669353

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

Depends on: 1671419

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.

Attachment #9168189 - Attachment is obsolete: true
Blocks: 1671906
Blocks: 1671926
Blocks: 1672614
Pushed by apoirot@mozilla.com: 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
Regressions: 1672660
Blocks: 1672778
Blocks: 1672826
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: