bz has thoughts here.

What we need here is the following: 1) Make CallSetup work on workers. In the constructor we can just check for main thread and if not: A) Avoid calling nsContentUtils::GetSafeJSContext: get the JSContext some other way. B) Avoid calling mCxPusher.Push. C) Avoid doing the security check. Not sure whether the destructor needs changes or whether the Pop() is a safe no-op if we never did Push(). 2) Assert that CallbackObjectHolderBase::ToXPCOMCallback is only called on the main thread. Similar for ToWebIDLCallback. Maybe we should just assert if CallbackObjectHolder is ever constructed off the main thread? That might be tough for events, though... 3) Make the this-object wrapping work on workers. Right now we use WrapCallThisObject, which typically calls WrapNativeParent. This might already be safe on workers, if all those objects are nsWrapperCache and WebIDL objects. If not, we just need to make it work.

Adds GetThreadSafeJSContext() to get the right JSContext based on main thread or worker thread. In the call chain of WrapCallThisObject, tries to first unwrap the object assuming it supports wrappercache, when not on the main thread.

https://tbpl.mozilla.org/?tree=Try&rev=a24bb4b934ea Try run with this patch applied on top of khuey's event target patches, and a patch to port Promises to workers.

Slightly updated patch to fix build bustage on Mac.

Comment on attachment 799636 [details] [diff] [review] Get WebIDL callbacks working on Workers. Review of attachment 799636 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good. r- because I asked you to rework the control flow and I'd like to read that again. I would be interested to know what bz thinks about the inlining in NativeInterface2JSObjectThrow.. and about the exception handling. ::: content/base/public/nsContentUtils.h @@ +1533,5 @@ > nsresult* aRv); > > static JSContext *GetCurrentJSContext(); > static JSContext *GetSafeJSContext(); > + static JSContext *GetThreadSafeJSContext(); Name this ThreadSafeGetSafeJSContext. All of the alternatives are far worse (especially GetThreadSafeSafeJSContext). ::: dom/bindings/BindingUtils.cpp @@ +634,5 @@ > const nsIID* aIID, > bool aAllowNativeWrapper) > { > nsresult rv; > + if (!NS_IsMainThread()) { I think we should inline this bit regardless of what thread we're on. ::: dom/bindings/CallbackObject.cpp @@ +56,3 @@ > > + // First, find the real underlying callback. > + JSObject* realCallback = js::UncheckedUnwrap(aCallback); I would prefer that we consolidated more of this, although that probably requires more complicated control flow. Something like bool isMainThread = NS_IsMainThread(); if (isMainThread) { // microtask } JSObject* realCallback ... if (isMainThread) { // Get context one way } else { // Get context the other way } xpc_UnmarkGray mRooted.construct if (isMainThread) { // securitah check } construct autocompartment set mCx, etc @@ +128,5 @@ > + // Make sure the JS engine doesn't report exceptions we want to re-throw > + if (mExceptionHandling == eRethrowExceptions) { > + mSavedJSContextOptions = JS_GetOptions(cx); > + JS_SetOptions(cx, mSavedJSContextOptions | JSOPTION_DONT_REPORT_UNCAUGHT); > + } I'm 99% sure we want this on workers too. ::: dom/workers/WorkerPrivate.h @@ +990,5 @@ > bool > IsCurrentThreadRunningChromeWorker(); > > +JSContext * > +GetCurrentThreadJSContext(); fwiw, I have basically the same thing in one of my patches

Reworked conditional and other changes as per last review.

> I would be interested to know what bz thinks about the inlining in > NativeInterface2JSObjectThrow Funny you should ask. So that code will handle the case when the thisobj thing passed in does not inherit from nsWrapperCache but does QI to it. I guess that will actually happen in event code.... I'm OK with I guess, except the code being inlined has some problems. Specifically: 1) The comment talks about IsProxy() when it means IsDOMBinding(). Fix that in XPCConvert too? 2) "flat" can be declared inside the "if (cache)" block. In fact inside the IsDOMBinding() block. And maybe call it "obj"? 3) You don't need the "global" temporar. Just pass aScope to WrapObject. 4) s/OBJECT_TO_JSVAL(flat)/JS::ObjectValue(*obj). 5) If !NS_IsMainThread, you need to actually throw an exception on aCx, not just return false, no? > Name this ThreadSafeGetSafeJSContext. We have ThreadsafeGetCurrentJSContext (note lowercase "safe"). We should pick one spelling of "safe" and stick with it... > bool isMainThread = NS_IsMainThread(); Store it in a bool member, so you don't have to NS_IsMainThread a second time in the dtor?

"ThreadSafeGetSafeJSContext" is a misnomer - there's no such thing as the SafeJSContext OMT. The caller should demultiplex the cases, or we should have a helper method with a more specific name like "GetContextForCallbacks" or something.

Updated per bz's comments and renamed ThreadSafeGetSafeJSContext() to GetDefaultJSContextForThread() per IRC discussion with bholley.

Comment on attachment 801034 [details] [diff] [review] Get WebIDL callbacks working on Workers. Review of attachment 801034 [details] [diff] [review]: ----------------------------------------------------------------- \o/ ::: content/base/src/nsContentUtils.cpp @@ +1763,5 @@ > namespace mozilla { > namespace dom { > namespace workers { > extern bool IsCurrentThreadRunningChromeWorker(); > +extern JSContext * GetCurrentThreadJSContext(); nit: JSContext* @@ +5198,5 @@ > /* static */ > +JSContext * > +nsContentUtils::GetDefaultJSContextForThread() > +{ > + if (NS_IsMainThread()) { if (MOZ_LIKELY(NS_IsMainThread())) ::: dom/bindings/BindingUtils.cpp @@ +635,5 @@ > bool aAllowNativeWrapper) > { > nsresult rv; > + // First, see if this object supports the wrapper cache. > + // Note: If |cache->IsDOMBinding()| is true, then it means that the object This comment is true, but vacuous, since cache->IsDOMBinding() is the only case we care about here. I'd just write that we're inlining the part of XPCConvert::NativeInterfaceToJSObject that we need on all threads. @@ +643,5 @@ > + // object will create (and fill the cache) from its WrapObject call. > + nsWrapperCache *cache = aHelper.GetWrapperCache(); > + > + if (cache) { > + if (cache->IsDOMBinding()) { collapse the two if statements. @@ +660,5 @@ > + } > + } > + } > + > + // On workers, don't attempt actions that require main thread. This should just be a crash, imo. ::: dom/bindings/CallbackObject.cpp @@ +98,5 @@ > + > + // Make sure our JSContext is pushed on the stack. > + mCxPusher.Push(cx); > + } else { > + cx = nsContentUtils::GetDefaultJSContextForThread(); I think you should just call the worker function directly since you already know you aren't on the main thread. @@ +99,5 @@ > + // Make sure our JSContext is pushed on the stack. > + mCxPusher.Push(cx); > + } else { > + cx = nsContentUtils::GetDefaultJSContextForThread(); > + xpc_UnmarkGrayContext(cx); xpc_UnmarkGrayContext is unnecessary and gone since bug 910937. @@ +183,5 @@ > mCxPusher.Pop(); > + > + if (mIsMainThread) { > + nsContentUtils::LeaveMicroTask(); > + } Please add a comment that is important that this be last (after we leave the compartment and pop the context). ::: dom/bindings/CallbackObject.h @@ +152,5 @@ > // we should re-throw them. > ErrorResult& mErrorResult; > const ExceptionHandling mExceptionHandling; > uint32_t mSavedJSContextOptions; > + bool mIsMainThread; super-nit: const bool ::: dom/workers/RuntimeService.cpp @@ +1127,5 @@ > { > NS_ASSERTION(!NS_IsMainThread(), "Wrong thread!"); > CycleCollectedJSRuntime* ccrt = nsCycleCollector_currentJSRuntime(); > if (!ccrt) { > + return NULL; nullptr.