495325 - ES5 indirect eval == global eval NOW!

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

'nuff said, except:

NOW! means mozilla-central and tracemonkey repos, but not 1.9.1 branch. We needed this last year.

/be

Comment 1

[Brendan Eich [:brendan]]

This needs an owner and a patch, pronto.

/be

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

I'll do it.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

So, in writing this patch, I noticed that we broke the no-scripted-caller case again. This patch just throws in this case. Given that it's been broken for a while and nobody has been clamoring, I think this should be OK. Otherwise, I think this patch does what's needed here.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: some testcases

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 394973 [details] [diff] [review]
Proposed fix

No-scripted-caller is setTimeout(eval, 0, str) or the like, right? How did we break it? I don't see how caller is dereferenced unless non-null (indirectCall will be false if !caller).

> #if JS_HAS_EVAL_THIS_SCOPE
>-        /* If obj.eval(str), emulate 'with (obj) eval(str)' in the caller. */
>+        /*
>+         * If we see an indirect call, then run eval in the global scope. We do
>+         * this so the compiler can make assumptions about what bindings may or
>+         * may not exist in the current frame if it doesn't see 'eval'.
>+         */
>         if (indirectCall) {
>             callerScopeChain = js_GetScopeChain(cx, caller);

Need GetGlobalForObject here, no?

Also, which global? The eval function object's global, or the caller's global? I hope ES5 says (it may not if it doesn't include enough multiple-globals magic).

/be

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #5)
> (From update of attachment 394973 [details] [diff] [review])
> No-scripted-caller is setTimeout(eval, 0, str) or the like, right? How did we
> break it? I don't see how caller is dereferenced unless non-null (indirectCall
> will be false if !caller).

We're both guilty here: caller->fun is dereferenced when we check if this script is in the cache, and I call it if (scopeobj). Is it an important enough usecase to keep around?

> >         if (indirectCall) {
> >             callerScopeChain = js_GetScopeChain(cx, caller);
> 
> Need GetGlobalForObject here, no?

I add the GetGlobalForObject below. If I added it here, then we'd restore the wrong object when fixing up the caller's scope chain.

> Also, which global? The eval function object's global, or the caller's global?
> I hope ES5 says (it may not if it doesn't include enough multiple-globals
> magic).

I think that for compatibilities with older SpiderMonkies, we need to use the global of the 'this' object (to support otherWindow.eval()). That will translate into the caller's global if this was a evil() indirect call as opposed to a window.evil() indirect call.

Comment 7

[Brendan Eich [:brendan]]

Talked to mrbkap, he is gonna update the patch not to use an implicit with.

/be

Comment 8

[Brendan Eich [:brendan]]

Ping?

/be

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 394973 [details] [diff] [review]
Proposed fix

Review ping2.

/be

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Sorry -- the last couple of days have been busy. I'll get to this tomorrow.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v2

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 396564 [details] [diff] [review]
patch v2

Er, this isn't right.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: testcases

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v3

I talked this over with brendan. If you want to augment the scope chain, you now have to type 'with (obj) eval(...)'. foo.eval() means that foo must be a global object. eval(..., foo); means that foo can be anything and we ignore the scope chain entirely.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 396625 [details] [diff] [review]
patch v3

I'm going to let Igor go first ;-).

/be

Comment 16

[Igor Bukanov]

Comment on attachment 396625 [details] [diff] [review]
patch v3

>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp
>--- a/js/src/jsobj.cpp
>+++ b/js/src/jsobj.cpp
>@@ -1290,131 +1296,124 @@ obj_eval(JSContext *cx, JSObject *obj, u
>+
>             OBJ_TO_INNER_OBJECT(cx, obj);
...
>+
>+            ok = js_CheckPrincipalsAccess(cx, obj,
>+                                          JS_StackFramePrincipals(cx, caller),
>+                                          cx->runtime->atomState.evalAtom);
...
>+
>+            /* NB: We know obj is a global object here. */
>+            JS_ASSERT(!OBJ_GET_PARENT(cx, obj));

Nit: Move the assert right after OBJ_TO_INNER_OBJECT to ensure that any OBJ_TO_INNER_OBJECT returns a global object.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Brendan: review ping.

Comment 18

[Brendan Eich [:brendan]]

Blake, could you refresh? I conflicted you a bit :-(.

/be

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v3.1

Updated to tip.

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 401336 [details] [diff] [review]
patch v3.1

(gdb) r
Starting program: /Users/brendaneich/Hacking/hg.mozilla.org/tracemonkey/js/src/Darwin_DBG.OBJ/js 
Reading symbols for shared libraries +++++....................................................................... done
js> function f(s){return eval(s)}
js> f("#2#")
Assertion failure: script->nfixed == 0, at ../jsinterp.cpp:1021

Program received signal SIGTRAP, Trace/breakpoint trap.
JS_Assert (s=0x1afec1 "script->nfixed == 0", file=0x1ae3c4 "../jsinterp.cpp", ln=1021) at ../jsutil.cpp:69
69	    abort();
(gdb) r
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Starting program: /Users/brendaneich/Hacking/hg.mozilla.org/tracemonkey/js/src/Darwin_DBG.OBJ/js 
Reading symbols for shared libraries . done
js> eval("#1=[#1#]")
Assertion failure: script->nfixed == 0, at ../jsinterp.cpp:1021

Program received signal SIGTRAP, Trace/breakpoint trap.
JS_Assert (s=0x1afec1 "script->nfixed == 0", file=0x1ae3c4 "../jsinterp.cpp", ln=1021) at ../jsutil.cpp:69
69	    abort();

The assertions that nfixed == 0 are bogus. Not sure you can assert anything about nfixed, or need to.

/be

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v4

diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
--- a/js/src/jsinterp.cpp
+++ b/js/src/jsinterp.cpp
@@ -1497,22 +1497,17 @@ js_Execute(JSContext *cx, JSObject *chai
     hookData = mark = NULL;
     oldfp = js_GetTopStackFrame(cx);
     frame.script = script;
     if (down) {
         /* Propagate arg state for eval and the debugger API. */
         frame.callobj = down->callobj;
         frame.argsobj = down->argsobj;
         frame.varobj = down->varobj;
-        if (script->staticLevel > 0) {
-            JS_ASSERT(script->nfixed == 0);
-            frame.fun = down->fun;
-        } else {
-            frame.fun = NULL;
-        }
+        frame.fun = (script->staticLevel > 0) ? down->fun : NULL;
         frame.thisv = down->thisv;
         if (down->flags & JSFRAME_COMPUTED_THIS)
             flags |= JSFRAME_COMPUTED_THIS;
         frame.argc = down->argc;
         frame.argv = down->argv;
         frame.annotation = down->annotation;
     } else {
         frame.callobj = NULL;

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 401349 [details] [diff] [review]
patch v4

Whew! Let's go...

/be

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Oops... http://hg.mozilla.org/tracemonkey/rev/de72243414cd

Comment 24

[Robert Sayre]

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