Show all registered service workers in about:debugging

RESOLVED FIXED in Firefox 47

Status

P1
normal
RESOLVED FIXED
3 years ago
7 months ago

People

(Reporter: janx, Assigned: janx)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
Firefox 47
dev-doc-complete
Dependency tree / graph

Firefox Tracking Flags

(firefox47+ fixed, relnote-firefox 47+)

Details

Attachments

(2 attachments, 12 obsolete attachments)

10.43 KB, patch
janx
: review+
Details | Diff | Splinter Review
12.29 KB, patch
janx
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
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

3 years ago
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)
(Assignee)

Comment 2

3 years ago
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)

Updated

3 years ago
Blocks: 1214248
(Assignee)

Comment 3

3 years ago
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)
(Assignee)

Updated

3 years ago
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
(Assignee)

Updated

3 years ago
Blocks: 1220747
Summary: List registrations instead of live Service Workers in about:debugging → Group service workers by registration in about:debugging
Depends on: 1224237
(Assignee)

Comment 5

3 years ago
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
(Assignee)

Updated

3 years ago
Blocks: 1209699
(Assignee)

Comment 6

3 years ago
Created attachment 8707362 [details] [diff] [review]
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).
Attachment #8707362 - Flags: feedback?(poirot.alex)
(Assignee)

Comment 7

3 years ago
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)
(Assignee)

Comment 8

3 years ago
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)
(Assignee)

Comment 9

3 years ago
Created attachment 8707901 [details] [diff] [review]
Refactor worker actors to protocol.js.
Attachment #8707901 - Flags: review?(poirot.alex)
(Assignee)

Comment 10

3 years ago
Created attachment 8707902 [details] [diff] [review]
Show all registered service workers in about:debugging.
Attachment #8707902 - Flags: feedback?(poirot.alex)
(Assignee)

Updated

3 years ago
Attachment #8707362 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Blocks: 1239705
(Assignee)

Comment 11

3 years ago
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)
(Assignee)

Updated

3 years ago
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+
(Assignee)

Comment 13

3 years ago
Created attachment 8708277 [details] [diff] [review]
Refactor worker actors to protocol.js.

Nits addressed, keeping r+.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=46aaf5a0f87e
Attachment #8708277 - Flags: review+
(Assignee)

Updated

3 years ago
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)
(Assignee)

Updated

3 years ago
Depends on: 1232931
No longer depends on: 1218817, 1220741, 1221892, 1226285
(Assignee)

Comment 15

3 years ago
Created attachment 8710401 [details] [diff] [review]
Refactor worker actors to protocol.js.

Rebased, r+ carried over.
Attachment #8710401 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8708277 - Attachment is obsolete: true
(Assignee)

Comment 16

3 years ago
Created attachment 8710402 [details] [diff] [review]
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.
Attachment #8710402 - Flags: review?(poirot.alex)
(Assignee)

Updated

3 years ago
Attachment #8707902 - Attachment is obsolete: true
(Assignee)

Comment 17

3 years ago
(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.
(Assignee)

Comment 18

3 years ago
Created attachment 8710406 [details] [diff] [review]
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.
Attachment #8710406 - Flags: review?(poirot.alex)
(Assignee)

Updated

3 years ago
Attachment #8710402 - Attachment is obsolete: true
Attachment #8710402 - Flags: review?(poirot.alex)
(Assignee)

Updated

3 years ago
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+
(Assignee)

Updated

3 years ago
Depends on: 1241841
(Assignee)

Comment 21

3 years ago
Created attachment 8710980 [details] [diff] [review]
Refactor worker actors to protocol.js.

Rebased, carried over.
Attachment #8710980 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8710401 - Attachment is obsolete: true
(Assignee)

Comment 22

3 years ago
Created attachment 8710981 [details] [diff] [review]
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.
Attachment #8710981 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8710406 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Depends on: 1242482
(Assignee)

Updated

3 years ago
Assignee: nobody → janx
(Assignee)

Comment 23

3 years ago
New try with new dependency patches by ochameau:

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

Comment 24

3 years ago
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.
(Assignee)

Comment 25

3 years ago
Actually, failure 2. seems to be fixed by Alex's latest patch in bug 1242482. \o/
(Assignee)

Comment 27

3 years ago
[Tracking Requested - why for this release]: Base bug for new Service Worker & Push features that we'd like to announce with Developer Edition 47.
tracking-firefox47: --- → ?
tracking-firefox47: ? → +
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)
(Assignee)

Comment 32

3 years ago
Created attachment 8719534 [details] [diff] [review]
Refactor worker actors to protocol.js.

Rebased, carried over.
Attachment #8719534 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8710980 - Attachment is obsolete: true
(Assignee)

Comment 33

3 years ago
Created attachment 8719535 [details] [diff] [review]
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
Attachment #8719535 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8710981 - Attachment is obsolete: true
(Assignee)

Comment 34

3 years ago
Created attachment 8719536 [details] [diff] [review]
interdiff.patch

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

Comment 35

3 years ago
Tree is looking great!
Blocks: 1201717
Priority: -- → P1
(Assignee)

Comment 36

3 years ago
Created attachment 8722999 [details] [diff] [review]
Refactor worker actors to protocol.js.

Rebased, carried over.
Attachment #8722999 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8719534 - Attachment is obsolete: true
Attachment #8719536 - Attachment is obsolete: true
(Assignee)

Comment 37

3 years ago
Created attachment 8723000 [details] [diff] [review]
Show all registered service workers in about:debugging.

Rebased, carried over.

New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5c01f1442a9
Attachment #8723000 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8719535 - Attachment is obsolete: true
(Assignee)

Comment 38

3 years ago
`dt5` oranges on linux debug look a bit worrying, but should be unrelated.
Keywords: checkin-needed

Comment 40

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2893ca049d37
https://hg.mozilla.org/mozilla-central/rev/f695820b8269
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
(Assignee)

Comment 41

3 years ago
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: --- → ?
(Assignee)

Comment 42

3 years ago
dev-doc-needed: The relevant MDN page is here https://developer.mozilla.org/en-US/docs/Tools/about:debugging
Added to Fx47 (Aurora) release notes.
relnote-firefox: ? → 47+
This is done: https://developer.mozilla.org/en-US/docs/Tools/about:debugging#Workers
Keywords: dev-doc-needed → dev-doc-complete

Updated

7 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.