Create ContentProcessTargets as soon as the content processes start, by using watchTargets API on the actor side
Categories
(DevTools :: Framework, enhancement, P2)
Tracking
(Fission Milestone:M7, firefox84 fixed)
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
ThecreateTargets
anddestroyTargets
should create and destroy the Content Process Targets for the processes that already exist.
watchResources
andunwatchResources
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 approachmaybeRegisterMessageListeners
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.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•4 years ago
|
Comment 3•4 years ago
|
||
Tracking dt-fission-m2 bugs for Fission Nightly (M6c)
Assignee | ||
Comment 4•4 years ago
|
||
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.
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 5•4 years ago
|
||
But avoid having this changeset applied when testing a full build
as it would call the JSM twice!!
Assignee | ||
Comment 6•4 years ago
|
||
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
Assignee | ||
Comment 7•4 years ago
|
||
Updated•4 years ago
|
Assignee | ||
Comment 8•4 years ago
|
||
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.
Assignee | ||
Comment 9•4 years ago
|
||
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•4 years ago
|
||
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•4 years ago
|
||
Bulk move of all m2-mvp devtools bugs to Fission M7.
Updated•4 years ago
|
Assignee | ||
Comment 12•4 years ago
|
||
It looks like I'm getting some platform leaks, but only on Mac and Windows:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b90be0e485a51706dc6972004adc0ef1c858fed7
Assignee | ||
Comment 13•4 years ago
|
||
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?
Assignee | ||
Comment 14•4 years ago
|
||
Assignee | ||
Comment 15•4 years ago
|
||
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.
Assignee | ||
Comment 16•4 years ago
|
||
Try looks green enough:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ee3a88022a6d4c3f96bd2a9a136c7ba34a54b93
Updated•4 years ago
|
Assignee | ||
Comment 17•4 years ago
|
||
Assignee | ||
Comment 18•4 years ago
|
||
Comment 19•4 years ago
|
||
Comment 20•4 years ago
|
||
bugherder |
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
Description
•