This patch is needed for what we discuss about C++ wrapper to support DataStore API on the worker.

Comment on attachment 8378980 [details] [diff] [review] global.patch > class PromiseResolverMixin What keeps the mGlobal member alive? Something needs to do that. >@@ -484,18 +481,19 @@ Promise::Constructor(const GlobalObject& >+ JS::Rooted<JSObject*> global(cx, JS::CurrentGlobalOrNull(cx)); >+ JSAutoCompartment ac(cx, global); Why would "global" be in the same compartment as "value" here? I see no guarantee that it would be. Maybe what we should do is just go ahead and JS_WrapValue the value into the current compartment of cx here. > CountdownHolder(const GlobalObject& aGlobal, Promise* aPromise, uint32_t aCountdown) With this change, aGlobal is unused, no? But also, what guarantees that the same global will be passed to SetValue for all indices? This patch certainly seems to be depending on that happening, but I don't see why that would be the case. We really need to write some evil testcases here involving promises from multiple compartments and methods from compartments that don't match the promises... In any case, probably the right behavior here is actually to create the result array in the right compartment (Which one? The spec doesn't say, but should; known spec bug. Just doing it in the compartment we do it in now is fine.) and then wrapping the values we plan to put into the array into the array's compartment. We already entered that compartment, but then we didn't wrap the values into it. >@@ -985,77 +985,81 @@ Promise::ResolveInternal(JSContext* aCx, Nothing guarantees that "exn" is in the compartment of aCx here. Need to JS_WrapValue into that compartment, assuming aCx is in a sane compartment here. Alternately, need to enter the compartment of mGlobal on aCx and wrap into _that_ compartment. Or something. Really, the only place we should be doing CurrentGlobalOrNull() is when we have a value we expect to be same-compartment with cx right now and want the corresponding global.

Bobby, who's a good person to write some evil tests here?

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #3) > Bobby, who's a good person to write some evil tests here? Baku, I would think, with appropriate guidance and review. ;-)

Kyle did something interesting at bug 974120, which might help keep the mGlobal here.

I have a few questions about this patch. I don't know if what I'm doing is right. Let's talk on IRC.

Comment on attachment 8402610 [details] [diff] [review] global.patch The Maybe<JSAutoCompartment> in Constructor is dead code now. We should document in Constuctor that we basically want the same behavior as this JS implementation: function Promise(arg) { try { arg(a, b); } catch (e) { this.reject(e); } } and that that implies wrapping the exception into the compartment the constructor call is happening in. Also worth adding an assert that at that point cx and aGlobal.Get() are same-compartment; if that ever changes for Xrays we'll need to decide which one to wrap into. We should likewise document in the CountdownHolder constructor that the only time aGlobal.GetContext() and aGlobal.Get() are not same-compartment is when we're called via Xrays, and in that situation we in fact want to create the array in the callee compartment (that is, that of aGlobal.Get()). >+ !JS_DefineElement(cx, mValues, index, aValue, nullptr, nullptr, >+ JSPROP_ENUMERATE)) { s/aValue/value/ here, right? Could use a test that would have caught that. >@@ -992,18 +989,20 @@ Promise::ResolveInternal(JSContext* aCx, I think I've convinced myself that JS_WrapValue here is correct. Just MOZ_CRASH if that fails, perhaps? Just going ahead with aCx and exn as-is if JS_WrapValue fails seems like a bad idea. > ResolvePromiseCallback::Call(JS::Handle<JS::Value> aValue) >+ if (!JS_WrapValue(cx, &value)) { This part I think is wrong. ThreadsafeAutoSafeJSContext is in no particular compartment, so this should actually crash at runtime at least on mainthread... What we should be doing instead is that we should be saving an object in ResolvePromiseCallback that tells us what compartment to enter. In the case of Promise::Race, that can probably just be JS::CurrentGlobalOrNull(aCx). In the case of Promise::Then, I'm not sure. Maybe that method should simply request a JSContext from the bindings and use that? The other option would be to enter the compartment of mPromise->GetOrCreateWrapper before doing the call and wrap the value into that compartment. But I would prefer it if we propagated the caller compartment instead. Similar for RejectPromiseCallback and WrapperPromiseCallback.

Comment on attachment 8402744 [details] [diff] [review] global.patch >+ MOZ_ASSERT(ok); No, please actually MOZ_CRASH if !ok. >+ NS_WARNING("Failed to wrap value in a context."); Maybe s/in a context/into the right compartment/? This happens several times. This is all good, except one thing: you need to trace mGlobal!

Comment on attachment 8402801 [details] [diff] [review] global.patch This is still missing the following: 1) NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS in the traverse methods, so your trace bit will actually be called. 2) Something in unlink that nulls out mGlobal. 3) HoldJSObjects/DropJSObjects calls.

https://tbpl.mozilla.org/?tree=Try&rev=ddb0e9acf304 green on try.

Comment on attachment 8403123 [details] [diff] [review] global.patch Great, thanks!

Backed out for build failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=37494061&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=37493494&tree=Mozilla-Inbound remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ddea7fdd09bf

https://hg.mozilla.org/integration/mozilla-inbound/rev/84a40e23911c Merge problem. Landing it again.

\^O^/ Thanks Andrea so much! Will test if this works for bug 949325.