Closed Bug 982726 Opened 11 years ago Closed 10 years ago

ServiceWorkers: Implement interaction with controlled windows from worker scope.

Categories

(Core :: DOM: Workers, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: nsm, Assigned: catalinb)

References

Details

Attachments

(10 files, 35 obsolete files)

(deleted), patch
RyanVM
: checkin+
Details | Diff | Splinter Review
(deleted), patch
RyanVM
: checkin+
Details | Diff | Splinter Review
(deleted), patch
RyanVM
: checkin+
Details | Diff | Splinter Review
(deleted), patch
baku
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
(deleted), patch
RyanVM
: checkin+
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
baku
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
(deleted), patch
RyanVM
: checkin+
Details | Diff | Splinter Review
(deleted), patch
nsm
: review+
catalinb
: checkin+
Details | Diff | Splinter Review
(deleted), patch
nsm
: review+
catalinb
: checkin+
Details | Diff | Splinter Review
Assignee: nobody → cbadea
Comment on attachment 8456568 [details] [diff] [review] WIP: ServiceWorkerClients and Client interfaces with getServiced(). Nikhil, could you please take a look at the patch and give me some early feedback?
Attachment #8456568 - Flags: review?(nsm.nikhil)
Comment on attachment 8456568 [details] [diff] [review] WIP: ServiceWorkerClients and Client interfaces with getServiced(). Review of attachment 8456568 [details] [diff] [review]: ----------------------------------------------------------------- Excellent! ::: dom/bindings/Bindings.conf @@ +230,5 @@ > }, > > +'Client': { > + 'nativeType': 'mozilla::dom::workers::Client', > + 'headerFile': 'mozilla/dom/workers/bindings/Client.h', If this is only available on ServiceWorkerGlobalScope, you should add |workers: True| and use the '_workers' form of namespace so that it isn't exposed on the main thread. @@ +1069,5 @@ > > +'ServiceWorkerClients': { > + 'nativeType': 'mozilla::dom::workers::ServiceWorkerClients', > + 'headerFile': 'mozilla/dom/workers/bindings/ServiceWorkerClients.h', > +}, same for this. ::: dom/workers/Client.h @@ +20,5 @@ > + > +namespace workers { > + > +class Client MOZ_FINAL : public nsISupports, > + public nsWrapperCache { Nit: { on newline. @@ +23,5 @@ > +class Client MOZ_FINAL : public nsISupports, > + public nsWrapperCache { > +public: > + NS_DECL_CYCLE_COLLECTING_ISUPPORTS > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(Client) This doesn't hold an jsvals, so don't need to be SCRIPT_HOLDER. ::: dom/workers/ServiceWorkerClients.cpp @@ +62,5 @@ > + } > + mPromise->MaybeResolve(ret); > + > + // release the reference on the worker thread. > + nsRefPtr<Promise> kungFu = mPromise.forget(); This isn't a kungFuDeathGrip :) I'd call it something different. @@ +75,5 @@ > + nsCString mDomain; > + nsCString mScope; > +public: > + explicit GetServicedRunnable(WorkerPrivate* aWorkerPrivate, > + nsRefPtr<Promise> aPromise, Promise* aPromise? @@ +83,5 @@ > + mPromise(aPromise), > + mDomain(aDomain), > + mScope(aScope) > + { > + } I'm assuming you intend to do AddFeature/RemoveFeature before landing this. ::: dom/workers/ServiceWorkerClients.h @@ +25,5 @@ > +class ServiceWorkerClients MOZ_FINAL : public nsISupports, > + public nsWrapperCache { > +public: > + NS_DECL_CYCLE_COLLECTING_ISUPPORTS > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(ServiceWorkerClients) Same here about SCRIPT_HOLDER. ::: dom/workers/ServiceWorkerManager.cpp @@ +1245,5 @@ > +ServiceWorkerManager::GetServicedClients(const nsCString& aDomain, > + const nsCString& aScope, nsTArray<uint64_t>* controlledDocuments) > +{ > + nsRefPtr<ServiceWorkerDomainInfo> domainInfo; > + mDomainMap.Get(aDomain, getter_AddRefs(domainInfo)); You can now use GetDomainInfo(). Although not strictly required here, let's keep accesses to mDomainMap in one location. @@ +1247,5 @@ > +{ > + nsRefPtr<ServiceWorkerDomainInfo> domainInfo; > + mDomainMap.Get(aDomain, getter_AddRefs(domainInfo)); > + > + nsRefPtr<ServiceWorkerRegistration> registration = domainInfo->GetRegistration(aScope); Add an assertion that this can never be null.
Attachment #8456568 - Flags: review?(nsm.nikhil) → feedback+
Attachment #8456568 - Attachment is obsolete: true
Attachment #8459813 - Attachment is obsolete: true
Attached patch test_kill_worker.patch (obsolete) (deleted) — Splinter Review
Close the worker right after dispatching GetServiced to test that the promise is released gracefully.
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #3) I made all the changes you requested, except for the SCRIPT_HOLDER as we've discussed offline. Try run: https://tbpl.mozilla.org/?tree=Try&rev=7767ee6b112a Try run with getServiced closing the worker right after the first dispatch (test_kill_worker attachment): https://tbpl.mozilla.org/?tree=Try&rev=dd74942841d8
Attached patch Patch 2 - Add postMessage to service workers. (obsolete) (deleted) — Splinter Review
Comment on attachment 8459855 [details] [diff] [review] ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced() Review of attachment 8459855 [details] [diff] [review]: ----------------------------------------------------------------- Kyle, please have a look at this patch - the PromiseHolder / WorkerFeature part in particular.
Attachment #8459855 - Flags: review?(khuey)
Attachment #8459874 - Flags: review?(nsm.nikhil)
Attachment #8459981 - Flags: review?(nsm.nikhil)
Comment on attachment 8459981 [details] [diff] [review] Patch 3 - Implement client.postMessage and add tests for getServiced(). Review of attachment 8459981 [details] [diff] [review]: ----------------------------------------------------------------- Please also add a test that enumerates the array of clients and postmessages to all of them and ensure that messages are received. r=me with the below changes. ::: dom/webidl/Client.webidl @@ +14,1 @@ > void postMessage(any message, DOMString targetOrigin, https://github.com/slightlyoff/ServiceWorker/issues/190 ? Please remove targetorigin. ::: dom/workers/Client.cpp @@ +91,5 @@ > + if (!window) { > + return NS_ERROR_FAILURE; > + } > + > + AutoSafeJSContext cx; Please use AutoJSAPI. @@ +99,5 @@ > + return NS_OK; > + } > +}; > + > +void Client::PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage, Do get a once over from khuey/bent (feedback?) on this method. It seems like you distilled much of the worker messaging stuff, into what is required only for worker thread to main thread messaging. Good! ::: dom/workers/test/serviceworkers/message_posting_worker.js @@ +2,5 @@ > + self.clients.getServiced().then(function(res) { > + if (!res.length) { > + dump("ERROR: no clients are currently controlled.\n"); > + } > + res[0].postMessage(e.data, "*"); Indentation. ::: dom/workers/test/serviceworkers/test_get_serviced_advanced.html @@ +15,5 @@ > +<pre id="test"></pre> > +<script class="testbody" type="text/javascript"> > + > + function start() { > + opened = []; Just define this outside of the function. Do use var. @@ +17,5 @@ > + > + function start() { > + opened = []; > + return navigator.serviceWorker.register("get_serviced_worker_advanced.js").then(function(e) { > + worker = e; Same two comments here as in test_post_message.html. ::: dom/workers/test/serviceworkers/test_post_message.html @@ +16,5 @@ > +<script class="testbody" type="text/javascript"> > + > + function start() { > + magic_value = "MAGIC_VALUE_123"; > + return navigator.serviceWorker.register("message_posting_worker.js").then(function(e) { Please pass a scope localized to the dom/workers/test/serviceworkers directory at least :) Also unregister() the worker in runTest() below. Nit: Call the parameter sw instead of e. @@ +17,5 @@ > + > + function start() { > + magic_value = "MAGIC_VALUE_123"; > + return navigator.serviceWorker.register("message_posting_worker.js").then(function(e) { > + worker = e; Please declare worker in the global scope before using it here. @@ +48,5 @@ > + .then(function(e) { > + SimpleTest.finish(); > + }).catch(function(e) { > + ok(false, "Some test failed with error " + e); > + SimpleTest.finish(); You can avoid the duplicate SimpleTest.finish() by using .then(testPostMessage) .catch(function(e) { ok(false, ...); }) .then(SimpleTest.finish)
Attachment #8459981 - Flags: review?(nsm.nikhil) → review+
Comment on attachment 8459874 [details] [diff] [review] Patch 2 - Add postMessage to service workers. Review of attachment 8459874 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the [Throws] removed. ::: dom/webidl/ServiceWorker.webidl @@ +19,5 @@ > attribute EventHandler onstatechange; > + > + // FIXME(catalinb): Temporary fix needed for testing service workers. > + [Throws] > + void postMessage(any message, optional sequence<Transferable> transferable); The worker spec does not define postMessage as throwing. See my comment below. ::: dom/workers/ServiceWorker.cpp @@ +49,5 @@ > +ServiceWorker::PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage, > + const Optional<Sequence<JS::Value>>& aTransferable, > + ErrorResult& aRv) > +{ > + WorkerPrivate* workerPrivate = GetWorkerPrivate(); Please add an assertion for workerPrivate. @@ +50,5 @@ > + const Optional<Sequence<JS::Value>>& aTransferable, > + ErrorResult& aRv) > +{ > + WorkerPrivate* workerPrivate = GetWorkerPrivate(); > + workerPrivate->PostMessage(aCx, aMessage, aTransferable, aRv); You can just create an |ErrorResult result;| on the stack here and pass it to PostMessage.
Attachment #8459874 - Flags: review?(nsm.nikhil) → review+
Attachment #8459855 - Flags: review?(nsm.nikhil)
Attachment #8459855 - Flags: review?(khuey)
Attachment #8459855 - Flags: feedback?(khuey)
Comment on attachment 8459855 [details] [diff] [review] ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced() Review of attachment 8459855 [details] [diff] [review]: ----------------------------------------------------------------- Kyle/Ben should look at the Promise keep alive. You'll need dom peer review on the webidl. ::: dom/webidl/Client.webidl @@ +9,5 @@ > + */ > + > +interface Client { > + readonly attribute unsigned long id; > + void postMessage(any message, DOMString targetOrigin, remove targetorigin here too. ::: dom/workers/Client.h @@ +35,5 @@ > + } > + > + uint32_t Id() const > + { > + // XXXcatalinb the id exposed to JS should be mangled. In discussion with catalin, this comment is no longer relevant. ::: dom/workers/ServiceWorkerClients.cpp @@ +202,5 @@ > + > + return promise.forget(); > +} > + > +already_AddRefed<Promise> ServiceWorkerClients::ReloadAll() Please add a FIXME comment with Bug number to actually implement this. ::: dom/workers/ServiceWorkerManager.cpp @@ +1252,5 @@ > + const nsCString& aScope, nsTArray<uint64_t>* controlledDocuments) > +{ > + nsRefPtr<ServiceWorkerDomainInfo> domainInfo = GetDomainInfo(aScriptURL); > + > + nsRefPtr<ServiceWorkerRegistration> registration = domainInfo->GetRegistration(aScope); You can just use SWM::GetRegistration(aScope) since aScope is a full URL. That way you don't need the aScriptURL argument.
Attachment #8459855 - Flags: review?(nsm.nikhil) → review+
Blocks: 1045257
Comment on attachment 8459855 [details] [diff] [review] ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced() Review of attachment 8459855 [details] [diff] [review]: ----------------------------------------------------------------- It's only ok to call Clean() on the worker thread (since you're going to call RemoveWorkerFeature). Please add an assertion for that. Checking mClean on the main thread is unnecessary. If the worker has shut down already ResolveWorkerPromiseRunnable will fail to dispatch, and if it shuts down between dispatch and running that event the runnable will be canceled and discarded. Getting rid of that check also lets you get rid of the lock. I didn't look at this exhaustively. ::: dom/workers/ServiceWorkerClients.cpp @@ +60,5 @@ > + } > + > + Promise* Get() const > + { > + return mPromise.get(); You should be able to just return mPromise, I would think. @@ +107,5 @@ > +public: > + ResolvePromiseWorkerRunnable(WorkerPrivate* aWorkerPrivate, > + PromiseHolder* aPromiseHolder, > + nsAutoPtr<nsTArray<uint64_t> > aValue) > + : WorkerRunnable(aWorkerPrivate, WorkerThreadUnchangedBusyCount), This causes script to run, so it needs to be WorkerThreadModifyBusyCount.
Attachment #8459855 - Flags: feedback?(khuey) → feedback-
Attachment #8459855 - Attachment is obsolete: true
Attachment #8459857 - Attachment is obsolete: true
Attached patch Patch 2 - Add postMessage to service workers. (obsolete) (deleted) — Splinter Review
Attachment #8459874 - Attachment is obsolete: true
Attachment #8459981 - Attachment is obsolete: true
Attachment #8466486 - Flags: review?(bent.mozilla)
Attachment #8466490 - Flags: review?(bent.mozilla)
Attachment #8466486 - Flags: review?(bent.mozilla) → review?(amarchesini)
Attachment #8466490 - Flags: review?(bent.mozilla) → review?(amarchesini)
Attachment #8466489 - Flags: review?(amarchesini)
Comment on attachment 8466486 [details] [diff] [review] Patch 1 - ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced() Review of attachment 8466486 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Bindings.conf @@ +238,5 @@ > 'nativeType': 'mozilla::dom::workers::ChromeWorkerPrivate', > }, > > +'Client': { > + 'workers': True, This is not needed. @@ +1073,5 @@ > 'headerFile': 'mozilla/dom/ServiceWorkerContainer.h', > }, > > +'ServiceWorkerClients': { > + 'workers': True, Remove this. ::: dom/webidl/Client.webidl @@ +6,5 @@ > + * The origin of this IDL file is > + * http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html > + * > + */ > + spec is changed. This is called ServiceWorkerClient. Use: [Exposed=ServiceWorker] to expose the object to worker. ::: dom/webidl/ServiceWorkerClients.webidl @@ +6,5 @@ > + * The origin of this IDL file is > + * http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html > + * > + */ > + [Exposed=ServiceWorker] ::: dom/workers/Client.h @@ +6,5 @@ > + > +#ifndef mozilla_dom_workers_client_h > +#define mozilla_dom_workers_client_h > + > +#include "mozilla/dom/ClientBinding.h" Remove this. @@ +9,5 @@ > + > +#include "mozilla/dom/ClientBinding.h" > + > +#include "nsISupportsImpl.h" > +#include "nsIGlobalObject.h" do you need this? @@ +40,5 @@ > + } > + > + void PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage, > + const Optional<Sequence<JS::Value>>& aTransferable) > + { This comes with the other patches, I guess. @@ +49,5 @@ > + return mOwner; > + } > + > + JSObject* WrapObject(JSContext* cx) MOZ_OVERRIDE > + { Move this to the .cpp file. @@ +58,5 @@ > + ~Client() > + { > + } > + > + nsISupports* mOwner; nsCOMPtr<nsISupports> mOwner; @@ +66,5 @@ > +} // namespace workers > +} // namespace dom > +} // namespace mozilla > + > +#endif // mozilla_dom_workers_client_h ::: dom/workers/ServiceWorkerClients.cpp @@ +7,5 @@ > +#include "ServiceWorkerManager.h" > +#include "WorkerScope.h" > +#include "Client.h" > + > +#include "nsDOMString.h" DO you need this? @@ +8,5 @@ > +#include "WorkerScope.h" > +#include "Client.h" > + > +#include "nsDOMString.h" > +#include "nsPIDOMWindow.h" this is not needed and probably RuntimeService.h can be removed as well. @@ +20,5 @@ > +using namespace mozilla::dom::workers; > + > +NS_IMPL_CYCLE_COLLECTING_ADDREF(ServiceWorkerClients) > +NS_IMPL_CYCLE_COLLECTING_RELEASE(ServiceWorkerClients) > +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(ServiceWorkerClients) NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(ServiceWorkerClients, mWorkerScope) @@ +35,5 @@ > +} > + > +// Helper class used for passing the promise between threads while > +// keeping the worker alive. > +class PromiseHolder : public WorkerFeature 1. MOZ_FINAL 2. put this class in an anonymous namespace. But most important, probably what you really want is just PromiseWorkerProxy. Take a look at ./dom/promise/PromiseWorkerProxy.h @@ +37,5 @@ > +// Helper class used for passing the promise between threads while > +// keeping the worker alive. > +class PromiseHolder : public WorkerFeature > +{ > + friend class GetServicedRunnable; Why this? @@ +42,5 @@ > + > + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(PromiseHolder) > + > +public: > + explicit PromiseHolder(WorkerPrivate* aWorkerPrivate, explicit is not needed if you have 2 params. @@ +97,5 @@ > + > + bool mClean; > +}; > + > +class ResolvePromiseWorkerRunnable : public WorkerRunnable anonymous namespace + MOZ_FINAL @@ +104,5 @@ > + nsAutoPtr<nsTArray<uint64_t> > mValue; > +public: > + ResolvePromiseWorkerRunnable(WorkerPrivate* aWorkerPrivate, > + PromiseHolder* aPromiseHolder, > + nsAutoPtr<nsTArray<uint64_t> > aValue) >> @@ +116,5 @@ > + { > + MOZ_ASSERT(aWorkerPrivate); > + aWorkerPrivate->AssertIsOnWorkerThread(); > + > + nsTArray<nsRefPtr<Client> > ret; >> @@ +117,5 @@ > + MOZ_ASSERT(aWorkerPrivate); > + aWorkerPrivate->AssertIsOnWorkerThread(); > + > + nsTArray<nsRefPtr<Client> > ret; > + for (size_t i = 0; i < mValue.get()->Length(); i++) { is get() needed? @@ +162,5 @@ > + return NS_OK; > + } > +}; > + > +already_AddRefed<Promise> ServiceWorkerClients::GetServiced() I guess you should mark this method as [Throws] in the the WebIDL file. Then use the ErrorResult object to propagate errors. @@ +167,5 @@ > +{ > + WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate(); > + MOZ_ASSERT(workerPrivate); > + > + DOMString scope; nsString scope; @@ +175,5 @@ > + > + ErrorResult rv; > + nsRefPtr<Promise> promise = Promise::Create(global, rv); > + if (rv.Failed()) { > + return nullptr; Once you have the ErrorResult object, this will be: nsRefPtr<Promise> promise = Promise::Create(global, aRv); if (NS_WARN_IF(aRv.Failed())) { return nullptr; } @@ +179,5 @@ > + return nullptr; > + } > + > + nsRefPtr<GetServicedRunnable> r = > + new GetServicedRunnable(workerPrivate, promise, NS_ConvertUTF16toUTF8(scope.AsAString())); NS_ConvertUTF16toUTF8(scope) @@ +180,5 @@ > + } > + > + nsRefPtr<GetServicedRunnable> r = > + new GetServicedRunnable(workerPrivate, promise, NS_ConvertUTF16toUTF8(scope.AsAString())); > + rv = NS_DispatchToMainThread(r); nsresult rv = NS_DispatchToMainThread(r); if (NS_WARN_IF(NS_FAILED(rv)) { ... @@ +190,5 @@ > + return promise.forget(); > +} > + > +// FIXME(catalinb): Bug 1045257 - Implement ReloadAll > +already_AddRefed<Promise> ServiceWorkerClients::ReloadAll() Also this method should have a ErrorResult param. Add [Throws] in the WebIDL. ::: dom/workers/ServiceWorkerClients.h @@ +6,5 @@ > + > +#ifndef mozilla_dom_workers_serviceworkerclients_h > +#define mozilla_dom_workers_serviceworkerclients_h > + > +#include "mozilla/dom/ServiceWorkerClientsBinding.h" remove this @@ +9,5 @@ > + > +#include "mozilla/dom/ServiceWorkerClientsBinding.h" > + > +#include "nsISupportsImpl.h" > +#include "nsIGlobalObject.h" do you need this? @@ +22,5 @@ > + > +class ServiceWorkerGlobalScope; > + > +class ServiceWorkerClients MOZ_FINAL : public nsISupports, > + public nsWrapperCache { { in a new line. @@ +34,5 @@ > + already_AddRefed<Promise> ReloadAll(); > + > + JSObject* WrapObject(JSContext* cx) > + { > + return ServiceWorkerClientsBinding_workers::Wrap(cx, this); in the cpp file. @@ +36,5 @@ > + JSObject* WrapObject(JSContext* cx) > + { > + return ServiceWorkerClientsBinding_workers::Wrap(cx, this); > + } > + nsISupports* GetParentObject() const; new line before nsISupports. Change this in: ServiceWorkerGlobalScope* GetParentObject() const { return mWorkerScope; } @@ +43,5 @@ > + ~ServiceWorkerClients() > + { > + } > + > + ServiceWorkerGlobalScope* mWorkerScope; nsRefPtr ::: dom/workers/ServiceWorkerManager.cpp @@ +1410,5 @@ > > aScope = NS_ConvertUTF8toUTF16(r->mScope); > return NS_OK; > } > all of this goes into an anonymous namespace. ::: dom/workers/ServiceWorkerManager.h @@ +315,5 @@ > static already_AddRefed<ServiceWorkerManager> > GetInstance(); > > + NS_IMETHODIMP > + GetServicedClients(const nsCString& aScope, You don't need this. Just change nsIServiceWorkerManager.idl
Attachment #8466486 - Flags: review?(amarchesini) → review-
I'll wait for a new version of patch 1 before reviewing patch 2 and 3. Is it ok?
(In reply to Andrea Marchesini (:baku) from comment #19) > I'll wait for a new version of patch 1 before reviewing patch 2 and 3. Is it > ok? Yes, I'll also update patches 2 and 3 based on your feedback for the first patch. I have another version for patch 1 (as per your comments), but I want to go through the code one more time before uploading. I have to work on my intern presentation right now. :D
Comment on attachment 8471249 [details] [diff] [review] Patch 1(v2) - ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced() I have addressed the issues you mentioned, with a few exceptions: (In reply to Andrea Marchesini (:baku) from comment #18) > @@ +35,5 @@ > > +} > > + > > +// Helper class used for passing the promise between threads while > > +// keeping the worker alive. > > +class PromiseHolder : public WorkerFeature > > But most important, probably what you really want is just > PromiseWorkerProxy. Take a look at ./dom/promise/PromiseWorkerProxy.h I've already looked at PromiseWorkerProxy. The reason I can't use it is because it requires creating a second promise on the main thread, which is not possible since there is no available global object on the main thread for sw. > @@ +167,5 @@ > > +{ > > + WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate(); > > + MOZ_ASSERT(workerPrivate); > > + > > + DOMString scope; > > nsString scope; The getter for the scope requires a DOMString and the compiler complains. > ::: dom/workers/ServiceWorkerManager.h > @@ +315,5 @@ > > static already_AddRefed<ServiceWorkerManager> > > GetInstance(); > > > > + NS_IMETHODIMP > > + GetServicedClients(const nsCString& aScope, > > You don't need this. Just change nsIServiceWorkerManager.idl I removed the NS_IMETHODIMP return type since it wasn't really being used. I've kept the declaration in the .h for the following reasons: 1. The method is only used from c++ code and I'm using the singleton instance getter for ServiceWorkerManager which returns the exact SWM type. 2. Converting the arguments to IDL types will add unnecessary conversions (or maybe I missing something): there is no IDL equivalent for nsTArray<uint64_t>. Also, in ServiceWorkerClients, because the reference to the parent object is now a strong reference, I had to add cycle collecting info for ServiceWorkerGlobalScope.
Attachment #8471249 - Flags: review?(amarchesini)
Attachment #8466489 - Attachment is obsolete: true
Attachment #8466489 - Flags: review?(amarchesini)
Attachment #8471259 - Flags: review?(amarchesini)
Attachment #8466490 - Attachment is obsolete: true
Attachment #8466490 - Flags: review?(amarchesini)
Attachment #8471261 - Flags: review?(amarchesini)
Comment on attachment 8471249 [details] [diff] [review] Patch 1(v2) - ServiceWorkers: ServiceWorkerClients and Client interfaces with getServiced() Review of attachment 8471249 [details] [diff] [review]: ----------------------------------------------------------------- Good. Can I see it again with this comments fixed? Thanks. ::: dom/webidl/ServiceWorkerClient.webidl @@ +9,5 @@ > + */ > + > +[Exposed=ServiceWorker] > +interface ServiceWorkerClient { > + readonly attribute unsigned long id; no postMessage? I guess it's in another patch. ::: dom/workers/ServiceWorkerClient.cpp @@ +20,5 @@ > + NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY > + NS_INTERFACE_MAP_ENTRY(nsISupports) > +NS_INTERFACE_MAP_END > + > +JSObject* ServiceWorkerClient::WrapObject(JSContext* cx) JSObject* Ser... aCx) ::: dom/workers/ServiceWorkerClient.h @@ +40,5 @@ > + { > + return mOwner; > + } > + > + JSObject* WrapObject(JSContext* cx) MOZ_OVERRIDE; aCx ::: dom/workers/ServiceWorkerClients.cpp @@ +13,5 @@ > + > +#include "mozilla/dom/Promise.h" > +#include "mozilla/dom/ServiceWorkerClientsBinding.h" > + > +using namespace mozilla; I guess this is just for ErrorResult, so just do: using mozilla::ErrorResult; @@ +33,5 @@ > + MOZ_ASSERT(mWorkerScope); > + SetIsDOMBinding(); > +} > + > +JSObject* ServiceWorkerClients::WrapObject(JSContext* cx) JSObject* ServiceWorkerClients::WrapObject(JSContext* aCx) @@ +111,5 @@ > + > +public: > + ResolvePromiseWorkerRunnable(WorkerPrivate* aWorkerPrivate, > + PromiseHolder* aPromiseHolder, > + nsAutoPtr<nsTArray<uint64_t>> aValue) nsAutoPtr<...>& @@ +172,5 @@ > +}; > + > +} // anonymous namespace > + > +already_AddRefed<Promise> ServiceWorkerClients::GetServiced(ErrorResult& aRv) returnType Class::Method() everywhere in any CPP file. @@ +175,5 @@ > + > +already_AddRefed<Promise> ServiceWorkerClients::GetServiced(ErrorResult& aRv) > +{ > + WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate(); > + MOZ_ASSERT(workerPrivate); Add mWorkerPrivate->AssertIsOnWorkerThread(); @@ +184,5 @@ > + nsRefPtr<Promise> promise = Promise::Create(mWorkerScope, aRv); > + if (NS_WARN_IF(aRv.Failed())) { > + return nullptr; > + } > + you want to add the feature at this point because the worker can be CC/GCed before the runnable is executed. Create the PromiseHolder obj here. ::: dom/workers/ServiceWorkerClients.h @@ +33,5 @@ > + > + already_AddRefed<Promise> GetServiced(ErrorResult& aRv); > + already_AddRefed<Promise> ReloadAll(ErrorResult& aRv); > + > + JSObject* WrapObject(JSContext* cx) MOZ_OVERRIDE; aCx ::: dom/workers/ServiceWorkerManager.cpp @@ +1433,5 @@ > + ServiceWorkerRegistration* aRegistration, > + void* aData) > +{ > + FilterRegistrationData* data = static_cast<FilterRegistrationData*>(aData); > + if (data->mRegistration == aRegistration) { if (data->mRegistration != aRegistration) { return PL_DHASH_NEXT; } @@ +1435,5 @@ > +{ > + FilterRegistrationData* data = static_cast<FilterRegistrationData*>(aData); > + if (data->mRegistration == aRegistration) { > + nsCOMPtr<nsIDocument> document = do_QueryInterface(aKey); > + if (!document->GetInnerWindow()) { document can be null. @@ +1447,5 @@ > + > +} // anonymous namespace > + > +void > +ServiceWorkerManager::GetServicedClients(const nsCString& aScope, nsTArray<uint64_t>* aControlledDocuments) indent in a new line the second param. ::: dom/workers/WorkerScope.h @@ +197,5 @@ > { > // FIXME(nsm): Bug 982728 > } > > + already_AddRefed<ServiceWorkerClients> ServiceWorkerClients* @@ +201,5 @@ > + already_AddRefed<ServiceWorkerClients> > + Clients() { > + if (!mClients) { > + mClients = new ServiceWorkerClients(this); > + MOZ_ASSERT(mClients); remove this assert. "new" doesn't fail. @@ +203,5 @@ > + if (!mClients) { > + mClients = new ServiceWorkerClients(this); > + MOZ_ASSERT(mClients); > + } > + return mClients;
Attachment #8471249 - Flags: review?(amarchesini) → review+
Comment on attachment 8471259 [details] [diff] [review] Patch 2(v2) - Add postMessage to service workers. Review of attachment 8471259 [details] [diff] [review]: ----------------------------------------------------------------- I think this patch should change ServiceWorkerClient and not ServiceWorker. This is what the spec says. ::: dom/webidl/ServiceWorker.webidl @@ +21,5 @@ > readonly attribute ServiceWorkerState state; > attribute EventHandler onstatechange; > + > + [Throws] > + void postMessage(any message, optional sequence<Transferable> transferable); This goes in ServiceWorkerClient... right?
Attachment #8471259 - Flags: review?(amarchesini)
Flags: needinfo?(cbadea)
Comment on attachment 8471261 [details] [diff] [review] Patch 3(v2) - Implement client.postMessage and add tests for getServiced(). Review of attachment 8471261 [details] [diff] [review]: ----------------------------------------------------------------- I would like to see a test for: 1. postMessage with DOMFile/DOMBlob, and maybe add also normal object, booleans, strings, arrays. 2. postMessage with transferable objects. ArrayBuffers are transferable and check that, after the postMessage, the arrayBuffer is empty from 1 side, and fully populated on the other side. A separated patch for this is ok. Smaug, can you take a look at how the MessageEvent is dispatched? ::: dom/workers/ServiceWorkerClient.cpp @@ +34,5 @@ > } > > +namespace { > + > +class ServiceWorkerPostMessageRunnable MOZ_FINAL : public nsRunnable ServiceWorkerClientPostMessageRunnable? @@ +38,5 @@ > +class ServiceWorkerPostMessageRunnable MOZ_FINAL : public nsRunnable > +{ > + uint64_t mId; > + JSAutoStructuredCloneBuffer mBuffer; > + nsTArray<nsCOMPtr<nsISupports> > mClonedObjects; >> @@ +43,5 @@ > + > +public: > + ServiceWorkerPostMessageRunnable(uint64_t aId, > + JSAutoStructuredCloneBuffer&& aData, > + nsTArray<nsCOMPtr<nsISupports> >& aClonedObjects) >> @@ +49,5 @@ > + mBuffer(Move(aData)) > + { > + mClonedObjects.SwapElements(aClonedObjects); > + } > + DispatchDOMEvent should be private. @@ +51,5 @@ > + mClonedObjects.SwapElements(aClonedObjects); > + } > + > + bool DispatchDOMEvent(JSContext* aCx, nsGlobalWindow* aTargetWindow) > + { AssertIsOnMainThread(); @@ +59,5 @@ > + clonedObjects.SwapElements(mClonedObjects); > + > + JS::Rooted<JS::Value> messageData(aCx); > + if (!mBuffer.read(aCx, &messageData, > + WorkerStructuredCloneCallbacks(false))) { why false? You want to dispatch events to the main-thread so this must be true. Add a mochitest where you pass a DOMBlob object. You should test also the transferable obj list. @@ +77,5 @@ > + if (NS_FAILED(rv)) { > + xpc::Throw(aCx, rv); > + return false; > + } > + I think you can just do this: event->SetTrusted(true); bool status = false; aTargetWindow->DispatchEvent(event, &status); if (!status) { ... I'll ask smaug a feedback about this. @@ +112,5 @@ > +}; > + > +} // anonymous namespace > + > +void ServiceWorkerClient::PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage, void ServiceWorkerClient:...
Attachment #8471261 - Flags: review?(bugs)
Attachment #8471261 - Flags: review?(amarchesini)
Attachment #8471261 - Flags: review+
Comment on attachment 8471261 [details] [diff] [review] Patch 3(v2) - Implement client.postMessage and add tests for getServiced(). >+ nsRefPtr<MessageEvent> event = new MessageEvent(aTargetWindow, nullptr, nullptr); >+ nsresult rv = >+ event->InitMessageEvent(NS_LITERAL_STRING("message"), >+ false /* non-bubbling */, >+ true /* cancelable */, Is this really supposed to be cancelable? Yet you don't do anything if preventDefault() is actually called >+ event->SetTrusted(true); >+ WidgetEvent* internalEvent = event->GetInternalNSEvent(); >+ >+ nsEventStatus status = nsEventStatus_eIgnore; >+ EventDispatcher::Dispatch(static_cast<nsPIDOMWindow*>(aTargetWindow), >+ presContext, >+ internalEvent, >+ static_cast<dom::Event*>(event.get()), >+ &status); Just do bool dummy; aTargetWindow->DispatchEvent(event, &dummy);
Attachment #8471261 - Flags: review?(bugs) → review+
(In reply to Andrea Marchesini (:baku) from comment #27) > Comment on attachment 8471259 [details] [diff] [review] > Patch 2(v2) - Add postMessage to service workers. > > Review of attachment 8471259 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think this patch should change ServiceWorkerClient and not ServiceWorker. > This is what the spec says. > > ::: dom/webidl/ServiceWorker.webidl > @@ +21,5 @@ > > readonly attribute ServiceWorkerState state; > > attribute EventHandler onstatechange; > > + > > + [Throws] > > + void postMessage(any message, optional sequence<Transferable> transferable); > > This goes in ServiceWorkerClient... right? No. The spec specifies that ServiceWorker inherits from Worker which has the postMessage method. With our current implementation, SW inherits from EventTarget because the inheritance was an open issue [1]. I need serviceWorker.postMessage for running the tests, so I decided to add the definition to the SW interface. Now that the issue has been decided, we should switch to inheriting from Worker, I guess. Either way, the implementation should be the same, only the webidl that's exposing postMessage will change. I will open a separate bug for it, but do you think we can land this and fix the webidl after? [1] https://github.com/slightlyoff/ServiceWorker/issues/189
Flags: needinfo?(cbadea)
Comment on attachment 8471259 [details] [diff] [review] Patch 2(v2) - Add postMessage to service workers. Review of attachment 8471259 [details] [diff] [review]: ----------------------------------------------------------------- Ok. Thanks for your comment. File a bug please. r=me ::: dom/webidl/ServiceWorker.webidl @@ +21,5 @@ > readonly attribute ServiceWorkerState state; > attribute EventHandler onstatechange; > + > + [Throws] > + void postMessage(any message, optional sequence<Transferable> transferable); File a follow up and write here the bug ID.
Attachment #8471259 - Flags: review+
Comment on attachment 8473539 [details] [diff] [review] Patch 1(v3) - Service Worker: ServiceWorkerClients and Client interfaces with getServiced() Updated with the requested changes, with some minor comments: (In reply to Andrea Marchesini (:baku) from comment #26) > ::: dom/workers/ServiceWorkerClients.cpp > @@ +13,5 @@ > > + > > +#include "mozilla/dom/Promise.h" > > +#include "mozilla/dom/ServiceWorkerClientsBinding.h" > > + > > +using namespace mozilla; > > I guess this is just for ErrorResult, so just do: > > using mozilla::ErrorResult; It's also needed for the js (AutoJSContext) context used in dispatching the runnable back to the worker thread. > @@ +184,5 @@ > > + nsRefPtr<Promise> promise = Promise::Create(mWorkerScope, aRv); > > + if (NS_WARN_IF(aRv.Failed())) { > > + return nullptr; > > + } > > + > > you want to add the feature at this point because the worker can be CC/GCed > before the runnable is executed. > Create the PromiseHolder obj here. Well, the PromiseHolder is created in the constructor of GetServicedRunnable, which is called right after the if statement. So, moving the PromiseHolder out won't make any difference. I personally prefer the current code, because it's similar to PromiseWorkerProxy.
Attachment #8473539 - Flags: review?(amarchesini)
Attachment #8471259 - Attachment is obsolete: true
Comment on attachment 8473545 [details] [diff] [review] Patch 2(v3) - Add postMessage to service workers. Added comment for follow-up.
Attachment #8473545 - Flags: review?(amarchesini)
Attachment #8471261 - Attachment is obsolete: true
Comment on attachment 8473547 [details] [diff] [review] Patch 3(v3) - Implement client.postMessage and add tests for getServiced(). Made all the requested changes. (In reply to Olli Pettay [:smaug] from comment #29) > Comment on attachment 8471261 [details] [diff] [review] > Patch 3(v2) - Implement client.postMessage and add tests for getServiced(). > > >+ nsRefPtr<MessageEvent> event = new MessageEvent(aTargetWindow, nullptr, nullptr); > >+ nsresult rv = > >+ event->InitMessageEvent(NS_LITERAL_STRING("message"), > >+ false /* non-bubbling */, > >+ true /* cancelable */, > Is this really supposed to be cancelable? Yet you don't do anything if > preventDefault() is actually called I checked the spec, and it shouldn't be cancelable - just changed the flag.
Attachment #8473547 - Flags: review?(amarchesini)
Attached patch Patch 4 - Extensive testing of postMessage. (obsolete) (deleted) — Splinter Review
Comment on attachment 8473548 [details] [diff] [review] Patch 4 - Extensive testing of postMessage. WIP. I only wrote a test for basic types + blob and file.
Attachment #8473539 - Flags: review?(amarchesini) → review+
Attachment #8473545 - Flags: review?(amarchesini) → review+
Comment on attachment 8473547 [details] [diff] [review] Patch 3(v3) - Implement client.postMessage and add tests for getServiced(). Review of attachment 8473547 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerClient.cpp @@ +114,5 @@ > +} // anonymous namespace > + > +void > +ServiceWorkerClient::PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage, > + const Optional<Sequence<JS::Value>>& aTransferable, indentation ::: dom/workers/test/serviceworkers/get_serviced_worker_enumerate.js @@ +1,4 @@ > +onmessage = function() { > + self.clients.getServiced().then(function(result) { > + dump("result: " + result.length + "\n\n"); > + dump("catalinn\n\n"); mm... remove these messages. ::: dom/workers/test/serviceworkers/message_posting_worker.js @@ +1,4 @@ > +onmessage = function(e) { > + self.clients.getServiced().then(function(res) { > + if (!res.length) { > + dump("ERROR: no clients are currently controlled.\n"); We should report this error properly.
Attachment #8473547 - Flags: review?(amarchesini) → review+
Attached patch Patch 1 - interdiff from v3 (obsolete) (deleted) — Splinter Review
Handle AddFeature failure gracefully. Add call to getServiced() in onclose. More thread assertions.
Attachment #8478030 - Attachment is patch: true
Attachment #8478030 - Attachment mime type: application/octet-stream → text/plain
Attachment #8478030 - Flags: review?(amarchesini)
Attached patch interdiff from Patch 3(v3) (obsolete) (deleted) — Splinter Review
Changes to test. I'm not happy with not using ready. Maybe we should wait for that.
Comment on attachment 8478030 [details] [diff] [review] Patch 1 - interdiff from v3 Review of attachment 8478030 [details] [diff] [review]: ----------------------------------------------------------------- lgtm ::: dom/workers/ServiceWorkerManager.cpp @@ +1981,5 @@ > class MOZ_STACK_CLASS FilterRegistrationData > { > public: > FilterRegistrationData(nsTArray<uint64_t>* aDocuments, > + ServiceWorkerRegistrationInfo* aRegistration) indentation
Attachment #8478030 - Flags: review?(amarchesini) → review+
Attachment #8478118 - Attachment is patch: true
Attachment #8478118 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 8478118 [details] [diff] [review] interdiff from Patch 3(v3) Review of attachment 8478118 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/test/serviceworkers/test_get_serviced_advanced.html @@ +46,5 @@ > } > }); > + var w; > + setTimeout(function() { > + w = window.open("sw_clients/service_worker_controlled.html"); this should be an iframe but it's ok for this patch.
Attachment #8478118 - Flags: review?(amarchesini) → review+
Attached file interdiff from Patch 4(v3) (obsolete) (deleted) —
The earlier patch passed a nsTArray where the WorkerStructuredCloneCallbacks expect a WorkerStructuredCloneClosure, obviously leading to all sorts of bad things.
Attachment #8478643 - Attachment is patch: true
Attachment #8478643 - Attachment mime type: application/octet-stream → text/plain
Attachment #8478643 - Flags: review?(amarchesini)
Comment on attachment 8478643 [details] interdiff from Patch 4(v3) Review of attachment 8478643 [details]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerClient.cpp @@ +54,5 @@ > + bool rv = mBuffer.write(aCx, aValue, aTransferredValue, aCallbacks, > + &mClosure); > + // This hashtable has to be empty because it could contain MessagePort > + // objects that cannot be freed on a different thread. > + mClosure.mTransferredPorts.Clear(); This is not required in this runnable, will get rid of it.
Comment on attachment 8478643 [details] interdiff from Patch 4(v3) WorkerStructuredCloneClosure is from MessagePort :(
Attachment #8478643 - Attachment is obsolete: true
Attachment #8478643 - Attachment is patch: false
Attachment #8478643 - Flags: review?(amarchesini)
(In reply to Andrea Marchesini (:baku) from comment #18) > Comment on attachment 8466486 [details] [diff] [review] > Patch 1 - ServiceWorkers: ServiceWorkerClients and Client interfaces with > getServiced() > > Review of attachment 8466486 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/bindings/Bindings.conf > @@ +238,5 @@ > > 'nativeType': 'mozilla::dom::workers::ChromeWorkerPrivate', > > }, > > > > +'Client': { > > + 'workers': True, > > This is not needed. > > @@ +1073,5 @@ > > 'headerFile': 'mozilla/dom/ServiceWorkerContainer.h', > > }, > > > > +'ServiceWorkerClients': { > > + 'workers': True, > > Remove this. > > ::: dom/webidl/Client.webidl > @@ +6,5 @@ > > + * The origin of this IDL file is > > + * http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html > > + * > > + */ > > + > > spec is changed. This is called ServiceWorkerClient. > > Use: [Exposed=ServiceWorker] to expose the object to worker. Andrea, it seems Exposed=ServiceWorker doesn't yet do what we want it to do. test_interfaces.html keeps complaining. Even if I try just Exposed=Worker. I think we don't want to generate main thread bindings at all for now. What this bug needs is two interfaces ServiceWorkerClient(s) that are solely exposed on ServiceWorkerGlobalScope. AFAIK there is no way to accomplish that right now except using bindings.conf. Boris, is that right?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(amarchesini)
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #53) > https://tbpl.mozilla.org/?tree=Try&rev=2a757b8cae02 Nikhil, thank you for taking over. Unfortunately, I won't be able to continue the work on SW until the beginning of September - I'm on a tight schedule with a school project. I'll ping you once I'm done with it. Assigning the bug to you in the meantime.
Assignee: catalin.badea392 → nsm.nikhil
> AFAIK there is no way to accomplish that right now except using bindings.conf. Other way around. You can't restrict to only ServiceWorkerGlobalScope (but not other worker scopes) using Bindings.conf, since Bindings.conf has no concept of different worker scopes. What you want to do for now, until bug 1048699 is fixed, is something like [Exposed=Workers,Func=something] that will enforce visibility only in service workers. Perhaps restoring the ServiceWorkerGlobalScope::Visible function that attachment 8464540 [details] [diff] [review] removed? Or better yet, just fix bug 1048699. ;)
Flags: needinfo?(bzbarsky)
Blocks: 982728
It's very likely the failures were intermittents as both are known bugs. Andrea landed a fix for Bug 1060517. This patch is completely unrelated to the failures. Trying this again.
Flags: needinfo?(nsm.nikhil)
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #61) > It's very likely the failures were intermittents as both are known bugs. > Andrea landed a fix for Bug 1060517. This patch is completely unrelated to > the failures. Trying this again. More like perma-fails hitting consistently across all platforms. And guess what, it still is. Backed out again. Please verify that this is green on Try before burning the tree a third time. https://hg.mozilla.org/integration/mozilla-inbound/rev/185126a2a4e9 https://tbpl.mozilla.org/php/getParsedLog.php?id=47362920&tree=Mozilla-Inbound
Blocks: 1065367
It seems like the test_scopes.html test for the toplevel scope leads to some ordering issue that causes failures. I've done a new try push with that specific test disabled (not the entire test_scopes.html). If this goes green, I'll land these patches. It is quite possible that the ordering changes will be fixed by bug 1066822. In either case I'll file a followup to enable that test when landing this.
Flags: needinfo?(amarchesini)
Pushed to maple for now. Disabling that test did not work, seems like something more insidious. I'm going to work on bug 1066822 and see if that helps.
This should help fix the b2g build bustages that resulted from the patches in comment 76. https://hg.mozilla.org/projects/maple/rev/a040c47e5db9
Blocks: 1065216
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d5f35227e58e Besides the build failing on Windows, it looks fine.
Attachment #8473539 - Attachment is obsolete: true
Attachment #8478030 - Attachment is obsolete: true
Attached patch Patch 2 - Add postMessage to service worker. (obsolete) (deleted) — Splinter Review
Rebased and fixed windows build bustage.
Attachment #8473545 - Attachment is obsolete: true
Attachment #8473547 - Attachment is obsolete: true
Attachment #8478118 - Attachment is obsolete: true
Attached patch Patch 4(v2) - Extensive testing of postMessage. (obsolete) (deleted) — Splinter Review
Attachment #8473548 - Attachment is obsolete: true
Assignee: nsm.nikhil → catalin.badea392
Comment on attachment 8512141 [details] [diff] [review] Part 1.3: Fix crash caused by a race in PromiseHolder. I think we can try to land 1.1, 1.2 and 1.3 Here are the try runs: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=55a9b2deec3e https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b570fa5d25a2
Attachment #8512141 - Flags: review?(nsm.nikhil)
Comment on attachment 8512141 [details] [diff] [review] Part 1.3: Fix crash caused by a race in PromiseHolder. Review of attachment 8512141 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Please land all relevant patches on m-c, and this fix on maple. ::: dom/promise/Promise.cpp @@ +1375,5 @@ > { > MOZ_ASSERT(NS_IsMainThread()); > > MutexAutoLock lock(mCleanUpLock); > + This file doesn't need to change. ::: dom/workers/ServiceWorkerClients.cpp @@ +53,5 @@ > PromiseHolder(WorkerPrivate* aWorkerPrivate, > Promise* aPromise) > : mWorkerPrivate(aWorkerPrivate), > mPromise(aPromise), > + mCleanUpLock("cleanUpLock"), name this "PromiseHolder cleanUpLock" or similar. @@ +219,5 @@ > } > > + nsRefPtr<PromiseHolder> promiseHolder = new PromiseHolder(workerPrivate, > + promise); > + if (!promiseHolder->Get()) { Please rename Get() to GetPromise().
Attachment #8512141 - Flags: review?(nsm.nikhil) → review+
Attachment #8512141 - Attachment is obsolete: true
Attachment #8512252 - Attachment is obsolete: true
Please land patches 1.1, 1.2 and 1.3. The patches have all been previously r+-ed and contain only small changes or have been rebased.
Keywords: checkin-needed
Flags: needinfo?(catalin.badea392)
(In reply to Carsten Book [:Tomcat] from comment #95) > sorry had to back this out since i guess this caused > https://treeherder.mozilla.org/ui/logviewer. > html#?job_id=3350702&repo=mozilla-inbound Yes, this was causing the failures. I want to give it another go. I've updated patch 1.3 to remove a deprecated feature from the test which I think was the reason for the assert. Try results (I've queued 2 full runs because the assert happens intermittently): https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2674a2ae79a0 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ab05c20ba1d9 Please land 1.1, 1.2 and 1.3 again.
Flags: needinfo?(catalin.badea392)
Keywords: checkin-needed
Actually, lets wait on this. I believe there is still a race condition and its just less likely to occur.
Keywords: checkin-needed
Depends on: 1090994
Attachment #8513793 - Flags: review?(nsm.nikhil)
Comment on attachment 8513793 [details] [diff] [review] Patch 1.4 - Fix second crash scenario caused by a race in PromiseHolder. Review of attachment 8513793 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me. Andrea, could you take a look at this patch and at patch 1.3. ::: dom/workers/ServiceWorkerClients.cpp @@ +232,5 @@ > + // Use a control runnable to release the runnable on the worker thread. > + nsRefPtr<ReleasePromiseRunnable> releaseRunnable = > + new ReleasePromiseRunnable(mWorkerPrivate, mPromiseHolder); > + > + if (!releaseRunnable->Dispatch(nullptr)) { Use the same cx you used above. @@ +233,5 @@ > + nsRefPtr<ReleasePromiseRunnable> releaseRunnable = > + new ReleasePromiseRunnable(mWorkerPrivate, mPromiseHolder); > + > + if (!releaseRunnable->Dispatch(nullptr)) { > + NS_RUNTIMEABORT("Failed to dispatch control runnable."); "PromiseHolder control runnable" or "ReleasePromiseRunnable"
Attachment #8513793 - Flags: review?(nsm.nikhil) → review?(amarchesini)
Attachment #8513793 - Attachment is obsolete: true
Attachment #8513793 - Flags: review?(amarchesini)
Attachment #8514656 - Flags: review?(amarchesini)
Comment on attachment 8516119 [details] [diff] [review] Patch 1.5 - Update getServiced test and fix an assert when the registration has been removed. Review of attachment 8516119 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/test/serviceworkers/test_get_serviced.html @@ +23,3 @@ > function simpleRegister() { > + return navigator.serviceWorker.register("get_serviced_worker.js", > + { scope: "./sw_clients/" }).then((swr) => registration = swr); Nit: Could you move the .then() to a new line.
Attachment #8516119 - Flags: review?(nsm.nikhil) → review+
Comment on attachment 8514656 [details] [diff] [review] Patch 1.4 - Fix second crash scenario caused by a race in PromiseHolder. Review of attachment 8514656 [details] [diff] [review]: ----------------------------------------------------------------- Looks ok but I want to ask a quick feedback to bent when he is online. I'll write a comment in a few hours. ::: dom/workers/ServiceWorkerClients.cpp @@ +222,5 @@ > swm->GetServicedClients(mScope, result); > nsRefPtr<ResolvePromiseWorkerRunnable> r = > new ResolvePromiseWorkerRunnable(mWorkerPrivate, mPromiseHolder, result); > > AutoSafeJSContext cx; We should use AutoJSAPI instead. But it's not an issue for this bug.
Comment on attachment 8514656 [details] [diff] [review] Patch 1.4 - Fix second crash scenario caused by a race in PromiseHolder. Review of attachment 8514656 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerClients.cpp @@ +222,5 @@ > swm->GetServicedClients(mScope, result); > nsRefPtr<ResolvePromiseWorkerRunnable> r = > new ResolvePromiseWorkerRunnable(mWorkerPrivate, mPromiseHolder, result); > > AutoSafeJSContext cx; We should use AutoJSAPI instead. But it's not an issue for this bug.
Attachment #8514656 - Flags: review?(amarchesini) → review+
Please land patches 1.1 through 1.5.
Keywords: checkin-needed
Reopening since the postMessage bits aren't landed yet.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Catalin, any updates on this? Any patches we could start on review would be good.
Flags: needinfo?(catalin.badea392)
Attached patch Patch 2 - Add postMessage to service worker. (obsolete) (deleted) — Splinter Review
Attachment #8500165 - Attachment is obsolete: true
Attachment #8559533 - Flags: review?(nsm.nikhil)
Attached patch Patch 3 - Implement client.postMessage. (obsolete) (deleted) — Splinter Review
This includes the client.postMessage implementation from Patch 3 and Patch 4.
Attachment #8500166 - Attachment is obsolete: true
Attachment #8500167 - Attachment is obsolete: true
Attachment #8559535 - Flags: review?(nsm.nikhil)
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #112) > Catalin, any updates on this? Any patches we could start on review would be > good. Tryrun for the two patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=342cf7afd5cc I will follow up with a patch that stubs the new API and then patches for individual features. getServiced/getAll/get is still missing tests on m-i, I will add those along with the updated implementation. Cheers!
Flags: needinfo?(catalin.badea392)
Attachment #8559535 - Flags: review?(nsm.nikhil)
Attached patch Patch 3 - Implement client.postMessage. (obsolete) (deleted) — Splinter Review
Attachment #8559535 - Attachment is obsolete: true
Attachment #8560141 - Flags: review?(nsm.nikhil)
This patch is a bit cumbersome to read. It does the following: 1. s/getServiced/getAll, s/ServiceWorkerClient(s)/Client(s) 2. add stubbed options parameter to getAll and return error for anything other than the default options.
Try run for Patch 2 and Patch 3 with correct tests for patch 3: https://treeherder.mozilla.org/#/jobs?repo=try&revision=37cae710ea5d
Comment on attachment 8559533 [details] [diff] [review] Patch 2 - Add postMessage to service worker. Review of attachment 8559533 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorker.h @@ +62,5 @@ > } > > +#ifdef PostMessage > +#undef PostMessage > +#endif WorkerPrivate.cpp seems to do this only on WIN_XP. Could you do that here too?
Attachment #8559533 - Flags: review?(nsm.nikhil) → review+
Comment on attachment 8560141 [details] [diff] [review] Patch 3 - Implement client.postMessage. Review of attachment 8560141 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerClient.h @@ +11,5 @@ > #include "nsWrapperCache.h" > > namespace mozilla { > + > +class ErrorResult; include ErrorResult @@ +17,5 @@ > namespace dom { > > class Promise; > +template<typename T> class Optional; > +template<typename T> class Sequence; Just import BindingDeclarations.h
Attachment #8560141 - Flags: review?(nsm.nikhil) → review+
Attachment #8559533 - Attachment is obsolete: true
Attachment #8560141 - Attachment is obsolete: true
Comment on attachment 8561588 [details] [diff] [review] Patch 2 - Add postMessage to service worker. The webidl change was reviewed by :amarchesini in a prevous patch.
Attachment #8561588 - Flags: checkin?
Attachment #8561589 - Flags: checkin?
Comment on attachment 8559533 [details] [diff] [review] Patch 2 - Add postMessage to service worker. Review of attachment 8559533 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorker.cpp @@ +77,5 @@ > +{ > + WorkerPrivate* workerPrivate = GetWorkerPrivate(); > + MOZ_ASSERT(workerPrivate); > + > + workerPrivate->PostMessage(aCx, aMessage, aTransferable, aRv); The spec says this one is should throw InvalidAccessError if state is redundant. Please attach a follow up patch.
Attachment #8561778 - Flags: review?(nsm.nikhil)
Attachment #8561778 - Attachment is obsolete: true
Attachment #8561778 - Flags: review?(nsm.nikhil)
Attachment #8561780 - Flags: review?(nsm.nikhil)
Target Milestone: mozilla36 → ---
Attachment #8560140 - Attachment is obsolete: true
Attachment #8512138 - Flags: checkin+
Attachment #8512139 - Flags: checkin+
Attachment #8512818 - Flags: checkin+
Attachment #8514656 - Flags: checkin+
Attachment #8516934 - Flags: checkin+
Attachment #8561588 - Flags: checkin? → checkin+
Attachment #8561589 - Flags: checkin? → checkin+
Comment on attachment 8561588 [details] [diff] [review] Patch 2 - Add postMessage to service worker. Review of attachment 8561588 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/ServiceWorker.webidl @@ +18,5 @@ > readonly attribute ServiceWorkerState state; > > attribute EventHandler onstatechange; > + > + // FIXME(catalinb): Should inherit this from Worker. Do we have a bug for that? Write the bug id here.
Attachment #8561588 - Flags: review+
Attachment #8561780 - Flags: review?(nsm.nikhil) → review+
(In reply to Andrea Marchesini (:baku) from comment #131) > Comment on attachment 8561588 [details] [diff] [review] > Patch 2 - Add postMessage to service worker. > > Review of attachment 8561588 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/webidl/ServiceWorker.webidl > @@ +18,5 @@ > > readonly attribute ServiceWorkerState state; > > > > attribute EventHandler onstatechange; > > + > > + // FIXME(catalinb): Should inherit this from Worker. > > Do we have a bug for that? Write the bug id here. Bug 1131347
Comment on attachment 8561780 [details] [diff] [review] Patch 3.1 Throw InvalidStateError when ServiceWorkerState == Redundant. https://hg.mozilla.org/integration/mozilla-inbound/rev/408f0108ff53
Attachment #8561780 - Flags: checkin+
Attachment #8576014 - Flags: review?(nsm.nikhil)
Keywords: leave-open
Attachment #8576014 - Flags: review?(nsm.nikhil) → review+
Attachment #8576014 - Flags: checkin+
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: