Closed Bug 1257401 Opened 4 years ago Closed 4 years ago

De-workerify PushSubscription and PushManager

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: Lina, Assigned: Lina)

References

Details

Attachments

(2 files, 1 obsolete file)

This would also reduce a lot of the duplication between PushSubscription and WorkerPushSubscription.
Kit, were you planning to do this, or just filing it for completeness? ;)
Flags: needinfo?(kcambridge)
I filed it mostly for completeness; mt wasn't happy with the duplication in bug 1247685. I was advised to wait until the spec settles down before landing that, so I can do this in the meantime and then rebase the other patches.
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
Flags: needinfo?(kcambridge)
Attached patch PushSubscription.patch (obsolete) — Splinter Review
Hey Kyle,

Here's a conversion of PushSubscription. Mostly, it's just moving code around, though I did have some questions for you:

* PushSubscription defines a chrome constructor, used by the JS-implemented PushManager. Previously, we also had a `setPrincipal` method that PushManager used to set the calling principal. It looks like I can QI the global to nsIScriptObjectPrincipal instead. Is that safe to do? I found `GetWebIDLCallerPrincipal()`, too, but AFAICT that's only for JS implementations.

* Is it OK for `GetParentObject` to return null on the worker thread? That's what WorkerPushSubscription did before; I'm wondering if there's anything different about having a single class that can be constructed from multiple threads.

Thanks!
Attachment #8732553 - Flags: feedback?(khuey)
Summary: De-workerify PushSubscription → De-workerify PushSubscription and PushManager
Comment on attachment 8732553 [details] [diff] [review]
PushSubscription.patch

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

I'm not really an expert on this code, but it looks good to me.

::: dom/push/PushSubscription.cpp
@@ +57,5 @@
> +
> +class UnsubscribeResultRunnable final : public WorkerRunnable
> +{
> +public:
> +  UnsubscribeResultRunnable(PromiseWorkerProxy* aProxy,

It's not a big deal, but this could take already_AddRefed<PromiseWorkerProxy>

@@ +118,5 @@
> +      return NS_OK;
> +    }
> +
> +    RefPtr<UnsubscribeResultRunnable> r =
> +      new UnsubscribeResultRunnable(proxy, aStatus, aSuccess);

and then this could do proxy.forget()

@@ +157,5 @@
> +      if (mProxy->CleanedUp()) {
> +        return NS_OK;
> +      }
> +      principal = mProxy->GetWorkerPrivate()->GetPrincipal();
> +    }

nit: \n before and after the scoping braces.

@@ +174,5 @@
> +    if (NS_WARN_IF(NS_FAILED(service->Unsubscribe(mScope, principal, callback)))) {
> +      callback->OnUnsubscribe(NS_ERROR_FAILURE, false);
> +      return NS_OK;
> +    }
> +    return NS_OK;

nit: \n before the final return

@@ +202,5 @@
> +    mGlobal = aGlobal;
> +  } else {
> +    WorkerPrivate* worker = GetCurrentThreadWorkerPrivate();
> +    MOZ_ASSERT(worker);
> +    worker->AssertIsOnWorkerThread();

#ifdef DEBUG this all, maybe?

@@ +273,5 @@
> +
> +  nsCOMPtr<nsIPushService> service =
> +    do_GetService("@mozilla.org/push/Service;1");
> +  if (NS_WARN_IF(!service)) {
> +    aRv = NS_ERROR_FAILURE;

aRv.Throw(NS_ERROR_FAILURE)

@@ +279,5 @@
> +  }
> +
> +  nsCOMPtr<nsIScriptObjectPrincipal> sop = do_QueryInterface(mGlobal);
> +  if (!sop) {
> +    aRv = NS_ERROR_FAILURE;

ibid

@@ +352,5 @@
> +  }
> +
> +  RefPtr<UnsubscribeRunnable> r =
> +    new UnsubscribeRunnable(proxy, mScope);
> +  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToMainThread(r)));

MOZ_ALWAYS_SUCCEEDS is a thing now!
Attachment #8732553 - Flags: feedback?(khuey) → feedback+
Thanks, Kyle! Here's an updated patch that incorporates your suggestions.
Attachment #8732553 - Attachment is obsolete: true
Attachment #8736815 - Flags: review?(khuey)
Here's part 2. The JS-implemented main thread PushManager made this a bit more tedious than it had to be, but it compiles, and the tests pass locally.

Instead of the `setScope` and `setPushManagerImpl` methods, I just gave both interfaces ChromeConstructors, and it looks like that will automatically call `ConstructJSImplementation` for me.

One thing I'm not sure of is the `AutoJSAPI` usage in ServiceWorkerRegistrationMainThread::GetPushManager. I borrowed that idiom from https://dxr.mozilla.org/mozilla-central/rev/494289c72ba3997183e7b5beaca3e0447ecaf96d/dom/network/TCPSocket.cpp#70-76. I don't think I can use nsContentUtils::RootingCxForThread without tripping a cross-compartment assertion in the PushManagerImpl::Constructor binding.
Attachment #8736817 - Flags: review?(khuey)
Comment on attachment 8736815 [details] [diff] [review]
PushSubscription.patch

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

::: dom/push/PushSubscription.cpp
@@ +118,5 @@
> +      return NS_OK;
> +    }
> +
> +    RefPtr<UnsubscribeResultRunnable> r =
> +      new UnsubscribeResultRunnable(mProxy->GetWorkerPrivate(), mProxy.forget(),

Hmm, I forgot C++ doesn't specify argument evaluation order. I should hoist mProxy->GetWorkerPrivate() out into a variable.

@@ +202,5 @@
> +  , mScope(aScope)
> +  , mRawP256dhKey(aRawP256dhKey)
> +  , mAuthSecret(aAuthSecret)
> +{
> +  if (NS_IsMainThread()) {

I wonder if it's worthwhile having separate constructors for main and worker threads, like PushManager. Though the signatures aren't as different compared to the latter.
Comment on attachment 8736815 [details] [diff] [review]
PushSubscription.patch

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

::: dom/push/PushSubscription.cpp
@@ +120,5 @@
> +
> +    RefPtr<UnsubscribeResultRunnable> r =
> +      new UnsubscribeResultRunnable(mProxy->GetWorkerPrivate(), mProxy.forget(),
> +                                    aStatus, aSuccess);
> +    r->Dispatch();

This could probably get a MOZ_ALWAYS_TRUE() annotation.

@@ +169,5 @@
> +      new WorkerUnsubscribeResultCallback(mProxy);
> +
> +    nsCOMPtr<nsIPushService> service =
> +      do_GetService("@mozilla.org/push/Service;1");
> +    if (!service) {

NS_WARN_IF?

@@ +196,5 @@
> +PushSubscription::PushSubscription(nsIGlobalObject* aGlobal,
> +                                   const nsAString& aEndpoint,
> +                                   const nsAString& aScope,
> +                                   const nsTArray<uint8_t>& aRawP256dhKey,
> +                                   const nsTArray<uint8_t>& aAuthSecret)

This should take move references to these arrays so we don't have to copy them, which is what the current code does.

@@ +202,5 @@
> +  , mScope(aScope)
> +  , mRawP256dhKey(aRawP256dhKey)
> +  , mAuthSecret(aAuthSecret)
> +{
> +  if (NS_IsMainThread()) {

I wouldn't bother.

@@ +247,5 @@
> +  nsTArray<uint8_t> rawKey;
> +  if (!aP256dhKey.IsNull()) {
> +    const ArrayBuffer& key = aP256dhKey.Value();
> +    key.ComputeLengthAndData();
> +    rawKey.SetLength(key.Length());

This is a content-controlled size, so please make this fallible (SetLength(blah, fallible)), and aRv.Throw(NS_ERROR_OUT_OF_MEMORY) if necessary.

@@ +255,5 @@
> +  nsTArray<uint8_t> authSecret;
> +  if (!aAuthSecret.IsNull()) {
> +    const ArrayBuffer& sekrit = aAuthSecret.Value();
> +    sekrit.ComputeLengthAndData();
> +    authSecret.SetLength(sekrit.Length());

ibid.

@@ +263,5 @@
> +  RefPtr<PushSubscription> sub = new PushSubscription(global,
> +                                                      aEndpoint,
> +                                                      aScope,
> +                                                      rawKey,
> +                                                      authSecret);

And then you can Move(rawKey) and Move(authSecret).

::: dom/push/PushSubscription.h
@@ +38,5 @@
> +  PushSubscription(nsIGlobalObject* aGlobal,
> +                   const nsAString& aEndpoint,
> +                   const nsAString& aScope,
> +                   const nsTArray<uint8_t>& aP256dhKey,
> +                   const nsTArray<uint8_t>& aAuthSecret);

This should take move references to these arrays, so we don't have to copy them.
Attachment #8736815 - Flags: review?(khuey) → review+
(In reply to Kit Cambridge [:kitcambridge] from comment #6)
> Created attachment 8736817 [details] [diff] [review]
> PushManager.patch
> 
> Here's part 2. The JS-implemented main thread PushManager made this a bit
> more tedious than it had to be, but it compiles, and the tests pass locally.
> 
> Instead of the `setScope` and `setPushManagerImpl` methods, I just gave both
> interfaces ChromeConstructors, and it looks like that will automatically
> call `ConstructJSImplementation` for me.
> 
> One thing I'm not sure of is the `AutoJSAPI` usage in
> ServiceWorkerRegistrationMainThread::GetPushManager. I borrowed that idiom
> from
> https://dxr.mozilla.org/mozilla-central/rev/
> 494289c72ba3997183e7b5beaca3e0447ecaf96d/dom/network/TCPSocket.cpp#70-76. I
> don't think I can use nsContentUtils::RootingCxForThread without tripping a
> cross-compartment assertion in the PushManagerImpl::Constructor binding.

Yeah, you can't use the rooting cx for anything more than rooting.
Comment on attachment 8736817 [details] [diff] [review]
PushManager.patch

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

::: dom/push/PushManager.cpp
@@ +404,5 @@
> +#ifdef DEBUG
> +    // There's only one global on a worker, so we don't need to specify.
> +    WorkerPrivate* worker = GetCurrentThreadWorkerPrivate();
> +    MOZ_ASSERT(worker);
> +    worker->AssertIsOnWorkerThread();

two space indentation, like the rest of this file please

::: dom/push/PushManager.h
@@ +98,4 @@
>  
>  private:
> +  nsCOMPtr<nsIGlobalObject> mGlobal;
> +  RefPtr<PushManagerImpl> mImpl;

Please document here in comments which of these are used on which thread.

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +768,5 @@
>        return nullptr;
>      }
>  
> +    AutoJSAPI api;
> +    if (NS_WARN_IF(!api.Init(globalObject))) {

You could just mark this attribute getter as implicitJSContext, and then this method will get a JSContext* argument, and you won't need the AutoJSAPI here.

@@ +1223,3 @@
>    return ret.forget();
>  
>    #endif /* ! MOZ_SIMPLEPUSH */

nit, while you're here, can you move this all the way to the left?
Attachment #8736817 - Flags: review?(khuey) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #9)
> This is a content-controlled size, so please make this fallible
> (SetLength(blah, fallible)), and aRv.Throw(NS_ERROR_OUT_OF_MEMORY) if
> necessary.

Done.

> This should take move references to these arrays, so we don't have to copy
> them.

Converted in part 2 to reduce rebase churn, since `PushManager` creates a `PushSubscription` and can use move references, too.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #11)
> You could just mark this attribute getter as implicitJSContext, and then
> this method will get a JSContext* argument, and you won't need the AutoJSAPI
> here.

Neat! Thanks.
https://hg.mozilla.org/mozilla-central/rev/f07da9ef1c67
https://hg.mozilla.org/mozilla-central/rev/e1a56a0d9db2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.