This removes the last nativeOwnership == worker classes, but doesn't remove the Codegen support. Blob/file are the only remaining things with manual JSAPI bindings after this too.

bent for changes in dom/workers, smaug for changes in content/events, and peterv for changes to codegen

60 files changed, 1558 insertions(+), 4852 deletions(-) \o/

Comment on attachment 819088 [details] [diff] [review] Patch Review of attachment 819088 [details] [diff] [review]: ----------------------------------------------------------------- I love you. ::: content/events/src/nsEventListenerManager.cpp @@ +1260,2 @@ > } > + else { This isn't bent code, cuddle else ;) ::: content/events/src/nsEventListenerManager.h @@ +483,5 @@ > + const nsEventHandler* handler; > + if (mIsMainThreadELM) { > + handler = GetEventHandlerInternal(nsGkAtoms::onerror, EmptyString()); > + } > + else { Ditto ::: dom/base/MessagePort.cpp @@ +195,5 @@ > if (NS_SUCCEEDED(rv)) { > + nsRefPtr<MessagePortBase> newPort = port->Clone(); > + > + if (!newPort) { > + return false; Does this need to throw? @@ -331,3 @@ > , mMessageQueueEnabled(false) > { > - MOZ_COUNT_CTOR(MessagePort); Why? ::: dom/base/MessagePort.h @@ +20,5 @@ > +class MessagePortBase : public nsDOMEventTargetHelper > +{ > +protected: > + MessagePortBase(nsPIDOMWindow* aWindow); > + MessagePortBase() {} The one with an argument will get SetIsDOMBinding() called by the nsDETH constructor, this one won't. Intentional? ::: dom/bindings/Codegen.py @@ +178,5 @@ > def __init__(self, descriptor): > CGThing.__init__(self) > self.descriptor = descriptor > # Our current reserved slot situation is unsafe for globals. Fix bug 760095! > + assert not ("Window" == descriptor.interface.identifier.name) !=? @@ +2398,5 @@ > + ' "nsISupports must be on our primary inheritance chain");\n') > + return """%s > +%s > + > + JS::Rooted<JSObject*> global(aCx, Seems like part of this might be able to move into a C++ function? @@ +2426,5 @@ > + > + if (!JS_SetPrototype(aCx, global, proto)) { > + return nullptr; > + } > + ws ::: dom/workers/MessagePort.h @@ +91,5 @@ > Start(); > } > > + virtual already_AddRefed<MessagePortBase> > + Clone() MOZ_OVERRIDE { { on the next line ::: dom/workers/RegisterBindings.cpp @@ +1,1 @@ > +#include "WorkerPrivate.h" License header ::: dom/workers/SharedWorker.h @@ +81,5 @@ > > virtual nsresult > PreHandleEvent(nsEventChainPreVisitor& aVisitor) MOZ_OVERRIDE; > > + WorkerPrivate* GetWorkerPrivate() const { { on the next line ::: dom/workers/WorkerPrivate.cpp @@ +4211,5 @@ > return true; > } > > void > +WorkerPrivate::TraceTimeouts(const TraceCallbacks &aCallbacks, void *aClosure) const &, * to the left ::: dom/workers/WorkerScope.cpp @@ +72,5 @@ > +{ > + MOZ_CRASH("We should never get here!"); > +} > + > +already_AddRefed<WorkerLocation> (Do these need to be already_AddRefed?) ::: dom/workers/WorkerScope.h @@ +26,5 @@ > +class WorkerGlobalScope : public nsDOMEventTargetHelper, > + public nsIGlobalObject > +{ > + typedef workers::WorkerLocation WorkerLocation; > + typedef workers::WorkerNavigator WorkerNavigator; Is this necessary? You're inside m::d::workers @@ +47,5 @@ > + WrapGlobalObject(JSContext* aCx, JS::CompartmentOptions& aOptions, > + JSPrincipals* aPrincipal) = 0; > + > + virtual JSObject* > + GetGlobalJSObject(void) MOZ_OVERRIDE Why the void? @@ +56,5 @@ > + NS_DECL_ISUPPORTS_INHERITED > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(WorkerGlobalScope, > + nsDOMEventTargetHelper) > + > + already_AddRefed<WorkerGlobalScope> Self() { { on the next line; why already_AddRefed? @@ +136,5 @@ > + WrapGlobalObject(JSContext* aCx, JS::CompartmentOptions& aOptions, > + JSPrincipals* aPrincipal) MOZ_OVERRIDE; > + > + void GetName(DOMString& aName) const { > + aName.AsAString() = mName; Might as well make it take nsString&, then, no? ::: dom/workers/XMLHttpRequest.cpp @@ +763,5 @@ > + mLoaded, mTotal); > + } > + } > + else { > + nsEventDispatcher::CreateEvent(target, nullptr, nullptr, Why not NS_NewDOMEvent? @@ +1439,5 @@ > return NS_OK; > } > > +XMLHttpRequest::XMLHttpRequest(WorkerPrivate* aWorkerPrivate) > +: mUpload(NULL), mWorkerPrivate(aWorkerPrivate), nullptr, please @@ +1482,5 @@ > + NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK(mStateData.mResponse) > +NS_IMPL_CYCLE_COLLECTION_TRACE_END > + > +JSObject* > +XMLHttpRequest::WrapObject(JSContext* aCx, JS::HandleObject aScope) JS::Handle<JSObject*> outside js/. @@ +1624,5 @@ > } > > + nsCOMPtr<nsIDOMEvent> event; > + if (aEventType.EqualsLiteral("readystatechange")) { > + nsEventDispatcher::CreateEvent(aTarget, nullptr, nullptr, Ditto @@ +1905,5 @@ > aRv.Throw(NS_ERROR_FAILURE); > return NULL; > } > > + mUpload = upload.forget(); I think you can assign to mUpload directly, if you want. ::: dom/workers/XMLHttpRequest.h @@ +75,2 @@ > > + nsISupports* GetParentObject() const { { ::: dom/workers/XMLHttpRequestUpload.cpp @@ +25,5 @@ > + > +NS_IMPL_CYCLE_COLLECTION_INHERITED_1(XMLHttpRequestUpload, nsXHREventTarget, mXHR) > + > +JSObject* > +XMLHttpRequestUpload::WrapObject(JSContext* aCx, JS::HandleObject aScope) <> ::: dom/workers/XMLHttpRequestUpload.h @@ +23,5 @@ > { } > > public: > + virtual JSObject* > + WrapObject(JSContext* aCx, JS::HandleObject aScope) MOZ_OVERRIDE; <>. I bet I missed some, please fix them all :) @@ +32,5 @@ > + NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(XMLHttpRequestUpload, nsXHREventTarget) > + > + NS_DECL_ISUPPORTS_INHERITED > + > + nsISupports* GetParentObject() const { { ::: dom/workers/test/atob_worker.js @@ +1,5 @@ > /** > * Any copyright is dedicated to the Public Domain. > * http://creativecommons.org/publicdomain/zero/1.0/ > */ > +var data = [ -1, 0, 1, 1.5, /* null ,*/ undefined, true, false, "foo", This is annoying to make a todo(), I guess. File a followup, at least. ::: dom/workers/test/eventDispatch_worker.js @@ +9,4 @@ > throw new Error("Event has a bad target!"); > } > + if (event.currentTarget) { > + throw new Error("Event has a bad currentTarget!"); Maybe "...should not have a currentTarget" @@ +57,5 @@ > + data: event.data, > + origin: "*", > + source: null > + }); > + dump(event); Leftover dump?

(In reply to :Ms2ger from comment #3) > Comment on attachment 819088 [details] [diff] [review] > Patch > > Review of attachment 819088 [details] [diff] [review]: > ----------------------------------------------------------------- > > I love you. <3 > ::: content/events/src/nsEventListenerManager.cpp > @@ +1260,2 @@ > > } > > + else { > > This isn't bent code, cuddle else ;) Fair enough. > ::: content/events/src/nsEventListenerManager.h > @@ +483,5 @@ > > + const nsEventHandler* handler; > > + if (mIsMainThreadELM) { > > + handler = GetEventHandlerInternal(nsGkAtoms::onerror, EmptyString()); > > + } > > + else { > > Ditto > > ::: dom/base/MessagePort.cpp > @@ +195,5 @@ > > if (NS_SUCCEEDED(rv)) { > > + nsRefPtr<MessagePortBase> newPort = port->Clone(); > > + > > + if (!newPort) { > > + return false; > > Does this need to throw? No. We're in a JS engine hook here, so when we return false we'll unwind the stack back to whatever Gecko code is calling JS_WriteStructuredClone, and that will set NS_DOM_DATA_CLONE_ERR if there is no existing exception. Might be worth adding a comment. > @@ -331,3 @@ > > , mMessageQueueEnabled(false) > > { > > - MOZ_COUNT_CTOR(MessagePort); > > Why? It's refcounted, so we don't need the MOZ_COUNT_CTOR/DTOR macros. > ::: dom/base/MessagePort.h > @@ +20,5 @@ > > +class MessagePortBase : public nsDOMEventTargetHelper > > +{ > > +protected: > > + MessagePortBase(nsPIDOMWindow* aWindow); > > + MessagePortBase() {} > > The one with an argument will get SetIsDOMBinding() called by the nsDETH > constructor, this one won't. Intentional? No. > ::: dom/bindings/Codegen.py > @@ +178,5 @@ > > def __init__(self, descriptor): > > CGThing.__init__(self) > > self.descriptor = descriptor > > # Our current reserved slot situation is unsafe for globals. Fix bug 760095! > > + assert not ("Window" == descriptor.interface.identifier.name) > > !=? Yeah I wasn't sure if that was a thing in python. > @@ +2398,5 @@ > > + ' "nsISupports must be on our primary inheritance chain");\n') > > + return """%s > > +%s > > + > > + JS::Rooted<JSObject*> global(aCx, > > Seems like part of this might be able to move into a C++ function? Yeah, we could probably move the guts into something in BindingUtils. > @@ +2426,5 @@ > > + > > + if (!JS_SetPrototype(aCx, global, proto)) { > > + return nullptr; > > + } > > + > > ws > > ::: dom/workers/MessagePort.h > @@ +91,5 @@ > > Start(); > > } > > > > + virtual already_AddRefed<MessagePortBase> > > + Clone() MOZ_OVERRIDE { > > { on the next line > > ::: dom/workers/RegisterBindings.cpp > @@ +1,1 @@ > > +#include "WorkerPrivate.h" > > License header > > ::: dom/workers/SharedWorker.h > @@ +81,5 @@ > > > > virtual nsresult > > PreHandleEvent(nsEventChainPreVisitor& aVisitor) MOZ_OVERRIDE; > > > > + WorkerPrivate* GetWorkerPrivate() const { > > { on the next line > > ::: dom/workers/WorkerPrivate.cpp > @@ +4211,5 @@ > > return true; > > } > > > > void > > +WorkerPrivate::TraceTimeouts(const TraceCallbacks &aCallbacks, void *aClosure) const > > &, * to the left > > ::: dom/workers/WorkerScope.cpp > @@ +72,5 @@ > > +{ > > + MOZ_CRASH("We should never get here!"); > > +} > > + > > +already_AddRefed<WorkerLocation> > > (Do these need to be already_AddRefed?) No, but is there a reason for them not to be? > ::: dom/workers/WorkerScope.h > @@ +26,5 @@ > > +class WorkerGlobalScope : public nsDOMEventTargetHelper, > > + public nsIGlobalObject > > +{ > > + typedef workers::WorkerLocation WorkerLocation; > > + typedef workers::WorkerNavigator WorkerNavigator; > > Is this necessary? You're inside m::d::workers Mmm I think I tried to put it in m::d at first and failed. This is probably left over from that. > @@ +47,5 @@ > > + WrapGlobalObject(JSContext* aCx, JS::CompartmentOptions& aOptions, > > + JSPrincipals* aPrincipal) = 0; > > + > > + virtual JSObject* > > + GetGlobalJSObject(void) MOZ_OVERRIDE > > Why the void? I probably copy/pasted from nsIGlobalObject.h. > @@ +56,5 @@ > > + NS_DECL_ISUPPORTS_INHERITED > > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(WorkerGlobalScope, > > + nsDOMEventTargetHelper) > > + > > + already_AddRefed<WorkerGlobalScope> Self() { > > { on the next line; why already_AddRefed? *shrug* This is what the codegen defaults to. > @@ +136,5 @@ > > + WrapGlobalObject(JSContext* aCx, JS::CompartmentOptions& aOptions, > > + JSPrincipals* aPrincipal) MOZ_OVERRIDE; > > + > > + void GetName(DOMString& aName) const { > > + aName.AsAString() = mName; > > Might as well make it take nsString&, then, no? Yeah probably. > ::: dom/workers/XMLHttpRequest.cpp > @@ +763,5 @@ > > + mLoaded, mTotal); > > + } > > + } > > + else { > > + nsEventDispatcher::CreateEvent(target, nullptr, nullptr, > > Why not NS_NewDOMEvent? Cause I forgot that was a thing, most likely. > @@ +1439,5 @@ > > return NS_OK; > > } > > > > +XMLHttpRequest::XMLHttpRequest(WorkerPrivate* aWorkerPrivate) > > +: mUpload(NULL), mWorkerPrivate(aWorkerPrivate), > > nullptr, please Actually this is an nsRefPtr now so we shouldn't init it at all. > @@ +1482,5 @@ > > + NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK(mStateData.mResponse) > > +NS_IMPL_CYCLE_COLLECTION_TRACE_END > > + > > +JSObject* > > +XMLHttpRequest::WrapObject(JSContext* aCx, JS::HandleObject aScope) > > JS::Handle<JSObject*> outside js/. > > @@ +1624,5 @@ > > } > > > > + nsCOMPtr<nsIDOMEvent> event; > > + if (aEventType.EqualsLiteral("readystatechange")) { > > + nsEventDispatcher::CreateEvent(aTarget, nullptr, nullptr, > > Ditto > > @@ +1905,5 @@ > > aRv.Throw(NS_ERROR_FAILURE); > > return NULL; > > } > > > > + mUpload = upload.forget(); > > I think you can assign to mUpload directly, if you want. Well upload doesn't need the ref anymore, so this saves an addref/release pair. > ::: dom/workers/XMLHttpRequest.h > @@ +75,2 @@ > > > > + nsISupports* GetParentObject() const { > > { > > ::: dom/workers/XMLHttpRequestUpload.cpp > @@ +25,5 @@ > > + > > +NS_IMPL_CYCLE_COLLECTION_INHERITED_1(XMLHttpRequestUpload, nsXHREventTarget, mXHR) > > + > > +JSObject* > > +XMLHttpRequestUpload::WrapObject(JSContext* aCx, JS::HandleObject aScope) > > <> > > ::: dom/workers/XMLHttpRequestUpload.h > @@ +23,5 @@ > > { } > > > > public: > > + virtual JSObject* > > + WrapObject(JSContext* aCx, JS::HandleObject aScope) MOZ_OVERRIDE; > > <>. I bet I missed some, please fix them all :) > > @@ +32,5 @@ > > + NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(XMLHttpRequestUpload, nsXHREventTarget) > > + > > + NS_DECL_ISUPPORTS_INHERITED > > + > > + nsISupports* GetParentObject() const { > > { > > ::: dom/workers/test/atob_worker.js > @@ +1,5 @@ > > /** > > * Any copyright is dedicated to the Public Domain. > > * http://creativecommons.org/publicdomain/zero/1.0/ > > */ > > +var data = [ -1, 0, 1, 1.5, /* null ,*/ undefined, true, false, "foo", > > This is annoying to make a todo(), I guess. File a followup, at least. Ok. > ::: dom/workers/test/eventDispatch_worker.js > @@ +9,4 @@ > > throw new Error("Event has a bad target!"); > > } > > + if (event.currentTarget) { > > + throw new Error("Event has a bad currentTarget!"); > > Maybe "...should not have a currentTarget" > > @@ +57,5 @@ > > + data: event.data, > > + origin: "*", > > + source: null > > + }); > > + dump(event); > > Leftover dump? Yeah.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #4) > (In reply to :Ms2ger from comment #3) > > Comment on attachment 819088 [details] [diff] [review] > > Patch > > > > Review of attachment 819088 [details] [diff] [review]: > > ----------------------------------------------------------------- > > ::: dom/workers/WorkerScope.cpp > > @@ +72,5 @@ > > > +{ > > > + MOZ_CRASH("We should never get here!"); > > > +} > > > + > > > +already_AddRefed<WorkerLocation> > > > > (Do these need to be already_AddRefed?) > > No, but is there a reason for them not to be? Saves an addref-release pair if they're already wrapped, and a line of code here. No big deal, of course. > > @@ +1905,5 @@ > > > aRv.Throw(NS_ERROR_FAILURE); > > > return NULL; > > > } > > > > > > + mUpload = upload.forget(); > > > > I think you can assign to mUpload directly, if you want. > > Well upload doesn't need the ref anymore, so this saves an addref/release > pair. My idea was to remove the upload local entirely.

Comment on attachment 819088 [details] [diff] [review] Patch >+nsDOMMessageEvent::SetPorts(mozilla::dom::MessagePortList* aPorts) { { goes to the next line. >+++ b/content/events/src/nsEventListenerManager.cpp >@@ -520,17 +520,18 @@ nsEventListenerManager::ListenerCanHandl > return true; > } > if (aEvent->message == NS_USER_DEFINED_EVENT) { > if (mIsMainThreadELM) { > return aLs->mTypeAtom == aEvent->userType; > } > return aLs->mTypeString.Equals(aEvent->typeString); > } >- MOZ_ASSERT(mIsMainThreadELM); >+ MOZ_ASSERT_IF(aEvent->eventStructType != NS_SCRIPT_ERROR_EVENT, >+ mIsMainThreadELM); > return aLs->mEventType == aEvent->message; Most probably wrong. >+ if (!aName && aTypeString.EqualsLiteral("error")) { >+ eventType = NS_LOAD_ERROR; >+ } Why? > nsEventListenerManager::SetEventHandler(OnErrorEventHandlerNonNull* aHandler) > { >- if (!aHandler) { >- RemoveEventHandler(nsGkAtoms::onerror, EmptyString()); >- return; >+ if (mIsMainThreadELM) { >+ if (!aHandler) { >+ RemoveEventHandler(nsGkAtoms::onerror, EmptyString()); >+ return; >+ } >+ >+ // Untrusted events are always permitted for non-chrome script >+ // handlers. >+ SetEventHandlerInternal(nullptr, JS::NullPtr(), nsGkAtoms::onerror, >+ EmptyString(), nsEventHandler(aHandler), >+ !nsContentUtils::IsCallerChrome()); > } >+ else { else { goes to the previous line >+++ b/content/events/src/nsEventListenerManager.h >@@ -475,18 +475,23 @@ public: > const nsAString& aTypeString) > { > const nsEventHandler* handler = > GetEventHandlerInternal(aEventName, aTypeString); > return handler ? handler->EventHandler() : nullptr; > } > mozilla::dom::OnErrorEventHandlerNonNull* GetOnErrorEventHandler() > { >- const nsEventHandler* handler = >- GetEventHandlerInternal(nsGkAtoms::onerror, EmptyString()); >+ const nsEventHandler* handler; >+ if (mIsMainThreadELM) { >+ handler = GetEventHandlerInternal(nsGkAtoms::onerror, EmptyString()); >+ } >+ else { else { goes to the previous line Whatever code is firing the error event on workers should set event.typeString, and then those hacks in ELM shouldn't be needed. nsJSEventListener::HandleEvent may need event->message == NS_LOAD_ERROR -> event->message == NS_LOAD_ERROR || event->typeString.Equals("error") Those fixed...

(In reply to Olli Pettay [:smaug] from comment #6) > Whatever code is firing the error event on workers should set > event.typeString, and then those hacks > in ELM shouldn't be needed. nsJSEventListener::HandleEvent may need > event->message == NS_LOAD_ERROR -> > event->message == NS_LOAD_ERROR || event->typeString.Equals("error") > > Those fixed... Which "hacks" do you think this allows me to remove? message is still NS_LOAD_ERROR so I still the need the changes to ListenerCanHandle.

when you dispatch the event, make its message NS_USER_DEFINED_EVENT?

Comment on attachment 819088 [details] [diff] [review] Patch Review of attachment 819088 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bindings/Codegen.py @@ +145,5 @@ > return "&sWorkerNativePropertyHooks" if descriptor.workers else "sNativePropertyHooks" > > def DOMClass(descriptor): > + def make_name(d): > + return "%s%s" % (d.interface.identifier.name, '_workers' if d.workers else '') I think I'd rather see you fix the make_name in Configuration.py so that we keep fixing up these names only once. @@ +1114,5 @@ > + args) > + > + def generate_code(self): > + return """ MOZ_ASSERT(js::GetObjectClass(obj)->flags & JSCLASS_DOM_GLOBAL); > + mozilla::dom::TraceProtoAndIfaceCache(trc, obj);""" We shouldn't generate this, move it to BindingUtils, name it "Global" + TRACE_HOOK_NAME. @@ +1810,5 @@ > else: > parentProtoName = self.descriptor.prototypeChain[-2] > + parentDesc = self.descriptor.getDescriptor(parentProtoName) > + if parentDesc.workers: > + parentProtoName += '_workers' This wouldn't be needed if you fixed up make_name in Configuration.py @@ +2394,5 @@ > + > + def definition_body(self): > + assertISupportsInheritance = ( > + ' MOZ_ASSERT(ToSupportsIsOnPrimaryInheritanceChain(aObject, aCache),\n' > + ' "nsISupports must be on our primary inheritance chain");\n') Why not move this into the code below directly instead of through %s? @@ +2398,5 @@ > + ' "nsISupports must be on our primary inheritance chain");\n') > + return """%s > +%s > + > + JS::Rooted<JSObject*> global(aCx, Yes, please into C++ and out of generated code. @@ +5666,5 @@ > + "if (!JS_ResolveStandardClass(cx, obj, id, &resolved)) {\n" > + " return false;\n" > + "}\n\n" > + "objp.set(resolved ? obj.get() : nullptr);\n" > + "return true;\n")) We shouldn't generate this, name it "Global" + NEWRESOLVE_HOOK_NAME (and I think you can remove CGAbstractNewResolveHook again). @@ +5718,5 @@ > + CGAbstractEnumerateHook.__init__(self, descriptor) > + > + def generate_code(self): > + return CGIndenter(CGGeneric( > + "return JS_EnumerateStandardClasses(cx, obj);\n")) We shouldn't generate this, name it "Global" + ENUMERATE_HOOK_NAME (and I think you can remove CGAbstractEnumerateHook again). ::: dom/webidl/MessagePort.webidl @@ +11,2 @@ > [Throws] > + void postMessage(any message, optional sequence<any> transferable); I did |typedef any Transferable;| in Window.webidl

Comment on attachment 819088 [details] [diff] [review] Patch Review of attachment 819088 [details] [diff] [review]: ----------------------------------------------------------------- This looks really good, thanks. ::: content/events/src/nsDOMMessageEvent.cpp @@ +200,5 @@ > return NS_OK; > } > > +void > +nsDOMMessageEvent::SetPorts(mozilla::dom::MessagePortList* aPorts) { Nit: { on its own line. ::: dom/base/MessagePort.cpp @@ +286,5 @@ > mPort->DispatchEvent(event, &status); > return status ? NS_OK : NS_ERROR_FAILURE; > } > > +MessagePortBase::MessagePortBase(nsPIDOMWindow* aWindow) I would inline this. @@ +287,5 @@ > return status ? NS_OK : NS_ERROR_FAILURE; > } > > +MessagePortBase::MessagePortBase(nsPIDOMWindow* aWindow) > +: nsDOMEventTargetHelper(aWindow) Nit: baku points out that the rest of the constructors indent their colon here :) @@ +370,5 @@ > + if (aTransferable.WasPassed()) { > + const Sequence<JS::Value>& realTransferable = aTransferable.Value(); > + JSObject* array = > + JS_NewArrayObject(aCx, realTransferable.Length(), > + const_cast<JS::Value*>(realTransferable.Elements())); Shouldn't |array| be JS::Rooted? It's unlikely that you could trigger GC here so maybe not. ::: dom/webidl/WorkerGlobalScope.webidl @@ +38,5 @@ > + > + void dump(optional DOMString str); > +}; > + > +[Global,Func="mozilla::dom::workers::DedicatedWorkerGlobalScope::Visible"] Nit: space after ',' and below too. @@ +39,5 @@ > + void dump(optional DOMString str); > +}; > + > +[Global,Func="mozilla::dom::workers::DedicatedWorkerGlobalScope::Visible"] > +interface DedicatedWorkerGlobalScope : WorkerGlobalScope { I think DedicatedWorkerGlobalScope and SharedWorkerGlobalScope should be in separate files. The URL at the top isn't right for these. @@ +47,5 @@ > + attribute EventHandler onmessage; > +}; > + > +[Global,Func="mozilla::dom::workers::SharedWorkerGlobalScope::Visible"] > +interface SharedWorkerGlobalScope : WorkerGlobalScope { This should have a commented out line about the application cache, right? ::: dom/workers/MessagePort.cpp @@ +18,5 @@ > USING_WORKERS_NAMESPACE > > namespace { > > +class DelayedEventRunnable MOZ_FINAL : public WorkerRunnable The changes here won't work. Apparently the old code was broken too, but we have to keep events queued on the port since any event could clone the port to some other event loop. See baku's test_messageChannel_pingpong.html. Just revert this and file a followup on fixing it for real. @@ +70,5 @@ > } > > // static > bool > MessagePort::PrefEnabled() So this should go away, right? @@ +119,5 @@ > if (!mQueuedEvents.IsEmpty()) { > + WorkerRunnable::Target target = > + mWorkerPrivate ? WorkerRunnable::WorkerThread : WorkerRunnable::ParentThread; > + WorkerPrivate* workerPrivate = > + mWorkerPrivate ? mWorkerPrivate : mSharedWorker->GetWorkerPrivate(); Don't check the same condition twice. ::: dom/workers/MessagePort.h @@ +9,5 @@ > #include "Workers.h" > > #include "mozilla/dom/BindingDeclarations.h" > #include "nsDOMEventTargetHelper.h" > +#include "mozilla/dom/MessagePort.h" Need. more. tidiness. @@ +47,3 @@ > > + virtual void > + Close() MOZ_OVERRIDE All of these functions that used to be inlines but are now virtuals can just move to the cpp. The compiler won't inline them anyway and it clutters up the file. @@ +74,2 @@ > > + return NS_IsMainThread() ? GetEventHandler(nsGkAtoms::onmessage, EmptyString()) Weird, where is nsGkAtoms getting included from? @@ +92,5 @@ > } > > + virtual already_AddRefed<MessagePortBase> > + Clone() MOZ_OVERRIDE { > + return nullptr; This should get a NS_WARNING or something since it's not implemented yet. @@ +111,5 @@ > +#ifdef DEBUG > + void > + AssertCorrectThread() const; > +#else > + inline void Nit: gratuitous inline ::: dom/workers/RegisterBindings.cpp @@ +1,2 @@ > +#include "WorkerPrivate.h" > +#include "Workers.h" Every worker header includes this already... @@ +3,5 @@ > +#include "ChromeWorkerScope.h" > +#include "File.h" > + > +#include "jsapi.h" > +#include "js/OldDebugAPI.h" Is this one needed? ::: dom/workers/ScriptLoader.cpp @@ +902,4 @@ > } > > + if (urlCount > MAX_CONCURRENT_SCRIPTS) { > + aRv.Throw(NS_ERROR_OUT_OF_MEMORY); Doesn't this turn into an uncatchable exception? If so that is wrong. @@ +911,2 @@ > > + for (unsigned index = 0; index < urlCount; index++) { Ick, not your fault, but s/unsigned/uint32_t/ ::: dom/workers/SharedWorker.h @@ +8,5 @@ > > #include "Workers.h" > > #include "mozilla/dom/BindingDeclarations.h" > +#include "mozilla/dom/workers/bindings/MessagePort.h" Why is this needed? Seems like this goes against our desire to remove headers. @@ +81,5 @@ > > virtual nsresult > PreHandleEvent(nsEventChainPreVisitor& aVisitor) MOZ_OVERRIDE; > > + WorkerPrivate* GetWorkerPrivate() const { And return type on its own line! ::: dom/workers/WorkerPrivate.cpp @@ +897,5 @@ > + nsDOMEventTargetHelper* aTarget, bool aIsMainThread) > + { > + // Release reference to objects that were AddRef'd for > + // cloning into worker when array goes out of scope. > + nsTArray<nsCOMPtr<nsISupports> > clonedObjects; Nit: >> @@ +903,5 @@ > + > + JS::Rooted<JS::Value> messageData(aCx); > + if (!mBuffer.read(aCx, messageData.address(), > + workers::WorkerStructuredCloneCallbacks(aIsMainThread))) { > + xpc::Throw(aCx, NS_ERROR_DOM_DATA_CLONE_ERR); dom::Throw right? @@ +918,5 @@ > + EmptyString(), > + EmptyString(), > + nullptr); > + if (NS_FAILED(rv)) { > + xpc::Throw(aCx, rv); Here too. @@ +1225,5 @@ > + > + nsEventStatus status = nsEventStatus_eIgnore; > + if (NS_FAILED( > + nsEventDispatcher::Dispatch(static_cast<nsIDOMEventTarget*>(globalTarget), > + nullptr, &event, nullptr, &status))) { Can we clean this up a bit? Temporaries are ok. @@ +4472,3 @@ > WorkerPrivate::PostMessageToParentInternal(JSContext* aCx, > + JS::Handle<JS::Value> aMessage, > + const Optional<Sequence<JS::Value>>& aTransferable, > 80 chars @@ +4495,5 @@ > + IsChromeWorker() ? > + &gChromeWorkerStructuredCloneCallbacks : > + &gWorkerStructuredCloneCallbacks; > + > + nsTArray<nsCOMPtr<nsISupports> > clonedObjects; >> @@ +4762,5 @@ > > // It's a script bug if setTimeout/setInterval are called from a close handler > // so throw an exception. > if (currentStatus == Closing) { > JS_ReportError(aCx, "Cannot schedule timeouts from the close handler!"); Hrm, this looks messed up (it should be returning false, wtf). You should set aRv and return here. @@ +4768,5 @@ > > // If the worker is trying to call setTimeout/setInterval and the parent > // thread has initiated the close process then just silently fail. > if (currentStatus >= Closing) { > + return 0; The previous behavior was to throw an uncatchable exception. This will let the JS keep running... If you do that then you can't return 0, that will confuse the script. I vote you just set aRv and return. @@ +4784,5 @@ > // Take care of the main argument. > + if (aHandler) { > + newInfo->mTimeoutCallable = JS::ObjectValue(*aHandler->Callable()); > + } > + else if (!aStringHandler.IsEmpty()) { You asserted this above... Is the bindings code guaranteeing this? If so you should remove the else block below. @@ +4795,5 @@ > } > > // See if any of the optional arguments were passed. > + if (aTimeout.WasPassed()) { > + newInfo->mInterval = TimeDuration::FromMilliseconds(aTimeout.Value()); What happens if aTimeout is negative? @@ +4821,5 @@ > NS_WARNING("Failed to get calling location!"); > } > } > > + auto insertedInfo = This is kind of an abuse of auto imo. Just put the type, it's easier to read. @@ +5232,1 @@ > return false; Should an exception be set? @@ +5268,5 @@ > > // The port may have already been removed from this list since either the main > // thread or the worker thread can remove it. > + // XXXkhuey mWorkerPorts is only touched from the worker thread so what is > + // this talking about? Dunno! Remove it? This is why I hate comments ;) @@ +5298,5 @@ > + else { > + globalScope = new DedicatedWorkerGlobalScope(this); > + } > + > + MOZ_ASSERT(globalScope); This isn't needed. @@ +5314,5 @@ > + JSAutoCompartment ac(aCx, global); > + > + if (!RegisterBindings(aCx, global)) { > + // If we destroy globalScope, we will release the WorkerPrivate on the > + // wrong thread. Just leak. How about we proxy this properly instead :) ::: dom/workers/WorkerPrivate.h @@ +782,5 @@ > > nsCOMPtr<nsITimer> mTimer; > nsRefPtr<MemoryReporter> mMemoryReporter; > > + nsRefPtrHashtable<nsUint64HashKey, MessagePort> mWorkerPorts; Hm, don't you need to tell the CC about this somehow? ::: dom/workers/WorkerScope.cpp @@ +73,5 @@ > + MOZ_CRASH("We should never get here!"); > +} > + > +already_AddRefed<WorkerLocation> > +WorkerGlobalScope::Location() I think you should add 'mWorkerPrivate->AssertIsOnWorkerThread();' to pretty much every method in this file. @@ +83,4 @@ > } > > + nsRefPtr<WorkerLocation> location = mLocation; > + MOZ_ASSERT(location); I'd just assert when you create the first time, same for Navigator. ::: dom/workers/WorkerScope.h @@ +60,5 @@ > + already_AddRefed<WorkerGlobalScope> Self() { > + return nsRefPtr<WorkerGlobalScope>(this).forget(); > + } > + > + already_AddRefed<WorkerLocation> Location(); This whole file needs return types on their own lines. @@ +65,5 @@ > + already_AddRefed<WorkerNavigator> Navigator(); > + void Close(JSContext* aCx); > + > + OnErrorEventHandlerNonNull* GetOnerror(); > + void SetOnerror(OnErrorEventHandlerNonNull* aHandler); Why can't you use the macros here? @@ +101,5 @@ > + > + void Dump(const Optional<nsAString>& aString) const; > +}; > + > +class DedicatedWorkerGlobalScope : public WorkerGlobalScope MOZ_FINAL, and a private destructor please. Same for Shared... @@ +147,5 @@ > CreateGlobalScope(JSContext* aCx); > > END_WORKERS_NAMESPACE > > +#endif /* mozilla_dom_workers_workerscope_h__ */ Forgot to change this. ::: dom/workers/XMLHttpRequest.cpp @@ +776,2 @@ > if (!event) { > return false; You'll need to set an exception here now. @@ +1439,5 @@ > return NS_OK; > } > > +XMLHttpRequest::XMLHttpRequest(WorkerPrivate* aWorkerPrivate) > +: mUpload(NULL), mWorkerPrivate(aWorkerPrivate), mUpload is a refptr... @@ +1457,5 @@ > + ReleaseProxy(XHRIsGoingAway); > + > + MOZ_ASSERT(!mRooted); > + > + mozilla::DropJSObjects(this); Is it safe to call this if you never called HoldJSObjects? @@ +1468,5 @@ > +NS_INTERFACE_MAP_END_INHERITING(nsXHREventTarget) > + > +NS_IMPL_CYCLE_COLLECTION_CLASS(XMLHttpRequest) > + > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(XMLHttpRequest, nsXHREventTarget) > 80 chars, below too. @@ +1472,5 @@ > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(XMLHttpRequest, nsXHREventTarget) > + NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mUpload) > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END > + > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(XMLHttpRequest, nsXHREventTarget) Don't you have to unset mStateData.mResponse here? @@ +1560,4 @@ > return; > } > > + NS_ADDREF_THIS(); This macro is longer than simply calling AddRef(). @@ +1575,5 @@ > > if (mProxy->mSeenUploadLoadStart) { > MOZ_ASSERT(mUpload); > > + DispatchPrematureAbortEvent(mUpload, NS_LITERAL_STRING("abort"), true, aRv); > 80 chars now, for most of these. @@ +1642,5 @@ > + aUploadTarget ? mProxy->mLastUploadLengthComputable : > + mProxy->mLastLengthComputable, > + aUploadTarget ? mProxy->mLastUploadLoaded : > + mProxy->mLastLoaded, > + aUploadTarget ? mProxy->mLastUploadTotal : Checking this condition three times is silly. Just use a nested if. @@ +1662,5 @@ > XMLHttpRequest::Unpin() > { > mWorkerPrivate->AssertIsOnWorkerThread(); > > + NS_ASSERTION(mRooted, "Mismatched calls to Unpin!"); MOZ_ASSERT while you're at it. @@ +1669,2 @@ > > + NS_RELEASE_THIS(); How do you know this isn't the last ref? Can you defer it? @@ +1898,5 @@ > return NULL; > } > > if (!mUpload) { > + nsRefPtr<XMLHttpRequestUpload> upload = XMLHttpRequestUpload::Create(this); > 80 chars @@ +2190,5 @@ > aResponseText = mStateData.mResponseText; > } > + > + > +void Nit: extra newline above @@ +2195,5 @@ > +XMLHttpRequest::UpdateState(const StateData& aStateData) > +{ > + mStateData = aStateData; > + if (JSVAL_IS_GCTHING(mStateData.mResponse)) { > + mozilla::HoldJSObjects(this); Is it safe to call this more than once? ::: dom/workers/XMLHttpRequest.h @@ +75,2 @@ > > + nsISupports* GetParentObject() const { Return type on its own line. ::: dom/workers/XMLHttpRequestUpload.cpp @@ +11,5 @@ > > USING_WORKERS_NAMESPACE > > +XMLHttpRequestUpload::XMLHttpRequestUpload(XMLHttpRequest* aXHR) > + : mXHR(aXHR) No indent for : @@ +22,5 @@ > + > +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(XMLHttpRequestUpload) > +NS_INTERFACE_MAP_END_INHERITING(nsXHREventTarget) > + > +NS_IMPL_CYCLE_COLLECTION_INHERITED_1(XMLHttpRequestUpload, nsXHREventTarget, mXHR) > 80 chars ::: dom/workers/XMLHttpRequestUpload.h @@ +11,5 @@ > BEGIN_WORKERS_NAMESPACE > > class XMLHttpRequest; > > +class XMLHttpRequestUpload : public nsXHREventTarget MOZ_FINAL, then lose the virtual dtor. @@ +18,2 @@ > > protected: private I guess @@ +37,5 @@ > + // There's only one global on a worker, so we don't need to specify. > + return nullptr; > + } > + > + bool HasListeners() Return types on their own lines. ::: dom/workers/moz.build @@ +9,5 @@ > MODULE = 'dom' > > # Public stuff. > EXPORTS.mozilla.dom += [ > 'WorkerPrivate.h', Can we remove this yet?

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #10) > @@ +370,5 @@ > > + if (aTransferable.WasPassed()) { > > + const Sequence<JS::Value>& realTransferable = aTransferable.Value(); > > + JSObject* array = > > + JS_NewArrayObject(aCx, realTransferable.Length(), > > + const_cast<JS::Value*>(realTransferable.Elements())); > > Shouldn't |array| be JS::Rooted? It's unlikely that you could trigger GC > here so maybe not. This is kind of awkward. array gets stored in a JS::Rooted before we do anything that might GC, but we want to null check it first and null checking it once we store it in the JS::Rooted is awkward. > @@ +47,5 @@ > > + attribute EventHandler onmessage; > > +}; > > + > > +[Global,Func="mozilla::dom::workers::SharedWorkerGlobalScope::Visible"] > > +interface SharedWorkerGlobalScope : WorkerGlobalScope { > > This should have a commented out line about the application cache, right? *shrug* I don't see much value in adding it. > ::: dom/workers/MessagePort.cpp > @@ +18,5 @@ > > USING_WORKERS_NAMESPACE > > > > namespace { > > > > +class DelayedEventRunnable MOZ_FINAL : public WorkerRunnable > > The changes here won't work. Apparently the old code was broken too, but we > have to keep events queued on the port since any event could clone the port > to some other event loop. See baku's test_messageChannel_pingpong.html. Just > revert this and file a followup on fixing it for real. Ok ... I kept them anyways to avoid fiddling with the patch more. I'll file a followup on fixing it for real. > @@ +74,2 @@ > > > > + return NS_IsMainThread() ? GetEventHandler(nsGkAtoms::onmessage, EmptyString()) > > Weird, where is nsGkAtoms getting included from? nsDOMEventTargetHelper.h > @@ +3,5 @@ > > +#include "ChromeWorkerScope.h" > > +#include "File.h" > > + > > +#include "jsapi.h" > > +#include "js/OldDebugAPI.h" > > Is this one needed? Yes, for JS_DefineProfilingFunctions. > ::: dom/workers/ScriptLoader.cpp > @@ +902,4 @@ > > } > > > > + if (urlCount > MAX_CONCURRENT_SCRIPTS) { > > + aRv.Throw(NS_ERROR_OUT_OF_MEMORY); > > Doesn't this turn into an uncatchable exception? If so that is wrong. Not as far as I can tell. What makes you think it becomes an uncatchable exception? > ::: dom/workers/SharedWorker.h > @@ +8,5 @@ > > > > #include "Workers.h" > > > > #include "mozilla/dom/BindingDeclarations.h" > > +#include "mozilla/dom/workers/bindings/MessagePort.h" > > Why is this needed? Seems like this goes against our desire to remove > headers. It's needed so that the generated SharedWorkerBinding.cpp knows how to downcast the worker MessagePort to MessagePortBase. > @@ +903,5 @@ > > + > > + JS::Rooted<JS::Value> messageData(aCx); > > + if (!mBuffer.read(aCx, messageData.address(), > > + workers::WorkerStructuredCloneCallbacks(aIsMainThread))) { > > + xpc::Throw(aCx, NS_ERROR_DOM_DATA_CLONE_ERR); > > dom::Throw right? xpc::Throw is already used a bunch here. I'll do a global replace in a followup. > @@ +4784,5 @@ > > // Take care of the main argument. > > + if (aHandler) { > > + newInfo->mTimeoutCallable = JS::ObjectValue(*aHandler->Callable()); > > + } > > + else if (!aStringHandler.IsEmpty()) { > > You asserted this above... Is the bindings code guaranteeing this? If so you > should remove the else block below. The assertion is wrong, you can do setTimeout("", N); > @@ +4795,5 @@ > > } > > > > // See if any of the optional arguments were passed. > > + if (aTimeout.WasPassed()) { > > + newInfo->mInterval = TimeDuration::FromMilliseconds(aTimeout.Value()); > > What happens if aTimeout is negative? Excellent question! Fixed. > @@ +5232,1 @@ > > return false; > > Should an exception be set? Well we don't set one today so ... > @@ +5314,5 @@ > > + JSAutoCompartment ac(aCx, global); > > + > > + if (!RegisterBindings(aCx, global)) { > > + // If we destroy globalScope, we will release the WorkerPrivate on the > > + // wrong thread. Just leak. > > How about we proxy this properly instead :) It's actually just wrong. In an older version of the patch WorkerGlobalScope held a strong reference to the WorkerPrivate, but that is no longer the case. Deleted. > ::: dom/workers/WorkerPrivate.h > @@ +782,5 @@ > > > > nsCOMPtr<nsITimer> mTimer; > > nsRefPtr<MemoryReporter> mMemoryReporter; > > > > + nsRefPtrHashtable<nsUint64HashKey, MessagePort> mWorkerPorts; > > Hm, don't you need to tell the CC about this somehow? I don't think so. mWorkerPorts today is a hashtable of effectively rooted ports. Not telling the CC about them has the same effect in the new world. > ::: dom/workers/WorkerScope.cpp > @@ +73,5 @@ > > + MOZ_CRASH("We should never get here!"); > > +} > > + > > +already_AddRefed<WorkerLocation> > > +WorkerGlobalScope::Location() > > I think you should add 'mWorkerPrivate->AssertIsOnWorkerThread();' to pretty > much every method in this file. Done. > @@ +65,5 @@ > > + already_AddRefed<WorkerNavigator> Navigator(); > > + void Close(JSContext* aCx); > > + > > + OnErrorEventHandlerNonNull* GetOnerror(); > > + void SetOnerror(OnErrorEventHandlerNonNull* aHandler); > > Why can't you use the macros here? The macros don't work for onerror because of the weird return type. > @@ +1457,5 @@ > > + ReleaseProxy(XHRIsGoingAway); > > + > > + MOZ_ASSERT(!mRooted); > > + > > + mozilla::DropJSObjects(this); > > Is it safe to call this if you never called HoldJSObjects? Yes. > @@ +1472,5 @@ > > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(XMLHttpRequest, nsXHREventTarget) > > + NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mUpload) > > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END > > + > > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(XMLHttpRequest, nsXHREventTarget) > > Don't you have to unset mStateData.mResponse here? I don't think so. Calling drop js objects seems to clear it. > @@ +1560,4 @@ > > return; > > } > > > > + NS_ADDREF_THIS(); > > This macro is longer than simply calling AddRef(). Much easier to grep ... > @@ +1669,2 @@ > > > > + NS_RELEASE_THIS(); > > How do you know this isn't the last ref? Can you defer it? I failed to convince myself that this is not the last ref, so I moved the release to the end of the function. > @@ +2195,5 @@ > > +XMLHttpRequest::UpdateState(const StateData& aStateData) > > +{ > > + mStateData = aStateData; > > + if (JSVAL_IS_GCTHING(mStateData.mResponse)) { > > + mozilla::HoldJSObjects(this); > > Is it safe to call this more than once? Yes. > ::: dom/workers/XMLHttpRequestUpload.cpp > @@ +11,5 @@ > > > > USING_WORKERS_NAMESPACE > > > > +XMLHttpRequestUpload::XMLHttpRequestUpload(XMLHttpRequest* aXHR) > > + : mXHR(aXHR) > > No indent for : I will note how ludicrous it is that earlier in this review you made me change this the other direction in a different piece of code. We really need to sort our style out. > ::: dom/workers/moz.build > @@ +9,5 @@ > > MODULE = 'dom' > > > > # Public stuff. > > EXPORTS.mozilla.dom += [ > > 'WorkerPrivate.h', > > Can we remove this yet? idk