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

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: lina, Assigned: nsm)

Tracking

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment)

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.
s/pushregistrationchange/pushsubscriptionchange.
Assignee: nobody → nsm.nikhil
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.
Attachment #8635543 - Flags: review?(kcambridge)
Attachment #8635543 - Flags: review?(catalin.badea392)
Attachment #8635543 - Flags: review?(bugs)

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.
Reporter

Updated

4 years ago
Attachment #8635543 - Flags: review?(kcambridge) → review+
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 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.
Attachment #8635543 - Flags: review?(bugs) → review+
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!
(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 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.
Attachment #8635543 - Flags: review?(catalin.badea392) → review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/bd5bad07a59a6b723e44f0447d0ed8a3103bc4f4
changeset:  bd5bad07a59a6b723e44f0447d0ed8a3103bc4f4
user:       Nikhil Marathe <nsm.nikhil@gmail.com>
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.
https://hg.mozilla.org/mozilla-central/rev/bd5bad07a59a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.