The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla21

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(12 attachments, 2 obsolete attachments)

3.41 KB, patch
Details | Diff | Splinter Review
15.57 KB, patch
Details | Diff | Splinter Review
9.43 KB, patch
Details | Diff | Splinter Review
9.05 KB, patch
Details | Diff | Splinter Review
7.28 KB, patch
Details | Diff | Splinter Review
11.28 KB, patch
Details | Diff | Splinter Review
2.84 KB, patch
Details | Diff | Splinter Review
5.02 KB, patch
Details | Diff | Splinter Review
5.92 KB, patch
Details | Diff | Splinter Review
12.84 KB, patch
Details | Diff | Splinter Review
9.63 KB, patch
bholley
: review+
Details | Diff | Splinter Review
6.50 KB, patch
bholley
: review+
Details | Diff | Splinter Review
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.
Summary: Hoist script compilation from nsJSEnvironment to nsJSUtils → Disentangle CompileFunction and EvaluateString{,WithValue} from the script context's active scope
https://tbpl.mozilla.org/?tree=Try&rev=9d26b4d7b3a3
Looks green. Uploading patches and flagging bz for review.
Created attachment 696463 [details] [diff] [review]
Part 1 - Implement nsContentUtils::GetObjectPrincipal. v1

The SSM interface is super awkward.
Attachment #696463 - Flags: review?(mrbkap)
Created attachment 696464 [details] [diff] [review]
Part 2 - Hoist the guts of CompileFunction into nsJSUtils. v1
Attachment #696464 - Flags: review?(bzbarsky)
Created attachment 696465 [details] [diff] [review]
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).
Attachment #696465 - Flags: review?(bzbarsky)
Created attachment 696466 [details] [diff] [review]
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.
Attachment #696466 - Flags: review?(mrbkap)
Created attachment 696467 [details] [diff] [review]
Part 5 - Move existing consumers of EvaluateString over to EvaluateStringWithValue. v1
Attachment #696467 - Flags: review?(bzbarsky)
Created attachment 696468 [details] [diff] [review]
Part 6 - Remove nsIScriptContext::EvaluateString. v1
Attachment #696468 - Flags: review?(bzbarsky)
Created attachment 696469 [details] [diff] [review]
Part 7 - Remove unused optional arguments from nsIScriptContext::ExecuteScript. v1

This lets us get rid of a bunch of junk.
Attachment #696469 - Flags: review?(bzbarsky)
Created attachment 696470 [details] [diff] [review]
Part 8 - Rename EvaluateStringWithValue to EvaluateString. v1

Now that there's only one of them, we can get rid of the silly suffix. \o/
Attachment #696470 - Flags: review?(bzbarsky)
Created attachment 696471 [details] [diff] [review]
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.
Attachment #696471 - Flags: review?(bzbarsky)
Created 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
Attachment #696472 - Flags: review?(bzbarsky)
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. ;-)
Attachment #696463 - Flags: review?(mrbkap) → review?(bzbarsky)
Attachment #696466 - Flags: review?(mrbkap) → review?(bzbarsky)
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
Attachment #696463 - Flags: review?(bzbarsky) → review+
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
Attachment #696466 - Flags: review?(bzbarsky) → review+
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.
Attachment #696467 - Flags: review?(bzbarsky) → review+
Comment on attachment 696468 [details] [diff] [review]
Part 6 - Remove nsIScriptContext::EvaluateString. v1

r=me
Attachment #696468 - Flags: review?(bzbarsky) → review+
Comment on attachment 696469 [details] [diff] [review]
Part 7 - Remove unused optional arguments from nsIScriptContext::ExecuteScript. v1

r=me
Attachment #696469 - Flags: review?(bzbarsky) → review+
Comment on attachment 696470 [details] [diff] [review]
Part 8 - Rename EvaluateStringWithValue to EvaluateString. v1

r=me
Attachment #696470 - Flags: review?(bzbarsky) → review+
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
Attachment #696471 - Flags: review?(bzbarsky) → review+
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
Attachment #696472 - Flags: review?(bzbarsky) → review+
Attachment #696464 - Flags: review?(bzbarsky) → review-
Attachment #696465 - Flags: review?(bzbarsky) → review-
(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.
Created attachment 702918 [details] [diff] [review]
Part 2 - Hoist the guts of CompileFunction into nsJSUtils. v2
Attachment #696464 - Attachment is obsolete: true
Attachment #702918 - Flags: review?(bzbarsky)
Created attachment 702919 [details] [diff] [review]
Part 3 - Move consumers of CompileFunction to the nsJSUtils version, and kill the nsJSContext version. v2
Attachment #696465 - Attachment is obsolete: true
Attachment #702919 - Flags: review?(bzbarsky)
Comment on attachment 702918 [details] [diff] [review]
Part 2 - Hoist the guts of CompileFunction into nsJSUtils. v2

r=me
Attachment #702918 - Flags: review?(bzbarsky) → review+
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.
Attachment #702919 - Flags: review?(bzbarsky) → review+
(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.
Created attachment 702942 [details] [diff] [review]
Part 11 - Pass EvaluateString out-param as a pointer, not a reference. v1 r=bz
Attachment #702942 - Flags: review+
https://tbpl.mozilla.org/?tree=Try&rev=85d748c1f450
Created attachment 703084 [details] [diff] [review]
Part 12 - Fix Ms2ger nits. r=me
Attachment #703084 - Flags: review+
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Blocks: 832041
Depends on: 832435

Updated

4 years ago
Depends on: 832599
Depends on: 832646
Depends on: 832986
Depends on: 833856
Depends on: 834785
Blocks: 826392
You need to log in before you can comment on or make changes to this bug.