Closed Bug 1633712 Opened 4 years ago Closed 4 years ago

Create WorkerTargets as soon as the worker thread start, by using watchTargets API on the actor side

Tracking

(Fission Milestone:M7, firefox84 fixed)

Status:

RESOLVED FIXED

Milestone:

84 Branch

Project Flags:

Fission Milestone

Tracking Flags:

Tracking

Status

firefox84

---

fixed

People

(Reporter: ochameau, Assigned: nchevobbe)

References

(Blocks 4 open bugs)

Details

(Whiteboard: dt-fission-m2-mvp)

Attachments

(4 files)

Bug 1633712 - [devtools] Create WorkerTargets as soon as possible. r=ochameau. 4 years ago Nicolas Chevobbe [:nchevobbe] (deleted), text/x-phabricator-request		Details
Bug 1633712 - [devtools] Add test cases to browser_target_list_tab_workers and remove the fission fail-if. r=jdescottes. 4 years ago Nicolas Chevobbe [:nchevobbe] (deleted), text/x-phabricator-request		Details
Bug 1633712 - [devtools] Add a console message resource test for worker targets. r=jdescottes. 4 years ago Nicolas Chevobbe [:nchevobbe] (deleted), text/x-phabricator-request		Details
Bug 1633712 - [devtools] Move shouldNotifyWindowGlobal and getAllRemoteBrowsingContexts to a util file. r=ochameau. 4 years ago Nicolas Chevobbe [:nchevobbe] (deleted), text/x-phabricator-request		Details

Alexandre Poirot [:ochameau]

Reporter

Description

•

4 years ago

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

We should introduce a "worker 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 Worker Targets for the worker that already exists.
watchResources and unwatchResources should communicate the new watched/unwatched resources for all existing worker targets.

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

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

The goal here would be to start by only supporting Tab Target. So only when WatcherActor._browser/WatcherActor.browsingContextID are set. And so only when we focus on just one BrowsingContext. Supporting the browser toolbox means having to inspect all the processes, while for one tab, we can focus on just one process at a time. (We may support navigation to another process in a followup)

Compared to the current implementation, using legacy listeners and TabTarget.listWorkers, here, we should avoid involving TabTarget/BrowsingContextTargetActor. We might have to introduce a new JS Window Actor, or reuse DevToolsFrame JS Window Actor (if relevant).

You might want to contribute to the existing TargetList test specific to Worker targets:
https://searchfox.org/mozilla-central/rev/21f2b48e01f2e14a94e8d39a665b56fcc08ecbdb/devtools/shared/resources/tests/browser_target_list_workers.js#190-234
In order to support more usecases:

destruction
workers created after the call to watchTargets
may be have two distinct test files? One for BrowserToolbox codepath and another one for Tabs?

Last but not list, a quick note about the existing codebase.
The code watching for new workers and workers being destroyed should be inspired from WorkerTargetActorList.

matchWorkerDebugger should be tweaked to check WatcherActor's BrowsingContextID instead of window.
WorkerPauser should be kept until we address bug 1573327
WorkerPauser is actually including a very important bit that is a bit convoluted and not very well known.
Right here, on every Worker instantiation, we pause the worker immediately, from the main thread, preventing it to run anything. And much later on, there, we resume it, still from the main thread. But the path between these two callsites is convoluted!

the worker just get created and we immediately call worker.setDebuggerReady(false), this is done by WorkerPauser
Completely independently of that, we start creating the WorkerTargetActor, on demand, via listWorkers and WorkerTargetActorList
We use WorkerConnector to create the WorkerTargetActor from the Worker thread
The frontend receive the worker target
The frontend attach the ThreadActor
Some magic code from the worker thread notify the main thread that the Thread Actor is attached
Some other magic code in the main thread receive this message and resume the worker

All this "magic" (complexity) relates to bug 1573327 as it will allow to pass the breakpoint to the Watcher Actor, and stop having to freeze the worker untils ThreadActor.attach is called. Today, we pass the breakpoint via this thread actor method. We will pass the breakpoints as we pass the "watched resource types" to all processes and threads.

Alexandre Poirot [:ochameau]

Reporter