1184574 - Service workers should be able to access the `PushManager` via `self.registration.pushManager`

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Lina Butler [:lina]]

This comment suggests that it should be possible for a worker to access its registration's `PushManager`: https://github.com/w3c/push-api/issues/82#issuecomment-59414449 This would allow the worker to resubscribe if a `pushregistrationchange` event is fired.

:eeejay pointed out on IRC that this isn't currently possible: the `PushManager` is only accessible from the main thread.

Comment 1

[Lina Butler [:lina]]

s/pushregistrationchange/pushsubscriptionchange.

Comment 3

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

Attachment: MozReview Request: Bug 1184574 - Allow access to PushManager on ServiceWorker. r?kitcambridge,smaug,catalinb

Bug 1184574 - Allow access to PushManager on ServiceWorker. r?kitcambridge,smaug,catalinb

Refactoring to allow access to PushManager in ServiceWorkerGlobalScope. See comment in PushManager.h for details.

Comment 4

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

Olli, could you review PushClient.js because I'm using it with addWeakMessageListener of the child process message manager, and create a few instances in PushManager.cpp and I want to be sure I'm not going to shutdown leak or something. Also, the CC parts of PushManager.{h,cpp}

Kit for making sure I'm still spec compliant and haven't broken anything during the JS refactoring.

Catalin for the liberal use of PromiseWorkerProxy.

Comment 5

[Lina Butler [:lina]]

Comment on attachment 8635543 [details]
MozReview Request: Bug 1184574 - Allow access to PushManager on ServiceWorker. r?kitcambridge,smaug,catalinb

https://reviewboard.mozilla.org/r/13563/#review12183

The JS changes look good, but they're a tiny portion of this patch. :-) I left some questions about the C++ part, too.

::: dom/push/Push.js:122
(Diff revision 1)
> -    let resolver = this.takePromiseResolver(json.requestID);
> +          fn(that._scope, that._principal, {

Question: I see you're passing client methods directly to `getEndpointResponse`...does XPCOM automatically bind methods, or do you need to do, e.g., `this.getEndpointResponse(this._client.subscribe.bind(this._client))`?

::: dom/push/Push.js:126
(Diff revision 1)
> +                if (endpoint && endpoint !== "") {

Nit: `if (endpoint)`.

::: dom/push/PushClient.js:36
(Diff revision 1)
> +  this._nextRequestId = 0;

Nit: Looks like this isn't used anymore.

::: dom/push/PushManager.cpp:115
(Diff revision 1)
> +PushSubscription::Constructor(GlobalObject& aGlobal, const nsAString& aEndpoint, const nsAString& aScope, ErrorResult& aRv)

This is the chrome constructor definition, right?

::: dom/push/PushManager.cpp:121
(Diff revision 1)
> +  return sub.forget();

What does `forget()` do? Reference counting?

::: dom/webidl/PushManager.webidl:14
(Diff revision 1)
> -interface PushManager {
> +interface PushManagerImpl {

Should this have `NoInterfaceObject`?

::: dom/push/PushManager.cpp:598
(Diff revision 1)
> +    nsresult rv = permManager->TestExactPermissionFromPrincipal(

So the `target.assertPermission("push")` check in `PushService.receivedMessage` is different from this permission check, right?

::: dom/push/PushManager.cpp:422
(Diff revision 1)
> +WorkerPushSubscription::Unsubscribe(ErrorResult &aRv)

Just to make sure I follow: calling `unsubscribe` (or any of the `WorkerPushManager` methods) from the worker thread executes a runnable on the main thread. The runnable creates a client, which sends a manager to the `PushService`. When the service sends a response, it's received by `WorkerUnsubscribeResultCallback`—still on the main thread—which passes the result back to the worker thread. The worker thread then settles the promise. Is that accurate?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8635543 [details]
MozReview Request: Bug 1184574 - Allow access to PushManager on ServiceWorker. r?kitcambridge,smaug,catalinb

r+ for cpmm usage and CC handling.
(though, mPrincipal isn't CCable, so there shouldn't be need to traverse/unlink it)

You may of course want to test a bit and check cc logs (which about:memory can create) after you've
closed the pages using Push.

And interface PushManagerImpl should definitely be NoInterfaceObject

I don't understand 
+[Exposed=(Window,Worker), Func="mozilla::dom::PushManager::Enabled",
+ Constructor(DOMString pushEndpoint, DOMString scope), ChromeOnly]
 interface PushSubscription
Exposed in worker, but ChromeOnly? What does that even mean? ChromeWorkers?
In the spec PushSubscription doesn't have ctor, so perhaps you want ChromeConstructor?
PushSubscription interface should be visible normally, but it shouldn't have a ctor for web pages to use.
Please explain, and fix the issue.

Comment 7

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

https://reviewboard.mozilla.org/r/13563/#review12183

> Question: I see you're passing client methods directly to `getEndpointResponse`...does XPCOM automatically bind methods, or do you need to do, e.g., `this.getEndpointResponse(this._client.subscribe.bind(this._client))`?

Didn't bind because it wasn't needed, but better safe than sorry. fixed.

> This is the chrome constructor definition, right?

Yes

> What does `forget()` do? Reference counting?

Yes. It takes the pointer out of the nsrefptr without decrementing the refcount.

> Just to make sure I follow: calling `unsubscribe` (or any of the `WorkerPushManager` methods) from the worker thread executes a runnable on the main thread. The runnable creates a client, which sends a manager to the `PushService`. When the service sends a response, it's received by `WorkerUnsubscribeResultCallback`—still on the main thread—which passes the result back to the worker thread. The worker thread then settles the promise. Is that accurate?

Yes that is correct.

> So the `target.assertPermission("push")` check in `PushService.receivedMessage` is different from this permission check, right?

Yes the former is more for security but the latter is easier to deal with and error out since it is synchronous on the child process.

> Should this have `NoInterfaceObject`?

Good point!

Comment 8

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

(In reply to Olli Pettay [:smaug] (about to have some vacation, expect slower than usual reviews) from comment #6)
> Comment on attachment 8635543 [details]
> MozReview Request: Bug 1184574 - Allow access to PushManager on
> ServiceWorker. r?kitcambridge,smaug,catalinb
> 
> r+ for cpmm usage and CC handling.
> (though, mPrincipal isn't CCable, so there shouldn't be need to
> traverse/unlink it)
> 
> You may of course want to test a bit and check cc logs (which about:memory
> can create) after you've
> closed the pages using Push.
> 
> And interface PushManagerImpl should definitely be NoInterfaceObject
> 
> I don't understand 
> +[Exposed=(Window,Worker), Func="mozilla::dom::PushManager::Enabled",
> + Constructor(DOMString pushEndpoint, DOMString scope), ChromeOnly]
>  interface PushSubscription
> Exposed in worker, but ChromeOnly? What does that even mean? ChromeWorkers?
> In the spec PushSubscription doesn't have ctor, so perhaps you want
> ChromeConstructor?
> PushSubscription interface should be visible normally, but it shouldn't have
> a ctor for web pages to use.
> Please explain, and fix the issue.


Thanks! I didn't know about ChromeConstructor, I thought [Constructor(...), ChromeOnly] would achieve that. Fixed.

Comment 9

[Cătălin Badea (:catalinb)]

Comment on attachment 8635543 [details]
MozReview Request: Bug 1184574 - Allow access to PushManager on ServiceWorker. r?kitcambridge,smaug,catalinb

https://reviewboard.mozilla.org/r/13563/#review12281

::: dom/push/PushManager.cpp:465
(Diff revision 1)
> +  GetSubscriptionResultRunnable(PromiseWorkerProxy *aProxy,

nit: PromiseWorkerProxy* aProxy

::: dom/push/PushManager.cpp:271
(Diff revision 1)
> +  UnsubscribeResultRunnable(PromiseWorkerProxy *aProxy,

nit: coding style

::: dom/push/PushManager.cpp:378
(Diff revision 1)
> +    if (!mProxy) {

If the proxy failed to add the worker feature, the runnable is still dispatched to the main thread and just returns here.

Is the promise rejected in this case? The destructor won't have any effect since mProxy is already null.

I think it would be better not to dispatch the runnable and just reject on the worker thread.

Comment 10

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

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

Comment 11

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

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

Comment 12

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

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/bd5bad07a59a6b723e44f0447d0ed8a3103bc4f4
changeset:  bd5bad07a59a6b723e44f0447d0ed8a3103bc4f4
user:       Nikhil Marathe <[e-mail]>
date:       Thu Jul 23 08:30:15 2015 -0700
description:
Bug 1184574 - Allow access to PushManager on ServiceWorker. r=kitcambridge,smaug,catalinb

Refactoring to allow access to PushManager in ServiceWorkerGlobalScope. See comment in PushManager.h for details.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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