1225473 - Support debugging service workers from e10s tabs

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect)

Description

[Alexandre Poirot [:ochameau]]

Currently, about:debugging only allows debugging service workers from non-e10s tabs.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: wip

Needs patch from bug 1225477 to live update when a new process is created.
This most likely lack some cleanup in the listeners to better support about:debugging reload/shutdown.
But it works great, even with mulitple child processes.
The only bad thing I can see is automatic worker shutdown after some timeout.

And tests... we would need tests for these new listWorkers methods.

Comment 2

[Jan Keromnes [:janx]]

Comment on attachment 8688449 [details] [diff] [review]
wip

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

Works great! Thank you Alex!

Also, Eddy is quit busy at the moment, but I've done similar reviews for him. If you'd like, I can review future iterations of this patch.

(In reply to Alexandre Poirot [:ochameau] from comment #1)
> This most likely lack some cleanup in the listeners to better support
> about:debugging reload/shutdown.

Not really sure what you mean here. Are you suspecting that some RDP messages might get stuck if about:debugging shuts down unexpectedly?

> But it works great, even with mulitple child processes.

It does!

> The only bad thing I can see is automatic worker shutdown after some timeout.

The is indeed not so great, but better than no worker debugging at all. And workers don't seem to time out when you leave them paused on a break point! (\o/)

> And tests... we would need tests for these new listWorkers methods.

Yes, tests for about:debugging would be cool!

::: devtools/client/aboutdebugging/components/workers.js
@@ +57,5 @@
>          targets: workers.other, client })
>      );
>    },
>  
> +  _listChildProcessWorkers() {

Nit: If you're going to use Promise, you might as well "un-spaghetti" this function by making it a generator (maybe using `Task.async`?)

Nit: Also please move the function to the end of the class, below the `update()` method, because it's a specific helper method we won't find in other components.

@@ +63,5 @@
> +    let client = this.props.client;
> +    return new Promise(resolve => {
> +      client.mainRoot.listProcesses(response => {
> +        // Iterate over all child processes
> +        let processes = response.processes.filter(p => (!p.parent));

Does it make sense to iterate over all processes, not just child processes? (With `client.mainRoot.listWorkers()` we get a few internal workers, I think it's fine to show them in "Other Workers").

@@ +89,5 @@
>      let workers = this.getInitialState().workers;
>      client.mainRoot.listWorkers(response => {
>        let forms = response.workers;
> +      this._listChildProcessWorkers().then(childForms => {
> +        forms = forms.concat(childForms);

Nit: Maybe move all the "form-getting" logic (including `client.mainRoot.listWorkers()`) into your new helper method (and maybe rename it something like "getWorkerForms()") in order to let the `update()` method focus on just transforming these forms into target objects?

::: devtools/server/actors/child-process.js
@@ +34,5 @@
>      .createInstance(Ci.nsIPrincipal);
>    let sandbox = Cu.Sandbox(systemPrincipal);
>    this._consoleScope = sandbox;
> +
> +  this._onWorkerListChanged = this.onWorkerListChanged.bind(this);

Nit: Please initialize your _workerActorPool and workerList attributes to `null` here. Also, please be consistent with your "_" prefix (either add it to the actual "onWorkerListChanged" method name, or remove it in the bind()ed attribute. Usually "private" attributes have a "_" prefix in devtools).

::: devtools/server/actors/worker.js
@@ +197,5 @@
>    _checkListening: function () {
>      if (this._onListChanged !== null && this._mustNotify) {
> +      try {
> +        // XXX: This throws if we register the same listener twice
> +        // Need to figure out why we end up calling twice?

FYI: Eddy found a way around this in bug 1218817 comment 3, but maybe there is a better solution.

::: devtools/server/content-server.jsm
@@ -18,5 @@
>  function init(msg) {
> -  if (started) {
> -    return;
> -  }
> -  started = true;

This looks like a drive-by change. What problem did you fix by doing that?

Comment 3

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8688449 [details] [diff] [review]
wip

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

Nothing wrong with this patch per se, but we don't actually need to be on the
child process unless we're forcing a ServiceWorker to create a WorkerPrivate.

The comments below explain what I mean in more detail:

1. This patch adds an onListWorkers request to ChildProcessActor. However, this
   request does not list ServiceWorkers: it lists their underlying
   WorkerPrivates, which represent the worker thread for a given ServiceWorker.

   This is a very important distinction to make. First of all, although you
   certainly can debug the WorkerPrivate of a ServiceWorker directly, you
   already noticed that these will expire eventually: ServiceWorkers are
   allowed to destroy their underlying WorkerPrivate when there are no more
   events to be processed.

2. To work around the problem described in 1, I've been working on extending the
   debugger protocol so that you can force a ServiceWorker to create a
   WorkerPrivate if one does not exist, and keep it alive while it is being
   debugged (even when we are not paused in it).

   The problem with forcing a WorkerPrivate to be created is that if you do
   this from the parent process, the WorkerPrivate will be created there, even
   when we already have a WorkerPrivate for the ServiceWorker in the child
   process.

   This is why we should only use ServiceWorkerActors from the child process,
   which is where your patch comes in.

3. In light of 2: if we're not forcing WorkerPrivates to be created, there is no
   reason for us to be on the child process. You can use onListWorkers to debug
   the WorkerPrivates for service workers directly if we wish, but in that case
   you could just implement it on RootActor. The onGetChildProcess machinery is
   only needed when you use the ServiceWorkerActors I'm currently implementing.

4. In short, we can do either of two things: decide to debug WorkerPrivates
   directly for now, in which case this patch is not needed. Or decide to use
   the new actors I'm writing, in which case you should use rewrite this
   patch so that it lists all child process service worker registrations,
   using the machinery in bug 1218817.

   I'm sufficiently close to finishing the debugger server parts (I will put
   the last patch up for review tomorrow. After that, there are 8 patches
   we need to land) that I think we should go with the second option.

********************************************************************************

Comment 4

[Alexandre Poirot [:ochameau]]

(In reply to Eddy Bruel [:ejpbruel] from comment #3)
> Comment on attachment 8688449 [details] [diff] [review]
> wip
> 
> Review of attachment 8688449 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nothing wrong with this patch per se, but we don't actually need to be on the
> child process unless we're forcing a ServiceWorker to create a WorkerPrivate.
> 
> The comments below explain what I mean in more detail:
> 
> 1. This patch adds an onListWorkers request to ChildProcessActor. However,
> this
>    request does not list ServiceWorkers: it lists their underlying
>    WorkerPrivates, which represent the worker thread for a given
> ServiceWorker.
> 
>    This is a very important distinction to make. First of all, although you
>    certainly can debug the WorkerPrivate of a ServiceWorker directly, you
>    already noticed that these will expire eventually: ServiceWorkers are
>    allowed to destroy their underlying WorkerPrivate when there are no more
>    events to be processed.

I'm all up to switch to ServiceWorkers API once you land them.

> 
> 2. To work around the problem described in 1, I've been working on extending
> the
>    debugger protocol so that you can force a ServiceWorker to create a
>    WorkerPrivate if one does not exist, and keep it alive while it is being
>    debugged (even when we are not paused in it).
> 
>    The problem with forcing a WorkerPrivate to be created is that if you do
>    this from the parent process, the WorkerPrivate will be created there,
> even
>    when we already have a WorkerPrivate for the ServiceWorker in the child
>    process.

And then, is the Requests and CacheStorage correctly be synced between processes?
Because if you end up spawning in parent process and it only affects the parent process requests and cache... it will be useless.

> 
>    This is why we should only use ServiceWorkerActors from the child process,
>    which is where your patch comes in.

It sounds like, no matter what, we need to be in child and this patch highlights a way to be in child from about:debugging. I don't get the feeback- here.

> 
> 3. In light of 2: if we're not forcing WorkerPrivates to be created, there
> is no
>    reason for us to be on the child process. You can use onListWorkers to
> debug
>    the WorkerPrivates for service workers directly if we wish, but in that
> case
>    you could just implement it on RootActor. The onGetChildProcess machinery
> is
>    only needed when you use the ServiceWorkerActors I'm currently
> implementing.

But then you can never debug the real world WorkerPrivates that are already running in childs.

> 
> 4. In short, we can do either of two things: decide to debug WorkerPrivates
>    directly for now, in which case this patch is not needed.

I repeat myself, but just to be clear, you need this patch to debug WorkerPrivates in e10s firefox.
Have you tested about:debugging with and without this patch against a simple SW demo?

>    Or decide to use
>    the new actors I'm writing, in which case you should use rewrite this
>    patch so that it lists all child process service worker registrations,
>    using the machinery in bug 1218817.

We will surely use your work to freeze the workers no matter in which process we end up into.
I'm especially looking forward your patch to freeze the workers. Will it be just a parameter on existing WorkerActor class. So that WorkerActorList would toggle this when instiating a dbg.type == Ci.nsIWorkerDebugger.TYPE_SERVICE, or may be just always toggle this until the actor is released.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: promisify all client methods

Small patch that would help taskifying the getWorkerForms helper...
I'm bit scared it may break tests:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5c6dbf9aa1d
but would help simplify tons of code in our codebase where we had
to create intermediate promises when using task and any client method!
I'll move that to its own bug if tests aren't like a chistmas tree.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

Addressing most review comments.

(In reply to Jan Keromnes [:janx] from comment #2)
> @@ +63,5 @@
> > +    let client = this.props.client;
> > +    return new Promise(resolve => {
> > +      client.mainRoot.listProcesses(response => {
> > +        // Iterate over all child processes
> > +        let processes = response.processes.filter(p => (!p.parent));
> 
> Does it make sense to iterate over all processes, not just child processes?
> (With `client.mainRoot.listWorkers()` we get a few internal workers, I think
> it's fine to show them in "Other Workers").

Hum may be. May be not. today listWorkers is only implemented on RootActor.
The parent process instanciates a ChromeActor via listProcess,
which inherits from TabActor. So it doesn't have listWorkers.
Whereas all the child processes instanciate ChildProcessActor,
to which I just added listWorkers.
The actor hierarchy and inheritance is quite complex.
So I don't know what is best.
But I think at least, we should share listWorkers implementation,
may be not by inheritance, but at least by composition.

> ::: devtools/server/content-server.jsm
> @@ -18,5 @@
> >  function init(msg) {
> > -  if (started) {
> > -    return;
> > -  }
> > -  started = true;
> 
> This looks like a drive-by change. What problem did you fix by doing that?

Yes, it's not directly related, it allows to create ChildProcessActor
more than just once. Here, every time we reload about:debugging we create a new one.
But I have to verify it doesn't brake the Browser Content Toolbox,
I remember it was an important check.


Still need to address some comments and a test, and we should be ready to go?

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

With a test working on non-e10s/e10s!!

I adressed all the comments except:
> ::: devtools/server/actors/worker.js
> @@ +197,5 @@
> >    _checkListening: function () {
> >      if (this._onListChanged !== null && this._mustNotify) {
> > +      try {
> > +        // XXX: This throws if we register the same listener twice
> > +        // Need to figure out why we end up calling twice?

Hopefully eddy's patch is going to land and fix that.

Otherwise, I don't remember exactly what was the real thing `started` fixed
in content-process.jsm. I thought it fixed some issues with content toolbox,
but I verified and actually, it seems to introduce one instead.
Because of this check, we can't spawn a content toolbox more than once for a given process.

Also, I had to tweak worker.js to ease testing:
- add an id to worker list elements
- send an event after render to know when to execute assertion against the UI
  (I used a observer service notification, but I'm open to alternatives)

Comment 8

[Jan Keromnes [:janx]]

Comment on attachment 8691975 [details] [diff] [review]
patch v3

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

Works great and looks good to me!

All except the test-specific observer event. Is it possible to use a MutationObserver instead? (more details inline)

(In reply to Alexandre Poirot [:ochameau] from comment #7)
> With a test working on non-e10s/e10s!!

Very cool, thanks! \o/

> Otherwise, I don't remember exactly what was the real thing `started` fixed
> in content-process.jsm. I thought it fixed some issues with content toolbox,
> but I verified and actually, it seems to introduce one instead.
> Because of this check, we can't spawn a content toolbox more than once for a
> given process.

Ok, I'll trust you on this drive-by fix.

(In reply to Alexandre Poirot [:ochameau] from comment #4)
> (In reply to Eddy Bruel [:ejpbruel] from comment #3)
> >    Or decide to use
> >    the new actors I'm writing, in which case you should use rewrite this
> >    patch so that it lists all child process service worker registrations,
> >    using the machinery in bug 1218817.
> 
> We will surely use your work to freeze the workers no matter in which
> process we end up into.
> I'm especially looking forward your patch to freeze the workers. Will it be
> just a parameter on existing WorkerActor class. So that WorkerActorList
> would toggle this when instiating a dbg.type ==
> Ci.nsIWorkerDebugger.TYPE_SERVICE, or may be just always toggle this until
> the actor is released.

I think that "freezing" a worker as soon as there is an actor for it (i.e. it's being listed somewhere in about:debugging) is too big of a behavior change relative to how these workers normally operate.

Instead, maybe we should freeze (i.e. prevent from being killed, I think that's actually different from freezing) a worker when we actually attach to its actor to get a client for it, i.e. a toolbox was opened? That's how Chromium does it: workers can die pretty fast, but once you've opened a toolbox on one it stays alive until you're done.

::: devtools/client/aboutdebugging/components/workers.js
@@ +34,5 @@
>    },
>  
>    componentDidMount() {
>      this.props.client.addListener("workerListChanged", this.update);
> +    this.props.client.addListener("processListChanged", this.update);

Nit: Please factor out `let client = this.props.client`.

@@ +44,1 @@
>      this.props.client.removeListener("workerListChanged", this.update);

Nit: Please factor out `let client = this.props.client`.

@@ +46,5 @@
>  
> +  componentDidUpdate() {
> +    // Notify tests about the update
> +    Services.obs.notifyObservers(null, "about-debugging-update-workers", null);
> +  },

If possible, could you instead please use a MutationObserver in the test, on the ID "service-workers"? I'd prefer it if we can avoid adding events that are only useful for tests.

@@ +94,5 @@
>        this.setState({ workers });
>      });
> +  },
> +
> +  _getWorkerForms: Task.async(function*() {

Nit: Since these React components don't expose "public" methods/attributes, there is no need to prefix "private" methods/attributes with a "_". Please remove the "_" prefix from this method.

@@ +99,5 @@
> +    let client = this.props.client;
> +
> +    // List workers from the Parent process
> +    let { workers } = yield client.mainRoot.listWorkers();
> +    let forms = workers;

Nit: No need for the unboxing, please use `let result = yield` and `let forms = result.workers`.

@@ +101,5 @@
> +    // List workers from the Parent process
> +    let { workers } = yield client.mainRoot.listWorkers();
> +    let forms = workers;
> +
> +    // And then the one from the Child processes

Nit: Please remove "the one".

::: devtools/client/aboutdebugging/test/browser_service_workers.js
@@ +28,5 @@
> +  });
> +
> +  let { tab, document } = yield openAboutDebugging("addons");
> +
> +  Services.prefs.setBoolPref("", true);

Why?

@@ +35,5 @@
> +
> +  yield waitForWorkersUpdate();
> +
> +  // Check that the service worker appears in the UI
> +  let names = [...document.querySelectorAll("#workers #service-workers .target-name")];

Nit: No need for the #workers selector here.

@@ +57,5 @@
> +
> +  yield waitForWorkersUpdate();
> +
> +  // Check that the service worker disappeared from the UI
> +  names = [...document.querySelectorAll("#workers #service-workers .target-name")];

Nit: No need for the #workers selector here.

Comment 9

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=775959b180fe

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8db0b48571b1
Everything addressed and with a MutationObserver instead of custom event.
(the empty setBoolPref was just a left-over)

And about the worker auto-kill, I imagine increasing the timeout while
devtools (about:debugging#workers) are opened may also help,
while also freezing it as soon as we open a toolbox against it.
But yes, freezing them completely while about:debugging is opened doesn't look like
a good option.

Comment 11

[Alexandre Poirot [:ochameau]]

The try build is orange as I forgot to push bug 1227474 patch to try...

Comment 12

[Jan Keromnes [:janx]]

Comment on attachment 8692485 [details] [diff] [review]
patch v4

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

All good now! \o/

::: devtools/client/aboutdebugging/components/workers.js
@@ +100,5 @@
> +    let forms = result.workers;
> +
> +    // And then from the Child processes
> +    let { processes } = yield client.mainRoot.listProcesses();
> +    for(let process of processes) {

Nit: Please add a space between `for` and `(`.

::: devtools/client/aboutdebugging/test/browser_service_workers.js
@@ +11,5 @@
> +
> +function waitForWorkersUpdate(document) {
> +  return new Promise(done => {
> +    var observer = new MutationObserver(function(mutations) {
> +      dump("mutation!\n");

Nit: Maybe we won't need this leftover debug log anymore?

Comment 13

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b7c904c27ac

Comment 14

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=911aa31d2eb8

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: patch v5

Rebased against last tip.
I was able to remove the TODO - fix duplicate call to wdm.addListener
as it should have been fixed on master.

Comment 16

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d687100ebc8a

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: Interdiff

The test was leaking (see next patch) and I had to tweak a few things:
 - ensure to open the right tab in tests,
 - reset onListChanged to ensure not sending RDP message after the client is closed.

Comment 18

[Alexandre Poirot [:ochameau]]

Attachment: fix leaks - v1

So running one test (the addons one) is fine,
but running more introduces various issues because:
 - about:debugging doesn't close its DebuggerClient instance.
   Instead it creates a new one every time, and let a zombie one still registered.
 - we fetch resources for all panels no matter which one you open.
   This is sub-optimal, especially if we plan to add more tabs.
   And worse, it appears to execute RDP requests after the test is finished.
   (listWorker request were still pending after the addons test is done!)

This patch simply:
- close the DebuggerClient when we close about:debugging
- execute Tab code only when we open it.

Comment 19

[Alexandre Poirot [:ochameau]]

Tests all green!

Comment 20

[Jan Keromnes [:janx]]

Comment on attachment 8694206 [details] [diff] [review]
fix leaks - v1

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

Thanks for fixing these issues, seems works fine on my side.

I guess this patch is one more point in favor of Reactifying all of about:debugging...

(In reply to Alexandre Poirot [:ochameau] from comment #19)
> Tests all green!

I think that sometimes, unexpected failures happen only on non-linux platforms. Just out of sheer paranoia, please include at least one Windows and on MacOS in your future try pushes (no need to do that now though).

::: devtools/client/aboutdebugging/aboutdebugging.js
@@ +70,5 @@
> +        document.querySelector("#addons"));
> +    } else if (category == "workers") {
> +      React.render(React.createElement(WorkersComponent, { client: this.client }),
> +        document.querySelector("#workers"));
> +    }

Note: This gets called several times if you switch between about:debugging tabs:
- I suppose it's not a problem to call `React.render()` several times with a new Component on the same DOM node?
- Or that we don't unmount Components when switching to a different tab?

Comment 21

[Jan Keromnes [:janx]]

Comment on attachment 8694205 [details] [diff] [review]
Interdiff

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

Thanks for the interdiff! Works fine on my side.

It's OK to fold this into your v5 patch.

::: devtools/server/actors/child-process.js
@@ +124,5 @@
>    disconnect: function() {
>      this.conn.removeActorPool(this._contextPool);
>      this._contextPool = null;
> +
> +    /* Tell the live lists we aren't watching any more. */

Nit: I think the expected comment style here would be `//` (`/* */` seems to be used only to document methods).

Comment 22

[Alexandre Poirot [:ochameau]]

(In reply to Jan Keromnes [:janx] from comment #20)
> Comment on attachment 8694206 [details] [diff] [review]
> ::: devtools/client/aboutdebugging/aboutdebugging.js
> @@ +70,5 @@
> > +        document.querySelector("#addons"));
> > +    } else if (category == "workers") {
> > +      React.render(React.createElement(WorkersComponent, { client: this.client }),
> > +        document.querySelector("#workers"));
> > +    }
> 
> Note: This gets called several times if you switch between about:debugging
> tabs:
> - I suppose it's not a problem to call `React.render()` several times with a
> new Component on the same DOM node?

Yes, I'm not an expert, but React should be smart here.

> - Or that we don't unmount Components when switching to a different tab?

That's a possible optimisation if we end up having tons of tabs, we may unload some?
But it doesn't sounds very important for now.

Comment 23

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/310bf739e766fb1755e48c3e53f9d9aa73352f56
Bug 1225473 - Support Service workers in child process. r=janx

https://hg.mozilla.org/integration/fx-team/rev/20fc590db5655f299da87df165845d2f0127fbb5
Bug 1225473 - Cleanup DebuggerClient on about:debugging close and fetch only the target of the currently opened tab. r=janx

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/310bf739e766
https://hg.mozilla.org/mozilla-central/rev/20fc590db565