1527203 - Set worker breakpoints before the worker begins executing

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement)

Description

[Brian Hackett [Laid off!]]

Attachment: WIP

Installed breakpoints should be installed before a worker starts executing code. This ensures that breakpoints will always be hit, regardless of timing issues when attaching to the worker's thread.

The attached patch takes the following steps to make this happen:

• When the client connects to the worker thread it sends the installed breakpoints in the connection options, which the server uses to install the breakpoints on the newly created thread actor.

• nsIWorkerDebugger gets disallowContent() and allowContent() methods that can be used to prevent the worker's main script from loading. Currently, the main script can start loading as soon as the registration callbacks in the nsIWorkerDebuggerManager have (synchronously) finished. These registration callbacks can invoke disallowContent() to delay that loading until allowContent() is later called asynchronously.

• disallowContent() and allowContent() are used to delay the loading of the main script until after the client and server have finished connecting.

The patch works, but needs tidying up, some simplifying, and more testing.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch

Updated patch, with test. I'm going to see how this looks on try before asking for review.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: patch

Updated patch which fixes a couple issues that showed up while testing:

• While the debugger is attaching to the worker, we need to prevent all debuggee runnables from running, not just the one which loads the main script. Otherwise messages posted to the worker can be received before it has installed message listeners.

• We should only prevent debuggee execution if the debugger is guaranteed to attach to the worker thread. Otherwise, the worker will never do anything. This is only guaranteed if we are doing windowless worker debugging, so the functionality here is only used if that preference is set.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Add nsIWorkerDebugger.setDebuggerReady.

Add a nsIWorkerDebugger.setDebuggerReady() interface. This can be used while registering the worker with the debugger to prevent debuggee runnables from executing in the worker until a later time.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Prevent worker debuggee execution until the thread actor has attached.

Use the new nsIWorkerDebugger.setDebuggerReady() interface to prevent workers from executing debuggee code until the debugger has finished attaching.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: Part 3 - Send breakpoints while attaching to thread.

Send the set of installed breakpoints to threads when attaching, and set them immediately in the server.

Comment 7

[Brian Hackett [Laid off!]]

Attachment: Part 4 - Test.

Test for the new functionality.

Comment 8

[Alexandre Poirot [:ochameau]]

Comment on attachment 9043447 [details] [diff] [review]
Part 3 - Send breakpoints while attaching to thread.

Review of attachment 9043447 [details] [diff] [review]:
-----------------------------------------------------------------

Overall it makes a lot of sense, but I have some concerns about the precise location of the setBreakpoint calls in the actor.

I would like to see your feedback on this before moving forward.

::: devtools/client/debugger/new/src/client/firefox/workers.js
@@ +27,4 @@
>    const { workers } = await tabTarget.listWorkers();
>    for (const workerTargetFront of workers) {
>      await workerTargetFront.attach();
> +    const [, workerThread] = await workerTargetFront.attachThread(options);

You are doing this only for the window-less worker usecase,
but I was wondering if that could be useful for regular tab target?

What do we do for tabs?
Do we have a code similar to the one you removed from commands.js?
```
- for (const { location, options } of (Object.values(breakpoints): any)) {	
-           client.setBreakpoint(location, options);	
- }
```
If yes, it would be really great to also use this new feature (modulo backward compatibility...)

That would be really handy if we could make it so that all the targets are managed the same way. It would help supporting fission and a world of different kinds of target.

::: devtools/server/actors/thread.js
@@ +277,5 @@
> +    if (request.options.breakpoints) {
> +      for (const { location, options } of Object.values(request.options.breakpoints)) {
> +        this.setBreakpoint(location, options);
> +      }
> +    }

At first it is a bit surprising to see a call to `setBreakpoint` *before* we call `dbg.addDebuggees()`.

As later, we end up iterating over the list of script here:
https://searchfox.org/mozilla-central/source/devtools/server/actors/source.js#438-441

It means that we won't iterate there, as I don't expect Debugger API to have any script to iterate over as I don't think any global has been registered to it yet.
But at the same time, this code is typical designed for worker debugging. The worker has been paused before any script can be executed. So... it should be fine... here. But I imagine it won't be fine if we start using it from some other purpose.

I don't know this codebase very well, especially the breakpoints world. Is there still someone knowledgeable on the backend side who could review this more precisely than me??

Otherwise, I imagine we should discuss a bit of the location of this piece of code.

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 9043446 [details] [diff] [review]
Part 2 - Prevent worker debuggee execution until the thread actor has attached.

Review of attachment 9043446 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/server/actors/thread.js
@@ +255,5 @@
>      }
>  
> +    if (this._parent.onThreadAttached) {
> +      this._parent.onThreadAttached();
> +    }

I have the same concern than the other patch of this queue: did you gave some thoughts about the very precise location of this call?

It will end up being *before* the call to `setBreakpoint`.
Shouldn't it be after? After the call to setBreakpoint, and after we enabled the Debugger?
I imagine it works because everything is very asynchronous, but it would help the comprehension of this code if the order of things actually made sense.


Otherwise, note that Eddy was doing worker specifics differently, like here:
https://searchfox.org/mozilla-central/rev/4587d146681b16ff9878a6fdcba53b04f76abe1d/devtools/shared/DevToolsUtils.js#753-761
But I think I prefer keeping all workers specifics into startup/worker.js instead of using these isWorker/rcp magic globals.

Note that ThreadActor should inherit from EventEmitter, so we could also integrate ThreadActor and startup/worker.js's `parent` via events. It can be surprising to see this call in the context of a BrowsingContextTarget actor as a parent.

::: devtools/server/actors/worker/worker-target-actor-list.js
@@ +120,5 @@
> +      // Prevent the debuggee from executing in this worker until the debugger
> +      // has finished attaching to it. We only do this if the windowless workers
> +      // preference is set. Otherwise the debugger is not guaranteed to connect
> +      // to the worker, in which case it will never do anything.
> +      if (Services.prefs.getBoolPref("devtools.debugger.features.windowless-workers")) {

You are using a frontend preference from the backend, this is not going to work for remote devices.

If you are only targeting Web Workers, you can add an option argument to the target actors's listWorkers methods as that's the one instantiating the WorkerTargetActorList object:
https://searchfox.org/mozilla-central/source/devtools/server/actors/targets/browsing-context.js#630-636
https://searchfox.org/mozilla-central/source/devtools/server/actors/targets/content-process.js#127-129
That's more complex for service worker and privileged workers as WorkerTargetActorList is instantiated before any request is received.
But I think your usecase is limited to web page's workers for now.

Comment 10

[Brian Hackett [Laid off!]]

(In reply to Alexandre Poirot [:ochameau] from comment #8)

You are doing this only for the window-less worker usecase,
but I was wondering if that could be useful for regular tab target?

What do we do for tabs?
Do we have a code similar to the one you removed from commands.js?

- for (const { location, options } of (Object.values(breakpoints): any)) {	
-           client.setBreakpoint(location, options);	
- }

If yes, it would be really great to also use this new feature (modulo
backward compatibility...)

That would be really handy if we could make it so that all the targets are
managed the same way. It would help supporting fission and a world of
different kinds of target.

I agree that we should be doing the same thing for all thread targets, though it seems better to do in followup to keep this bug more focused. In the tab target case we set breakpoints in the thread from the async store, syncBreakpoints() in devtools/client/debugger/new/src/client/index.js, so for consistent handling we would need to load breakpoints from the async store before attaching and then supply them to the tab target's thread via the attach options.

::: devtools/server/actors/thread.js
@@ +277,5 @@

• if (request.options.breakpoints) {

•  for (const { location, options } of Object.values(request.options.breakpoints)) {
  

•    this.setBreakpoint(location, options);
  

•  }
  

• }

At first it is a bit surprising to see a call to setBreakpoint before we
call dbg.addDebuggees().

As later, we end up iterating over the list of script here:
https://searchfox.org/mozilla-central/source/devtools/server/actors/source.
js#438-441

It means that we won't iterate there, as I don't expect Debugger API to have
any script to iterate over as I don't think any global has been registered
to it yet.
But at the same time, this code is typical designed for worker debugging.
The worker has been paused before any script can be executed. So... it
should be fine... here. But I imagine it won't be fine if we start using it
from some other purpose.

I don't know this codebase very well, especially the breakpoints world. Is
there still someone knowledgeable on the backend side who could review this
more precisely than me??

Otherwise, I imagine we should discuss a bit of the location of this piece
of code.

I'll ask for an additional r? for this piece of code. The handling of breakpoints was just refactored in bug 1524374. The breakpoint store used by a sever thread is now independent from sources, and only refers to URLs and, for URL-less sources, source IDs. Breakpoints and sources can be added in any order, and matching breakpoints on a Debugger.Script will be installed appropriately.

Comment 11

[Brian Hackett [Laid off!]]

Comment on attachment 9043447 [details] [diff] [review]
Part 3 - Send breakpoints while attaching to thread.

Jason, can you look at the placement of the setBreakpoint() call in this patch per comment 10?

Comment 12

[Brian Hackett [Laid off!]]

(In reply to Alexandre Poirot [:ochameau] from comment #9)

Comment on attachment 9043446 [details] [diff] [review]
Part 2 - Prevent worker debuggee execution until the thread actor has
attached.

Review of attachment 9043446 [details] [diff] [review]:

::: devtools/server/actors/thread.js
@@ +255,5 @@

 }

• if (this._parent.onThreadAttached) {

•  this._parent.onThreadAttached();
  

• }

I have the same concern than the other patch of this queue: did you gave
some thoughts about the very precise location of this call?

It will end up being before the call to setBreakpoint.
Shouldn't it be after? After the call to setBreakpoint, and after we enabled
the Debugger?
I imagine it works because everything is very asynchronous, but it would
help the comprehension of this code if the order of things actually made
sense.

It would be fine to put the onThreadAttached() call at the end of onAttach(), but it doesn't matter where in this function we call it. It is dispatching a message to the main thread which unblocks content execution on the worker thread, but since onAttach() runs synchronously on the worker thread there won't be any difference in the possible thread interleavings.

::: devtools/server/actors/worker/worker-target-actor-list.js
@@ +120,5 @@

•  // Prevent the debuggee from executing in this worker until the debugger
  

•  // has finished attaching to it. We only do this if the windowless workers
  

•  // preference is set. Otherwise the debugger is not guaranteed to connect
  

•  // to the worker, in which case it will never do anything.
  

•  if (Services.prefs.getBoolPref("devtools.debugger.features.windowless-workers")) {
  

You are using a frontend preference from the backend, this is not going to
work for remote devices.

If you are only targeting Web Workers, you can add an option argument to the
target actors's listWorkers methods as that's the one instantiating the
WorkerTargetActorList object:
https://searchfox.org/mozilla-central/source/devtools/server/actors/targets/
browsing-context.js#630-636
https://searchfox.org/mozilla-central/source/devtools/server/actors/targets/
content-process.js#127-129
That's more complex for service worker and privileged workers as
WorkerTargetActorList is instantiated before any request is received.
But I think your usecase is limited to web page's workers for now.

devtools.debugger.features.windowless-workers has been turned on by default and pretty soon we should be removing support for the old windowed worker interface, so that the debugger attaches to all workers it knows about. I'm not sure if this patch will need modifying in order for these changes to coexist with how service and privileged workers are handled, but I can attach a followup in that case. Would it be OK to just remove this test and always call dbg.setDebuggerReady(false)? This could be a little quirky until the pref is removed entirely (when using the debugger with the pref turned off, new workers will not be active until their toolbox is opened) but would simplify things in the interim.

Comment 13

[Brian Hackett [Laid off!]]

ni? for comment 12

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 9043445 [details] [diff] [review]
Part 1 - Add nsIWorkerDebugger.setDebuggerReady.

Review of attachment 9043445 [details] [diff] [review]:
-----------------------------------------------------------------

Did you also mean to make CompileScriptRunnable be a WorkerDebugeeRunnable (or at least report itself as one)?  CompileScriptRunnable (: WorkerRunnable) and CompileDebuggerScriptRunnable (: WorkerDebuggerRunnable) both use workerinternals::LoadMainScript which synchronously does the loading.  It seems like that's essential to the thesis statement of this bug in terms of eliminating races for attaching.

I tried to map out how we get to a point where we're race-proof below.  It took longer than I naively was thinking it would, but was very informative.  (And I think extensive props are owed to the devtools team in terms of how well structured and documented all of this code is.  This is obviously a very complex problem domain, so it's not surprising there's some complexity.)  It looks like there's a bunch of async bounces between various processes/threads before ThreadActor.onAttach puts the debugger into a stable "paused" state.  And so it's not clear to me that the CompileScriptRunnable couldn't successfully sneak into being executed if a race is lost.  (But again, having CompileScriptRunnable be a debugee runnable fixes that.)

Here's what seems like happens/is meant to happen relative to when nsIWorkerDebuggerManagerListener::OnRegister() is invoked:
- In the content process main thread:
  - nsIWorkerDebuggerManagerListener::OnRegister() calls into WorkerTargetActorList.onRegister:
    - WorkerTargetActorList._notifyListChanged calls _onListChanged
    - _onListChanged was set by RootActor.onListWorkers (in "devtools/server/actors/root.js") to its bound onWorkerListChanged method, which sends a "workerListChanged" message.
- In the parent process main thread:
  - workerListChanged() in "devtools/client/debugger/new/src/client/firefox/events.js" is invoked because it was added as a listener via setupEvents() in the same file.  It calls:
  - updateWorkers() (in "devtools/client/debugger/new/src/actions/debuggee.js") is invoked and it calls:
  - fetchWorkers() (in "devtools/client/debugger/new/src/client/firefox/commands.js") is invoked and it calls:
  - updateWorkerClients (in "devtools/client/debugger/new/src/client/firefox/workers.js") is invoked:
    - workerTargetFront.attach() is invoked and awaited
  - WorkerTargetFront.attach (in "devtools/shared/fronts/targets/worker.js") is invoked via message:
    - "attach" message is sent to the WorkerTargetActor and reply awaited.
- In the content process main thread:
  - WorkerTargetActor.attach (in "devtools/server/actors/target/worker.js") is invoked:
    - If the worker was a ServiceWorker, the nsIServiceWorkerInfo's attachDebugger method is invoked, causing the ServiceWorker to be spun up if it exists, or kept alive if it already exists.  (In either event the worker will not be terminated until `detachDebugger` is invoked.)
- In the parent process main thread:
  - WorkerTargetFront.attach (in "devtools/shared/fronts/targets/worker.js") continues execution following receipt of the "attach" request reply.
    - "connect" message is sent to the WorkerTargetActor and reply awaited.
- In the content process main thread:
  - WorkerTargetActor.connect (in "devtools/server/actors/target/worker.js") is invoked:
    - DebuggerServer.connectToWorker (in "devtools/server/main.js") invokes nsIWorkerDebugger::Initialize(("resource://devtools/server/startup/worker.js").
      - This creates a CompileDebuggerScriptRunnable which is a WorkerDebuggerRunnable which returns true for IsDebuggerRunnable() and so will be placed on the WorkerPrivate::mDebuggerQueue which is drained in preference to the normal queue.  (The overall processing order is [control runnable, debugger runnable, normal runnable].)
    - A "connect" message is nsIWorkerDebugger.postMessage()d which means it's a debugger runnable.  Because the worker scriptloader is currently dependent on the main thread, this runnable is guaranteed to precede any script-loading.
- On the worker:
  - The CompilerDebuggScriptRunnable ends up being run and it invokes workerinternals::LoadMainScript which will synchronously load the "devtools/server/startup/worker.js" script and its dependencies.
  - The "connect" message is received by the startup script and processed, creating a ThreadActor (from "devtools/server/actors/thread").
  - A "connected" reply is sent.
- In the parent process main thread:
  - In updateWorkerClients (in "devtools/client/debugger/new/src/client/firefox/workers.js"):
    - The workerTargetFront.attach() that was being awaited returns.
    - workerTargetFront.attachThread() is invoked.
  - WorkerTargetFront.attachThread is invoked via message:
    - An "attach" message is sent to the thread actor.
- On the worker:
  - ThreadActor.onAttach (in devtools/server/actors/thread.js) is invoked by way of the "attach" requestTypes binding.  This method pauses the debugger, which means at a low level it ends up using WorkerPrivate::EnterDebuggEventLoop to push a nested event loop that will only process control runnables and debugger runnables.

::: dom/workers/WorkerPrivate.h
@@ +909,4 @@
>               data->mHolders.IsEmpty());
>    }
>  
> +  nsresult DispatchLockHeld(already_AddRefed<WorkerRunnable> aRunnable,

It's convention in DOM code to add an `const MutexAutoLock& aProofOfLock` argument to enforce that the lock is actually held.

Also, it'd be great to add a comment here like: Internal method containing the actual dispatch logic.  Extracted out of Dispatch so that when SetIsDebuggerReady(true) is invoked the delayed runnables can be atomically dispatched in bulk.

::: dom/workers/nsIWorkerDebugger.idl
@@ +47,5 @@
>    void addListener(in nsIWorkerDebuggerListener listener);
>  
>    void removeListener(in nsIWorkerDebuggerListener listener);
> +
> +  void setDebuggerReady(in boolean ready);

Meta: It would be great to add some documentation to this idl file to contextualize how the devtools actors are expected to use this interface and what the assumptions are in terms of sequencing.  If you have a current understanding you'd like to add to other bits, that'd be awesome.

If not, a comment here along the lines of the following would be a nice baby step.  (No need to actually use this comment, but if it seems right to you or can be amended to be more accurate, feel free to use it!  Also, this assumes you fix up the compilation runnable.):

"""
Setting ready to false causes all WorkerDebuggeeRunnables dispatched to the worker to be queued until ready is set to true.  When ready is set to true, all events will be atomically re-dispatched.  It is intended to both 1) prevent the worker from evaluating any script until the worker debugger logic has been fully initialized and configured and 2) prevent the loss or reordering of any messages sent to the worker by holding them until the worker has been allowed to initialize.

Debuggee runnables currently include both script compilation and postMessage message events sent from the worker's parent document or parent worker.  This set can and will expand in the future.
"""

For example, right now, "the future" would mean that we want the Clients API postMessage to a worker end up in the queue.  Right now we don't actually support that on workers, so it's not a concern.  See https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/dom/clients/manager/ClientSource.cpp#546

Comment 15

[Brian Hackett [Laid off!]]

(In reply to Andrew Sutherland [:asuth] from comment #14)

Comment on attachment 9043445 [details] [diff] [review]
Part 1 - Add nsIWorkerDebugger.setDebuggerReady.

Review of attachment 9043445 [details] [diff] [review]:

Did you also mean to make CompileScriptRunnable be a WorkerDebugeeRunnable
(or at least report itself as one)?

The patch overrides IsDebuggeeRunnable() in CompileScriptRunnable so that it reports itself as a debuggee runnable and will go into the delayed runnable queue until the debugger finishes initializing. I'll change CompileScriptRunnable to inherit from WorkerDebuggeeRunnable, which seems clearer. We definitely can't guarantee that the CompileScriptRunnable runs at the appropriate time if it doesn't report itself as a debuggee runnable.

Comment 16

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Add nsIWorkerDebugger.setDebuggerReady.

Updated patch per comments. This also fixes a bug that showed up when I did additional testing. If a debuggee runnable is dispatched before WorkerPrivate->mThread has been set, the runnable would go into mPreStartRunnables and unconditionally run once the worker thread started.

I fixed this by reordering the tests in Dispatch(), though it's not obvious in the patch due the code moving into DispatchLockHeld and the indentation level changing.

Comment 17

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Prevent worker debuggee execution until the thread actor has attached.

Updated patch. This moves the parent.onThreadAttached() call to a more intuitive place, and removes the test for the windowless workers preference now that it is the default.

This also fixes a bug that showed up in additional testing. WorkerTargetActorList does not always listen to worker list changes, and if its listener is not called then the worker will start running content immediately. This changes WorkerTargetActorList to always listen to worker list changes.

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 9045835 [details] [diff] [review]
Part 1 - Add nsIWorkerDebugger.setDebuggerReady.

Review of attachment 9045835 [details] [diff] [review]:
-----------------------------------------------------------------

Huh, so, somehow I had entirely missed that you had overridden `IsDebuggeeRunnable` already in the first revision of the patch.  I guess my splinter-using review skills have really atrophied!  In any event, I do think it is an improvement to explicitly be subclassing WorkerDebugeeRunnable.  (And I now have a better handle on how this area of code works, which will be imminently relevant for ServiceWorkers.)

Thanks for the patch and helping get some more comments into the worker code-base!

Comment 19

[Alexandre Poirot [:ochameau]]

Comment on attachment 9045840 [details] [diff] [review]
Part 2 - Prevent worker debuggee execution until the thread actor has attached.

Review of attachment 9045840 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/server/actors/worker/worker-target-actor-list.js
@@ +39,3 @@
>    this.onRegister = this.onRegister.bind(this);
>    this.onUnregister = this.onUnregister.bind(this);
> +  wdm.addListener(this);

It is not clear why you are changing the logic of this list class. All the list classes are following the exact same behavior and API. If we were to change this one, we should probably align the other ones.

Is `listWorkers` called too late by the frontend? Or is there a race between the `workerListChanged` event reception and the next `listWorkers` call?

I'm a bit concerned to register listener immediately, no matter what as this WorkerTargetActorList is used by other environment that don't need/except this feature (see my other comment).
If that's only a race between listWorker and workerListChanged, we may start listening on the list listWorkers and never stop listening until destroy?

@@ +99,5 @@
>        this._notifyListChanged();
> +
> +      // Prevent the debuggee from executing in this worker until the debugger
> +      // has finished attaching to it.
> +      dbg.setDebuggerReady(false);

I'm expecting this to freeze all the workers with this patch and not only the web page workers.
The WorkerTargetActorList used from RootActor will iterate over all privileged workers and freeze them without having any frontend code to attach to them and resume them.

I'm sorry to insist, but I think it would be easier to control if the client explicitly ask for this feature. I suggested to pass something to `listWorkers`, as that's easy change regarding backward compatibility. But feel free to suggest something else.

Comment 20

[Alexandre Poirot [:ochameau]]

(In reply to Brian Hackett (:bhackett) from comment #10)

I'll ask for an additional r? for this piece of code. The handling of breakpoints was just refactored in bug 1524374. The breakpoint store used by a sever thread is now independent from sources, and only refers to URLs and, for URL-less sources, source IDs. Breakpoints and sources can be added in any order, and matching breakpoints on a Debugger.Script will be installed appropriately.

I would like to understand how this actually work when we do this:

    if (request.options.breakpoints) {
      for (const { location, options } of Object.values(request.options.breakpoints)) {
        this.setBreakpoint(location, options);
      }
    }

Just before this:

    this.dbg.addDebuggees();

When we call setBreakpoint, we end up trying to apply the breakpoint on the Debugger API here:
https://searchfox.org/mozilla-central/source/devtools/server/actors/source.js#440

  let scripts = this._findDebuggeeScripts({ line });

I'm expecting this to return an empty list as we haven't added any debuggee yet. Am I correct?
So I'm not expecting the breakpoint to be applied at this stage.
We do call applyBreakpoint from other callsite, ThreadActor._addSource, which is only called from ThreadActor.onNewScript.
But I'm not expecting onNewScript to be called for already existing scripts, or am I wrong?

Now, again, that's not an issue for worker, because you assume that there is no already existing scripts.
But that's from from being an obvious exception when you read this attach function.

Comment 21

[Jason Laster [:jlast]]

Comment on attachment 9043447 [details] [diff] [review]
Part 3 - Send breakpoints while attaching to thread.

Review of attachment 9043447 [details] [diff] [review]:
-----------------------------------------------------------------

This looks okay to me. 

I agree that it would be nice if the targets were symmetric. My understanding though is that client/workers.js is in some way a placeholder until the toolbox attaches to the workers for us. At this point, the toolbox will be attaching to the main thread and all other relevant threads. At that point, it would be nice if we could make the APIs more consistent.

::: devtools/client/debugger/new/src/client/firefox/workers.js
@@ +27,4 @@
>    const { workers } = await tabTarget.listWorkers();
>    for (const workerTargetFront of workers) {
>      await workerTargetFront.attach();
> +    const [, workerThread] = await workerTargetFront.attachThread(options);

Do you mean, it would be nice to send breakpoints to all the threads when the debugger attaches? 

We currently send breakpoints to the server when attaching here
https://searchfox.org/mozilla-central/source/devtools/client/debugger/new/src/client/index.js#96

::: devtools/server/actors/thread.js
@@ +274,5 @@
>        thread: this,
>      });
>  
> +    if (request.options.breakpoints) {
> +      for (const { location, options } of Object.values(request.options.breakpoints)) {

minor nit, it would be nice to send the values instead of needing to do that here. Hopefully that would be more feature proof. 

Also, I wonder if we want to format the pending breakpoint data so that the server only receives what it cares about.  For instance, here we are using location which is fine if there aren't sourcemaps, but otherwise we would need to use generatedLocation.

Comment 22

[Brian Hackett [Laid off!]]

(In reply to Alexandre Poirot [:ochameau] from comment #20)

(In reply to Brian Hackett (:bhackett) from comment #10)

I'll ask for an additional r? for this piece of code. The handling of breakpoints was just refactored in bug 1524374. The breakpoint store used by a sever thread is now independent from sources, and only refers to URLs and, for URL-less sources, source IDs. Breakpoints and sources can be added in any order, and matching breakpoints on a Debugger.Script will be installed appropriately.

I would like to understand how this actually work when we do this:

    if (request.options.breakpoints) {
      for (const { location, options } of Object.values(request.options.breakpoints)) {
        this.setBreakpoint(location, options);
      }
    }

Just before this:

    this.dbg.addDebuggees();

When we call setBreakpoint, we end up trying to apply the breakpoint on the Debugger API here:
https://searchfox.org/mozilla-central/source/devtools/server/actors/source.js#440

  let scripts = this._findDebuggeeScripts({ line });

I'm expecting this to return an empty list as we haven't added any debuggee yet. Am I correct?
So I'm not expecting the breakpoint to be applied at this stage.
We do call applyBreakpoint from other callsite, ThreadActor._addSource, which is only called from ThreadActor.onNewScript.
But I'm not expecting onNewScript to be called for already existing scripts, or am I wrong?

Now, again, that's not an issue for worker, because you assume that there is no already existing scripts.
But that's from from being an obvious exception when you read this attach function.

Oops, yes, you're right, we won't apply breakpoints to any sources here. The ordering of the setBreakpoint() and addDebuggees() call won't affect things though, as we can't apply breakpoints until we have created a source actor for the source. We should be using _addSource when responding to a client query for the existing sources, so that existing breakpoints get installed and so that the other updates in _addSource happen. Since this doesn't affect the worker case, would it be ok to just add a comment here for now?

Comment 23

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Prevent worker debuggee execution until the thread actor has attached.

This patch has the client indicate to the server that it wants workers to pause on startup.

The earlier patch changed WorkerTargetActorList's listener because there is no listener installed on the WorkerDebuggerManager between the time when the worker list changes and the time when a request for the workers is received from the client, during which the page is unpaused and can create new workers that will be unpaused.

This patch avoids changing WorkerTargetActorList's listener behavior by using a second listener on the WorkerDebuggerManager that is installed when the main thread actor has been configured to pause workers on startup.

Comment 24

[Alexandre Poirot [:ochameau]]

Comment on attachment 9046078 [details] [diff] [review]
Part 2 - Prevent worker debuggee execution until the thread actor has attached.

Review of attachment 9046078 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for all the delay in intermediate reviews...

It looks good now, thanks a lot for the additional comments.
Could you please test your patch against the Browser toolbox to ensure it doesn't throw?

::: devtools/server/actors/thread.js
@@ +459,5 @@
>  
> +    if ("pauseWorkersUntilAttach" in options) {
> +      if (this._parent.pauseWorkersUntilAttach) {
> +        this._parent.pauseWorkersUntilAttach(options.pauseWorkersUntilAttach);
> +      }

Instead of going through the target actor and the WorkerTargetActorList; wouldn't it be easier to instantiate a `PauseMatchingWorkers` directly from the thread actor?
```
new PauseMatchingWorkers(this.conn, {
  type: Ci.nsIWorkerDebugger.TYPE_DEDICATED,	
  window: this._parent.window,	
});
```
I'm trying to reduce the dependencies between thread and target as things start to be complicated to understand between these two.

::: devtools/server/actors/worker/worker-target-actor-list.js
@@ +161,5 @@
> +        this._pauseMatchingWorkers.destroy();
> +        this._pauseMatchingWorkers = null;
> +      }
> +    }
> +  },

If you start using PauseMatchingWorkers from the thread actor, I think you can drop the modifications made to WorkerTargetActorList.

Comment 25

[Brian Hackett [Laid off!]]

(In reply to Alexandre Poirot [:ochameau] from comment #24)

Comment on attachment 9046078 [details] [diff] [review]
Part 2 - Prevent worker debuggee execution until the thread actor has
attached.

Review of attachment 9046078 [details] [diff] [review]:

Sorry for all the delay in intermediate reviews...

It looks good now, thanks a lot for the additional comments.
Could you please test your patch against the Browser toolbox to ensure it
doesn't throw?

::: devtools/server/actors/thread.js
@@ +459,5 @@

• if ("pauseWorkersUntilAttach" in options) {

•  if (this._parent.pauseWorkersUntilAttach) {
  

•    this._parent.pauseWorkersUntilAttach(options.pauseWorkersUntilAttach);
  

•  }
  

Instead of going through the target actor and the WorkerTargetActorList;
wouldn't it be easier to instantiate a PauseMatchingWorkers directly from
the thread actor?

new PauseMatchingWorkers(this.conn, {
  type: Ci.nsIWorkerDebugger.TYPE_DEDICATED,	
  window: this._parent.window,	
});

I'm trying to reduce the dependencies between thread and target as things
start to be complicated to understand between these two.

This would be simpler, but it seems fragile. In order for workers to pause at the correct times with this patch, the workers returned by the WorkerTargetActorList need to be exactly those workers which are paused by the PauseMatchingWorkers. Otherwise, we get workers that don't pause when they should, or pause forever because the debugger never attached to them. So, this patch makes the PauseMatchingWorkers object a part of the WorkerTargetActorList, so they can use the same options and tests for whether a worker should be paused/listed, and there is no chance they can be out of sync with each other.

Comment 26

[Alexandre Poirot [:ochameau]]

Comment on attachment 9043447 [details] [diff] [review]
Part 3 - Send breakpoints while attaching to thread.

Review of attachment 9043447 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Brian Hackett (:bhackett) from comment #22)

> Oops, yes, you're right, we won't apply breakpoints to any sources here.

> The ordering of the setBreakpoint() and addDebuggees() call won't affect things though, as we can't apply breakpoints until we have created a source actor for the source.  We should be using _addSource when responding to a client query for the existing sources, so that existing breakpoints get installed and so that the other updates in _addSource happen.  

I'm bit confused and I would like to better understand what is going on.
You told me in comment 10:
> The handling of breakpoints was just refactored in bug 1524374.
> The breakpoint store used by a sever thread is now independent from sources, 
> and only refers to URLs and, for URL-less sources, source IDs. 
> Breakpoints and sources can be added in any order, and matching breakpoints on a Debugger.Script will be installed appropriately.

This seems to say the opposite of what you just said.
Was "Breakpoints and sources can be added in any order":
 * something still not fully refactored, but it will be eventually true at some point?
 * doesn't conflict with "we can't apply breakpoints until we have created a source actor for the source" because I don't fully understand something here?
 * 3rd response, I really miss something ? :)

When you say:
  "We should be using _addSource when responding to a client query for the existing sources, so that existing breakpoints get installed and so that the other updates in _addSource happen." Doesn't that mean that, if a script for which we set a breakpoint execute between attach() and sources() requests (and the related _addSource call), 
the breakpoint won't break?

Also, outside of the worker issue, don't we miss something like this in ThreadActor.attach:
  for (const source of this.dbg.findSources())
    this._addSource(source);
Now that breakpoints can be set without a source actor, we could pass only URLs and line/column, but the breakppoints would not work until we call sources().
I'm starting to wonder if there is a design issue between attach() and sources() requests. (An issue that was always here, outside of the worker work you are doing)

> Since this doesn't affect the worker case, would it be ok to just add a comment here for now?

Is there any particular reason to not move that code *after* addDebuggees?
I do understand we want to set the breakpoint before the call to this._paused, but I'm wondering if there is any argument to do this *before* addDebuggees?

Otherwise, if there is no possible races between attach() and sources() for worker, a comment would be enough to proceed with this patch!
We can surely polish existing caveats of the thread actor in followups.

::: devtools/client/debugger/new/src/client/firefox/workers.js
@@ +27,4 @@
>    const { workers } = await tabTarget.listWorkers();
>    for (const workerTargetFront of workers) {
>      await workerTargetFront.attach();
> +    const [, workerThread] = await workerTargetFront.attachThread(options);

Yes. All targets should be handled the same way. So if we do set breakpoints via attach for worker, we should do that for the regular tab target as well.

But that's totally fine to do that in a followup, it is only important for fission.

Comment 27

[Brian Hackett [Laid off!]]

(In reply to Alexandre Poirot [:ochameau] from comment #26)

Comment on attachment 9043447 [details] [diff] [review]
Part 3 - Send breakpoints while attaching to thread.

Review of attachment 9043447 [details] [diff] [review]:

(In reply to Brian Hackett (:bhackett) from comment #22)

Oops, yes, you're right, we won't apply breakpoints to any sources here.

The ordering of the setBreakpoint() and addDebuggees() call won't affect things though, as we can't apply breakpoints until we have created a source actor for the source. We should be using _addSource when responding to a client query for the existing sources, so that existing breakpoints get installed and so that the other updates in _addSource happen.

I'm bit confused and I would like to better understand what is going on.
You told me in comment 10:

The handling of breakpoints was just refactored in bug 1524374.
The breakpoint store used by a sever thread is now independent from sources,
and only refers to URLs and, for URL-less sources, source IDs.
Breakpoints and sources can be added in any order, and matching breakpoints on a Debugger.Script will be installed appropriately.

This seems to say the opposite of what you just said.
Was "Breakpoints and sources can be added in any order":

• something still not fully refactored, but it will be eventually true at
  some point?
• doesn't conflict with "we can't apply breakpoints until we have created a
  source actor for the source" because I don't fully understand something here?
• 3rd response, I really miss something ? :)

The statement I originally made is correct in its intent, but is not literally true. We should be applying breakpoints in response to the sources() request, but we aren't, because ThreadActor.onSources() has a bug.

When you say:
"We should be using _addSource when responding to a client query for the
existing sources, so that existing breakpoints get installed and so that the
other updates in _addSource happen." Doesn't that mean that, if a script for
which we set a breakpoint execute between attach() and sources() requests
(and the related _addSource call),
the breakpoint won't break?

Yes, it does.

Also, outside of the worker issue, don't we miss something like this in
ThreadActor.attach:
for (const source of this.dbg.findSources())
this._addSource(source);
Now that breakpoints can be set without a source actor, we could pass only
URLs and line/column, but the breakppoints would not work until we call
sources().
I'm starting to wonder if there is a design issue between attach() and
sources() requests. (An issue that was always here, outside of the worker
work you are doing)

I don't think this makes a difference in the end. If we're attaching to a thread that already has sources then we don't really have control over the exact timing of when that attach happens wrt content execution on the page.

::: devtools/client/debugger/new/src/client/firefox/workers.js
@@ +27,4 @@

const { workers } = await tabTarget.listWorkers();
for (const workerTargetFront of workers) {
await workerTargetFront.attach();

• const [, workerThread] = await workerTargetFront.attachThread(options);

Yes. All targets should be handled the same way. So if we do set breakpoints
via attach for worker, we should do that for the regular tab target as well.

But that's totally fine to do that in a followup, it is only important for
fission.

Yeah, I agree that we should be doing this for all threads in the future. This will allow breakpoints to hit when we first open a page with the debugger open, instead of requiring a reload to hit early breakpoints. Doing this for the main thread is a different problem from the one in this bug, though.

Comment 28

[Alexandre Poirot [:ochameau]]

(In reply to Brian Hackett (:bhackett) from comment #27)

The statement I originally made is correct in its intent, but is not literally true. We should be applying breakpoints in response to the sources() request, but we aren't, because ThreadActor.onSources() has a bug.

Is there a bug filed about this?
Is there still a refactoring happening around the debugger actors or is it fully completed?

Comment 29

[Brian Hackett [Laid off!]]

(In reply to Alexandre Poirot [:ochameau] from comment #28)

(In reply to Brian Hackett (:bhackett) from comment #27)

The statement I originally made is correct in its intent, but is not literally true. We should be applying breakpoints in response to the sources() request, but we aren't, because ThreadActor.onSources() has a bug.

Is there a bug filed about this?
Is there still a refactoring happening around the debugger actors or is it fully completed?

I just filed bug 1530699 for this. The refactoring is done for now, but we're still dealing with fallout from it. There might be followup work to remove source actors but I'm not sure at this point when/if that will happen.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ba97389b183
Part 1 - Add interface to delay execution of debuggee content in workers during debugger registration, r=asuth.
https://hg.mozilla.org/integration/mozilla-inbound/rev/34d35ba26c07
Part 2 - Prevent worker debuggee execution until the thread actor has attached, r=ochameau.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5f0800182ae
Part 3 - Send and install breakpoints when attaching to a thread, r=ochameau,jlast.
https://hg.mozilla.org/integration/mozilla-inbound/rev/389de12c8724
Part 4 - Add test for hitting breakpoints early in a worker's execution.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5580a62e4e6
Part 5 - Listen to workerListChanged events on the tab target, r=jlast.

Comment 31

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7ba97389b183
https://hg.mozilla.org/mozilla-central/rev/34d35ba26c07
https://hg.mozilla.org/mozilla-central/rev/e5f0800182ae
https://hg.mozilla.org/mozilla-central/rev/389de12c8724
https://hg.mozilla.org/mozilla-central/rev/f5580a62e4e6