Stop allowing random objects on function scope chains

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

({dev-doc-needed})

unspecified
mozilla36
x86
macOS
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 9 obsolete attachments)

10.21 KB, patch
shu
: review+
peterv
: review+
Details | Diff | Splinter Review
7.59 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.34 KB, patch
shu
: review+
Details | Diff | Splinter Review
11.55 KB, patch
shu
: review+
Details | Diff | Splinter Review
6.31 KB, patch
Details | Diff | Splinter Review
33.27 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8511448 - Attachment is obsolete: true
Attachment #8511448 - Flags: review?(shu)
Attachment #8511477 - Attachment is obsolete: true
Attachment #8511477 - Flags: review?(shu)
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.
Ah, I bet I know the issue.  Need to use a funky scope chain in the subscript loader.
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?
Attachment #8511781 - Flags: review?(shu)
Attachment #8511486 - Attachment is obsolete: true
Attachment #8511486 - Flags: review?(shu)
Also, please let me know if I need to get someone else to review the API change bits here...
Attachment #8511780 - Attachment is obsolete: true
Attachment #8511780 - Flags: review?(peterv)
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?
Attachment #8511779 - Flags: review?(shu) → review+
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 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)
{
     ...
}
Attachment #8511447 - Flags: review?(shu) → review+
> 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.
Flags: needinfo?(shu)
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?
Flags: needinfo?(bobbyholley)
Attachment #8511809 - Flags: review?(peterv)
I guess the other question is why the parent is skipping the Scope stuff and whether we actually want that in this case.
(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.
Flags: needinfo?(bobbyholley)
Attachment #8511809 - Attachment is obsolete: true
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.
Attachment #8512758 - Flags: review?(jorendorff)
Attachment #8511781 - Attachment is obsolete: true
Attachment #8511781 - Flags: review?(shu)
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.
Attachment #8512756 - Flags: review?(bobbyholley) → review+
> 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.
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...
Attachment #8512755 - Attachment is obsolete: true
Attachment #8512755 - Flags: review?(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.
Attachment #8512835 - Flags: review?(shu) → review+
(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.
Flags: needinfo?(shu)
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.
> On the other hand, GetObjectEnvironmentObjectForFunction

Done.
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!
Attachment #8511779 - Attachment is obsolete: true
Attachment #8512758 - Attachment is obsolete: true
Attachment #8512758 - Flags: review?(jorendorff)
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.
Attachment #8513205 - Flags: review?(jwalden+bmo) → review+

Updated

5 years ago
Attachment #8513202 - Flags: review?(shu) → review+
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 :-).
Attachment #8511447 - Flags: review?(peterv) → review+
Keywords: dev-doc-needed
We don't have any existing CompileFunction docs, right?
Flags: needinfo?(jwalden+bmo)
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.
Flags: needinfo?(jwalden+bmo)
You need to log in before you can comment on or make changes to this bug.