824864 - Disentangle CompileFunction and EvaluateString{,WithValue} from the script context's active scope

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

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.

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=9d26b4d7b3a3

Comment 3

[Bobby Holley (:bholley)]

Looks green. Uploading patches and flagging bz for review.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Implement nsContentUtils::GetObjectPrincipal. v1

The SSM interface is super awkward.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Hoist the guts of CompileFunction into nsJSUtils. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Move consumers of CompileFunction to the nsJSUtils version, and kill the nsJSContext version. v1

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).

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 4 - Improve the API for EvaluateStringWithValue. v1

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.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 5 - Move existing consumers of EvaluateString over to EvaluateStringWithValue. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 6 - Remove nsIScriptContext::EvaluateString. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 7 - Remove unused optional arguments from nsIScriptContext::ExecuteScript. v1

This lets us get rid of a bunch of junk.

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 8 - Rename EvaluateStringWithValue to EvaluateString. v1

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

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 9 - Use an nsCxPusher in EvaluateString, and pull the principal off the target object. v1

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

Comment 13

[Bobby Holley (:bholley)]

Attachment: Part 10 - Move responsibility for checking for JSVERSION_UNKNOWN to the one caller of EvaluateString that might pass it. v1

Comment 14

[Bobby Holley (:bholley)]

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 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 16

[Masatoshi Kimura [:emk]]

(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 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 {}

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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 21

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 22

[Boris Zbarsky [:bzbarsky]]

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 23

[Boris Zbarsky [:bzbarsky]]

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?

Comment 24

[Bobby Holley (:bholley)]

(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.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> 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.  :(

Comment 26

[Bobby Holley (:bholley)]

(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.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> 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 28

[Boris Zbarsky [:bzbarsky]]

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 29

[Boris Zbarsky [:bzbarsky]]

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 30

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 31

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 32

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 33

[Boris Zbarsky [:bzbarsky]]

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 34

[Boris Zbarsky [:bzbarsky]]

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

Comment 35

[Bobby Holley (:bholley)]

(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 36

[Bobby Holley (:bholley)]

Attachment: Part 2 - Hoist the guts of CompileFunction into nsJSUtils. v2

Comment 37

[Bobby Holley (:bholley)]

Attachment: Part 3 - Move consumers of CompileFunction to the nsJSUtils version, and kill the nsJSContext version. v2

Comment 38

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 39

[Boris Zbarsky [:bzbarsky]]

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.

Comment 40

[Bobby Holley (:bholley)]

(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.

Comment 41

[Bobby Holley (:bholley)]

Attachment: Part 11 - Pass EvaluateString out-param as a pointer, not a reference. v1 r=bz

Comment 42

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=85d748c1f450

Comment 43

[Bobby Holley (:bholley)]

Attachment: Part 12 - Fix Ms2ger nits. r=me

Comment 44

[Bobby Holley (:bholley)]

Looks green. Pushed along with the nit-fix patch:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=3b3c304723cc

Comment 45

[Ed Morley [:emorley]]

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