Closed
Bug 1257401
Opened 8 years ago
Closed 8 years ago
De-workerify PushSubscription and PushManager
Categories
(Core :: DOM: Notifications, defect)
Core
DOM: Notifications
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: lina, Assigned: lina)
References
Details
Attachments
(2 files, 1 obsolete file)
34.76 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
29.17 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
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+
Assignee | ||
Comment 5•8 years ago
|
||
Thanks, Kyle! Here's an updated patch that incorporates your suggestions.
Attachment #8732553 -
Attachment is obsolete: true
Attachment #8736815 -
Flags: review?(khuey)
Assignee | ||
Comment 6•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=81554fa6b607
Assignee | ||
Comment 8•8 years ago
|
||
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+
Assignee | ||
Comment 12•8 years ago
|
||
(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.
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f07da9ef1c67 https://hg.mozilla.org/integration/mozilla-inbound/rev/e1a56a0d9db2
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f07da9ef1c67 https://hg.mozilla.org/mozilla-central/rev/e1a56a0d9db2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•