Closed Bug 1212797 Opened 5 years ago Closed 5 years ago

Show all registered service workers in about:debugging

Categories

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

defect

Tracking

(firefox47+ fixed, relnote-firefox 47+)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 + fixed
relnote-firefox --- 47+

People

(Reporter: janx, Assigned: janx)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 12 obsolete files)

10.43 KB, patch
janx
: review+
Details | Diff | Splinter Review
12.29 KB, patch
janx
: review+
Details | Diff | Splinter Review
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.
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.
Flags: needinfo?(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.
Flags: needinfo?(janx)
Blocks: 1214248
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?
Flags: needinfo?(ejpbruel)
Component: Developer Tools → Developer Tools: about:debugging
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.
Flags: needinfo?(ejpbruel)
Depends on: 1216566
Depends on: 1218363
Depends on: 1218817
Depends on: 1219205
Depends on: 1219255
Depends on: 1220740
Depends on: 1220741
Depends on: 1221892
Depends on: 1223766
Depends on: 1207702
Blocks: 1220747
Summary: List registrations instead of live Service Workers in about:debugging → Group service workers by registration in about:debugging
Depends on: 1224237
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).
Summary: Group service workers by registration in about:debugging → Show all registered service workers in about:debugging
Depends on: 1226285
Blocks: 1209699
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).
Attachment #8707362 - Flags: feedback?(poirot.alex)
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)
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.
Attachment #8707362 - Flags: feedback?(poirot.alex)
Attachment #8707901 - Flags: review?(poirot.alex)
Attachment #8707902 - Flags: feedback?(poirot.alex)
Attachment #8707362 - Attachment is obsolete: true
Blocks: 1239705
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)
Attachment #8707902 - Flags: feedback?(poirot.alex) → review?(poirot.alex)
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.
Attachment #8707901 - Flags: review?(poirot.alex) → review+
Attachment #8707901 - Attachment is obsolete: true
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.
Attachment #8707902 - Flags: review?(poirot.alex)
Depends on: 1232931
No longer depends on: 1218817, 1220741, 1221892, 1226285
Rebased, r+ carried over.
Attachment #8710401 - Flags: review+
Attachment #8708277 - Attachment is obsolete: true
(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.
Attachment #8710402 - Flags: review?(poirot.alex)
Attachment #8707902 - Attachment is obsolete: true
(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.
(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.
Attachment #8710406 - Flags: review?(poirot.alex)
Attachment #8710402 - Attachment is obsolete: true
Attachment #8710402 - Flags: review?(poirot.alex)
Keywords: dev-doc-needed
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.
Attachment #8710406 - Flags: review?(poirot.alex) → review+
Depends on: 1241841
Rebased, carried over.
Attachment #8710980 - Flags: review+
Attachment #8710401 - Attachment is obsolete: true
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.
Attachment #8710981 - Flags: review+
Attachment #8710406 - Attachment is obsolete: true
Depends on: 1242482
Assignee: nobody → janx
New try with new dependency patches by ochameau:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fa6dfc90de0
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.
Actually, failure 2. seems to be fixed by Alex's latest patch in bug 1242482. \o/
[Tracking Requested - why for this release]: Base bug for new Service Worker & Push features that we'd like to announce with Developer Edition 47.
Ritu just want to make sure this is on your radar
Flags: needinfo?(rkothari)
Tracking because this part of a new feature
Hi Kit, is this something you might help investigate and possibly fix? Thanks!
Flags: needinfo?(rkothari) → needinfo?(kcambridge)
(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.
Flags: needinfo?(kcambridge)
Rebased, carried over.
Attachment #8719534 - Flags: review+
Attachment #8710980 - Attachment is obsolete: true
Rebased, carried over.

New try including latest dependency patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=046b1634f7b3
Attachment #8719535 - Flags: review+
Attachment #8710981 - Attachment is obsolete: true
Attached patch interdiff.patch (obsolete) — Splinter Review
Interdiff of the second patch FYI, because a few things changed during rebase.
Tree is looking great!
Priority: -- → P1
Rebased, carried over.
Attachment #8722999 - Flags: review+
Attachment #8719534 - Attachment is obsolete: true
Attachment #8719536 - Attachment is obsolete: true
Attachment #8719535 - Attachment is obsolete: true
`dt5` oranges on linux debug look a bit worrying, but should be unrelated.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2893ca049d37
https://hg.mozilla.org/mozilla-central/rev/f695820b8269
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
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)]:
relnote-firefox: --- → ?
dev-doc-needed: The relevant MDN page is here https://developer.mozilla.org/en-US/docs/Tools/about:debugging
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.