1188981 - Show Push service connection state in `about:debugging`

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect, P2)

Description

[Lina Butler [:lina]]

We've had several reports on IRC where a service worker claims to have a subscription, but `PushService.jsm` isn't actually connected to the Push service. We should add an indicator for whether the client is connected, and show the device ID for debugging.

Comment 1

[Andrew Overholt [:overholt]]

Since we'd like to transition from about:serviceworkers to about:debugging I'm moving this bug there and altering the summary appropriately.

Comment 2

[Jan Keromnes [:janx]]

This would be an interesting feature for about:debugging's workers panel (bumping the bug so it stays on my radar).

Comment 3

[Jan Keromnes [:janx]]

Before allowing Push Service subscription changes, we should first show Push Service subscription details in about:debugging.

Bumping priority accordingly.

Comment 4

[Jan Keromnes [:janx]]

Attachment: Show Push Service subscription details for Service Workers in about:debugging.

Comment 5

[Jan Keromnes [:janx]]

Comment on attachment 8740036 [details] [diff] [review]
Show Push Service subscription details for Service Workers in about:debugging.

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

Ryan, I'd love your opinion on this proof of concept, and especially on the following topics:
- Should we create a dedicated PushSubscriptionActor?
- What do you think of pushSubscription living in ServiceWorkerActor's state?

::: devtools/client/aboutdebugging/components/service-worker-target.js
@@ +59,5 @@
> +          ),
> +          (pushSubscription ?
> +            dom.li({ className: "target-detail" },
> +              dom.strong(null, Strings.GetStringFromName("pushService")),
> +              dom.span({ className: "service-worker-push-url" },

Note: This className will be used in an upcoming test.

::: devtools/client/aboutdebugging/components/workers-tab.js
@@ +87,5 @@
>      ));
>    },
>  
>    update() {
> +    let { client } = this.props;

I'll remove this leftover, sorry.

::: devtools/server/actors/worker.js
@@ +370,5 @@
> +    let registration = this._registration;
> +    return new Promise((resolve, reject) => {
> +      PushService.getSubscription(registration.scope, registration.principal,
> +        (result, subscription) => {
> +          if (!subscription /* Components.isSuccessCode(result) */) {

I saw some other code use `Components.isSuccessCode`, but it's probably OK to just check for `subscription`, right?

@@ +371,5 @@
> +    return new Promise((resolve, reject) => {
> +      PushService.getSubscription(registration.scope, registration.principal,
> +        (result, subscription) => {
> +          if (!subscription /* Components.isSuccessCode(result) */) {
> +            reject({ error: "noPushSubscription" });

There errors are logged, maybe that's not the right way to say there is no subscription for this registration. Maybe return `{subscription: null}`?

Comment 6

[Jan Keromnes [:janx]]

Attachment: push-service.png

Comment 7

[Jan Keromnes [:janx]]

Comment on attachment 8740042 [details]
push-service.png

Hi Helen!

I'm adding another detail to some Service Worker registrations: a "Push Service" URL (and probably an "unsubscribe" or "clear" link later).

The screeshot is here: https://bug1188981.bmoattachments.org/attachment.cgi?id=8740042

For Service Workers subscribed to a Push Service (for Push Notifications), it shows the subscription's "endpoint" (URL of the remote server that will send the notifications to your Service Worker). Do you think it makes sense to display it like that?

Comment 8

[Jan Keromnes [:janx]]

(In reply to Jan Keromnes [:janx] from comment #7)
> The screeshot is here:
> https://bug1188981.bmoattachments.org/attachment.cgi?id=8740042

(Sorry for the double screenshot link, I originally planned to post this comment in bug 1260568, but then decided to leave it for more general UX discussion).

Comment 9

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Jan Keromnes [:janx] from comment #7)
> Comment on attachment 8740042 [details]
> push-service.png
> 
> Hi Helen!
> 
> I'm adding another detail to some Service Worker registrations: a "Push
> Service" URL (and probably an "unsubscribe" or "clear" link later).
> 
> The screeshot is here:
> https://bug1188981.bmoattachments.org/attachment.cgi?id=8740042
> 
> For Service Workers subscribed to a Push Service (for Push Notifications),
> it shows the subscription's "endpoint" (URL of the remote server that will
> send the notifications to your Service Worker). Do you think it makes sense
> to display it like that?

It seems like it makes sense to me—I don't have any issues with the layout from what I see!

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8740036 [details] [diff] [review]
Show Push Service subscription details for Service Workers in about:debugging.

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

::: devtools/server/actors/worker.js
@@ +365,5 @@
>      request: {},
>      response: RetVal("json")
>    }),
> +
> +  getPushSubscription: method(function () {

I think it depends a bit on what the future looks like... are there other actions you want to perform on push subscriptions after getting them, like to modify them in some way?  Or you are just making a list of them, and there's nothing else to do?

If there are future actions to perform on push subscriptions, a separate actor would seem like a better fit.

@@ +370,5 @@
> +    let registration = this._registration;
> +    return new Promise((resolve, reject) => {
> +      PushService.getSubscription(registration.scope, registration.principal,
> +        (result, subscription) => {
> +          if (!subscription /* Components.isSuccessCode(result) */) {

Reading the IDL, your version seems okay, since you only care about the case where a subscription exists.

@@ +371,5 @@
> +    return new Promise((resolve, reject) => {
> +      PushService.getSubscription(registration.scope, registration.principal,
> +        (result, subscription) => {
> +          if (!subscription /* Components.isSuccessCode(result) */) {
> +            reject({ error: "noPushSubscription" });

I think your intuition is right, something like `{subscription: null}` sounds better, since it's not really an error exactly.  You are asking about a push subscription when you don't know ahead of time if there is one or not.  A protocol error would make more sense to me if you are passed some arguments that are invalid in some way.

Comment 11

[Jan Keromnes [:janx]]

Attachment: WIP Show Push Service subscription details for Service Workers in about:debugging.

Thanks Helen and Ryan for your feedback!

New work-in-progress patch addressing comments (separate PushSubscriptionActor, resolve to `null` instead of rejecting ro `error`).

Comment 12

[Jan Keromnes [:janx]]

(Skipping the Dev Edition 48 branch point because this needs more time to get right.)

Comment 13

[Jan Keromnes [:janx]]

Attachment: Show Push Service subscription endpoint URL for Service Workers in about:debugging.

This took surprizingly long for such a simple feature, but this is ready for another round!

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #10)
> I think it depends a bit on what the future looks like... are there other
> actions you want to perform on push subscriptions after getting them, like
> to modify them in some way?  Or you are just making a list of them, and
> there's nothing else to do?
> 
> If there are future actions to perform on push subscriptions, a separate
> actor would seem like a better fit.

I agree. There is now a separate PushSubscriptionActor, which already gives more information than this patch displays in about:debugging (in the future, we could easily show push count, time of last push, and quota information, which are all included in the actor's form).

I would also like to show a light indicator of Push Service status (e.g. a green dot if Firefox is connected to Mozilla's Push Service, or orange/red if there are network troubles / the user disabled the Push Service, with additional status information in the dot's tooltip).

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8754951 [details] [diff] [review]
Show Push Service subscription endpoint URL for Service Workers in about:debugging.

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

Thanks for working on this!  Getting closer, but a few actor issues that I'd like to see another round for.

::: devtools/client/aboutdebugging/components/workers/service-worker-target.js
@@ +5,5 @@
>  /* eslint-env browser */
>  
>  "use strict";
>  
> +loader.lazyImporter(this, "setTimeout",

`setTimeout` should already be defined as a loader global[1].

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/shared/builtin-modules.js#287

::: devtools/client/aboutdebugging/test/browser_service_workers_push_service.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/* eslint-disable mozilla/no-cpows-in-tests */

Hmm, can you avoid CPOWs instead?  Do you have to use them?  Which lines are triggering this?  If they must be present, can you wrap them in eslint-disable / eslint-enable block so the rest of the test is covered by the rule?

@@ +10,5 @@
> +// in about:debugging if it exists, and disappears when unregistered.
> +
> +// Service workers can't be loaded from chrome://, but http:// is ok with
> +// dom.serviceWorkers.testing.enabled turned on.
> +const HTTP_ROOT = CHROME_ROOT.replace(

Do you import `shared-head`?  Can you use this instead:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/test/shared-head.js#35

@@ +29,5 @@
> +      ["dom.serviceWorkers.testing.enabled", true],
> +      // Enable the push service.
> +      ["dom.push.connection.enabled", true],
> +    ]};
> +    SpecialPowers.pushPrefEnv(options, done);

`pushPrefEnv` was recently upgraded to return a promise itself, let's use that (may need to rebase first).

::: devtools/server/actors/worker.js
@@ +316,5 @@
> +
> +  initialize(conn, subscription) {
> +    protocol.Actor.prototype.initialize.call(this, conn);
> +    this._subscription = subscription;
> +    this.manage(this);

This is not quite right.  The parent actor that creates this child actor should itself call `this.manage(child)` after the `new ...Actor()` step.

It's harder to find examples of child actors, so the expected pattern is less obvious, see[1] for example.

[1]: https://dxr.mozilla.org/mozilla-central/rev/16663eb3dcfa759f25b5e27b101bc79270c156f2/devtools/server/actors/inspector.js#966-970

@@ +333,5 @@
> +      quota: subscription.quota
> +    };
> +  },
> +
> +  destroy() {

You need to also call the super class destroy, like in `initialize`:

Actor.prototype.destroy.call(this);

See best practices[1] page.

[1]: https://wiki.mozilla.org/DevTools/Actor_Best_Practices

@@ +342,4 @@
>  // Lazily load the service-worker-child.js process script only once.
>  let _serviceWorkerProcessScriptLoaded = false;
>  
>  let ServiceWorkerRegistrationActor = protocol.ActorClass({

This actor is the first actor in a tree to use protocol.js.  For its `destroy` to be called in all cases, you must also implement `disconnect` which calls destroy.  See the wiki[1].

[1]: https://wiki.mozilla.org/DevTools/Actor_Best_Practices

@@ +367,1 @@
>      this.manage(this);

`this.manage(this)` should only be needed on _fronts_ for types that begin a protocol.js actor tree.  It should not be needed on this actor, please remove it.

@@ +379,4 @@
>      };
>    },
>  
> +  destroy() {

You need to also call the super class destroy, like in `initialize`:

Actor.prototype.destroy.call(this);

See best practices[1] page.

[1]: https://wiki.mozilla.org/DevTools/Actor_Best_Practices

@@ +471,5 @@
> +      );
> +    });
> +  }, {
> +    request: {},
> +    response: RetVal("json")

You're returning an actor, so your response should probably be `RetVal("pushSubscription")`.  Then can resolve the actor object itself.  protocol.js takes care of calling form() and writing it out.  You shouldn't need to wrap it in an object either.  Just `resolve(pushSubscriptionActor)` or similar.

Comment 15

[Jan Keromnes [:janx]]

Attachment: Switch about:debugging tests to devtools shared-head.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)
> 
> devtools/client/aboutdebugging/test/browser_service_workers_push_service.js
> @@ +1,4 @@
> > +/* Any copyright is dedicated to the Public Domain.
> > +   http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +/* eslint-disable mozilla/no-cpows-in-tests */
> 
> Hmm, can you avoid CPOWs instead?  Do you have to use them?  Which lines are
> triggering this?  If they must be present, can you wrap them in
> eslint-disable / eslint-enable block so the rest of the test is covered by
> the rule?

This exception was left over from before our ContentTask switch. I was able to remove it everywhere without issue.

> @@ +10,5 @@
> > +// in about:debugging if it exists, and disappears when unregistered.
> > +
> > +// Service workers can't be loaded from chrome://, but http:// is ok with
> > +// dom.serviceWorkers.testing.enabled turned on.
> > +const HTTP_ROOT = CHROME_ROOT.replace(
> 
> Do you import `shared-head`?  Can you use this instead:
> 
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/
> test/shared-head.js#35

I didn't know "http://example.com/" was equivalent to "http://mochi.test:8888/"! I imported shared-head.js into about:debugging's head.js, and switched all tests over to `URL_ROOT` in this separate patch (rest coming up in a second).

Comment 16

[Jan Keromnes [:janx]]

Attachment: Show Push Service subscription endpoint URL for Service Workers in about:debugging.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)
> Thanks for working on this!  Getting closer, but a few actor issues that I'd
> like to see another round for.

Thanks! This is the next round.

> devtools/client/aboutdebugging/components/workers/service-worker-target.js
> > +loader.lazyImporter(this, "setTimeout",
> 
> `setTimeout` should already be defined as a loader global[1].
> 
> [1]:
> https://dxr.mozilla.org/mozilla-central/source/devtools/shared/builtin-
> modules.js#287

Indeed, removed the duplicate import.

> @@ +29,5 @@
> > +      ["dom.serviceWorkers.testing.enabled", true],
> > +      // Enable the push service.
> > +      ["dom.push.connection.enabled", true],
> > +    ]};
> > +    SpecialPowers.pushPrefEnv(options, done);
> 
> `pushPrefEnv` was recently upgraded to return a promise itself, let's use
> that (may need to rebase first).

Switched to the promise version (only in this test, for now).

> ::: devtools/server/actors/worker.js
> @@ +316,5 @@
> > +
> > +  initialize(conn, subscription) {
> > +    protocol.Actor.prototype.initialize.call(this, conn);
> > +    this._subscription = subscription;
> > +    this.manage(this);
> 
> This is not quite right.  The parent actor that creates this child actor
> should itself call `this.manage(child)` after the `new ...Actor()` step.
> 
> It's harder to find examples of child actors, so the expected pattern is
> less obvious, see[1] for example.
> 
> [1]:
> https://dxr.mozilla.org/mozilla-central/rev/
> 16663eb3dcfa759f25b5e27b101bc79270c156f2/devtools/server/actors/inspector.
> js#966-970

Thanks for catching this! I wasn't quite sure what `this.manage()` was doing. I moved the manage call next to the creation of the actor.

> > +  destroy() {
> 
> You need to also call the super class destroy, like in `initialize`:
> 
> Actor.prototype.destroy.call(this);

Done (in `ServiceWorkerRegistrationActor.destroy()` and `PushSubscriptionActor.destroy()`).

> @@ +342,4 @@
> >  // Lazily load the service-worker-child.js process script only once.
> >  let _serviceWorkerProcessScriptLoaded = false;
> >  
> >  let ServiceWorkerRegistrationActor = protocol.ActorClass({
> 
> This actor is the first actor in a tree to use protocol.js.  For its
> `destroy` to be called in all cases, you must also implement `disconnect`
> which calls destroy.  See the wiki[1].
> 
> [1]: https://wiki.mozilla.org/DevTools/Actor_Best_Practices

Indeed. Added the `disconnect` method.

Late thought: It's probably also the case for the `WorkerActor` class.

> @@ +367,1 @@
> >      this.manage(this);
> 
> `this.manage(this)` should only be needed on _fronts_ for types that begin a
> protocol.js actor tree.  It should not be needed on this actor, please
> remove it.

Fair enough, removed all occurences of "this.manage(this)" in worker.js, since presumably none of the classes in there are fronts.

> @@ +471,5 @@
> > +      );
> > +    });
> > +  }, {
> > +    request: {},
> > +    response: RetVal("json")
> 
> You're returning an actor, so your response should probably be
> `RetVal("pushSubscription")`.  Then can resolve the actor object itself. 
> protocol.js takes care of calling form() and writing it out.  You shouldn't
> need to wrap it in an object either.  Just `resolve(pushSubscriptionActor)`
> or similar.

Changed to `response: { subscription: RetVal("nullable:pushSubscription") }`.

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8760344 [details] [diff] [review]
Switch about:debugging tests to devtools shared-head.

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

::: devtools/client/aboutdebugging/test/browser_service_workers.js
@@ +1,4 @@
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> +/* import-globals-from ../../framework/test/shared-head.js */

Do you actually need this in each test?  It is necessary in head.js, but individual tests are meant to transitively pick up things from their head.js file.

Keep it if you must, but try to remove it from the tests first.

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8760353 [details] [diff] [review]
Show Push Service subscription endpoint URL for Service Workers in about:debugging.

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

Looks like bug 1277951 landed which decoupled this actor into a separate spec file (like is being done with all actors for devtools.html).  It's probably worth one more review after you rebase and account for this.

It seems good overall!

::: devtools/client/aboutdebugging/test/browser_service_workers_push_service.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/* import-globals-from ../../framework/test/shared-head.js */

Shouldn't be needed in each test, as mentioned on previous patch.

::: devtools/client/aboutdebugging/test/service-workers/push-sw.html
@@ +13,5 @@
>  sw.then(
>    function (registration) {
>      dump("SW registered\n");
> +    registration.pushManager.subscribe().then(
> +        function (subscription) {

Nit: strange formatting, why 4 spaces?

::: devtools/server/actors/worker.js
@@ +51,3 @@
>    typeName: "worker",
>  
>    initialize: function (conn, dbg) {

Can you either implement `disconnect` for this one as well, or file a follow up for it?

@@ +393,5 @@
> +
> +  /**
> +   * Standard observer interface to listen to push messages and changes.
> +   *
> +   * @param subject

Probably don't need to list these params in the comment, it's a common Gecko API.

@@ +464,5 @@
> +            return;
> +          }
> +          pushSubscriptionActor = new PushSubscriptionActor(this._conn, subscription);
> +          this._pushSubscriptionActor = pushSubscriptionActor;
> +          this.manage(pushSubscriptionActor);

My apologies... it looks like you do not need `this.manage` here for the case where the parent actor that returns the child actor is the one that should own it, which is what you are doing here.

The protocol.js docs help to explain this[1] and the code seems to confirm this behavior[2].

Sorry for the confusion... it's hard to remember all the rules. :)

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/server/docs/protocol.js.md#550-574
[2]: https://dxr.mozilla.org/mozilla-central/rev/1828937da9493b2cd54862b9c520b2ba5c7db92b/devtools/shared/protocol.js#292

Comment 19

[Jan Keromnes [:janx]]

Attachment: Switch about:debugging tests to devtools shared-head.

Thanks Ryan!

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #17)
> ::: devtools/client/aboutdebugging/test/browser_service_workers.js
> @@ +1,4 @@
> >  /* Any copyright is dedicated to the Public Domain.
> >     http://creativecommons.org/publicdomain/zero/1.0/ */
> >  
> > +/* import-globals-from ../../framework/test/shared-head.js */
> 
> Do you actually need this in each test?  It is necessary in head.js, but
> individual tests are meant to transitively pick up things from their head.js
> file.
> 
> Keep it if you must, but try to remove it from the tests first.

Indeed, they were unnecessary, so I removed them.

(Carrying over r+.)

Comment 20

[Jan Keromnes [:janx]]

Attachment: Show Push Service subscription endpoint URL for Service Workers in about:debugging.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #18)
> Looks like bug 1277951 landed which decoupled this actor into a separate
> spec file (like is being done with all actors for devtools.html).  It's
> probably worth one more review after you rebase and account for this.

Indeed, I rebased my patch and added a spec for the new pushSubscriptionActor.

> > +/* import-globals-from ../../framework/test/shared-head.js */
> 
> Shouldn't be needed in each test, as mentioned on previous patch.

Removed.

> > +    registration.pushManager.subscribe().then(
> > +        function (subscription) {
> 
> Nit: strange formatting, why 4 spaces?

Fixed (Cloud9 defaults to 4 spaces, which is annoying).

> >    typeName: "worker",
> >  
> >    initialize: function (conn, dbg) {
> 
> Can you either implement `disconnect` for this one as well, or file a follow
> up for it?

Implemented `disconnect()` and `destroy()` methods on workerActor (which calls `_detach()` if necessary).

> > +  /**
> > +   * Standard observer interface to listen to push messages and changes.
> > +   *
> > +   * @param subject
> 
> Probably don't need to list these params in the comment, it's a common Gecko
> API.

Granted. Removed.

> > +          pushSubscriptionActor = new PushSubscriptionActor(this._conn, subscription);
> > +          this._pushSubscriptionActor = pushSubscriptionActor;
> > +          this.manage(pushSubscriptionActor);
> 
> My apologies... it looks like you do not need `this.manage` here for the
> case where the parent actor that returns the child actor is the one that
> should own it, which is what you are doing here.

Removed `this.manage(pushSubscriptionActor)`. Thanks for checking and linking to the docs!

Comment 21

[Jan Keromnes [:janx]]

Attachment: Show Push Service subscription endpoint URL for Service Workers in about:debugging.

(In reply to Jan Keromnes [:janx] from comment #20)
> > > +/* import-globals-from ../../framework/test/shared-head.js */
> > 
> > Shouldn't be needed in each test, as mentioned on previous patch.
> 
> Removed.

Now actually removed.

Comment 22

[Jan Keromnes [:janx]]

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

Comment 23

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8761543 [details] [diff] [review]
Show Push Service subscription endpoint URL for Service Workers in about:debugging.

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

Okay, looks good to me!

Comment 24

[Jan Keromnes [:janx]]

Thanks Ryan! It seems I misspelled a try job on the last push, here is a new one:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=88d6adfa187d

Comment 25

[Jan Keromnes [:janx]]

Well, it seems that browser_service_workers_push_service.js is now leaking 2 windows. This is going to be fun.

Comment 26

[Jan Keromnes [:janx]]

Attachment: Switch about:debugging tests to devtools shared-head.

Rebased, carried over.

Comment 27

[Jan Keromnes [:janx]]

Attachment: Show Push Service subscription endpoint URL for Service Workers in about:debugging.

Rebased, carried over.

Fixed leaks with:

>   info("Unmock the push service");
>   PushService.service = null;

in browser_service_workers_push_service.js.

Comment 28

[Jan Keromnes [:janx]]

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

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/17a89f9b5d55
Switch about:debugging tests to devtools shared-head. r=jryans
https://hg.mozilla.org/integration/fx-team/rev/110c7ad7d399
Show Push Service subscription endpoint URL for Service Workers in about:debugging. r=jryans

Comment 30

[Guilherme Lima]

Landed this morning:
https://hg.mozilla.org/mozilla-central/rev/17a89f9b5d55
https://hg.mozilla.org/mozilla-central/rev/110c7ad7d399