Right now, script compilation goes through nsIScriptContext, which implicitly passes in the associated cx along with some other things. This makes it hard to compile scripts when you don't have an nsIScriptContext, and generally makes the JSContext more important than it should be. I'm going to hoist the functionality into static methods on nsJSUtils, and have the nsJSEnvironment version call them.

So, as some context - my goal in writing these patches was to allow the XBL scope (in bug 821850) to properly evaluate field strings in its own scope while generally being a different agent from the content scope (in particular, having a different principal). This is a problem because the XBL scope doesn't have its own nsIScriptContext, and using the one associated with the content was causing principal mismatches and such. So my initial goal was to move all of the compile/evaluate/execute junk out of nsJSEnvironment and into static methods on nsJSUtils. This is general a noble goal (given that the non-static aspects of nsJSContext are the primary barrier to single-cxing the DOM), and was easy to do for CompileFunction. However, for other stuff, it turned out to be more involved, largely because of all the junk that goes on in there (termination functions, microtasks, cx pushing, etc). This will have to happen at some point, but I don't want it on my critical path. So I scaled back my approach, and instead opted to clean up a lot of the crap that goes on in EvaluateString, consolidate the function, and finally divorce it from using anything related to the associated DOM window / scope. This is sufficient for my XBL work, and is a nice step in the right direction.

Looks green. Uploading patches and flagging bz for review.

The SSM interface is super awkward.

Note that the three consumers were all XBL, and were all passing aShared = true, which had the effect of passing null for the target object. So we actually want to pass JS::NullPtr() (the HandleObject version of nullptr) instead of aClassObject in order to maintain the old behavior (whatever that is).

There are a few changes we make here: 1 - Having callers pass JS::CompileOptions directly. 2 - Removing aUndefined, which makes no sense and is unused by consumers. 3 - Making aScopeObject and aRetValue non-optional, via references. 3 - Adding an argument to optionally coerce the return value to a string. Coercing jsvals to strings is the reason we currently have two nearly-identical functions, EvaluateString and EvaluateStringWithValue, since the coercion can trigger arbitrary script and thus needs to be bracketed by all the junk that nsJSContext does. However, if callers can be guaranteed that the return value will be a bonafide string, then they can easily extract the string themselves if they so desire. This will allow us to combine the two functions.

This lets us get rid of a bunch of junk.

Now that there's only one of them, we can get rid of the silly suffix. \o/

This simplifies a lot of code, and makes the function scope-agnostic.

Comment on attachment 696463 [details] [diff] [review] Part 1 - Implement nsContentUtils::GetObjectPrincipal. v1 Whoops, I meant bz. Muscle memory from uploading a lot of patches. ;-)

Comment on attachment 696464 [details] [diff] [review] Part 2 - Hoist the guts of CompileFunction into nsJSUtils. v1 Review of attachment 696464 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsJSUtils.cpp @@ +158,5 @@ > + const nsAString& aBody, > + JSObject** aFunctionObject) > +{ > + > + MOZ_ASSERT(!aTarget || js::IsObjectInContextCompartment(aTarget, aCx)); MOZ_ASSERT_IF(aTarget, js::IsObjectInContextCompartment(aTarget, aCx)); ? @@ +164,5 @@ > + > + // Since aTarget and aCx are same-compartment, there should be no distinction > + // between the object principal and the cx principal. > + // However, aTarget may be null in the wacky aShared case. So use the cx. > + JSPrincipals *p = JS_GetCompartmentPrincipals(js::GetContextCompartment(aCx)); * goes on the left here @@ +178,5 @@ > + aArgCount, aArgArray, > + PromiseFlatString(aBody).get(), > + aBody.Length()); > + if (!fun) > + return NS_ERROR_FAILURE; NS_ENSURE_TRUE, I'd say. Or braces.

(In reply to :Ms2ger from comment #15) > > + if (!fun) > > + return NS_ERROR_FAILURE; > > NS_ENSURE_TRUE, I'd say. Or braces. We shouldn't add new NS_ENSURE_* macros anymore.

Comment on attachment 696466 [details] [diff] [review] Part 4 - Improve the API for EvaluateStringWithValue. v1 Review of attachment 696466 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsJSEnvironment.cpp @@ +1303,5 @@ > + js::RootedObject rootedScope(mContext, &aScopeObject); > + ok = JS::Evaluate(mContext, rootedScope, aOptions, > + PromiseFlatString(aScript).get(), > + aScript.Length(), &aRetValue); > + if (ok && !JSVAL_IS_VOID(aRetValue) && aCoerceToString) { !aRetValue.isUndefined() @@ +1306,5 @@ > + aScript.Length(), &aRetValue); > + if (ok && !JSVAL_IS_VOID(aRetValue) && aCoerceToString) { > + JSString* str = JS_ValueToString(mContext, aRetValue); > + ok = !!str; > + aRetValue = ok ? JS::StringValue(str) : JSVAL_VOID; UndefinedValue(), please

Comment on attachment 696467 [details] [diff] [review] Part 5 - Move existing consumers of EvaluateString over to EvaluateStringWithValue. v1 Review of attachment 696467 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsScriptLoader.cpp @@ +854,5 @@ > nsContentUtils::GetWrapperSafeScriptFilename(mDocument, aRequest->mURI, url); > > + JS::CompileOptions options(context->GetNativeContext()); > + options.setFileAndLine(url.get(), aRequest->mLineNo) > + .setVersion(JSVersion(aRequest->mJSVersion)); Could you file a followup to make nsScriptLoadRequest keep a JSVersion? @@ +856,5 @@ > + JS::CompileOptions options(context->GetNativeContext()); > + options.setFileAndLine(url.get(), aRequest->mLineNo) > + .setVersion(JSVersion(aRequest->mJSVersion)); > + if (aRequest->mOriginPrincipal) > + options.setOriginPrincipals(nsJSPrincipals::get(aRequest->mOriginPrincipal)); {} ::: dom/src/jsurl/nsJSProtocolHandler.cpp @@ +254,5 @@ > > useSandbox = !subsumes; > } > > + JS::Value v = JSVAL_VOID; UndefinedValue() @@ +351,5 @@ > rv = NS_ERROR_DOM_RETVAL_UNDEFINED; > } > else { > + nsDependentJSString result; > + if (!result.init(cx, JSVAL_TO_STRING(v))) { This can just be init(cx, v).

Comment on attachment 696469 [details] [diff] [review] Part 7 - Remove unused optional arguments from nsIScriptContext::ExecuteScript. v1 Review of attachment 696469 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsJSEnvironment.cpp @@ -1335,5 @@ > -// exception flags if the conversion fails. > -static nsresult > -JSValueToAString(JSContext *cx, jsval val, nsAString *result, > - bool *isUndefined) > -{ \o/ @@ +1434,1 @@ > ReportPendingException(); Little consistency here, but {}

(In reply to Masatoshi Kimura [:emk] from comment #16) > (In reply to :Ms2ger from comment #15) > > > + if (!fun) > > > + return NS_ERROR_FAILURE; > > > > NS_ENSURE_TRUE, I'd say. Or braces. > > We shouldn't add new NS_ENSURE_* macros anymore. You are mistaken. That rule only applies to code bsmedberg owns.

Comment on attachment 696463 [details] [diff] [review] Part 1 - Implement nsContentUtils::GetObjectPrincipal. v1 r=me

Comment on attachment 696464 [details] [diff] [review] Part 2 - Hoist the guts of CompileFunction into nsJSUtils. v1 So this actually changes the principal used, from that of the global to that of aTarget. Furthermore, for the cases when aTarget is null, this uses whatever compartment happened to be current on aCx for the principal. Why is this change ok? It seems pretty fishy to me...

Comment on attachment 696465 [details] [diff] [review] Part 3 - Move consumers of CompileFunction to the nsJSUtils version, and kill the nsJSContext version. v1 The copy/paste of the jsoptions bits is rather annoying. Can we make that better somehow?

(In reply to Boris Zbarsky (:bz) from comment #22) > Comment on attachment 696464 [details] [diff] [review] > Part 2 - Hoist the guts of CompileFunction into nsJSUtils. v1 > > So this actually changes the principal used, from that of the global to that > of aTarget. Anything else would be nonsensical. The cx must be in the compartment of the target object, because we'd otherwise hit a compartment mismatch. And because the principal is computed from the compartment, having a script whose principals don't match those of its compartment is totally crazy. In general, I think the sanest thing is for the caller to enter a compartment, and for all descendant compilation to use that compartment and its principals. > Furthermore, for the cases when aTarget is null, this uses whatever > compartment happened to be current on aCx for the principal. Yeah, looks like I messed that up. Here's what I propose: (1) In patch 2, enter the compartment of the target in nsJSContext::CompileFunction, falling back on the global. (2) In patch 3, require that the cx already be in the right compartment (and in a request), and hoist that stuff into the 2 XBL callers. (In reply to Boris Zbarsky (:bz) from comment #23) > The copy/paste of the jsoptions bits is rather annoying. Can we make that > better somehow? Discussed on IRC.

> Anything else would be nonsensical. Can we possibly add asserts that nonsensical things are not happening? > In general, I think the sanest thing is for the caller to enter a compartment Entering a compartment is almost never the sane thing to do for an API consumer. People just sprinkle it on like black magic. In this particular case, the relationship between the scope object and the resulting script is rather tenuous... I can see requiring that the scope object be wrapped into the compartment whose principal you want to execute with, but it seems really really fragile. :( > Here's what I propose: I guess I can live with that... Can we push down the compartment-entering and request stuff into the shared code, though? I'd really rather not have it proliferate all over. :(

(In reply to Boris Zbarsky (:bz) from comment #25) > > Anything else would be nonsensical. > > Can we possibly add asserts that nonsensical things are not happening? We have them elsewhere, for EvaluateString and stuff, and they haven't fired. I don't think CompileFunction is too big of a deal, because there are only two callers and we know what they do. I filed bug 830855 on removing JSScript::principals. > > In general, I think the sanest thing is for the caller to enter a compartment > > Entering a compartment is almost never the sane thing to do for an API > consumer. People just sprinkle it on like black magic. > > In this particular case, the relationship between the scope object and the > resulting script is rather tenuous... I can see requiring that the scope > object be wrapped into the compartment whose principal you want to execute > with, but it seems really really fragile. :( Yeah. The weirdness is really that both of the two callers pass aShared=true, meaning that we have nothing to infer the compartment from other than the cx. > I guess I can live with that... Can we push down the compartment-entering > and request stuff into the shared code, though? I'd really rather not have > it proliferate all over. :( Well, we could except for the aShared issue. We could also do the old thing, which is to pass a scope objects whose sole purpose is to indicate the compartment (since we otherwise null it out before passing it into JSAPI due to aShared). But that doesn't buy us much IMO, because the caller still probably has to be worrying about compartments because they're manipulating JS objects enough to be passing the scope object. So given that the callers are already working with raw JS objects, I'd rather just inherit their semantics, and assert that the compartment matches the scope object if one is provided.

> because there are only two callers Ah, fair. OK, let's do what comment 24 suggests and then I'll review these two parts. Moving on with the rest for now.

Comment on attachment 696466 [details] [diff] [review] Part 4 - Improve the API for EvaluateStringWithValue. v1 >+++ b/dom/base/nsIScriptContext.h >+ JS::Value& aRetValue) = 0; I think we should keep the out param a Value* for now (to become a MutableHandleValue or whatever once the jsapi folks get around to it). That's how all Value outparams work, and this one should not be different, imo. >+++ b/dom/base/nsJSEnvironment.cpp > nsJSContext::EvaluateStringWithValue(const nsAString& aScript, > nsCOMPtr<nsIScriptObjectPrincipal> objPrincipal = do_QueryInterface(GetGlobalObject()); Why not just get the principal off aScopeObject and be done with it? And perhaps assert that the two match? Why not keep the conversion to nsAString in here, as opposed to having callers have to deal with it? You could keep passing in a null nsAString* if no conversion is desired.... r=me with those addressed

Comment on attachment 696467 [details] [diff] [review] Part 5 - Move existing consumers of EvaluateString over to EvaluateStringWithValue. v1 >+++ b/dom/src/jsurl/nsJSProtocolHandler.cpp Why do you need the inner JSAutoRequest in the useSandbox case? I guess this is the only consumer of the string coercion stuff, and it has to deal with the sandbox case anyway, hence the API in the previous patch? r=me with the extra JSAutoRequest nuked.

Comment on attachment 696468 [details] [diff] [review] Part 6 - Remove nsIScriptContext::EvaluateString. v1 r=me

Comment on attachment 696469 [details] [diff] [review] Part 7 - Remove unused optional arguments from nsIScriptContext::ExecuteScript. v1 r=me

Comment on attachment 696470 [details] [diff] [review] Part 8 - Rename EvaluateStringWithValue to EvaluateString. v1 r=me

Comment on attachment 696471 [details] [diff] [review] Part 9 - Use an nsCxPusher in EvaluateString, and pull the principal off the target object. v1 r=me

Comment on attachment 696472 [details] [diff] [review] Part 10 - Move responsibility for checking for JSVERSION_UNKNOWN to the one caller of EvaluateString that might pass it. v1 r=me

(In reply to Boris Zbarsky (:bz) from comment #28) > I think we should keep the out param a Value* for now (to become a > MutableHandleValue or whatever once the jsapi folks get around to it). > That's how all Value outparams work, and this one should not be different, > imo. I'll do this as a followup on top, as discussed on IRC. > >+++ b/dom/base/nsJSEnvironment.cpp > > nsJSContext::EvaluateStringWithValue(const nsAString& aScript, > > nsCOMPtr<nsIScriptObjectPrincipal> objPrincipal = do_QueryInterface(GetGlobalObject()); > > Why not just get the principal off aScopeObject and be done with it? That happens in part 9.

Comment on attachment 702918 [details] [diff] [review] Part 2 - Hoist the guts of CompileFunction into nsJSUtils. v2 r=me

Comment on attachment 702919 [details] [diff] [review] Part 3 - Move consumers of CompileFunction to the nsJSUtils version, and kill the nsJSContext version. v2 When compiling mSetterText, don't you need to enter a request and a compartment? You need to change the nsIScriptContext IID. r=me with those fixed.

(In reply to Boris Zbarsky (:bz) from comment #39) > Comment on attachment 702919 [details] [diff] [review] > Part 3 - Move consumers of CompileFunction to the nsJSUtils version, and > kill the nsJSContext version. v2 > > When compiling mSetterText, don't you need to enter a request and a > compartment? Good catch, thanks. > You need to change the nsIScriptContext IID. Sure. That was in part 4, but hoisting it to part 3 is probably better.

Looks green. Pushed along with the nit-fix patch: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=3b3c304723cc

https://hg.mozilla.org/mozilla-central/rev/e51150a044d7 https://hg.mozilla.org/mozilla-central/rev/33d6b596f5d2 https://hg.mozilla.org/mozilla-central/rev/533bc8e4981b https://hg.mozilla.org/mozilla-central/rev/c5fb603dfa84 https://hg.mozilla.org/mozilla-central/rev/3cb7ad47f6d9 https://hg.mozilla.org/mozilla-central/rev/22dc6532b40b https://hg.mozilla.org/mozilla-central/rev/badfbc904df6 https://hg.mozilla.org/mozilla-central/rev/43458e543877 https://hg.mozilla.org/mozilla-central/rev/b40a217c85e3 https://hg.mozilla.org/mozilla-central/rev/53469a0e1ddd https://hg.mozilla.org/mozilla-central/rev/de4229773698 https://hg.mozilla.org/mozilla-central/rev/3b3c304723cc