For service worker debugging, we want to update the about:debugging UI whenever the installing/waiting/active worker for a given service worker registration changes. To implement this, we will extend the debugger protocol to get the installing/waiting/active worker for a given service worker registration, and to send a notification when these workers change.

I found a way to significantly clean up the logic in onGetServiceWorkerRegistration, with the additional advantage that we can reuse it for ServiceWorkerRegistrationActor (which will get similar getters for installing/active/waitingWorker).

Comment on attachment 8687149 [details] [diff] [review] Implement defineActorGetter. Review of attachment 8687149 [details] [diff] [review]: ----------------------------------------------------------------- I like the idea of making this simpler and more generic, but it seems weird to do it just for the "_actor" getter (more like an implementation detail) instead of bootstrapping the whole "onGetServiceWorkerRegistration" method. I'm thinking about "DebuggerClient.requester", because it's a simple factory that can create an entire method (e.g. "TabClient.listWorkers") with just a few parameters. Your solutions requires to start implementing a method, and only helps somewhat with the "_actor" getter, while I think it could completely take over the method implementation. 1) Isn't something "even simpler" like this possible? > TabActor.prototype = { > onGetServiceWorkerRegistration: delegateActorGetter({ > getDelegate: function() {}, > createActor: function() {}, > destroyActor: function() {}, > onChanged: function() {} > }); > } > function delegateActorGetter(methods) { > // Implement the getter. > return getter; > } Or maybe if the getter can't be fully self-contained: > defineDelegateActorGetter(TabActor.prototype, "onGetServiceWorkerRegistration", { > getDelegate, > createActor, > destroyActor > }); > function defineDelegateActorGetter(object, name, methods) { > // Implementation that mutates TabActor.prototype. > } 2) Given that this is just code improvement and not a required functionality, and if you think this would take you too much time to get right, maybe we can open a follow-up bug for this and come back to it after your time-critical work? ::: devtools/shared/DevToolsUtils.js @@ +851,5 @@ > + * @param Object object > + * The object on which the getter is to be defined. > + * @param String name > + * The name of the getter to be defined. > + * @param Object options Nit: This got me confused, because these are not really "options" (they aren't "parameters" or "settings", and they don't seem to be optional). What about "methods"? @@ +857,5 @@ > + * - getDelegate > + * Get the current delegate. > + * - createActor > + * Creates a new actor for the given delegate, and adds it to an actor > + * pool. Nit: Why does the new actor *have* to be added to a pool?

(convenient needinfo just so you notice that I reviewed)

Comment on attachment 8687149 [details] [diff] [review] Implement defineActorGetter. (re-reviewing because 1) from comment 2 won't work with the planned "onGetInfo" method)

Comment on attachment 8687149 [details] [diff] [review] Implement defineActorGetter. Review of attachment 8687149 [details] [diff] [review]: ----------------------------------------------------------------- In regard of our IRC discussion earlier, and since this change doesn't break any existing feature, let's go ahead with this patch. But please address or refuse my nits before. I'll copy/paste the two from comment 2: ::: devtools/shared/DevToolsUtils.js @@ +851,5 @@ > + * @param Object object > + * The object on which the getter is to be defined. > + * @param String name > + * The name of the getter to be defined. > + * @param Object options Nit: This got me confused, because these are not really "options" (they aren't "parameters" or "settings", and they don't seem to be optional). What about "methods"? @@ +857,5 @@ > + * - getDelegate > + * Get the current delegate. > + * - createActor > + * Creates a new actor for the given delegate, and adds it to an actor > + * pool. Nit: Why does the new actor *have* to be added to a pool? Some more nits: ::: devtools/server/actors/webbrowser.js @@ +1177,5 @@ > this._mustNotifyServiceWorkerRegistrationChanged = true; > } > > // Return the actor for the current service worker registration, or null if > // no such registration exists. Nit: Maybe change "Return" to "Get" in this comment? Since you're not returning on the following line anymore. @@ +1178,5 @@ > } > > // Return the actor for the current service worker registration, or null if > // no such registration exists. > + let actor = this._serviceWorkerRegistrationActor; Nit: Maybe remove the "_"? @@ +1192,5 @@ > this._mustNotifyServiceWorkerRegistrationChanged = false; > }, > > onRegister: function () { > + if (this._serviceWorkerRegistrationActorChanged) { Nit: Maybe remove the "_"? @@ +1198,5 @@ > } > }, > > onUnregister: function () { > + if (this._serviceWorkerRegistrationActorChanged) { Nit: Maybe remove the "_"? @@ +1911,5 @@ > } > }, > }; > > +defineActorGetter(TabActor.prototype, "_serviceWorkerRegistrationActor", { Nit: I don't think we ever had getters with "_"prefixed attributes (generally there is an internal "_attribute", and a "get attribute()" getter). Maybe removing the "_" makes more sense here? ::: devtools/shared/DevToolsUtils.js @@ +861,5 @@ > + * pool. > + * - destroyActor > + * Destroys the given actor, by removing it from its actor pool. > + */ > +exports.defineActorGetter = function (object, name, options) { Nit: I think that this utility's name should convey the fact that there is a "delegate" involved in getting the actor. Maybe change "defineActorGetter" to "defineDelegateActorGetter"?

Bug 1221892, which is necessary for the toolbox based solution, is currently blocked on a crashing test that is not reproducible locally. Alex came up with a patch that should allow us to land bug 1218817, which is necessary for the about:debugging based solution. With this in mind, and given the fact that most of the other work can be based on top of either bug 1221892 or bug 1218817, Jan, Alex and I decided to move forward with the about:debugging solution based first. I've updated the dependencies for this bug accordingly.

Here's a new version of this patch, that's based on top of bug 1218817 instead of 1221892. Since you already r+'d this patch before, I expect this to be a rubberstamp review. I addressed most of your review comments, with the exception of removing the '_' prefix for the getters. I feel the prefix should be there, as these getters are an implementation detail for the actor on which they are defined, and should never be accessed from outside the actor. If you really feel strongly about this, please r+ the patch with comments, and we'll hash it out on irc.

Registering/unregistering service workers is something we're going to have to do more often, so I've written some test helpers to make this process cleaner/easier to understand. I also took the opportunity to clean up the test for listing service worker registrations a bit, and remove some dead code from code_frame_script.js

While I was thinking about how ServiceWorkerRegistrationActor should work, I realised we have some bugs in how we deal with attaching to/detaching from tab actors. For top level actors, such as tab or worker actors, we want to avoid using resources for these actors unless a client actually wants to debug them. To express interest in debugging an actor, the client attaches to it. This is when we should do things like set up listeners, initialise a debugger server in a worker, etc (n.b. we actually don't do this correctly for WorkerActors either, which I intend to fix in the future). Among other things, this means (or at least I interpret it to mean) that a client should not be able to interact with a top level actor until it is attached to the actor. Moreover, the client should no longer receive things like change notifications from the actor after it detached from it. As it turns out, you list the workers in a tab actor before you are attached to it. This is not what you want, because it can cause a detached tab actor to send workerListChanged notifications. Similarly, when we detach from the tab actor, we should cancel any such notifications.

This patch implements the ServiceWorkerRegistrationActor.getInfo request. This request allows you to get the current state of a service worker registration (which consists of its script URL, installing, waiting, and active worker), and ensures that a one-shot notification is sent when this state changes. Note that, due to the asynchronous nature of client/server requests, the state could have changed again between the time we receive the one-shot notification and the next getInfo request. Because of this, we can make very few hard guarantees as to what the state will be after a one-shot notification: the only thing that's guaranteed is that it will be different from the previous state. In the test, I use a helper function waitForActiveWorker, which sends a getInfo request, and checks if the activeWorker property is set. If it is not, it waits for a one-shot notification, and then tries again. When a service worker registration is updated, once its activeWorker becomes non-null, the state will no longer change, so this seemed like a good point to test the result of the getInfo request.

Thanks Eddy! Let's have a look...

Comment on attachment 8689626 [details] [diff] [review] Implement test helpers to register/unregister service workers. Review of attachment 8689626 [details] [diff] [review]: ----------------------------------------------------------------- Doesn't break the test for me, so r+ (subject to a green try push). (In reply to Eddy Bruel [:ejpbruel] from comment #8) > Registering/unregistering service workers is something we're going to have > to do more often, so I've written some test helpers to make this process > cleaner/easier to understand. Great initiative! > I also took the opportunity to clean up the test for listing service worker > registrations a bit, and remove some dead code from code_frame_script.js +1 Nit: One of your "jsonrpc" messages seems to be sending something weird in this test: JavaScript Warning: "Sending message that cannot be cloned. Are you trying to send an XPCOM object?" {file: "chrome://mochitests/content/browser/devtools/client/debugger/test/mochitest/code_frame-script.js" line: 87} ::: devtools/client/debugger/test/mochitest/getserviceworkerregistration/code_getserviceworkerregistration-worker.js @@ +1,1 @@ > +"use strict"; Nit: I think this file doesn't have anything to do with the current bug.

Comment on attachment 8689632 [details] [diff] [review] We should not be able to interact with a detached BrowserTabActor. Review of attachment 8689632 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Eddy Bruel [:ejpbruel] from comment #9) > Created attachment 8689632 [details] [diff] [review] > We should not be able to interact with a detached BrowserTabActor. > > While I was thinking about how ServiceWorkerRegistrationActor should work, I > realised we have some bugs in how we deal with attaching to/detaching from > tab actors. > > For top level actors, such as tab or worker actors, we want to avoid using > resources for these actors unless a client actually wants to debug them. To > express interest in debugging an actor, the client attaches to it. This is > when we should do things like set up listeners, initialise a debugger server > in a worker, etc (n.b. we actually don't do this correctly for WorkerActors > either, which I intend to fix in the future). > > Among other things, this means (or at least I interpret it to mean) that a > client should not be able to interact with a top level actor until it is > attached to the actor. Moreover, the client should no longer receive things > like change notifications from the actor after it detached from it. > > As it turns out, you list the workers in a tab actor before you are attached > to it. This is not what you want, because it can cause a detached tab actor > to send workerListChanged notifications. Similarly, when we detach from the > tab actor, we should cancel any such notifications. This makes sense to me, and related worker tests still work locally. Still, I'd like someone more familiar with actor and attach/detach to rubberstamp on this. Panos, does this also make sense to you? Also, as always, please run this by treeherder to ensure it doesn't break tests on a weird platform.

Comment on attachment 8689623 [details] [diff] [review] Implement defineDelegateActorGetter Review of attachment 8689623 [details] [diff] [review]: ----------------------------------------------------------------- LGTM

Comment on attachment 8689634 [details] [diff] [review] Implement ServiceWorkerRegistration.getInfo. Review of attachment 8689634 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me and your test seems to work. Please reply to the nits before we move forward with this. ::: devtools/client/debugger/test/mochitest/browser_dbg_ServiceWorkerRegistrationActor.getInfo.js @@ +1,1 @@ > +var TAB_URL = EXAMPLE_URL + "doc_ServiceWorkerRegistrationActor.getInfo-tab.html"; Please note that you're the only one using caps in your test file names. Not a big issue though, I'm mainly interested in the tests working :) ::: devtools/server/actors/worker.js @@ +256,5 @@ > + }, > + > + form: function () { > + return { > + scriptSpec: this.scriptSpec Shouldn't the form also include `actor: this.actorID`? ::: devtools/shared/client/main.js @@ +1406,5 @@ > + getInfo: DebuggerClient.requester({ > + type: "getInfo" > + }, { > + telemetry: "GETINFO" > + }), Nit: Isn't "getInfo" a bit too generic a name for a Debugger RDP call? Maybe something like "getServiceWorkerRegistrationInfo" would be more explicit.

Comment on attachment 8689632 [details] [diff] [review] We should not be able to interact with a detached BrowserTabActor. Review of attachment 8689632 [details] [diff] [review]: ----------------------------------------------------------------- This makes sense for me for normal workers. I think we do things differently for service and shared workers, but I can't remember how exactly. Just wanted to bring it up to let you confirm that the client doesn't have to attach to a tab in order to get a list of non-tab-scoped workers. ::: devtools/server/actors/webbrowser.js @@ +1349,5 @@ > } > > + // Make sure that no more workerListChanged notifications are sent. > + if (this._workerActorList !== null) { > + this._workerActorList.onListChanged = null; I would move the workerListChanged and serviceWorkerRegistrationChanged silencing bits to the top of the function just to minimize the (perhaps purely theoretical) chance of notifications racing with _detach.

Try push for the "We should not be able to interact with a detached BrowserTabActor." patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbf1a20d2977

Try push for the "We should not be able to interact with a detached BrowserTabActor." patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbf1a20d2977

Some patches still didn't land actually.

Comment on attachment 8689632 [details] [diff] [review] We should not be able to interact with a detached BrowserTabActor. This is https://hg.mozilla.org/mozilla-central/rev/03d9d90e41c2

This is no longer required for Developer Edition 47, because we worked around this issue to get all the workers directly from all child processes.

The leave-open keyword is there and there is no activity for 6 months. :jlast, maybe it's time to close this bug?

Thanks for pinging me.

comment 23 mentioned that we did exposed service workers differently. Eddy tried to expose them out of the BrowsingContextTargetActor. But instead, we ended up exposing all SW at once, out of the RootActor, via its listServiceWorkerRegistrations method. There is an helper on RootFront called listAllWorkers, which processes all the workers. That's typically used by about:debugging. https://searchfox.org/mozilla-central/source/devtools/shared/fronts/root.js#60 ({ registrations } = await this.listServiceWorkerRegistrations()) https://searchfox.org/mozilla-central/source/devtools/shared/fronts/root.js#86-96 registrations.forEach(form => { result.service.push({ name: form.url, url: form.url, scope: form.scope, fetch: form.fetch, registrationActor: form.actor, active: form.active, lastUpdateTime: form.lastUpdateTime, }); });