Closed Bug 495325 Opened 11 years ago Closed 10 years ago

ES5 indirect eval == global eval NOW!

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: brendan, Assigned: mrbkap)

References

(Blocks 1 open bug)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files, 5 obsolete files)

'nuff said, except:

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

/be
Summary: ES5 indirect eval NOW! → ES5 indirect eval == global eval NOW!
This needs an owner and a patch, pronto.

/be
I'll do it.
Assignee: general → mrbkap
Attached patch Proposed fix (obsolete) — Splinter Review
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.
Attachment #394973 - Flags: review?(brendan)
Attached file some testcases (obsolete) —
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
(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.
Talked to mrbkap, he is gonna update the patch not to use an implicit with.

/be
Ping?

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

Review ping2.

/be
Sorry -- the last couple of days have been busy. I'll get to this tomorrow.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #394973 - Attachment is obsolete: true
Attachment #396564 - Flags: review?(brendan)
Attachment #394973 - Flags: review?(brendan)
Comment on attachment 396564 [details] [diff] [review]
patch v2

Er, this isn't right.
Attachment #396564 - Attachment is obsolete: true
Attachment #396564 - Flags: review?(brendan)
Attached file testcases
Attachment #394974 - Attachment is obsolete: true
Attached patch patch v3 (obsolete) — Splinter Review
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.
Attachment #396625 - Flags: review?(igor)
Attachment #396625 - Flags: review?(brendan)
Comment on attachment 396625 [details] [diff] [review]
patch v3

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

/be
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.
Attachment #396625 - Flags: review?(igor) → review+
Brendan: review ping.
Blake, could you refresh? I conflicted you a bit :-(.

/be
Attached patch patch v3.1 (obsolete) — Splinter Review
Updated to tip.
Attachment #396625 - Attachment is obsolete: true
Attachment #401336 - Flags: review?(brendan)
Attachment #396625 - Flags: review?(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
Attachment #401336 - Flags: review?(brendan) → review-
Attached patch patch v4Splinter Review
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;
Attachment #401336 - Attachment is obsolete: true
Attachment #401349 - Flags: review?(brendan)
Comment on attachment 401349 [details] [diff] [review]
patch v4

Whew! Let's go...

/be
Attachment #401349 - Flags: review?(brendan) → review+
Whiteboard: fi,
Oops... http://hg.mozilla.org/tracemonkey/rev/de72243414cd
Whiteboard: fi, → fixed-in-tracemonkey
Depends on: 519359
http://hg.mozilla.org/mozilla-central/rev/de72243414cd
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 520511
Depends on: 523530
Depends on: 525618
Flags: wanted1.9.2-
Blocks: 531675
Depends on: 531682
Depends on: 538159
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.