1089026 - Stop allowing random objects on function scope chains

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Once bug 1088228 is fixed, we can change JSAPI to not allow cloning or compilation of a function with a random scope chain; only With objects and globals.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Eliminate the "parent" argument to JS_CloneFunctionObject to make callers use the scopeChain version if they want something other than the global

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Eliminate the ability to provide a non-global parent object to JS::CompileFunction and company except via the scopeChain API

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Eliminate the ability to provide a non-global parent object to JS::CompileFunction and company except via the scopeChain API

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Eliminate the ability to provide a non-global parent object to JS::CompileFunction and company except via the scopeChain API

Comment 5

[Boris Zbarsky [:bzbarsky]]

Something about part 2 causes every single test on "B2G ICS Emulator Opt" (but _only_ that platform) to go orange...  Looking into what that something is.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ah, I bet I know the issue.  Need to use a funky scope chain in the subscript loader.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Refactor the CompileFunction code to allow cleanly separating whether we're passing in an object to define the function on or a scope chain. This change should not cause any behavior changes

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Change the CompileFunction calls in the subscript loader to pass in their desired scope chains

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Eliminate the ability to provide a non-global parent object to JS::CompileFunction and company except via the scopeChain API

This isThis is the last bit: the non-backwards-compatible API change.  If this sticks, can we also get rid of the defineOnScope bit in the CompileOptions?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Also, please let me know if I need to get someone else to review the API change bits here...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Change the CompileFunction calls in the subscript loader and component loader to pass in their desired scope chains

Comment 12

[Shu-yu Guo [:shu]]

Comment on attachment 8511779 [details] [diff] [review]
part 2.  Refactor the CompileFunction code to allow cleanly separating whether we're passing in an object to define the function on or a scope chain.  This change should not cause any behavior changes

Review of attachment 8511779 [details] [diff] [review]:
-----------------------------------------------------------------

Nice API cleanup.

::: js/src/jsapi.cpp
@@ +4641,5 @@
>      if (!frontend::CompileFunctionBody(cx, fun, options, formals, srcBuf,
>                                         enclosingStaticScope))
>          return false;
>  
> +    if (objToDefineOn && funAtom && options.defineOnScope) {

Now that objToDefineOn is separated out from the scope chain object, can we do away with options.defineOnScope altogether?

Comment 13

[Shu-yu Guo [:shu]]

Comment on attachment 8511781 [details] [diff] [review]
part 4.  Eliminate the ability to provide a non-global parent object to JS::CompileFunction and company except via the scopeChain API

Review of attachment 8511781 [details] [diff] [review]:
-----------------------------------------------------------------

Are the callers of this function all changed to anticipate this change elsewhere?

Comment 14

[Shu-yu Guo [:shu]]

Comment on attachment 8511447 [details] [diff] [review]
part 1.  Eliminate the "parent" argument to JS_CloneFunctionObject to make callers use the scopeChain version if they want something other than the global

Review of attachment 8511447 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the js/src/ parts.

::: js/src/jsapi.cpp
@@ +3870,5 @@
>      staticScopeObj.set(staticEnclosingScope);
>      return true;
>  }
>  
> +static JSObject*

Nit: SM style is JSObject * still.

@@ +3871,5 @@
>      return true;
>  }
>  
> +static JSObject*
> +CloneFunctionObject(JSContext *cx, HandleObject funobj, HandleObject parentArg)

Rename parentArg to dynamicScope for consistency please.

@@ +3876,2 @@
>  {
>      RootedObject parent(cx, parentArg);

Remove this re-root.

::: js/src/jsapi.h
@@ +3497,4 @@
>  /*
>   * Clone a top-level function into a new scope. This function will dynamically
> + * fail if funobj was lexically nested inside some other function.  The scope to
> + * clone into is provided by the current global of cx.

Nit: I think the wording would be clearer if the last sentence is removed and the first sentence changed to something like "Clone a top-level function into cx's global."

::: js/src/shell/js.cpp
@@ +2439,5 @@
>      } else {
>          parent = JS_GetParent(&args.callee());
>      }
>  
> +    // Sdould it worry us that we might be getting with wrappers

Typo.

@@ +2443,5 @@
> +    // Sdould it worry us that we might be getting with wrappers
> +    // around with wrappers here?
> +    JS::AutoObjectVector scopeChain(cx);
> +    if (!parent->is<GlobalObject>() &&
> +        !scopeChain.append(parent))

Nit: either put both conjuncts on one line or use { } like:

if (cond1 &&
    cond2)
{
     ...
}

Comment 15

[Boris Zbarsky [:bzbarsky]]

> can we do away with options.defineOnScope altogether?

That's what I asked in comment 9.

> Are the callers of this function all changed to anticipate this change elsewhere?

That's the theory.

In practice, part 3 is still orange on try on b2g opt ICS emulator only, for reasons I'm still trying to figure out.  Even without part 4...

And of course non-Gecko consumers of this API would need to be updated.  Maybe I should change the argument order around to force them to do that or something.

> Nit: SM style is JSObject * still.

Done.

> Rename parentArg to dynamicScope for consistency please.

Ok, but should I leave "parent"?

> Remove this re-root.

Can't; it gets written below like so:

    if (!parent)
        parent = cx->global();

> Nit: I think the wording would be clearer

Done.

> Typo.

Fixed.

> Nit: either put both conjuncts on one line or use { } like:

Put on one line.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8511809 [details] [diff] [review]
part 3.  Change the CompileFunction calls in the subscript loader and component loader to pass in their desired scope chains

OK, this is not good enough.

And the reason it's not good enough is that in the new setup the function ends up with an environment based on scopeChain but with the global as parent.  That happens because NewFunctionWithProto() does SkipScopeParent() when calling NewObjectWithClassProto.  It does use the passed-in dynamic scope when calling fun->initEnvironment.

It looks like there are two or three places in Gecko where we use the function parent in an interesting way.  One is XrayAwareCalleeGlobal, but that shouldn't be affected by skipping scope objects, since it just wants the global anyway, and in any case the functions its working on don't go through the new compilation APIs.  Another is mozJSComponentLoader::FindTargetObject, which looks for a FakeBackstagePass as the function parent.  This is now failing, since it gets the actual BacstagePass global instead.  Finally, there's FixUpThisIfBroken in XPCWrappedNativeJSOps, but again that shouldn't end up getting called on compiled functions.

So basically, we need to make FindTargetObject work.

I could expose friend API for calling fun->environment(), I guess.  That will return the thing that actually got passed in.... which is a With wrapper around the FakeBackstagePass.  It's possible to punch through that in my API too, of course; it'll be a weird puppy of an API, but doable.

Alternately, would the world explode if I just JS_SetParent on the function after compiling it, with nice long comments explaining why?  Does that cause deoptimization or anything?  It shouldn't affect the scope chain, right?

Comment 17

[Boris Zbarsky [:bzbarsky]]

I guess the other question is why the parent is skipping the Scope stuff and whether we actually want that in this case.

Comment 18

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #16)
> I could expose friend API for calling fun->environment(), I guess.  That
> will return the thing that actually got passed in.... which is a With
> wrapper around the FakeBackstagePass.  It's possible to punch through that
> in my API too, of course; it'll be a weird puppy of an API, but doable.

Yes, let's just do that. It's not all that much weirder than js::GetOutermostEnclosingFunctionOfScriptedCaller, which we'll be invoking right above.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Add a friend API for getting the scope object for a function, if it has one

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Change the CompileFunction calls in the component loader and subscript loader to pass in their desired scope chains

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Eliminate the ability to provide a non-global parent object to JS::CompileFunction and company except via the scopeChain API

Jason, could you review the really substantive API change (as opposed to addition) here?  Please let me know if you want me to make this one obvious to consumers by reordering arguments or something.

Comment 22

[Bobby Holley (:bholley)]

Comment on attachment 8512756 [details] [diff] [review]
part 4.  Change the CompileFunction calls in the component loader and subscript loader to pass in their desired scope chains

Review of attachment 8512756 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +786,5 @@
> +                // Note: exceptions will get handled further down;
> +                // don't early return for them here.
> +                AutoObjectVector scopeChain(cx);
> +                if (scopeChain.append(obj)) {
> +                    CompileFunction(cx, obj, scopeChain, options,

Please add /* objToDefineOn = */ for all of these. In general, that API change is confusing given the historical meaning of |obj| as a second argument being "parent". I would have preferred that argument to go at the end.

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
@@ +90,5 @@
>      return NS_OK;
>  }
>  
> +static bool
> +BuildScopeChainForObject(JSContext *cx, HandleObject obj,

Add a comment indicating that there probably aren't consumers that actually rely on having the full parent chain in the environment, but that this was just done as a compat-preserving change. At some point, we might consider getting rid of this behavior.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> I would have preferred that argument to go at the end.

That's totally doable; the API was just added in an earlier patch in this bug.  Shu-yu, thoughts?  Presumably it would go right before the mutable handle outparam.

> Add a comment

Done.

Comment 24

[Boris Zbarsky [:bzbarsky]]

And we could change the other signatures similarly, which would address the "at least make consumers recompile so they can see if they need to change something" API change issue...

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Add a friend API for getting the scope object for a function, if it has one

Comment 26

[Boris Zbarsky [:bzbarsky]]

https://tbpl.mozilla.org/?tree=Try&rev=efba3e3db87d is greenish now.  ;)

Comment 27

[Shu-yu Guo [:shu]]

Comment on attachment 8512835 [details] [diff] [review]
part 3.  Add a friend API for getting the scope object for a function, if it has one

Review of attachment 8512835 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsfriendapi.h
@@ +2681,5 @@
> + * scripted or does not have an object environment, just returns the function's
> + * parent.
> + */
> +extern JS_FRIEND_API(JSObject *)
> +GetEnvironmentObjectForFunction(JSFunction *fun);

On the one hand, I think from the name it is not clear that it in fact returns fun's parent it has a non-with environment.

On the other hand, GetObjectEnvironmentObjectForFunction is real ugly. I think I still prefer that though, unless you strongly object.

Comment 28

[Shu-yu Guo [:shu]]

(In reply to Boris Zbarsky [:bz] from comment #15)
> > can we do away with options.defineOnScope altogether?
> 
> That's what I asked in comment 9.
> 

Ah sorry, missed that. Looks like this is the only use for that option, so by all means let's kill it.

Comment 29

[Boris Zbarsky [:bzbarsky]]

OK, I talked to waldo about the API design here and he asked me to nuke the objToDefineOn bits and instead just have callers do that manually, and also nuke the APIs that don't take a scope chain.  I'll rework the patches here accordingly.

Comment 30

[Boris Zbarsky [:bzbarsky]]

> On the other hand, GetObjectEnvironmentObjectForFunction

Done.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Oh, lovely.  The subscript and component loaders, which were what I added the objToDefineOn thing for, are passing a null name anyway, so they don't use the auto-definition behavior!

Comment 32

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Refactor the CompileFunction code to allow cleanly separating whether we're passing in an object to define the function on or a scope chain. This change should not cause any behavior changes

Interdiff from the last part 2 coming up

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff for part 2

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Eliminate the ability to provide a non-global parent object to JS::CompileFunction and company except via the scopeChain API

Comment 35

[Jeff Walden [:Waldo]]

Comment on attachment 8513205 [details] [diff] [review]
part 5.  Eliminate the ability to provide a non-global parent object to JS::CompileFunction and company except via the scopeChain API

Review of attachment 8513205 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi-tests/testEnclosingFunction.cpp
@@ +40,5 @@
>  
>      const char s1chars[] = "checkEnclosing()";
> +    JS::AutoObjectVector emptyScopeChain(cx);
> +    JS::CompileFunction(cx, emptyScopeChain, options, "s1", 0, nullptr, s1chars,
> +                        strlen(s1chars), &fun);

Put this in a CHECK().

@@ +48,5 @@
>      CHECK(foundFun == fun);
>  
>      const char s2chars[] = "return function() { checkEnclosing() }";
> +    JS::CompileFunction(cx, emptyScopeChain, options, "s2", 0, nullptr, s2chars,
> +                        strlen(s2chars), &fun);

Ditto.

@@ +56,5 @@
>      CHECK(foundFun == fun);
>  
>      const char s3chars[] = "return function() { let (x) { function g() { checkEnclosing() } return g() } }";
> +    JS::CompileFunction(cx, emptyScopeChain, options, "s3", 0, nullptr, s3chars,
> +                        strlen(s3chars), &fun);

Ditto.

Comment 36

[Peter Van der Beken [:peterv]]

Comment on attachment 8511447 [details] [diff] [review]
part 1.  Eliminate the "parent" argument to JS_CloneFunctionObject to make callers use the scopeChain version if they want something other than the global

Review of attachment 8511447 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/xbl/nsXBLProtoImplMethod.cpp
@@ +108,5 @@
> +    JS::Rooted<JSObject*> globalObject(aCx, JS_GetGlobalForObject(aCx, aTargetClassObject));
> +    MOZ_ASSERT(xpc::IsInContentXBLScope(globalObject) ||
> +               xpc::IsInAddonScope(globalObject) ||
> +               globalObject == xpc::GetXBLScope(aCx, globalObject));
> +    MOZ_ASSERT(JS::CurrentGlobalOrNull(aCx) == globalObject);

I think JS_GetGlobalForObject already assert this, but I guess we can't be too safe :-).

Comment 37

[Boris Zbarsky [:bzbarsky]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2887e7c320fe
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dd4ec0807838
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cb53a4bac4c8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5ccd3c29891c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/715ded1f9639

Comment 38

[Boris Zbarsky [:bzbarsky]]

We don't have any existing CompileFunction docs, right?

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2887e7c320fe
https://hg.mozilla.org/mozilla-central/rev/dd4ec0807838
https://hg.mozilla.org/mozilla-central/rev/cb53a4bac4c8
https://hg.mozilla.org/mozilla-central/rev/5ccd3c29891c
https://hg.mozilla.org/mozilla-central/rev/715ded1f9639

Comment 40

[Jeff Walden [:Waldo]]

Fairly sure we don't, no.  Skeletal discussion on MDN, and as /**/ docs in jsapi.h, are fine.  I'm not actually sure this is exactly what the API will be in the long term as JSAPI evolves, in any case, so not worth spending huge amounts of time on.