Move the actual Worker object (not the worker global) to use nsDOMEventTargetHelper and WebIDL.

Worker is complex because the underlying C++ object is held alive from two threads (although all reference counting happens on the parent thread). We may want to talk about this in Toronto this weekend.

Comment on attachment 813004 [details] [diff] [review] Patch ::: dom/bindings/Bindings.conf @@ -26,16 +26,21 @@ >+# * wantsQI - Indicates whether the interface should have a QueryInterface >+# method available to chrome. wantsQI was removed by bug 920196. ::: dom/bindings/Configuration.py @@ -351,22 +352,27 @@ class Descriptor(DescriptorProvider): > if desc.get('wantsQI', None) != None: > self._wantsQI = desc.get('wantsQI', None) Looks like this is (perhaps accidentally) added back in bug 919457.

Comment on attachment 813004 [details] [diff] [review] Patch Review of attachment 813004 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/RuntimeService.cpp @@ +996,4 @@ > return true; > } > > + WorkerBinding::GetConstructorObject(aCx, aObj); Can these fail? ::: dom/workers/WorkerPrivate.cpp @@ +2642,5 @@ > +bool > +ChromeWorkerPrivate::WorkerAvailable(JSContext* /* unused */, JSObject* /* unused */) > +{ > + // Chrome is always allowed to use workers, and content is never allowed to > + // use ChromeWorker, so all we have to check Trailing ws @@ +4545,5 @@ > > BEGIN_WORKERS_NAMESPACE > > +WorkerCrossThreadDispatcher* > +GetWorkerCrossThreadDispatcher(JSContext* aCx, jsval aWorker) const JS::Value& @@ +4547,5 @@ > > +WorkerCrossThreadDispatcher* > +GetWorkerCrossThreadDispatcher(JSContext* aCx, jsval aWorker) > +{ > + if (JSVAL_IS_PRIMITIVE(aWorker)) { !aWorker.isObject(), please ::: dom/workers/WorkerPrivate.h @@ +979,5 @@ > }; > > +// This class is only used to trick the DOM bindings. We never create > +// instances of it, and static_casting to it is fine since it doesn't add > +// anything to WorkerPrivate. :/ ::: dom/workers/WorkerScope.cpp @@ +1119,5 @@ > } > > // Init other paris-bindings. > + if (!(!worker->IsChromeWorker() || > + ChromeWorkerBinding::GetConstructorObject(aCx, global)) || Doesn't exactly end up readable :) @@ +1154,5 @@ > > +bool > +GetterOnlyJSNative(JSContext* aCx, unsigned aArgc, JS::Value* aVp) > +{ > + JS_ReportErrorNumber(aCx, js_GetErrorMessage, nullptr, JSMSG_GETTER_ONLY); 2 spaces

> Can these fail? In theory, yes, mostly due to OOM...

Comment on attachment 813004 [details] [diff] [review] Patch >+++ b/dom/bindings/Codegen.py > def addHeaderForFunc(func): >+ if func is None: When would it be None? >+ if not desc.headerIsDefault: >+ # An explicit header file was provided, assume that we know >+ # what we're doing. This requires all functions for the interface to be in that header. Ben had some thoughts about finding the fully qualified classname for the func here, finding the descriptor with that nativeType (if any), and if so using that descriptor's headerFile. Will that address the issue you're trying to address here? Followup bug on doing that is ok, in the interests of getting this landed... >+++ b/dom/bindings/test/TestBindingHeader.h >+#include "nsGenericHTMLElement.h" Huh. Why is that needed? >+++ b/dom/webidl/Worker.webidl >+ void postMessage(any message, optional any transfer); The second arg should really become sequence<any> at the very least, or better yet a sequence of something like Transferable... Followup is OK, I guess, but please make it happen? >+++ b/dom/workers/RuntimeService.cpp >+ WorkerBinding::GetConstructorObject(aCx, aObj); >+ ChromeWorkerBinding::GetConstructorObject(aCx, aObj); >+ ErrorEventBinding::GetConstructorObject(aCx, aObj); >+ MessageEventBinding::GetConstructorObject(aCx, aObj); At least for the moment those can fail. If they do, need to return false here. >+++ b/dom/workers/WorkerPrivate.cpp >+ void >+ PostDispatch(JSContext* aCx, WorkerPrivate* aWorkerPrivate, >+ bool aDispatchResult) virtual, and MOZ_OVERRIDE, right? >+ ErrorEventInitInitializer init; Drop the "Initializer" bit; we killed it off. > WorkerPrivateParent<Derived>::PostMessage(JSContext* aCx, >+ aTransferable.WasPassed() ? aTransferable.Value() >+ : JS::NullValue(), Why not just give the argument "null" as a default value in the IDL? >+WorkerPrivate::Constructor(GlobalObject& aGlobal, const nsAString& aScriptURL, >+ nsString scriptURL(aScriptURL); Why do you need this extra string object? Can you not just use aScriptURL? >+GetWorkerCrossThreadDispatcher(JSContext* aCx, jsval aWorker) >+ if (JSVAL_IS_PRIMITIVE(aWorker)) { if (!aWorker.isObject()) { Also, should aWorker be a Handle<Value>? I'm assuming the PostDispatch bit above is reasonable. r=me with the nits fixed and followups filed.

Rebased through the SharedWorker landing. I've made no effort to ensure this builds or to address review comments yet.

After this we compile and pass tests.

Patch with bz's comments addressed

(In reply to Boris Zbarsky [:bz] (Vacation Oct 12 - Oct 27) from comment #5) > Comment on attachment 813004 [details] [diff] [review] > Patch > > >+++ b/dom/bindings/Codegen.py > > def addHeaderForFunc(func): > >+ if func is None: > > When would it be None? I don't know, but that's what the existing code does ... > >+ if not desc.headerIsDefault: > >+ # An explicit header file was provided, assume that we know > >+ # what we're doing. > > This requires all functions for the interface to be in that header. > > Ben had some thoughts about finding the fully qualified classname for the > func here, finding the descriptor with that nativeType (if any), and if so > using that descriptor's headerFile. Will that address the issue you're > trying to address here? > > Followup bug on doing that is ok, in the interests of getting this landed... I think so. I'll file a followup. > >+++ b/dom/bindings/test/TestBindingHeader.h > >+#include "nsGenericHTMLElement.h" > > Huh. Why is that needed? Because I removed the "automatically include stuff" bit above. > >+++ b/dom/webidl/Worker.webidl > >+ void postMessage(any message, optional any transfer); > > The second arg should really become sequence<any> at the very least, or > better yet a sequence of something like Transferable... > > Followup is OK, I guess, but please make it happen? Fixed. > >+++ b/dom/workers/RuntimeService.cpp > >+ WorkerBinding::GetConstructorObject(aCx, aObj); > >+ ChromeWorkerBinding::GetConstructorObject(aCx, aObj); > >+ ErrorEventBinding::GetConstructorObject(aCx, aObj); > >+ MessageEventBinding::GetConstructorObject(aCx, aObj); > > At least for the moment those can fail. If they do, need to return false > here. Fixed. > >+++ b/dom/workers/WorkerPrivate.cpp > >+ void > >+ PostDispatch(JSContext* aCx, WorkerPrivate* aWorkerPrivate, > >+ bool aDispatchResult) > > virtual, and MOZ_OVERRIDE, right? Yeah, but this file doesn't use that anywhere. I can file a followup on adding it across the board if you like. > >+ ErrorEventInitInitializer init; > > Drop the "Initializer" bit; we killed it off. Fixed. > > WorkerPrivateParent<Derived>::PostMessage(JSContext* aCx, > >+ aTransferable.WasPassed() ? aTransferable.Value() > >+ : JS::NullValue(), > > Why not just give the argument "null" as a default value in the IDL? This went away with the sequence change. > >+WorkerPrivate::Constructor(GlobalObject& aGlobal, const nsAString& aScriptURL, > >+ nsString scriptURL(aScriptURL); > > Why do you need this extra string object? Can you not just use aScriptURL? I don't. Removed. > >+GetWorkerCrossThreadDispatcher(JSContext* aCx, jsval aWorker) > >+ if (JSVAL_IS_PRIMITIVE(aWorker)) { > > if (!aWorker.isObject()) { Fixed. > Also, should aWorker be a Handle<Value>? I don't think it needs to be, although it probably wouldn't hurt. > I'm assuming the PostDispatch bit above is reasonable. r=me with the nits > fixed and followups filed.

> I don't know, but that's what the existing code does ... Oh, right. I thought you were after that check for some reason. It's None if not set. > Because I removed the "automatically include stuff" bit above. OK, please make sure to remove that in the followup? > I can file a followup on adding it across the board if you like. Please. > I don't think it needs to be, although it probably wouldn't hurt. If it's not hard, it would be vastly preferable.

Comment on attachment 815472 [details] [diff] [review] Rebased patch Review of attachment 815472 [details] [diff] [review]: ----------------------------------------------------------------- This looks pretty close but there was enough stuff that I'd like to see the next version. You should be able to remove Worker.h/Worker.cpp right? And some code in MessageEvent? ::: dom/base/nsDOMClassInfo.cpp @@ -194,4 @@ > > using namespace mozilla; > using namespace mozilla::dom; > -using mozilla::dom::workers::ResolveWorkerClasses; There should be an #include you can lose here somewhere, right? ::: dom/bindings/Codegen.py @@ +701,3 @@ > if desc.interface.isExternal(): > continue > def addHeaderForFunc(func): Yeah, maybe I forgot to file a followup on this, I can't find it now (though I could have sworn that I did...) but can you use the header from the descriptor if the native type matches? ::: dom/workers/RuntimeService.cpp @@ +967,4 @@ > > BEGIN_WORKERS_NAMESPACE > > +// Entry point for XPConnect globals. Nit: It's really for main thread non-window globals... It's not clear what "XPConnect" really means here. @@ +1004,5 @@ > } > > + WorkerBinding::GetConstructorObject(aCx, aObj); > + ChromeWorkerBinding::GetConstructorObject(aCx, aObj); > + ErrorEventBinding::GetConstructorObject(aCx, aObj); I can't remember, will this handle the 'Event' binding automatically? @@ +1007,5 @@ > + ChromeWorkerBinding::GetConstructorObject(aCx, aObj); > + ErrorEventBinding::GetConstructorObject(aCx, aObj); > + MessageEventBinding::GetConstructorObject(aCx, aObj); > + > + if (!events::InitClasses(aCx, aObj, true)) { We need a followup to convert these. @@ +1010,5 @@ > + > + if (!events::InitClasses(aCx, aObj, true)) { > + return false; > + } > + Nit: extra whitespace ::: dom/workers/WorkerPrivate.cpp @@ +81,4 @@ > // GC will run five seconds after the last event is processed. > #define IDLE_GC_TIMER_DELAY_MS 5000 > > +#define PREF_WORKERS_PREFIX "dom.workers." Nit: You only use this once, just add to PREF_WORKERS_ENABLED @@ +921,5 @@ > + // cloning into worker when array goes out of scope. > + nsTArray<nsCOMPtr<nsISupports> > clonedObjects; > + clonedObjects.SwapElements(mClonedObjects); > + > + JS::Rooted<JS::Value> messageData(aCx); Nit: JS::RootedValue here, and JS::RootedFoo everywhere on anything that you're touching. (Also need JS::HandleFoo in a few places). There are a bunch of these so please search/replace. @@ +923,5 @@ > + clonedObjects.SwapElements(mClonedObjects); > + > + JS::Rooted<JS::Value> messageData(aCx); > + if (!mBuffer.read(aCx, messageData.address(), > + workers::WorkerStructuredCloneCallbacks(mainRuntime))) { I really hope the workers:: is unnecessary here? @@ +929,5 @@ > } > > + nsCOMPtr<nsIDOMEvent> domEvent; > + nsresult rv = > + nsEventDispatcher::CreateEvent(aWorkerPrivate, nullptr, nullptr, Blegh, just make a new nsDOMMessageEvent like DispatchMessageEventToMessagePort() does. No need for this extra indirection. @@ +932,5 @@ > + nsresult rv = > + nsEventDispatcher::CreateEvent(aWorkerPrivate, nullptr, nullptr, > + NS_LITERAL_STRING("messageevent"), > + getter_AddRefs(domEvent)); > + NS_ENSURE_SUCCESS(rv, false); This (and your other NS_ENSURE_SUCCESS below) will trigger an uncatchable exception. You need to |Throw(aCx, rv)| before returning false from this method. @@ +944,5 @@ > + EmptyString(), > + nullptr); > + NS_ENSURE_SUCCESS(rv, false); > + > + message->SetTrusted(aWorkerPrivate->IsChromeWorker()); This should always be true. Trusted events just mean "generated by the browser", not by page script. @@ +951,5 @@ > + aWorkerPrivate->DispatchDOMEvent(nullptr, message, nullptr, &dummy); > + return true; > + } > + > + NS_ASSERTION(aWorkerPrivate == GetWorkerPrivateFromContext(aCx), Nit: Since you're touching this you might as well make this MOZ_ASSERT @@ +955,5 @@ > + NS_ASSERTION(aWorkerPrivate == GetWorkerPrivateFromContext(aCx), > + "Badness!"); > + if (mToMessagePort) { > + WorkerMessagePort* port = > + aWorkerPrivate->GetMessagePort(mMessagePortSerial); Crap, I know this isn't your fault, but can you make that a nsRefPtr? It's bad to call out without ensuring a ref. @@ +1148,4 @@ > WorkerRunnable::PostRun(aCx, aWorkerPrivate, aRunResult); > } > > + // NB: aWorkerPrivate is the worker thread we're on (or the main thread, if Nit: 'NB' or 'Note' or 'Important!' and other such stuff in a comment is silly. You're already calling it out and if it wasn't important I would tell you not to add it ;) @@ +1191,3 @@ > > + nsRefPtr<ErrorEvent> event = > + ErrorEvent::Constructor(aTarget, NS_LITERAL_STRING("error"), init); This could fail, right? @@ +2082,4 @@ > MOZ_ASSERT_IF(!aIsSharedWorker, aObject && aSharedWorkerName.IsEmpty()); > > if (aLoadInfo.mWindow) { > + MOZ_ASSERT(NS_IsMainThread()); Nit: AssertIsOnMainThread() @@ +2126,5 @@ > + > +template <class Derived> > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(WorkerPrivateParent<Derived>, > + nsDOMEventTargetHelper) > + // Nothing else to traverse Mention that LoadInfo stuff here too? @@ +2466,5 @@ > + ClearWrapper(); > + > + // Ensure that we're held alive across the TerminatePrivate call, and then > + // release the reference our wrapper held to us. > + nsRefPtr<WorkerPrivateParent<Derived> > kungFuDeathGrip = dont_AddRef(this); Nit: >>, here and in a few other places @@ +2545,2 @@ > } > + NS_RELEASE_THIS(); Wait, you can't call this and then touch mRooted, can you? Are you guaranteed to never be releasing the last ref here? I think you need something a little trickier here. @@ +2614,4 @@ > } > } > > + JS::Rooted<JS::Value> transferable(aCx, JS::UndefinedValue()); Nit: Hm, undefined should be the default right? @@ +2618,5 @@ > + if (aTransferable.WasPassed()) { > + const Sequence<JS::Value>& realTransferable = aTransferable.Value(); > + JSObject* array = > + JS_NewArrayObject(aCx, realTransferable.Length(), > + const_cast<jsval*>(realTransferable.Elements())); JS::RootedObject for the array, also (nit) s/jsval/JS::Value/ @@ +2631,5 @@ > > JSAutoStructuredCloneBuffer buffer; > + if (!buffer.write(aCx, aMessage, transferable.WasPassed(), callbacks, > + &clonedObjects)) { > + aRv.Throw(NS_ERROR_DOM_DATA_CLONE_ERR); Hrm. Does our code deal with this properly? If there's a pending exception set on aCx after write() returns then this Throw() call won't overwrite it, will it? @@ +3287,3 @@ > { > + return WorkerPrivate::Constructor(aGlobal, aScriptURL, false, false, > + EmptyString(), aRv); Can this not be inlined? @@ +3294,5 @@ > +WorkerPrivate::WorkerAvailable(JSContext* /* unused */, JSObject* /* unused */) > +{ > + // If we're already on a worker workers are clearly enabled. > + if (!NS_IsMainThread()) { > + return true; You could assert GetWorkerPrivateFromContext() here. @@ +3298,5 @@ > + return true; > + } > + > + // If our caller is chrome, workers are always available. > + if (nsContentUtils::IsCallerChrome()) { Actually, this might be wrong, chrome calling a content window's Worker constructor should fail if the pref isn't set, shouldn't it? @@ +3312,5 @@ > +ChromeWorkerPrivate::Constructor(GlobalObject& aGlobal, const nsAString& aScriptURL, > + mozilla::ErrorResult& aRv) > +{ > + return WorkerPrivate::Constructor(aGlobal, aScriptURL, true, false, > + EmptyString(), aRv).downcast<ChromeWorkerPrivate>(); This seems inline-able too @@ +3320,5 @@ > +bool > +ChromeWorkerPrivate::WorkerAvailable(JSContext* /* unused */, JSObject* /* unused */) > +{ > + // Chrome is always allowed to use workers, and content is never allowed to > + // use ChromeWorker, so all we have to check Nit: incomplete comment @@ +3321,5 @@ > +ChromeWorkerPrivate::WorkerAvailable(JSContext* /* unused */, JSObject* /* unused */) > +{ > + // Chrome is always allowed to use workers, and content is never allowed to > + // use ChromeWorker, so all we have to check > + return nsContentUtils::ThreadsafeIsCallerChrome(); Same concern about chrome code calling this on a content window. @@ +3332,5 @@ > + const nsAString& aSharedWorkerName, > + LoadInfo* aLoadInfo, mozilla::ErrorResult& aRv) > +{ > + WorkerPrivate* parent = NS_IsMainThread() ? nullptr > + : GetCurrentThreadWorkerPrivate(); Nit: pattern usually is all on one line or: foo = cond ? bar : baz; @@ +3367,5 @@ > + if (!parent) { > + runtimeService = RuntimeService::GetOrCreateService(); > + if (!runtimeService) { > + JS_ReportError(cx, "Failed to create runtime service!"); > + aRv.Throw(NS_ERROR_FAILURE); Same concern above, which error will JS actually see? @@ +3372,5 @@ > + return nullptr; > + } > + } > + else { > + runtimeService = RuntimeService::GetService(); You should MOZ_ASSERT |runtimeService| after this. @@ +3390,5 @@ > + aRv.Throw(NS_ERROR_UNEXPECTED); > + return nullptr; > + } > + > + // Worker now also owned by its thread. Nit: 'also' doesn't make sense here like it did before. This should be more verbose maybe. @@ +5312,5 @@ > > BEGIN_WORKERS_NAMESPACE > > +WorkerCrossThreadDispatcher* > +GetWorkerCrossThreadDispatcher(JSContext* aCx, jsval aWorker) Something is funny, I don't see where in this patch you're moving this from. Splinter bug? In any case, JS::Value @@ +5314,5 @@ > > +WorkerCrossThreadDispatcher* > +GetWorkerCrossThreadDispatcher(JSContext* aCx, jsval aWorker) > +{ > + if (JSVAL_IS_PRIMITIVE(aWorker)) { Nit: aWorker.isPrimitive() @@ +5321,5 @@ > + > + WorkerPrivate* w = nullptr; > + UNWRAP_OBJECT(Worker, aCx, &aWorker.toObject(), w); > + if (!w) { > + return nullptr; This should probably be asserted, only C++ can call this. ::: dom/workers/WorkerPrivate.h @@ +365,4 @@ > > public: > + > + virtual JSObject* WrapObject(JSContext* aCx, Nit: return type on its own line @@ +365,5 @@ > > public: > + > + virtual JSObject* WrapObject(JSContext* aCx, > + JS::Handle<JSObject*> aScope) MOZ_OVERRIDE; JS::HandleObject @@ +438,2 @@ > PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage, > + const Optional<Sequence<JS::Value> >& aTransferable, Nit: >> is fine now, in all the places you're touching. @@ +797,1 @@ > ~WorkerPrivate(); Hm, can this be protected now? @@ +1027,5 @@ > GetMessagePort(uint64_t aMessagePortSerial); > > +protected: > + > + static already_AddRefed<WorkerPrivate> Nit: No need for the newline above. @@ +1118,5 @@ > > +// This class is only used to trick the DOM bindings. We never create > +// instances of it, and static_casting to it is fine since it doesn't add > +// anything to WorkerPrivate. > +class ChromeWorkerPrivate : public WorkerPrivate { Nit: { on its own line @@ +1128,5 @@ > + static bool > + WorkerAvailable(JSContext* /* unused */, JSObject* /* unused */); > + > +private: > + ChromeWorkerPrivate() MOZ_DELETE; Add (er, delete) the copy constructor too? ::: dom/workers/WorkerScope.cpp @@ +1465,5 @@ > } > > // Init other paris-bindings. > + if (!(!worker->IsChromeWorker() || > + ChromeWorkerBinding::GetConstructorObject(aCx, global)) || Eww! Move this above to the block with the other chrome worker stuff. @@ +1491,5 @@ > return global; > } > > +bool > +GetterOnlyJSNative(JSContext* aCx, unsigned aArgc, JS::Value* aVp) I don't understand why this is here... Another Splinter bug?

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #11) > ::: dom/workers/WorkerPrivate.cpp > @@ +921,5 @@ > > + // cloning into worker when array goes out of scope. > > + nsTArray<nsCOMPtr<nsISupports> > clonedObjects; > > + clonedObjects.SwapElements(mClonedObjects); > > + > > + JS::Rooted<JS::Value> messageData(aCx); > > Nit: JS::RootedValue here, and JS::RootedFoo everywhere on anything that > you're touching. (Also need JS::HandleFoo in a few places). There are a > bunch of these so please search/replace. Not in dom/.

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #11) > Comment on attachment 815472 [details] [diff] [review] > Rebased patch > > Review of attachment 815472 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks pretty close but there was enough stuff that I'd like to see the > next version. > > You should be able to remove Worker.h/Worker.cpp right? And some code in > MessageEvent? Yes, git rm Worker.h/cpp was just missing from this. idk about MessageEvent, I would prefer to just convert the global (which I have another patch for) and kill the worker events entirely. > ::: dom/base/nsDOMClassInfo.cpp > @@ -194,4 @@ > > > > using namespace mozilla; > > using namespace mozilla::dom; > > -using mozilla::dom::workers::ResolveWorkerClasses; > > There should be an #include you can lose here somewhere, right? Yeah. > ::: dom/bindings/Codegen.py > @@ +701,3 @@ > > if desc.interface.isExternal(): > > continue > > def addHeaderForFunc(func): > > Yeah, maybe I forgot to file a followup on this, I can't find it now (though > I could have sworn that I did...) but can you use the header from the > descriptor if the native type matches? bz talked about it. We can do it in a followup. > ::: dom/workers/RuntimeService.cpp > @@ +967,4 @@ > > > > BEGIN_WORKERS_NAMESPACE > > > > +// Entry point for XPConnect globals. > > Nit: It's really for main thread non-window globals... It's not clear what > "XPConnect" really means here. Ok. > @@ +1004,5 @@ > > } > > > > + WorkerBinding::GetConstructorObject(aCx, aObj); > > + ChromeWorkerBinding::GetConstructorObject(aCx, aObj); > > + ErrorEventBinding::GetConstructorObject(aCx, aObj); > > I can't remember, will this handle the 'Event' binding automatically? Yes. > @@ +1007,5 @@ > > + ChromeWorkerBinding::GetConstructorObject(aCx, aObj); > > + ErrorEventBinding::GetConstructorObject(aCx, aObj); > > + MessageEventBinding::GetConstructorObject(aCx, aObj); > > + > > + if (!events::InitClasses(aCx, aObj, true)) { > > We need a followup to convert these. events? We'll just get rid of them entirely soon. > @@ +1010,5 @@ > > + > > + if (!events::InitClasses(aCx, aObj, true)) { > > + return false; > > + } > > + > > Nit: extra whitespace > > ::: dom/workers/WorkerPrivate.cpp > @@ +81,4 @@ > > // GC will run five seconds after the last event is processed. > > #define IDLE_GC_TIMER_DELAY_MS 5000 > > > > +#define PREF_WORKERS_PREFIX "dom.workers." > > Nit: You only use this once, just add to PREF_WORKERS_ENABLED Done. > @@ +921,5 @@ > > + // cloning into worker when array goes out of scope. > > + nsTArray<nsCOMPtr<nsISupports> > clonedObjects; > > + clonedObjects.SwapElements(mClonedObjects); > > + > > + JS::Rooted<JS::Value> messageData(aCx); > > Nit: JS::RootedValue here, and JS::RootedFoo everywhere on anything that > you're touching. (Also need JS::HandleFoo in a few places). There are a > bunch of these so please search/replace. dom/ doesn't do this. Please file another bug on doing a global search/replace. > @@ +923,5 @@ > > + clonedObjects.SwapElements(mClonedObjects); > > + > > + JS::Rooted<JS::Value> messageData(aCx); > > + if (!mBuffer.read(aCx, messageData.address(), > > + workers::WorkerStructuredCloneCallbacks(mainRuntime))) { > > I really hope the workers:: is unnecessary here? It's needed to disambiguate http://hg.mozilla.org/mozilla-central/annotate/672cd63528d3/dom/workers/WorkerPrivate.cpp#l220 and http://hg.mozilla.org/mozilla-central/annotate/672cd63528d3/dom/workers/WorkerPrivate.h#l1121. > @@ +929,5 @@ > > } > > > > + nsCOMPtr<nsIDOMEvent> domEvent; > > + nsresult rv = > > + nsEventDispatcher::CreateEvent(aWorkerPrivate, nullptr, nullptr, > > Blegh, just make a new nsDOMMessageEvent like > DispatchMessageEventToMessagePort() does. No need for this extra indirection. Ok. > @@ +932,5 @@ > > + nsresult rv = > > + nsEventDispatcher::CreateEvent(aWorkerPrivate, nullptr, nullptr, > > + NS_LITERAL_STRING("messageevent"), > > + getter_AddRefs(domEvent)); > > + NS_ENSURE_SUCCESS(rv, false); > > This (and your other NS_ENSURE_SUCCESS below) will trigger an uncatchable > exception. You need to |Throw(aCx, rv)| before returning false from this > method. Ok. > @@ +944,5 @@ > > + EmptyString(), > > + nullptr); > > + NS_ENSURE_SUCCESS(rv, false); > > + > > + message->SetTrusted(aWorkerPrivate->IsChromeWorker()); > > This should always be true. Trusted events just mean "generated by the > browser", not by page script. Uh, yeah, idk wtf I was doing there. > @@ +951,5 @@ > > + aWorkerPrivate->DispatchDOMEvent(nullptr, message, nullptr, &dummy); > > + return true; > > + } > > + > > + NS_ASSERTION(aWorkerPrivate == GetWorkerPrivateFromContext(aCx), > > Nit: Since you're touching this you might as well make this MOZ_ASSERT Ok. > @@ +955,5 @@ > > + NS_ASSERTION(aWorkerPrivate == GetWorkerPrivateFromContext(aCx), > > + "Badness!"); > > + if (mToMessagePort) { > > + WorkerMessagePort* port = > > + aWorkerPrivate->GetMessagePort(mMessagePortSerial); > > Crap, I know this isn't your fault, but can you make that a nsRefPtr? It's > bad to call out without ensuring a ref. Fixed. > @@ +1148,4 @@ > > WorkerRunnable::PostRun(aCx, aWorkerPrivate, aRunResult); > > } > > > > + // NB: aWorkerPrivate is the worker thread we're on (or the main thread, if > > Nit: 'NB' or 'Note' or 'Important!' and other such stuff in a comment is > silly. You're already calling it out and if it wasn't important I would tell > you not to add it ;) Ok. > @@ +1191,3 @@ > > > > + nsRefPtr<ErrorEvent> event = > > + ErrorEvent::Constructor(aTarget, NS_LITERAL_STRING("error"), init); > > This could fail, right? No. Generated event ctors are infallible. > @@ +2082,4 @@ > > MOZ_ASSERT_IF(!aIsSharedWorker, aObject && aSharedWorkerName.IsEmpty()); > > > > if (aLoadInfo.mWindow) { > > + MOZ_ASSERT(NS_IsMainThread()); > > Nit: AssertIsOnMainThread() Done. > @@ +2126,5 @@ > > + > > +template <class Derived> > > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(WorkerPrivateParent<Derived>, > > + nsDOMEventTargetHelper) > > + // Nothing else to traverse > > Mention that LoadInfo stuff here too? Ok. > @@ +2466,5 @@ > > + ClearWrapper(); > > + > > + // Ensure that we're held alive across the TerminatePrivate call, and then > > + // release the reference our wrapper held to us. > > + nsRefPtr<WorkerPrivateParent<Derived> > kungFuDeathGrip = dont_AddRef(this); > > Nit: >>, here and in a few other places That compiles now? > @@ +2545,2 @@ > > } > > + NS_RELEASE_THIS(); > > Wait, you can't call this and then touch mRooted, can you? Are you > guaranteed to never be releasing the last ref here? I think you need > something a little trickier here. The caller should be holding a reference (if not literally, then ensuring that something else is for the duration of the call). Truth be told I'm not entirely convinced the addref/release are necessary here. > @@ +2614,4 @@ > > } > > } > > > > + JS::Rooted<JS::Value> transferable(aCx, JS::UndefinedValue()); > > Nit: Hm, undefined should be the default right? Maybe. I copied this from the existing code. > @@ +2618,5 @@ > > + if (aTransferable.WasPassed()) { > > + const Sequence<JS::Value>& realTransferable = aTransferable.Value(); > > + JSObject* array = > > + JS_NewArrayObject(aCx, realTransferable.Length(), > > + const_cast<jsval*>(realTransferable.Elements())); > > JS::RootedObject for the array, also (nit) s/jsval/JS::Value/ This is code you wrote last week you're complaining about ... > @@ +2631,5 @@ > > > > JSAutoStructuredCloneBuffer buffer; > > + if (!buffer.write(aCx, aMessage, transferable.WasPassed(), callbacks, > > + &clonedObjects)) { > > + aRv.Throw(NS_ERROR_DOM_DATA_CLONE_ERR); > > Hrm. Does our code deal with this properly? If there's a pending exception > set on aCx after write() returns then this Throw() call won't overwrite it, > will it? Yes. http://hg.mozilla.org/mozilla-central/annotate/672cd63528d3/dom/bindings/Exceptions.cpp#l106 > @@ +3287,3 @@ > > { > > + return WorkerPrivate::Constructor(aGlobal, aScriptURL, false, false, > > + EmptyString(), aRv); > > Can this not be inlined? *shrug* > @@ +3294,5 @@ > > +WorkerPrivate::WorkerAvailable(JSContext* /* unused */, JSObject* /* unused */) > > +{ > > + // If we're already on a worker workers are clearly enabled. > > + if (!NS_IsMainThread()) { > > + return true; > > You could assert GetWorkerPrivateFromContext() here. assert that we're on a worker that is? > @@ +3298,5 @@ > > + return true; > > + } > > + > > + // If our caller is chrome, workers are always available. > > + if (nsContentUtils::IsCallerChrome()) { > > Actually, this might be wrong, chrome calling a content window's Worker > constructor should fail if the pref isn't set, shouldn't it? This is what the existing code in ResolveWorkerClasses does. If you want it to do something different please file a followup. > @@ +3312,5 @@ > > +ChromeWorkerPrivate::Constructor(GlobalObject& aGlobal, const nsAString& aScriptURL, > > + mozilla::ErrorResult& aRv) > > +{ > > + return WorkerPrivate::Constructor(aGlobal, aScriptURL, true, false, > > + EmptyString(), aRv).downcast<ChromeWorkerPrivate>(); > > This seems inline-able too Do we really care? > @@ +3320,5 @@ > > +bool > > +ChromeWorkerPrivate::WorkerAvailable(JSContext* /* unused */, JSObject* /* unused */) > > +{ > > + // Chrome is always allowed to use workers, and content is never allowed to > > + // use ChromeWorker, so all we have to check > > Nit: incomplete comment Fixed. > @@ +3321,5 @@ > > +ChromeWorkerPrivate::WorkerAvailable(JSContext* /* unused */, JSObject* /* unused */) > > +{ > > + // Chrome is always allowed to use workers, and content is never allowed to > > + // use ChromeWorker, so all we have to check > > + return nsContentUtils::ThreadsafeIsCallerChrome(); > > Same concern about chrome code calling this on a content window. Same response ;-) > @@ +3332,5 @@ > > + const nsAString& aSharedWorkerName, > > + LoadInfo* aLoadInfo, mozilla::ErrorResult& aRv) > > +{ > > + WorkerPrivate* parent = NS_IsMainThread() ? nullptr > > + : GetCurrentThreadWorkerPrivate(); > > Nit: pattern usually is all on one line or: > > foo = cond ? > bar : > baz; Ok. > @@ +3367,5 @@ > > + if (!parent) { > > + runtimeService = RuntimeService::GetOrCreateService(); > > + if (!runtimeService) { > > + JS_ReportError(cx, "Failed to create runtime service!"); > > + aRv.Throw(NS_ERROR_FAILURE); > > Same concern above, which error will JS actually see? > > @@ +3372,5 @@ > > + return nullptr; > > + } > > + } > > + else { > > + runtimeService = RuntimeService::GetService(); > > You should MOZ_ASSERT |runtimeService| after this. Done. > @@ +3390,5 @@ > > + aRv.Throw(NS_ERROR_UNEXPECTED); > > + return nullptr; > > + } > > + > > + // Worker now also owned by its thread. > > Nit: 'also' doesn't make sense here like it did before. This should be more > verbose maybe. Ok. > @@ +5312,5 @@ > > > > BEGIN_WORKERS_NAMESPACE > > > > +WorkerCrossThreadDispatcher* > > +GetWorkerCrossThreadDispatcher(JSContext* aCx, jsval aWorker) > > Something is funny, I don't see where in this patch you're moving this from. > Splinter bug? This comes from Worker.cpp. > In any case, JS::Value ok. > @@ +5314,5 @@ > > > > +WorkerCrossThreadDispatcher* > > +GetWorkerCrossThreadDispatcher(JSContext* aCx, jsval aWorker) > > +{ > > + if (JSVAL_IS_PRIMITIVE(aWorker)) { > > Nit: aWorker.isPrimitive() bz told me !isObject() :-P > @@ +5321,5 @@ > > + > > + WorkerPrivate* w = nullptr; > > + UNWRAP_OBJECT(Worker, aCx, &aWorker.toObject(), w); > > + if (!w) { > > + return nullptr; > > This should probably be asserted, only C++ can call this. Ok. > ::: dom/workers/WorkerPrivate.h > @@ +365,4 @@ > > > > public: > > + > > + virtual JSObject* WrapObject(JSContext* aCx, > > Nit: return type on its own line Ok. > @@ +365,5 @@ > > > > public: > > + > > + virtual JSObject* WrapObject(JSContext* aCx, > > + JS::Handle<JSObject*> aScope) MOZ_OVERRIDE; > > JS::HandleObject > > @@ +438,2 @@ > > PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage, > > + const Optional<Sequence<JS::Value> >& aTransferable, > > Nit: >> is fine now, in all the places you're touching. Are you sure? > @@ +797,1 @@ > > ~WorkerPrivate(); > > Hm, can this be protected now? Yes. > @@ +1027,5 @@ > > GetMessagePort(uint64_t aMessagePortSerial); > > > > +protected: > > + > > + static already_AddRefed<WorkerPrivate> > > Nit: No need for the newline above. Ok. > @@ +1118,5 @@ > > > > +// This class is only used to trick the DOM bindings. We never create > > +// instances of it, and static_casting to it is fine since it doesn't add > > +// anything to WorkerPrivate. > > +class ChromeWorkerPrivate : public WorkerPrivate { > > Nit: { on its own line Ok. > @@ +1128,5 @@ > > + static bool > > + WorkerAvailable(JSContext* /* unused */, JSObject* /* unused */); > > + > > +private: > > + ChromeWorkerPrivate() MOZ_DELETE; > > Add (er, delete) the copy constructor too? Ok. I threw in operator= too. > ::: dom/workers/WorkerScope.cpp > @@ +1465,5 @@ > > } > > > > // Init other paris-bindings. > > + if (!(!worker->IsChromeWorker() || > > + ChromeWorkerBinding::GetConstructorObject(aCx, global)) || > > Eww! Move this above to the block with the other chrome worker stuff. Ok. > @@ +1491,5 @@ > > return global; > > } > > > > +bool > > +GetterOnlyJSNative(JSContext* aCx, unsigned aArgc, JS::Value* aVp) > > I don't understand why this is here... Another Splinter bug? More stuff I moved out of Worker.cpp

Comment on attachment 815799 [details] [diff] [review] Full patch Review of attachment 815799 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Bindings.conf @@ +31,5 @@ > # true for workers, false otherwise). > # * customFinalize - The native class will use a custom finalize hook > # (defaults to true for workers, false otherwise). > +# * customWrapperManagement - The native class will be responsible for > +# preserving its own wrapper (no AddProperty) This ) may be in the wrong position. ::: dom/workers/RuntimeService.cpp @@ +1012,2 @@ > > + if (!events::InitClasses(aCx, aObj, true)) { Hrm. Looks like this will define WorkerEvent and friends, but not when you're actually resolving those? (Because you removed them from gStringChars.) @@ +2000,4 @@ > rv = loadInfo.mResolvedScriptURI->GetSpec(scriptSpec); > NS_ENSURE_SUCCESS(rv, rv); > > + nsRefPtr<WorkerPrivate> workerPrivate = nullptr; Don't need the = nullptr anymore @@ +2020,5 @@ > + ErrorResult rv; > + workerPrivate = > + WorkerPrivate::Constructor(aGlobal, aScriptURL, false, true, aName, > + &loadInfo, rv); > + NS_ENSURE_TRUE(workerPrivate, NS_ERROR_FAILURE); Not checking rv? ::: dom/workers/SharedWorker.cpp @@ +35,4 @@ > mSuspended(false) > { > AssertIsOnMainThread(); > + MOZ_ASSERT(mWorkerPrivate); Why these changes? @@ +78,4 @@ > } > > nsRefPtr<SharedWorker> sharedWorker; > + nsresult rv = rts->CreateSharedWorker(aGlobal, aScriptURL, name, Followup on removing the |'implicitJSContext': [ 'constructor' ],| bit from Bindings.conf, please. ::: dom/workers/Worker.cpp @@ -1,1 @@ > -/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */ Woo! ::: dom/workers/WorkerPrivate.cpp @@ +923,5 @@ > + > + JS::Rooted<JS::Value> messageData(aCx); > + if (!mBuffer.read(aCx, messageData.address(), > + workers::WorkerStructuredCloneCallbacks(!aWorkerPrivate->GetParent()))) { > + xpc::Throw(aCx, NS_ERROR_DOM_DATA_CLONE_ERR); dom::Throw? @@ +938,5 @@ > + EmptyString(), > + EmptyString(), > + nullptr); > + if (NS_FAILED(rv)) { > + xpc::Throw(aCx, rv); Ditto @@ +1181,5 @@ > // they show up in the error console. > if (!JSREPORT_IS_WARNING(aFlags)) { > // First fire an ErrorEvent at the worker. > + if (aTarget) { > + ErrorEventInit init; Much nicer :) @@ +2541,3 @@ > } > + } > + else { } else { @@ +2585,3 @@ > bool aToMessagePort, > + uint64_t aMessagePortSerial, > + mozilla::ErrorResult& aRv) Drop the mozilla::? @@ +2619,5 @@ > + if (aTransferable.WasPassed()) { > + const Sequence<JS::Value>& realTransferable = aTransferable.Value(); > + JSObject* array = > + JS_NewArrayObject(aCx, realTransferable.Length(), > + const_cast<JS::Value*>(realTransferable.Elements())); Followup to make JS_NewArrayObject take a const pointer? @@ +3281,5 @@ > // static > already_AddRefed<WorkerPrivate> > +WorkerPrivate::Constructor(const GlobalObject& aGlobal, > + const nsAString& aScriptURL, > + mozilla::ErrorResult& aRv) Should be able to drop this mozilla:: too @@ +3309,5 @@ > +// static > +already_AddRefed<ChromeWorkerPrivate> > +ChromeWorkerPrivate::Constructor(const GlobalObject& aGlobal, > + const nsAString& aScriptURL, > + mozilla::ErrorResult& aRv) Ditto @@ +3374,5 @@ > + aRv.Throw(NS_ERROR_FAILURE); > + return nullptr; > + } > + } > + else { } else { @@ +5312,5 @@ > > +WorkerCrossThreadDispatcher* > +GetWorkerCrossThreadDispatcher(JSContext* aCx, JS::Value aWorker) > +{ > + if (aWorker.isPrimitive()) { !isObject() is nicer, as you do toObject() below ::: dom/workers/WorkerPrivate.h @@ +303,5 @@ > WorkerPrivate* mParent; > nsString mScriptURL; > nsString mSharedWorkerName; > LocationInfo mLocationInfo; > + // NB: The lifetime of these objects within LoadInfo is managed explicitly; Another NB @@ +798,5 @@ > > +public: > + static already_AddRefed<WorkerPrivate> > + Constructor(const GlobalObject& aGlobal, const nsAString& aScriptURL, > + ErrorResult& rv); aRv @@ +804,5 @@ > static already_AddRefed<WorkerPrivate> > + Constructor(const GlobalObject& aGlobal, const nsAString& aScriptURL, > + bool aIsChromeWorker, bool aIsSharedWorker, > + const nsAString& aSharedWorkerName, > + LoadInfo* aLoadInfo, mozilla::ErrorResult& aRv); Drop mozilla:: @@ +1028,4 @@ > GetMessagePort(uint64_t aMessagePortSerial); > > private: > + WorkerPrivate(JSContext* aCx, WorkerPrivate* aParent, Double space after |aCx,| ::: dom/workers/WorkerScope.cpp @@ +1492,5 @@ > > +bool > +GetterOnlyJSNative(JSContext* aCx, unsigned aArgc, JS::Value* aVp) > +{ > + JS_ReportErrorNumber(aCx, js_GetErrorMessage, nullptr, JSMSG_GETTER_ONLY); Two-space indentation

I made all of Ms2ger's suggested changes except: - Changing the else cuddling: half cuddling is consistent with the rest of the directory and other bent code. - Changing xpc::Throw to dom::Throw: using xpc::Throw is consistent with the rest of this file, and since that may change behavior I'd prefer to fix it in a followup.

Comment on attachment 815797 [details] [diff] [review] Interdiff Review of attachment 815797 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/WorkerPrivate.cpp @@ +2126,5 @@ > template <class Derived> > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(WorkerPrivateParent<Derived>, > nsDOMEventTargetHelper) > // Nothing else to traverse > + // The various strong references in LoadInfo are managed manually andcannot Nit: s/andcannot/and cannot/

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #13) > dom/ doesn't do this. Please file another bug on doing a global > search/replace. Really? Crap. I coulda sworn the newsgroup decision was to use the typedefs from now on in Gecko. > That compiles now? Yep, I added a few with SharedWorker. > The caller should be holding a reference (if not literally, then ensuring > that something else is for the duration of the call). Maybe, but that wasn't the case before (since GC would have to happen after this call finished). Have you checked the code here or are you just philosophizing? > This is code you wrote last week you're complaining about ... Ha! Technically it was "many months ago" and I blame my reviewer ;)

> I coulda sworn the newsgroup decision was to use the typedefs from now on in Gecko. The only decision so far was about the JS engine...

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e8bb002bc13a - one of those two broke osfile/tests/xpcshell/test_exception.js rather thoroughly, https://tbpl.mozilla.org/php/getParsedLog.php?id=29068111&tree=Mozilla-Inbound.

Backed out for frequent Android Armv6 mochitest-8 crashes caused by either this or bug 919885. https://hg.mozilla.org/mozilla-central/rev/30afbcdcec4d https://tbpl.mozilla.org/php/getParsedLog.php?id=29092370&tree=Mozilla-Central

More stacks: https://tbpl.mozilla.org/php/getParsedLog.php?id=29091987&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=29082049&tree=Mozilla-Inbound

Retriggers are green post-backout.