1328293 - show if SW will receive fetch events in about:debugging

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

In bug 1181127 and bug 1325101 we implemented an optimization where the service worker will only receive fetch events if there is a fetch event handler registered.  To help developers understand what is going on we should show this state on about:debugging.

Catalin, any interest in working on this as a follow-on to the no-fetch optimization?

Comment 1

[Cătălin Badea (:catalinb)]

I can take this.

Comment 2

[Cătălin Badea (:catalinb)]

Attachment: Show if a service worker is listening for fetch events in about:debugging.

Open to suggestions on the right UI for displaying this information.

Comment 3

[Julian Descottes [:jdescottes]]

Comment on attachment 8827431 [details] [diff] [review]
Show if a service worker is listening for fetch events in about:debugging.

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

This looks good, thanks! Some comments to address, and this needs an additional browser mochitest.
The aboutdebugging tests are quite verbose and painful to write so sorry about asking this :/

But I think we should be able to create one easily for this feature, by doing something similar as what is done in browser_service_workers_push_service.
Give it a try and ping me if you have any issue with the test.

Adding :bkelly to review the c++ and idl changes.

::: devtools/client/aboutdebugging/components/workers/panel.js
@@ +91,5 @@
>          };
>          switch (form.type) {
>            case Ci.nsIWorkerDebugger.TYPE_SERVICE:
>              let registration = this.getRegistrationForWorker(form, workers.service);
> +            worker.fetch = form.fetch;

I think this is never defined because we get WorkerActor forms here and never ServiceWorkerActor forms.

::: devtools/client/locales/en-US/aboutdebugging.properties
@@ +20,5 @@
>  unregister = unregister
>  
>  pushService = Push Service
>  
> +fetch = Fetch

Maybe localization notes for the three new strings?
I don't think 'fetch' should be translated as its a technical event type, and we could make that clear in the notes.

::: devtools/server/actors/worker.js
@@ +295,5 @@
>        url: registration.scriptSpec,
>        installingWorker,
>        waitingWorker,
>        activeWorker,
> +      fetch: newestWorker.fetch,

I am seeing several errors in the browser console when a new sw is registering, apparently because newestWorker is null.
(and I am also seeing new unrelated errors but which seem unrelated to your change)

To be safe newestWorker && newestWorker.fetch ?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8827431 [details] [diff] [review]
Show if a service worker is listening for fetch events in about:debugging.

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

c++ and idl changes look good to me.

Comment 5

[Cătălin Badea (:catalinb)]

Attachment: Show if a service worker is listening for fetch events in about:debugging.

Comment 6

[Cătălin Badea (:catalinb)]

Attachment: Add test for new entry in about:debugging#workers.

Comment 7

[Cătălin Badea (:catalinb)]

Comment on attachment 8828319 [details] [diff] [review]
Show if a service worker is listening for fetch events in about:debugging.

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

::: devtools/server/actors/worker.js
@@ +55,5 @@
>        form.scope = registration.scope;
> +      let newestWorker = (registration.activeWorker ||
> +                          registration.waitingWorker ||
> +                          registration.installingWorker);
> +      form.fetch = newestWorker && newestWorker.handlesFetchEvents;

I don't really understand how we get on this code path. In the testing I've done, SWs are added from iterating the registrations and no WorkerActors of the SW type are created.

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8828320 [details] [diff] [review]
Add test for new entry in about:debugging#workers.

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

R+ here, but be sure to read and apply my comment about empty-sw.html before landing.

::: devtools/client/aboutdebugging/test/browser.ini
@@ +15,5 @@
>    service-workers/empty-sw.js
>    service-workers/push-sw.html
>    service-workers/push-sw.js
> +  service-workers/fetch-sw.js
> +  service-workers/fetch-sw.html

list is alphabetically sorted, move lines after service-workers/empty-sw.js

@@ +42,5 @@
>  [browser_service_workers_timeout.js]
>  skip-if = true # Bug 1232931
>  [browser_service_workers_unregister.js]
>  [browser_tabs.js]
> +[browser_service_workers_fetch_flag.js]

list is alphabetically sorted, move lines after [browser_service_workers.js]

::: devtools/client/aboutdebugging/test/browser_service_workers_fetch_flag.js
@@ +21,5 @@
> +
> +  let swTab = yield addTab(url);
> +
> +  let serviceWorkersElement = getServiceWorkerList(document);
> +  yield waitForMutation(serviceWorkersElement, { childList: true });

I guess the reason you added scopes that don't include the test pages we visit is to avoid the sw to start activating.

From what I could see, waitForMutation will yield when we transition between two states, when "newestWorker" is undefined and fetch is then false.
Leaving the workers stopped works around this issue.

To correctly implement this, we would need to :
- fix the scopes
- wait for the service worker to start  
- check the flag

I think this calls for some refactoring of the aboutdebugging test code (at least so that we can share a helper to wait for a given sw to start).
So I won't ask you to modify it here. 

But at least remove the scope from empty-sw.html, otherwise tests will fail.

I will log a follow up to do what I mentioned above.

@@ +23,5 @@
> +
> +  let serviceWorkersElement = getServiceWorkerList(document);
> +  yield waitForMutation(serviceWorkersElement, { childList: true });
> +
> +  let names = [...document.querySelectorAll("#service-workers .service-worker-fetch-flag")];

nit: names -> fetchFlags

::: devtools/client/aboutdebugging/test/service-workers/empty-sw.html
@@ +7,5 @@
>  <body>
>  <script type="text/javascript">
>  "use strict";
>  
> +var sw = navigator.serviceWorker.register("empty-sw.js", {scope: "empty-sw/"});

adding this scope makes two other tests fail :
- browser_service_workers_start.js
- browser_service_workers_unregister.js

This scope is never ok for our tests, so the sw never start, which we need for some of the tests.

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8828319 [details] [diff] [review]
Show if a service worker is listening for fetch events in about:debugging.

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

Thanks, just a small comment but you can go ahead!

::: devtools/client/aboutdebugging/components/workers/panel.js
@@ +91,5 @@
>          };
>          switch (form.type) {
>            case Ci.nsIWorkerDebugger.TYPE_SERVICE:
>              let registration = this.getRegistrationForWorker(form, workers.service);
> +            worker.fetch = form.fetch;

Can you move this in the else statement below ?

It's really on ever useful in the e10s only case, that will get fixed eventually (if not already fixed).

::: devtools/server/actors/worker.js
@@ +55,5 @@
>        form.scope = registration.scope;
> +      let newestWorker = (registration.activeWorker ||
> +                          registration.waitingWorker ||
> +                          registration.installingWorker);
> +      form.fetch = newestWorker && newestWorker.handlesFetchEvents;

We independently get both:
- the list of all workers
- the list of al registrations
 
See getWorkerForms in devtools/client/aboutdebugging/modules/worker.js
We then mix and match to have all the information we need.

Comment 10

[Cătălin Badea (:catalinb)]

Attachment: Show if a service worker is listening for fetch events in about:debugging.

Comment 11

[Cătălin Badea (:catalinb)]

Attachment: Add test for new entry in about:debugging#workers.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42823a93cd16
Show if a service worker is listening for fetch events in about:debugging. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/a76a454541fc
Add test for new entry in about:debugging#workers. r=jdescottes

Comment 13

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/14b3fa51bb12 for eslint failures, https://treeherder.mozilla.org/logviewer.html#?job_id=70964946&repo=mozilla-inbound

Comment 14

[Cătălin Badea (:catalinb)]

Attachment: Add test for new entry in about:debugging#workers.

Fixed lint errors.

Comment 15

[Cătălin Badea (:catalinb)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b32501ee40341e48a14276d780ec1d1833b9840&selectedJob=70976431

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/abe4ae295d86
Show if a service worker is listening for fetch events in about:debugging. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/b108c67ae385
Add test for new entry in about:debugging#workers. r=jdescottes

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/abe4ae295d86
https://hg.mozilla.org/mozilla-central/rev/b108c67ae385