The simplified test case is: eval = evalcx("").eval; eval("eval('3')"); Apply the patch below to catch the problem earlier, and the test case can be simplified a bit further. eval = evalcx("").eval; eval(""); The first line gets a wrapper of the sandbox's eval and stores that in eval. What should happen when we call that wrapper? diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp --- a/js/src/jsobj.cpp +++ b/js/src/jsobj.cpp @@ -1247,16 +1247,18 @@ obj_eval(JSContext *cx, uintN argc, Valu script = Compiler::compileScript(cx, scopeobj, callerFrame, principals, tcflags, chars, length, NULL, file, line, str, staticLevel); if (!script) return JS_FALSE; } + assertSameCompartment(cx, scopeobj, script, *vp); + /* * Belt-and-braces: check that the lesser of eval's principals and the * caller's principals has access to scopeobj. */ JSBool ok = js_CheckPrincipalsAccess(cx, scopeobj, principals, cx->runtime->atomState.evalAtom) && Execute(cx, scopeobj, script, callerFrame, JSFRAME_EVAL, vp);

The assertion still trips with mrbkap's whole patch stack pushed. I think indirect eval semantics are perfect for this situation, so I'll patch it that way.

While in here I noticed that we ban this: var x = 1; var obj = {x: 2}; obj.eval = eval; obj.eval("x"); // throws EvalError ...but I don't see where ES5 allows us to do that. My reading of the spec is that it must be treated as an indirect eval, so the result is 1. Also ... ES5 §15.1.2.1.1 says in full: > 15.1.2.1.1 - Direct Call to Eval > > A direct call to the eval function is one that is expressed as a > CallExpression that meets the following two conditions: > > The Reference that is the result of evaluating the MemberExpression in the > CallExpression has an environment record as its base value and its > reference name is "eval". > > The result of calling the abstract operation GetValue with that Reference > as the argument is the standard built-in function defined in 15.1.2.1. The last paragraph is mysterious to me. Calling GetValue is part of evaluating any CallExpression that satisfies the first condition. If that hadn't returned the eval function, then... eval wouldn't have been called. So we wouldn't get here. So, what is that supposed to mean?

The actual phrase wich disallows obj.eval("x"); Is > has an environment record as its base value > reference name is "eval" Eg obj.eval("x") The base would be an object and not environment record. var exec = eval; The reference name would be "exec" and not eval

(In reply to comment #3) > The actual phrase wich disallows > > obj.eval("x"); > > Is > > has an environment record as its base value > > reference name is "eval" That doesn't disallow obj.eval("x"), though. It only makes it indirect.

The issue with obj.eval("x") is already filed: bug 592664.

Smashed something together, but indrect eval should have global scope. So you should get "1".

trace-test/tests/jaeger/bug588363-2.js wants to do a direct eval. The change in jsobj.cpp makes it indirect, which causes the test to fail with a ReferenceError. So I changed the test to make it do a direct eval again.

Comment on attachment 479099 [details] [diff] [review] v1 Remove the printf. The rest looks sweet.