1218817 - Extend the debugger protocol to list all service worker registrations.

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Eddy Bruel [:ejpbruel]]

As a first step towards implementing service worker debugging, we want to extend the debugger protocol to list all service worker registrations for the current process.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: patch

Comment 2

[Jan Keromnes [:janx]]

Comment on attachment 8679474 [details] [diff] [review]
patch

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

Thanks Eddy, works great! (With both dependency patches applied)

Just a few comments and a small complaint: I know that what we're listing are "ServiceWorkerRegistrations", but being fully explicit is causing very long / unreadable class and method names (some of the code almost looks like Java!) Maybe we can find a compromise by naming these either "ServiceWorkers" or "WorkerRegistrations"? Or do you feel this would hurt readability instead of improving it?

Please also ask a telemetry peer to review your new histograms (see last comment).

::: devtools/client/debugger/test/mochitest/browser_dbg_listserviceworkerregistrations.js
@@ +14,5 @@
> +
> +      let client = new DebuggerClient(DebuggerServer.connectPipe());
> +      yield connect(client);
> +
> +

Nit: No need for the extra line break.

::: devtools/client/debugger/test/mochitest/head.js
@@ +1050,5 @@
>  
> +function listServiceWorkerRegistrations(client) {
> +  info("Listing service worker registrations.");
> +  return new Promise(function (resolve) {
> +    client.listServiceWorkerRegistrations(function (response) {

If you remove the helper as I suggested, please use `client.mainRoot` here.

::: devtools/server/actors/worker.js
@@ +249,5 @@
> +
> +ServiceWorkerRegistrationActorList.prototype = {
> +  getList: function () {
> +    let regs = new Set();
> +    let arr = swm.getAllRegistrations();

`arr` and `regs` aren't great variable names. Please use something like `registrations` and `array` (or `data` as in aboutServiceWorkers.js).

@@ +263,5 @@
> +
> +    for (let reg of regs) {
> +      if (!this._actors.has(reg)) {
> +        this._actors.set(reg, new ServiceWorkerRegistrationActor(reg));
> +      }

Maybe this can be moved inside the `getAllRegistrations()` for loop?

@@ +269,5 @@
> +
> +    let actors = [];
> +    for (let [, actor] of this._actors) {
> +      actors.push(actor);
> +    }

Nit: `let actors = [...this._actors.values()]`.

::: devtools/shared/client/main.js
@@ +415,5 @@
> +  /*
> +   * This function exists only to preserve DebuggerClient's interface;
> +   * new code should say 'client.mainRoot.listServiceWorkerRegistrations()'.
> +   */
> +  listServiceWorkerRegistrations: function (aOnResponse) { return this.mainRoot.listServiceWorkerRegistrations(aOnResponse); },

Maybe this helper is not necessary: No existing code out there currently expects DebuggerClient's interface to have this.

::: toolkit/components/telemetry/Histograms.json
@@ +6021,5 @@
>      "n_buckets": "1000",
>      "description": "The time (in milliseconds) that it took a 'listTabs' request to go round trip."
>    },
> +  "DEVTOOLS_DEBUGGER_RDP_LOCAL_LISTSERVICEWORKERREGISTRATIONS_MS": {
> +    "expires_in_version": "never",

This won't fly because every new telemetry probe is expected to have an expiration version. How about "50"? And an email alert is probably a good idea too so we can be notified if expiration is close.

In any case, your Histograms.json changes will need to be reviewed by a data collection peer (either :ally, :vladan or :bsmedberg).

Comment 3

[Eddy Bruel [:ejpbruel]]

Attachment: patch

New patch, with comments addressed.

It turns out that the _checkListening was fundamentally flawed, and could cause us to add or remove ourselves as a listener twice (which ServiceWorkerManager does not allow). I've rearchitected that part of the code to make sure we only add/remove ourselves as a listener once.

I took your suggestion to use better variable names for arr, idx, and reg. Your suggestion to use the spread operator was really clever, so I took that as well.

I opted not to merge the loop in which we create new actors with the one in which we build the set of registrations, because I like to think of those as separate steps. I've added some comments to that effect, that will hopefully make the separation between steps more clear.

Because ServiceWorkerRegistrationActorList is essentially the same as WorkerActorList, I've also updated that class to incorporate the same suggestions there.

With regard to using a different name for ServiceWorkerRegistration, I agree that the current name is somewhat on the long side. However, in general I don't think we should try to shorten names when doing so introduces ambiguity. Both 'ServiceWorkers' and 'WorkerRegistrationList' are misleading, since both ServiceWorker and Worker have a very specific meaning, which is different from ServiceWorkerRegistration.

I can't really come up with a better name for this at the moment. Until we do, I propose we keep it as is for now.

I've added an expiration date to the telemetry probe, like you said. Flagging vladan for superreview for these parts.

Comment 4

[Jan Keromnes [:janx]]

Comment on attachment 8682482 [details] [diff] [review]
patch

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

Looks good to me, but please address my nits and wait for Vladan's rubberstamp on the histograms.

Note: Histogram approvals don't usually use "superreview", but either "needinfo" or "addl. review" (I know :ally prefers "addl. review" because it helps her prioritize the rubberstamp request correctly).

::: devtools/client/debugger/test/mochitest/head.js
@@ +1050,5 @@
>  
> +function listServiceWorkerRegistrations(client) {
> +  info("Listing service worker registrations.");
> +  return new Promise(function (resolve) {
> +    client.listServiceWorkerRegistrations(function (response) {

Nit: Please use `client.mainRoot.listX()` here.

::: devtools/server/actors/worker.js
@@ +187,5 @@
>    },
>  
>    get onListChanged() {
>      return this._onListChanged;
>    },

Nit: Is this getter actually used anywhere?

@@ +289,5 @@
> +  },
> +
> +  get onListchanged() {
> +    return this._onListchanged;
> +  },

Nit: Is this getter actually used anywhere?

::: devtools/shared/client/main.js
@@ +415,5 @@
> +  /*
> +   * This function exists only to preserve DebuggerClient's interface;
> +   * new code should say 'client.mainRoot.listServiceWorkerRegistrations()'.
> +   */
> +  listServiceWorkerRegistrations: function (aOnResponse) { return this.mainRoot.listServiceWorkerRegistrations(aOnResponse); },

Oops, you forgot to address my remark on the previous version of this patch.

::: toolkit/components/telemetry/Histograms.json
@@ +6060,5 @@
>      "n_buckets": "1000",
>      "description": "The time (in milliseconds) that it took a 'listTabs' request to go round trip."
>    },
> +  "DEVTOOLS_DEBUGGER_RDP_LOCAL_LISTSERVICEWORKERREGISTRATIONS_MS": {
> +    "expires_in_version": "50",

Thanks for setting an expiration version!

Please also add an "alert_emails" field, so that we get notified before the probes are about to expire, and extend their lifetime if they're still relevant (look how this field works with other recent devtools probes).

Comment 5

[Vladan Djeric (:vladan)]

Comment on attachment 8682482 [details] [diff] [review]
patch

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

Make Jan's changes from comment 4, aside from that, it looks good to me

Comment 6

[Eddy Bruel [:ejpbruel]]

It turns out that there are some fundamental issues with listing service workers on the root actor. See bug 1221892 for an explanation of the problem.

We still want to land this, but we can't really do so until our service worker implementation becomes fully e10s ready. In particular, we want to be able to start a thread for a service worker in a child process, using an object representing that service worker in the parent process.

Comment 7

[Eddy Bruel [:ejpbruel]]

Alex came up with a patch in bug 1225473 that should allow us to move forward with this bug (see the bug for details).

Here's a try push for the patch, with comments by Jan addressed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b611e5cfffb7

Comment 8

[Eddy Bruel [:ejpbruel]]

The try push in comment #7 shows the same heap corruption that we're seeing in bug 1221892. I still have no idea what is causing that test to fail, as it is not directly related to the patches in either bug, and has been failing intermittently before (bug 1220970).

On top of this, the test for this patch seems to time out on Linux, but not on OS X. From the try log, it looks like unregistering the service worker on Linux never causes the onUnregister handler on nsIServiceWorkerManagerListener to be called.

Ben, do you have any idea why the behaviour on Linux is different here than on OS X?

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Eddy, I probably won't have time to look at this before tomorrow afternoon.  We are trying to get a bunch of stuff landed in aurora 44 by the end of the week.  If I don't have time tomorrow you'll probably want to ask Ehsan or someone else since I'm going to be on PTO next week.  Sorry!

Comment 10

[Jan Keromnes [:janx]]

Eddy, please don't forget to upload the updated patch that you pushed to try in comment 7, and mark the current patch here as obsolete. Otherwise we're at risk of landing it.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

I'm sorry, I ran out of time here.  Ehsan, is there any chance you could help Eddy with this problem?

Comment 12

[(no longer active)]

(In reply to Eddy Bruel [:ejpbruel] from comment #8)
> On top of this, the test for this patch seems to time out on Linux, but not
> on OS X. From the try log, it looks like unregistering the service worker on
> Linux never causes the onUnregister handler on
> nsIServiceWorkerManagerListener to be called.
> 
> Ben, do you have any idea why the behaviour on Linux is different here than
> on OS X?

The timeouts are happening in e10s devtools tests, as far as I can tell, which only run on Linux, and not on Mac.  I would expect the same problem to be reproducible if you run these tests in e10s mode locally on Mac.

Comment 13

[Eddy Bruel [:ejpbruel]]

(In reply to :Ehsan Akhgari from comment #12)
> (In reply to Eddy Bruel [:ejpbruel] from comment #8)
> > On top of this, the test for this patch seems to time out on Linux, but not
> > on OS X. From the try log, it looks like unregistering the service worker on
> > Linux never causes the onUnregister handler on
> > nsIServiceWorkerManagerListener to be called.
> > 
> > Ben, do you have any idea why the behaviour on Linux is different here than
> > on OS X?
> 
> The timeouts are happening in e10s devtools tests, as far as I can tell,
> which only run on Linux, and not on Mac.  I would expect the same problem to
> be reproducible if you run these tests in e10s mode locally on Mac.

Oh, of course. That probably explains the problem. We need Alex's patch for the listServiceWorkerRegistrations to work properly with e10s.

It should be good enough to simply disable the test for e10s right now, and re-enable it once Alex's patch lands.

@Ehsan: thanks for pointing me in the right direction!

Comment 14

[Eddy Bruel [:ejpbruel]]

I just had a hunch. Here's a new try push that might resolve the problem:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1114c72a187e

Comment 15

[Eddy Bruel [:ejpbruel]]

Try is still crashing all over the place. New strategy: split the patch up into smaller patches, and push them to try piecemeal.

Here's a try run for the WorkerActorList changes only, which I suspect to be the culprit for the crashing browser_dbg_worker-window.js test.

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

Comment 16

[Eddy Bruel [:ejpbruel]]

The above try run looks ok. There are several oranges, but they all look like known bugs.

I'm still baffled as to what's causing the crashes we're seeing. At this point, the best strategy seems to me to land the above patch piecemeal, until we hit whatever part of the patch is causing the crash.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5e199d17e77

Comment 18

[Eddy Bruel [:ejpbruel]]

Try push for the ServiceWorkerActorRegistration changes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d5b24270d0b

Comment 19

[Eddy Bruel [:ejpbruel]]

Try push for the ServiceWorkerRegistrationActorList changes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6c471ad51f8

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/244c7dcd5a10

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d52bfe63522e

Comment 22

[Eddy Bruel [:ejpbruel]]

Try push for the RootActor.listServiceWorkerRegistration changes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbf1a20d2977

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d5e199d17e77

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47075637410f

Comment 25

[Eddy Bruel [:ejpbruel]]

Try push for testing RootActor.listServiceWorkerRegistrations with no registrations:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f71d511795f0

This is where I'd expect things to start breaking, since the code in the previous patches is actually exercised.

Comment 26

[Eddy Bruel [:ejpbruel]]

Try push for testing RootActor.listServiceWorkerRegistrations with one registration:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=12d6ba0db7de

Note to self that I added the test helpers from the patch in bug 1220741 here.

Comment 27

[Eddy Bruel [:ejpbruel]]

One of the patches I landed earlier had a missing part, which broke the test in comment 25.

Here is another try push for testing RootActor.listServiceWorkerRegistrations with no registrations:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=da218124c462

Comment 28

[Eddy Bruel [:ejpbruel]]

And another try push for testing RootActor.listserviceWorkerRegistrations with one registration:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=58e19196d405

Comment 29

[Eddy Bruel [:ejpbruel]]

While I'm at it, here's a try run for the final part of the patch, which tests RootActor.listServiceWorkerRegistrations with two registrations:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba0b228dcfbc

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/244c7dcd5a10
https://hg.mozilla.org/mozilla-central/rev/d52bfe63522e
https://hg.mozilla.org/mozilla-central/rev/47075637410f

Comment 31

[Jan Keromnes [:janx]]

Eddy, does the [leave-open] still make sense here?

Comment 32

[Eddy Bruel [:ejpbruel]]

I don't think we ever landed the final patch for this (the one with the test). It's probably a good idea to try to land this, provided the test still makes sense.

Jan, would you be willing to look into this for me?

Comment 33

[Jan Keromnes [:janx]]

I'm not sure which final patch you are referring to, but judging from DXR it looks like attachment 8682482 [details] [diff] [review] mostly landed except for the test part.

Now that we've fixed most of the crashers, could you please try to rebase the patch and send it to try?

Comment 34

[Eddy Bruel [:ejpbruel]]

Hi Jan.

This is still on my radar, but I have several bugs that are higher on the priority list than this one, including several crashers. I'll try to look into it soon, but can't make any promises. Sorry!

Comment 35

[Jan Keromnes [:janx]]

The feature has already landed, and the attachment is now obsolete. Closing this bug.