1212797 - Show all registered service workers in about:debugging

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect, P1)

Description

[Jan Keromnes [:janx]]

Currently, we list *live* service workers in about:debugging. The problem is that they are transient (and tend to die frequently).

Instead, we should list service worker *registrations*, which are more permanent.

However, there can be either 0, 1 or 2 live service worker(s) associated to one registration at any given time:

0) Clicking on "Debug" next to the registration should fire up a new service worker, then open a toolbox on it.

1) Clicking on "Debug" should open a toolbox on the one live service worker.

2) A service worker is finishing up, and a new one is installing. Clicking on "Debug" should probably wait for the new service worker to be live, then open a toolbox on it.

Comment 1

[Axel Kratel]

Jan, doesn't this bug duplicate https://bugzilla.mozilla.org/show_bug.cgi?id=1209369? The user story in that bug says:

From a global discovery list:

(To see how this works in Chrome, use the demo https://people.mozilla.org/~ewong2/push-notification-test/ and then go to: chrome://serviceworker-internals/ to see the list of workers.

Global service worker discovery: a developer should be able to go to a global list of all service workers across all tabs, and be able to see what service workers are registered.

Each service worker listing includes meta-data about the worker: 
Start time
Script path
Scope, i.e. which page/URL registered the SW
Registration metadata
SW type, e.g. push/batch, etc...

For all types of SW, should be able to start, stop, unregister. 

For SW types, additional buttons/functions need to be included:
Push: add a sample push notification test

A developer can select for inspection an SW from the global list

There is an option to check to make sure the SW is paused on start when loaded

When the SW is selected for inspection, the SW automatically loaded/running so it can be inspected and debugged. If the option to pause was checked, the SW is paused in the debugger on load.

An inspection/debugging/console tool box opens (UI? Is that via app manager?) and the developer can now inspect and debug the service worker.

Platform: The SW stays alive as long as the developer is inspecting it and/or debugging it, i.e. until he closes the tool box.

Comment 2

[Jan Keromnes [:janx]]

Bug 1209369 says many things:
- Service worker discovery (done because about:debugging landed)
- Show service worker meta-data (TODO) and status (paused, pause-on-start, etc, TODO)
- Ability to start/stop/unregister a service worker (TODO)
- Push sample payload (bug 1209699)
- Opening a toolbox on a service worker (done in about:debugging)
- Platform: Service workers need to stay alive as long as the developer is inspecting/debugging it (I'm not sure we still want to go that route)

It also vaguely hints toward the fact that we should list "registrations" (i.e. even when no live service worker is running for that registration).

I generally prefer to give bugs more focus by making them about one particular feature. I'm fine with using 1209369 as a meta-bug, or to close it because it's called "Service worker discovery" which already exists in about:debugging.

Comment 3

[Jan Keromnes [:janx]]

Eddy, we talked about this on IRC. You mentioned that, following a discussion with Ehsan, you could give me some pointers about how to address this. Could you please share them here?

Comment 4

[Eddy Bruel [:ejpbruel]]

I talked with Jan about this on irc. The gist of it is that we need to extend the debugger protocol to list service worker registrations. Internally, the debugger server will use ServiceWorkerManager to obtain a global list of registrations.

Comment 5

[Jan Keromnes [:janx]]

Right now in about:debugging, we enumerate all nsIWorkerDebugger's of TYPE_SERVICE to get the current list of Service Workers.

The problem with this approach is that these things are "worker privates", which can be killed at any time when they're idle. If one is killed, it won't show up in our "Service Workers" list anymore, even though it's still a registered Service Worker.

The solution to this is to list all registered Service Workers, even when there is no worker private (bug 1218817 should give us ServiceWorkerRegistrationActors), and find out if there are any installing/waiting/active Service Workers we can debug (bug 1220741 will give us ServiceWorkerActors, and bug 1219255 will make sure that when we ask to debug a ServiceWorkerActor but no worker private is available, a new one is fired up for us).

Comment 6

[Jan Keromnes [:janx]]

Attachment: WIP Show all registered service workers in about:debugging.

Work-in-progress.

In about:debugging > Workers > Service Workers, this patch lists ServiceWorkerRegistrationActors instead of WorkerActors (but if there is a WorkerActor available, it saves its actor ID on the registration).

When there is no WorkerActor for a registration, this patch doesn't show the "Debug" or "Push" buttons, but ideally we could find a way to spawn a WorkerActor if needed, and always show these buttons.

Also, this patch shows weird issues when we try "Push" on a WorkerActor: it only works once. The second time, the request doesn't event seem to reach the actor, until the page is refreshed (new connection, works again but still only once).

Comment 7

[Jan Keromnes [:janx]]

To reproduce the problem I'm seeing:

1. Apply patch
2. Go to about:debugging > Workers
3. Open tab on http://simple-push-demo.appspot.com/, click on "send a push to gcm via xhr" (notification appears)
4. In about:debugging > Workers > Service Workers, click on "Push" next to simple-push-demo (2nd notification appears)
5. Then, click on "Push" again.

Expected: 3rd notification.

Actual: Nothing happens.

Here are my logs:
REGISTRATION https://simple-push-demo.appspot.com/service-worker.js server1.conn2.serviceWorkerRegistration1
REGISTRATION https://mdn.github.io/sw-test/sw.js server1.conn2.serviceWorkerRegistration2
REGISTRATION https://plus.google.com/serviceworker.js server1.conn2.serviceWorkerRegistration3
SERVICEWORKER https://simple-push-demo.appspot.com/service-worker.js server1.conn2.worker6
PUSHING TO server1.conn2.worker6 (first click on "Push" works!)
WORKER onPush server1.conn2.worker6
WORKER onPush sent!
PUSHING TO server1.conn2.worker6 (subsequent clicks never seem to reach the actor, nothing happens)

Comment 8

[Jan Keromnes [:janx]]

Note: Problem was resolved via IRC (thanks Alex!), the "push" WorkerActor call didn't return anything so the protocol ended up waiting for a response forever. Fixed by refactoring WorkerActor to use protocol.js.

Comment 9

[Jan Keromnes [:janx]]

Attachment: Refactor worker actors to protocol.js.

Comment 10

[Jan Keromnes [:janx]]

Attachment: Show all registered service workers in about:debugging.

Comment 11

[Jan Keromnes [:janx]]

Alex, a few notes on the patches:

Refactor worker actors to protocol.js
- This is mostly your patch from bug 1226285, applied also to ServiceWorkerRegistrationActor.
- I also added a few minor things I'll need later (some form attributes and a private method).

Show all registered service workers in about:debugging
- This patch makes about:debugging list registrations instead debuggers, for service workers.
- If a worker debugger is found, its actor is added to the registration, so that you can debug it (even in multi-process e10s)

Comment 12

[Alexandre Poirot [:ochameau]]

Comment on attachment 8707901 [details] [diff] [review]
Refactor worker actors to protocol.js.

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

Looks good, check that tests are green.

::: devtools/server/actors/worker.js
@@ +2,5 @@
>  
>  var { Ci, Cu } = require("chrome");
>  var { DebuggerServer } = require("devtools/server/main");
> +const protocol = require("devtools/server/protocol");
> +const { Arg, Option, method, RetVal, types } = protocol;

nit: Option and types aren't used.

Comment 13

[Jan Keromnes [:janx]]

Attachment: Refactor worker actors to protocol.js.

Nits addressed, keeping r+.

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

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 8707902 [details] [diff] [review]
Show all registered service workers in about:debugging.

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

Looks good, but can you write a test for regristration?
Doesn't that break the existing tests?
It would be great to cover a service worker registration with and without workerActor.

::: devtools/client/aboutdebugging/components/target.js
@@ +38,5 @@
>          React.createElement("div", { className: "target-name" }, target.name),
>          React.createElement("div", { className: "target-url" }, target.url)
>        ),
> +      (isRunning ? React.createElement("button", { onClick: this.debug },
> +        Strings.GetStringFromName("debug")) : null)

nit: the indentation doesn't help reading this code.

May be something like that would help?
(isRunning ?
 React.createElement("button",
   { onClick: this.debug },
   Strings.GetStringFromName("debug")) :
 null)

::: devtools/client/aboutdebugging/components/workers.js
@@ +73,5 @@
> +      forms.registrations.forEach(form => {
> +        workers.service.push({
> +          type: "serviceworker",
> +          icon: WorkerIcon,
> +          name: form.url,

Shouldn't we use `url` as attribute name?
Especially since we compare it later for form.url on line 93.

Comment 15

[Jan Keromnes [:janx]]

Attachment: Refactor worker actors to protocol.js.

Rebased, r+ carried over.

Comment 16

[Jan Keromnes [:janx]]

Attachment: Show all registered service workers in about:debugging.

(In reply to Alexandre Poirot [:ochameau] from comment #14)
> Looks good, but can you write a test for regristration?
> Doesn't that break the existing tests?
> It would be great to cover a service worker registration with and without
> workerActor.

It did break existing tests, but registrations with/out a workerActor are now covered by browser_service_worker_timeout.js.

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

> > +      (isRunning ? React.createElement("button", { onClick: this.debug },
> > +        Strings.GetStringFromName("debug")) : null)
> 
> nit: the indentation doesn't help reading this code.

Fixed to your suggestion.

> > +        workers.service.push({
> > +          type: "serviceworker",
> > +          icon: WorkerIcon,
> > +          name: form.url,
> 
> Shouldn't we use `url` as attribute name?
> Especially since we compare it later for form.url on line 93.

Yes, but actually I changed this to `scope`, because sometimes registrations don't have a `url` (scriptSpec) yet.

Comment 17

[Jan Keromnes [:janx]]

(In reply to Jan Keromnes [:janx] from comment #16)
> > > +      (isRunning ? React.createElement("button", { onClick: this.debug },
> > > +        Strings.GetStringFromName("debug")) : null)
> > 
> > nit: the indentation doesn't help reading this code.
> 
> Fixed to your suggestion.

Not here actually, but in a follow-up patch for bug 1239705, sorry.

Comment 18

[Jan Keromnes [:janx]]

Attachment: Show all registered service workers in about:debugging.

(In reply to Jan Keromnes [:janx] from comment #17)
> > Fixed to your suggestion.
> 
> Not here actually, but in a follow-up patch for bug 1239705, sorry.

Well, actually that little rebase problem also causes the "target-button-debug" className to be missing, so I fixed it.

Comment 19

[Jan Keromnes [:janx]]

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

Comment 20

[Alexandre Poirot [:ochameau]]

Comment on attachment 8710406 [details] [diff] [review]
Show all registered service workers in about:debugging.

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

::: devtools/client/aboutdebugging/components/target-list.js
@@ +23,5 @@
>  
>    render() {
>      let client = this.props.client;
> +    let targets = this.props.targets
> +      .filter(target => target.name)

Do we have target with no name?
What are those? Shouldn't we just not have them in targets list at first place??
Please at least put a comment.

::: devtools/client/aboutdebugging/components/target.js
@@ +32,5 @@
> +    let isRunning = (!isServiceWorker || target.workerActor);
> +    return React.createElement("div", { className: "target" },
> +      React.createElement("img", {
> +        className: "target-icon",
> +        src: target.icon }),

nit: may be a good followup, it looks weird to specify the url from code like this, may be we should use a data attribute or classname to let the css control the icon?

::: devtools/client/aboutdebugging/components/workers.js
@@ +77,5 @@
> +          type: "serviceworker",
> +          icon: WorkerIcon,
> +          name: form.url,
> +          scope: form.scope,
> +          actor: form.actor

It may help to name this one `registrationActor`, as we later have `workerActor`. Or we can rename workers.service to something else like workers.serviceRegistrations.

@@ +93,5 @@
>          switch (form.type) {
>            case Ci.nsIWorkerDebugger.TYPE_SERVICE:
> +            for (let registration of workers.service) {
> +              if (registration.scope === form.scope) {
> +                registration.name = form.url;

Why do we rename the registration with the actor script url?
What are the precise scenario where it is different?
Please at least put a comment.

Comment 21

[Jan Keromnes [:janx]]

Attachment: Refactor worker actors to protocol.js.

Rebased, carried over.

Comment 22

[Jan Keromnes [:janx]]

Attachment: Show all registered service workers in about:debugging.

Rebased, carried over.

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

(In reply to Alexandre Poirot [:ochameau] from comment #20)
> >    render() {
> >      let client = this.props.client;
> > +    let targets = this.props.targets
> > +      .filter(target => target.name)
> 
> Do we have target with no name?
> What are those? Shouldn't we just not have them in targets list at first
> place??
> Please at least put a comment.
> 
> > +            for (let registration of workers.service) {
> > +              if (registration.scope === form.scope) {
> > +                registration.name = form.url;
> 
> Why do we rename the registration with the actor script url?
> What are the precise scenario where it is different?
> Please at least put a comment.

Well, the issue with `target`s that don't have a `name` came from a race causing some `ServiceWorkerRegistrationInfo`s that did't have a `scriptSpec` yet.

I changed the code to explicitely filter out these registrations instead of all targets, and added some comments to explain.

> > +      React.createElement("img", {
> > +        className: "target-icon",
> > +        src: target.icon }),
> 
> nit: may be a good followup, it looks weird to specify the url from code
> like this, may be we should use a data attribute or classname to let the css
> control the icon?

Well, I think it's good that icons are set from the code because sometimes we'll need to set different icons. Addons for example each have their own icon, so do tabs (favicons). Workers don't right now, but we never know (and maybe we'd like them to show website favicon as well?)

> ::: devtools/client/aboutdebugging/components/workers.js
> @@ +77,5 @@
> > +          type: "serviceworker",
> > +          icon: WorkerIcon,
> > +          name: form.url,
> > +          scope: form.scope,
> > +          actor: form.actor
> 
> It may help to name this one `registrationActor`, as we later have
> `workerActor`. Or we can rename workers.service to something else like
> workers.serviceRegistrations.

Good idea, done.

Comment 23

[Jan Keromnes [:janx]]

New try with new dependency patches by ochameau:

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

Comment 24

[Jan Keromnes [:janx]]

Summary of all currently blocking failures:

1. Crash at end of browser_service_workers_timeout.js on Linux debug (GC problem, LinkedList not empty, bug 1241841).

2. Time out in browser_service_workers_timeout.js in e10s because platform doesn't propagate "Unregister" correctly (bug 1242482).

Failure 2. can be worked around by extracting the code that tests "Unregister" into a separate mochitest, and add it to bug 1242482. Hopefully, failure 1. can also be worked around by tweaking the test. All hope is not lost.

Comment 25

[Jan Keromnes [:janx]]

Actually, failure 2. seems to be fixed by Alex's latest patch in bug 1242482. \o/

Comment 26

[Jan Keromnes [:janx]]

Attempting to confirm comment 25: https://treeherder.mozilla.org/#/jobs?repo=try&revision=321043ee37d2

Comment 27

[Jan Keromnes [:janx]]

[Tracking Requested - why for this release]: Base bug for new Service Worker & Push features that we'd like to announce with Developer Edition 47.

Comment 28

[Benjamin Kerensa [:bkerensa]]

Ritu just want to make sure this is on your radar

Comment 29

[Benjamin Kerensa [:bkerensa]]

Tracking because this part of a new feature

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Kit, is this something you might help investigate and possibly fix? Thanks!

Comment 31

[Lina Butler [:lina]]

(In reply to Ritu Kothari (:ritu) from comment #30)
> Hi Kit, is this something you might help investigate and possibly fix?
> Thanks!

It looks like :bkelly already has bug 1242482 on his radar. I don't really know the service worker code...just the parts involved in Push.

Comment 32

[Jan Keromnes [:janx]]

Attachment: Refactor worker actors to protocol.js.

Rebased, carried over.

Comment 33

[Jan Keromnes [:janx]]

Attachment: Show all registered service workers in about:debugging.

Rebased, carried over.

New try including latest dependency patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=046b1634f7b3

Comment 34

[Jan Keromnes [:janx]]

Attachment: interdiff.patch

Interdiff of the second patch FYI, because a few things changed during rebase.

Comment 35

[Jan Keromnes [:janx]]

Tree is looking great!

Comment 36

[Jan Keromnes [:janx]]

Attachment: Refactor worker actors to protocol.js.

Rebased, carried over.

Comment 37

[Jan Keromnes [:janx]]

Attachment: Show all registered service workers in about:debugging.

Rebased, carried over.

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

Comment 38

[Jan Keromnes [:janx]]

`dt5` oranges on linux debug look a bit worrying, but should be unrelated.

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2893ca049d37
https://hg.mozilla.org/integration/fx-team/rev/f695820b8269

Comment 40

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

https://hg.mozilla.org/mozilla-central/rev/2893ca049d37
https://hg.mozilla.org/mozilla-central/rev/f695820b8269

Comment 41

[Jan Keromnes [:janx]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Part of new Devtools for Service Workers, allows users to view and debug all the registered SWs in about:debugging#workers.
[Suggested wording]:
[Links (documentation, blog post, etc)]:

Comment 42

[Jan Keromnes [:janx]]

dev-doc-needed: The relevant MDN page is here https://developer.mozilla.org/en-US/docs/Tools/about:debugging

Comment 43

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx47 (Aurora) release notes.

Comment 44

[Will Bamberg [:wbamberg]]

This is done: https://developer.mozilla.org/en-US/docs/Tools/about:debugging#Workers