1241958 - We should have a windowless debugger UI for web workers.

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Eddy Bruel [:ejpbruel]]

Our current UI for worker debugging involves opening a separate toolbox for the worker. This is awkward to work with when debugging multiple workers at the same time. To provide a better user experience, we should have a windowless debugger UI for web workers, where both the main thread and worker threads are debugged from the same toolbox.

Comment 1

[Eddy Bruel [:ejpbruel]]

Integrating the worker debugging UI with the main toolbox is probably the most important feature to improve the worker debugging experience. That said, it's also a long term project, which is why I'm marking it as P2.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch

WIP to support debugging a single main thread and its workers from a toolbox.  This operates entirely on the frontend, and has the following new features:

- The workers secondary pane shows both the main thread and workers, and selecting one updates the UI to the state of the selected thread.  Paused threads have a placeholder 'PAUSED' indicator.

- The sources pane includes sources from workers as well as the main thread.  Sources for workers are shown in different parts of the directory tree than sources for the main thread.  This is similar to what Chrome does but I'm not sure it's the best idea: if there are multiple workers with the same sources then there will be multiple copies of that source in the directory; should opening them all up create multiple file tabs with the same name, or a single file tab corresponding to several different sources?

- Buttons in the command bar operate on the current thread.  Pause/resume should work, I haven't tested the others.

- Breakpoints can be set and if a thread hits them it will pause and become the selected thread.

So, there is some basic functionality working here but things will not be robust and this is pretty far from being done.  At this stage though it would be good to make sure this is taking the right approach to this problem.  The main changes to the logic are in the paused redux store, where we keep track of the paused state for all threads instead of just a single one.

Comment 3

[Jim Blandy :jimb]

🎉 This is pretty great.

Comment 4

[Brian Hackett [Laid off!]]

(In reply to Brian Hackett (:bhackett) from comment #2)
> - The sources pane includes sources from workers as well as the main thread.
> Sources for workers are shown in different parts of the directory tree than
> sources for the main thread.  This is similar to what Chrome does but I'm
> not sure it's the best idea: if there are multiple workers with the same
> sources then there will be multiple copies of that source in the directory;
> should opening them all up create multiple file tabs with the same name, or
> a single file tab corresponding to several different sources?

To elaborate here (there was some discussion on Slack), the handling of the sources pane is the part of this patch that I'm least sure about what the final design should look like.  There are two options that I see, which have ramifications both for how we store the data internally and how it is represented in the UI.

#1. have a map (URL x thread) -> source

It supports showing sources for each thread separately in the sources pane.  if multiple threads open the same URL multiple times then that URL will be shown in multiple places in the source pane.  If the user opens up all those source tree items, should they get (#1a) multiple tabs for the same URL, associated with different threads, or (#1b) a single tab for the URL which is associated with all the threads (i.e. setting a breakpoint will set it in all threads).

#2. have a map URL -> source[]

This would be geared towards having a uniform source tree regardless of which threads are running.  Items in the tree are those associated with one or more threads, and will only appear once regardless of how many threads have loaded that source's URL.  Opening the tree item will then have to produce a tab associated with all threads that have opened the URL for that item.

The current patch is closest to #1b.  The way the UI is currently set up, #2 seems the most intuitive to me.  #1a could work too, but the titles of the tabs would I think have to be differentiated to keep users from getting confused.  An advantage of #1a is that it would support setting breakpoints that in a single thread, instead of breakpoints at the same site in all threads.  It also might mesh better with the breakpoints logic, in that breakpoints that have been set in the UI will be 1:1 with breakpoints we end up installing in server.

Comment 5

[Jason Laster [:jlast]]

I'm in favor of the most literal solution. I believe this is 1a. 


1. the source tree shows all of the sources
2. there is one tab per source

We can always help the user do more "interesting" things in the future, but i think we should focus on the simple case for now.

Comment 6

[Alexandre Poirot [:ochameau]]

Note that here, you are only integrating the workers.
The bigger picture may help taking decisions here.
In the browser toolbox context, you will suddently have to list lots of JS files coming from various contexts.

Today, it only lists main process ones: xpcom, JSMs, Sandboxes, chrome document scripts, ...

But bug 1488204 is aiming to display these resources for all the processes, so including the child processes ones, which will also include websites files. In this setup, you may have two websites loading the same library file from a CDN and having it displayed only once may be disturbing.
That also leads me to believe that 1a is a good choice, especially if you think about firefox debugging by platform folks which are looking for super precise powers.

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 9030825 [details] [diff] [review]
patch

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

I have only comments about the client usages as I know nothing about the debugger frontend.

I'm looking forward more progress on this to see if the currentWorkerThread() trick will apply to all methods from commands.js.

I'm also wondering how much this work is going to collide with bug 1494796, it will switch from using actor IDs to front references everything in the client modules. But at first sight, we should still be able to have similar lookup method, accepting an actor ID and returning a front, so, that should be fine.

And I suggested a couple of code moves that would help abstracting the pattern that can be shared/reused by other panels.

Otherwise, it is very existing to see workers being debugged in the same window \o/

::: devtools/client/debugger/new/src/client/firefox/commands.js
@@ +129,4 @@
>  }
>  
> +function sourceContents(sourceId: SourceId, thread: String): Source {
> +  const sourceClient = threadClient.source({ actor: sourceId, thread });

It looks like that's the only one request that you have to be doing to a specific threadClient.
It will be interesting for me to know if that's the only one as you make progress on this patch.
That's because it relates to actor proxies which I would like to align between tools and will surely require some special treatment when converting thread client to protocol.js.

Also, it looks like here you should be able to do:
  threadClient.lookupWorkerThread(thread).source({ actor: sourceId });
And revert the change made to SourceClient constructor.
So that it keeps all the mapping logic in commands.js

::: devtools/shared/client/thread-client.js
@@ +436,5 @@
> +  getSources: async function() {
> +    const rv = await this._getSourcesRaw();
> +    if (rv.sources) {
> +      for (const source of rv.sources) {
> +        source.threadActor = this.actor;

I believe we should streamline this idea of "parent"/"target".
This is also something I had to do for the inspector:
https://phabricator.services.mozilla.com/differential/changeset/?ref=101473&whitespace=ignore-most
This walker is similar to threadActor from inspector standpoint.

Also, ThreadClient will be soon the very last old fashion client. Yulia is converting the console client and is planning to convert the thread client to protocol.js the first weeks of january.
Then, we may streamline this into protocol.js and have all fronts have a unified way to expose its target context.

Note that here you are using the actor ID. That's something we will try/have to avoid in protocol.js as, by design we pass over fronts rather than actor ID. When migration to protocol.js, you can only do requests by calling a method on the right front instance.
Note that the usages of _activeThread in SourceClient are also going to be replaced by front method calls. We will pass around thread front instance rather than thread actor ID when doing the migration.

I well received Jason's request about fronts being painful to use from a redux context as you can only pass around JSON data and not object instances like fronts.
We should discuss about that and come up with a solution shared by other panels like about:debugging, but in the meantime I would suggest having some lookups method like what you did, or have maps if that's necessary/helpful.

@@ +469,5 @@
> +      workerThread.resume();
> +    }
> +    this._workerThreads = workerThreadClients;
> +    return workerThreadClients;
> +  },

This is for sure going to change with the upcoming work to define and implement a resource API. Hopefully this will happen in the first weeks of January.
Also, this code is independant from ThreadClient, out of `this`, it only uses `this._parent`.
It would be better to keep that next to its callsite, i.e. commands.js and have it accept threadClient._parent as argument. This is the target front. When debugging a web page, this is going to be an instance of https://searchfox.org/mozilla-central/source/devtools/shared/fronts/targets/browsing-context.js#11

@@ +477,5 @@
> +  },
> +
> +  currentWorkerThread() {
> +    return this._currentWorker ? this.lookupWorkerThread(this._currentWorker) : this;
> +  },

Ah! This is very interesting!
This is very similar to our discussion in fission group to have a proxy, that would automatically redirect the request to a given actor.
Here this is a one way router, you send the request to only one actor at a time.
This is different from inspector needs, that typically have to send the same request to all frames/targets and merge the result.
See https://phabricator.services.mozilla.com/differential/changeset/?ref=94044&whitespace=ignore-most
Line 557, we do the getSuggestionsForQuery for each target and later merge the result.

I draw two conclusions here:
* we should evaluate that. See if that one way proxy will fit for all debugger needs. See if that aligns with another panel requirements. The console looks like a good candidate.
* as for getWorkerClient, this looks like something that can be pulled out of ThreadClient and be its own proxy/wrapper/singleton living in commands.js or its dedicated module. This would help reuse it for another tool/actor.

Comment 8

[Brian Hackett [Laid off!]]

Attachment: WIP

Nearly complete WIP.  This has the following main changes from the earlier patch:

- The windowless worker UI is gated behind a new devtools.debugger.features.windowless-workers pref (off by default).

- All changes are now isolated to devtools/client/debugger/new/src, except for adding the above pref.

- Any notion of a 'current' thread has been isolated to the paused redux state.  In other places we either pass around the thread being operated on, or derive it from other information (like a source ID).

- firefox/commands.js has to have some more state so it can lookup information associated with each thread that was previously stored on the thread client.  Most of this seems fine, except we unfortunately need a map from source IDs to their associated thread.  This is because the thread client manages its sources, so we can't go from an arbitrary source to the associated source client without knowing which thread owns the source.  It would be nice to fix this in followup but it seems outside the scope of this bug.

This works pretty well from the testing I've done.  There are a couple known issues, which I think would best be fixed in followup:

- When multiple threads have sources for the same URL, we will only show one source tab for that URL.  The tab represents whichever source was most recently clicked on to open it.

- XHR breakpoints will only be set on the main thread.

Landing this might be tricky.  The problem is that this also needs to modify some files that are only stored in the github debugger.html repo (CSS and Flow type files), but worker debugging (both the old and new stuff) doesn't work in the standalone debugger.html.  My plan is to get this to pass try and then update it for github and review/land it there.

Comment 9

[Brian Hackett [Laid off!]]

Attachment: add pref

Add a preference for windowless workers.  The other changes can be made in github, the PR is here:

https://github.com/devtools-html/debugger.html/pull/7525

Comment 10

[Brian Hackett [Laid off!]]

Attachment: test

This landed to debugger.html and is now in m-c, though gated behind the devtools.debugger.features.windowless-workers pref for now.  The attached patch adds some utilities and a test for basic functionality.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/062104031126
Add test for windowless workers, r=jlast.

Comment 12

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/062104031126