500580 - JS_CallFunction path isn't ~JIT guarded against non-global scope chains

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Graydon Hoare :graydon]

Bug 497060 comment 32 notes that paths into the JIT from event handlers can have long scope chains that are not the global object as well; this needs a fix similar to the fix in that bug.

Comment 1

[Graydon Hoare :graydon]

Attachment: Proposed fix, straight copy of the other

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 385295 [details] [diff] [review]
Proposed fix, straight copy of the other

>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
>--- a/js/src/jsinterp.cpp
>+++ b/js/src/jsinterp.cpp
>@@ -1429,16 +1429,30 @@ JSBool
> js_InternalInvoke(JSContext *cx, JSObject *obj, jsval fval, uintN flags,
>                   uintN argc, jsval *argv, jsval *rval)
> {
>     jsval *invokevp;
>     void *mark;
>     JSBool ok;
> 
>     js_LeaveTrace(cx);
>+
>+#ifdef JS_TRACER
>+    /*
>+     * The JIT requires that the scope chain here is equal to
>+     * its global object. Disable the JIT for this call if this
>+     * condition is not true.
>+     */
>+    uint32 oldOptions = cx->options;
>+    if ((oldOptions & JSOPTION_JIT) && obj != JS_GetGlobalForObject(cx, obj)) {
..12345678901234567890123456789012345678901234567890123456789012345678901234567890

Looks good, one nit: even ye olde 80 column limit allows this to fit on one line and the consequent to be unbraced. The comment could be wrapped more generously too.

/be

Comment 3

[Graydon Hoare :graydon]

http://hg.mozilla.org/tracemonkey/rev/d783ad5c9975

(With style nit corrected, and applied retroactively to former bug's patch too).

Comment 4

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/d783ad5c9975

Comment 5

[Boris Zbarsky [:bzbarsky]]

This change causes us to never trace anything running off an event handler, obviously.  See bug 503694.  That's really not acceptable for the browser end of things...

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> This change causes us to never trace anything running off an event handler,
> obviously.  See bug 503694.  That's really not acceptable for the browser end
> of things...

It's also unnecessary for the typical case of the event handler function calling a function defined in a <script> tag. See 503694 comment 13 :-(.

/be