573651 - Object.create use of JS_GetScopeChain (and comment) are wrong

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

/*
     * It's plausible that it's safe to just use the context's global object,
     * but since we're not completely sure, better safe than sorry.
     */
    JSObject *obj =
        NewObjectWithGivenProto(cx, &js_ObjectClass, JSVAL_TO_OBJECT(v), JS_GetScopeChain(cx));

The patch in bug 492840 had a comment talking about JS_GetScopeChain bailing off trace, which was worth keeping. The comment here about using the context's global object being "plausible" is wrong: cx->globalObject is not necessarily related to the global for the callee Object.create, *or* related to the scope chain of the top-most frame on cx->fp.

ES5 does not address multiple global objects (no ECMA-262 edition does) but to avoid dynamic scoping, we should use the global of the particular Object.create callee that was invoked: JSVAL_TO_OBJECT(JS_CALLEE(cx, vp))->getGlobal().

/be

Comment 1

[Jeff Walden [:Waldo]]

I ran this by mrbkap way back; my memory was that he didn't think it mattered a whole lot what actually got passed here, but it's been awhile.

I'm guessing the compartments work would probably address this in some fashion.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

fwiw, JS_GetGlobalForScopeChain is exactly equivalent to JSVAL_TO_OBJECT(JS_CALLEE(cx, vp))->getGlobal(). We should just switch to it and stop bailing off trace.

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> fwiw, JS_GetGlobalForScopeChain is exactly equivalent to
> JSVAL_TO_OBJECT(JS_CALLEE(cx, vp))->getGlobal(). We should just switch to it
> and stop bailing off trace.

Cool, except: JS_PUBLIC_API overhead on some systems, not optimized internally; and no CHECK_REQUEST (need one, eh?). New bug on internal/inline-if-appropriate helper for this API?

/be

Comment 4

[Jeff Walden [:Waldo]]

This got removed (by Brendan even, before comment 3!) in bug 535416:

http://hg.mozilla.org/tracemonkey/rev/8c2faceba7bf