Extend the debugger protocol to get the installing/waiting/active worker for a given service worker registration.

REOPENED
Unassigned

Status

REOPENED
3 years ago
4 months ago

People

(Reporter: ejpbruel, Unassigned)

Tracking

(Blocks: 1 bug, {leave-open})

unspecified
Firefox 45
leave-open
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Updated

3 years ago
Blocks: 1219255
(Reporter)

Updated

3 years ago
No longer blocks: 1219255
(Reporter)

Updated

3 years ago
Depends on: 1224237
(Reporter)

Comment 1

3 years ago
Created attachment 8687149 [details] [diff] [review]
Implement defineActorGetter.

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).
Attachment #8687149 - Flags: review?(janx)
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?
Attachment #8687149 - Flags: review?(janx) → review-
(convenient needinfo just so you notice that I reviewed)
Flags: needinfo?(ejpbruel)
Comment on attachment 8687149 [details] [diff] [review]
Implement defineActorGetter.

(re-reviewing because 1) from comment 2 won't work with the planned "onGetInfo" method)
Attachment #8687149 - Flags: review- → review?(janx)
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"?
Attachment #8687149 - Flags: review?(janx) → review+
(Reporter)

Comment 6

3 years ago
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.
Depends on: 1220740, 1218817
No longer depends on: 1219205
Flags: needinfo?(ejpbruel)
(Reporter)

Comment 7

3 years ago
Created attachment 8689623 [details] [diff] [review]
Implement defineDelegateActorGetter

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.
Attachment #8689623 - Flags: review?(janx)
(Reporter)

Updated

3 years ago
Attachment #8687149 - Attachment is obsolete: true
(Reporter)

Comment 8

3 years ago
Created attachment 8689626 [details] [diff] [review]
Implement test helpers to register/unregister service workers.

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
Attachment #8689626 - Flags: review?(janx)
(Reporter)

Comment 9

3 years ago
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.
Attachment #8689632 - Flags: review?(janx)
(Reporter)

Comment 10

3 years ago
Created attachment 8689634 [details] [diff] [review]
Implement ServiceWorkerRegistration.getInfo.

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.
Attachment #8689634 - Flags: review?(janx)
(Reporter)

Updated

3 years ago
Blocks: 1226285
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.
Attachment #8689626 - Flags: review?(janx) → review+
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.
Attachment #8689632 - Flags: review?(past)
Attachment #8689632 - Flags: review?(janx)
Attachment #8689632 - Flags: feedback+
Comment on attachment 8689623 [details] [diff] [review]
Implement defineDelegateActorGetter

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

LGTM
Attachment #8689623 - Flags: review?(janx) → review+
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.
Attachment #8689634 - Flags: review?(janx) → feedback+
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.
Attachment #8689632 - Flags: review?(past) → review+
(Reporter)

Comment 17

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

Comment 18

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

Comment 20

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/03d9d90e41c2
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Some patches still didn't land actually.
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
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
Attachment #8689632 - Flags: checkin+
No longer blocks: 1212797
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.
No longer blocks: 1214248
(Reporter)

Updated

2 years ago
Assignee: ejpbruel → nobody

Updated

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