1205109 - Make `pushsubscriptionchange` extendable

Status: VERIFIED FIXED

(Core :: DOM: Push Subscriptions, defect)

Comment 1

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1205109 - Make `pushsubscriptionchange` extendable. r?mt

Bug 1205109 - Make `pushsubscriptionchange` extendable. r?mt,nsm

Comment 2

[Lina Butler [:lina]]

Comment on attachment 8661518 [details]
MozReview Request: Bug 1205109 - Make `pushsubscriptionchange` extendable. r?mt

Bug 1205109 - Make `pushsubscriptionchange` extendable. r?mt,nsm

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8661518 [details]
MozReview Request: Bug 1205109 - Make `pushsubscriptionchange` extendable. r?mt

https://reviewboard.mozilla.org/r/19381/#review17349

This looks good if the spec simply makes the event an ExtendableEvent. Otherwise it may declare PushSubscriptionLostEvent inheriting ExtendableEvent (although I don't see the point since the subclass has no new fields/methods).

::: dom/push/test/test_subscription_change.html:4
(Diff revision 2)
> +Bug XXX: Make `pushsubscriptionchange` extendable.

Nit: Please fix the XXX here and below.

::: dom/push/test/test_subscription_change.html:38
(Diff revision 2)
> +      iframe.src = "http://mochi.test:8888/tests/dom/push/test/frame.html";

Please use a relative URL.

::: dom/push/test/worker.js:70
(Diff revision 2)
> +  ).then(([client]) => {
> +    client.postMessage({type: "changed", okay: "yes"});
> +  }));

This will fail rather silently if client is undefined for whatever reason, can you add a catch that dumps pretty loudly? Thanks

::: dom/workers/ServiceWorkerManager.cpp:2373
(Diff revision 2)
> -    WorkerGlobalScope* globalScope = aWorkerPrivate->GlobalScope();
> +    GlobalObject globalObj(aCx, aWorkerPrivate->GlobalScope()->GetWrapper());

ExtendableEvent::Constructor() has a second form that takes an event target, so you don't have to do this hack. See DispatchLifecycleEvent.

::: dom/workers/ServiceWorkerManager.cpp:2381
(Diff revision 2)
> +      ExtendableEvent::Constructor(globalObj,
> +                                   NS_LITERAL_STRING("pushsubscriptionchange"),
> +                                   init, result);

The second form also skips having to deal with `result`

Comment 4

[Martin Thomson [:mt:]]

Comment on attachment 8661518 [details]
MozReview Request: Bug 1205109 - Make `pushsubscriptionchange` extendable. r?mt

https://reviewboard.mozilla.org/r/19381/#review17421

::: dom/push/PushService.jsm:1025
(Diff revision 2)
> -    this[aMessage.name.slice("Push:".length).toLowerCase()](pageRecord, mm);
> +    this[name.toLowerCase()](pageRecord, mm);

For later...

```
this[name.toLowerCase()](pageRecord)
  .then(message => {
    message.requestID = pageRecord.requestID;
    mm.sendAsyncMessage("PushService:" + name + ":OK", message);
  }, error => {
    let message = {
      requestID: pageRecord.requestID,
      error: error
    };
    mm.sendAsyncMessage("PushService:" + name + ":KO", message);
  });
```

Then reset() can just route to dropRegistrations()

::: dom/push/test/frame.html:8
(Diff revision 2)
> +    function waitOnSubscriptionChange()
> +    {
> +      var p = new Promise(function(res, rej) {
> +        function onMessage(e) {
> +          if (e.data.type == "changed") {
> +            navigator.serviceWorker.removeEventListener("message", onMessage);
> +            (e.data.okay == "yes" ? res : rej)(e.data);
> +          }
> +        }
> +        navigator.serviceWorker.addEventListener("message", onMessage);
> +      });
> +      return p;
> +    }

This just duplicates waitOnPushmessage.  Refactor please.

::: dom/push/test/test_subscription_change.html:4
(Diff revision 2)
> +Bug XXX: Make `pushsubscriptionchange` extendable.

Bug number

::: dom/push/test/test_subscription_change.html:11
(Diff revision 2)
> +  <title>Test for Bug XXX</title>

XXX

::: dom/push/test/test_subscription_change.html:16
(Diff revision 2)
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=XXX">Mozilla Bug XXX</a>

XXX

::: dom/push/test/test_subscription_change.html:41
(Diff revision 2)
> +        res(swr)

arrow function or semi-colon.

Why do you resolve with the service worker registration?  You saved that to a global already...

::: dom/push/test/test_subscription_change.html:42
(Diff revision 2)
> +      }

Missing semi-colon.

::: dom/workers/ServiceWorkerManager.cpp:2373
(Diff revision 2)
> -    WorkerGlobalScope* globalScope = aWorkerPrivate->GlobalScope();
> +    GlobalObject globalObj(aCx, aWorkerPrivate->GlobalScope()->GetWrapper());

Two lines and assert for null on the return from aWorkPrivate->GlobalScope().

Comment 5

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/19381/#review17349

> This will fail rather silently if client is undefined for whatever reason, can you add a catch that dumps pretty loudly? Thanks

Do workers have access to `dump`? The test will time out if it doesn't receive a response, so we'll still know something is wrong...

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8661518 [details]
MozReview Request: Bug 1205109 - Make `pushsubscriptionchange` extendable. r?mt

Bug 1205109 - Make `pushsubscriptionchange` extendable. r?mt,nsm

Comment 7

[Lina Butler [:lina]]

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

Comment 8

[Martin Thomson [:mt:]]

(In reply to Kit Cambridge [:kitcambridge] from comment #5)
> Do workers have access to `dump`? The test will time out if it doesn't
> receive a response, so we'll still know something is wrong...

Yes, dump() works in workers.

Comment 9

[Lina Butler [:lina]]

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

Comment 10

[Lina Butler [:lina]]

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

Comment 11

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02d5b9e9a3486b5dfcfa4db774db0aae5307e211
Bug 1205109 - Make `pushsubscriptionchange` extendable. r=mt,nsm

Comment 12

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e20ca51d4f4d76c8e94825896df033c9a87821f
Back out bug 1205109 for Push mochitest failure.

Comment 13

[Lina Butler [:lina]]

Comment on attachment 8661518 [details]
MozReview Request: Bug 1205109 - Make `pushsubscriptionchange` extendable. r?mt

Bug 1205109 - Make `pushsubscriptionchange` extendable. r?mt

Comment 14

[Martin Thomson [:mt:]]

Kit, is this review request right?  It looks like you are rolling back all the changes.

Comment 15

[Lina Butler [:lina]]

(In reply to Martin Thomson [:mt:] from comment #14)
> Kit, is this review request right?  It looks like you are rolling back all
> the changes.

It is. `DispatchExtendableEventOnWorkerScope` really cleans up the logic. I did roll back the test and `Push:Reset` addition. Now that we have the permission observer, I think the test in bug 1219064 is cleaner.

Comment 16

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf81a62b5a374679029aa29fad6b81f0bca69cf7
Bug 1205109 - Make `pushsubscriptionchange` extendable. r=mt

Comment 17

[Phil Ringnalda (:philor)]

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

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/cf81a62b5a37

Comment 19

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

Comment 20

[Lina Butler [:lina]]

Attachment: 1.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1233387.
[User impact if declined]: The `pushsubscriptionchange` event may not keep the worker alive long enough to request a new subscription. Consequently, sites with expired push subscriptions may not be able to request new ones, and the user won't receive push messages from that site.
[Describe test coverage new/current, TreeHerder]: Covered by existing mochitests.
[Risks and why]: Low risk. On Nightly for more than six weeks; straightforward change to the worker code.
[String/UUID change made/needed]: None.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8700917 [details] [diff] [review]
1.patch

This has been on Central for 6 weeks, seems safe to uplift to Beta44.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/810ac5245b14

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/810ac5245b14

Comment 24

[Michelle Funches - QA]

Verified Beta 44.0b4