Although ES3 allowed prohibiting indirect eval, ES5 doesn't anymore (even in strict mode). [eval][0]("this") should not throw EvalError.

Busted! I was talking about this in other fora recently. This should block. Thanks for reporting, /be

Likewise (from bug 600193 comment 3), this should be allowed: var x = 1; var obj = {x: 2}; obj.eval = eval; assertEq(obj.eval("x"), 1); Currently the call to eval throws an EvalError.

What the heck, I'm in this code anyway. There's a lot of not-particularly-relevant tidying up in the patch. I think it's still possible to see what's going on, but if not, I can split that out.

v1 was produced by hg qdiff. It has the tests in it, and it shows exactly what changed in the eval cache code (nothing significant I hope), so it's worth looking at. Here is what GNU diff has to say about the exact same set of changes to jsobj.cpp. It shows what changed about the actual eval part of obj_eval, which is more interesting.

Comment on attachment 480439 [details] [diff] [review] v1 Thanks for the better alterna-diff. >+ /* >+ * CSP check: Is eval() allowed at all? >+ * Report errors via CSP is done in the script security mgr. >+ */ >+ if (!js_CheckContentSecurityPolicy(cx)) { >+ JS_ReportError(cx, "call to eval() blocked by CSP"); >+ return false; >+ } The comment (you touched it ;-) seems wrong -- the JS_ReportError call (no js.msg l10n support) is here, not in some callback impl'ed in the script secmgr. Fix or followup (or a bit of both -- fix the comment now and cite the followup on js.msg usage). Great tests. /be

v1 wouldn't even compile; I must have built and run tests in the wrong tree. The fix is pretty straightforward though. I noticed that we used to cache even eval scripts that could never be used, such as indirect eval scripts. With this patch we don't do that anymore. I also fixed the CSP error message and comment.

How about we try that one with Unix newlines.

Sorry, undercaffeinated this morning. The CSP error message patch is in bug 602212 after all.

Comment on attachment 481243 [details] [diff] [review] v2 Does JS_ALWAYS_INLINE matter for EvalCacheLookup? r=me in any event, just thought this might be better for perf and codesize (both). /be

Sadly, it *does* matter, in MSVC anyway -- even though this is a static function only called from one place. I wonder why MSVC doesn't inline that unless we insist. Anyway, I made it JS_ALWAYS_INLINE for checkin. http://hg.mozilla.org/tracemonkey/rev/89006937466d

Backed out. Crashing tests.

I deleted a little too eagerly. This version of the patch restores the code that bails out early if there's no calling script.

The diff between v2 and (the two v3 patches combined).

Now, v2 was crashing because I had deleted the "if (!caller)" check quoted below, and then soon after that we do caller->script(). My first inclination was to go through and fix up all the places where obj_eval uses caller. The comment I'm adding here is meant to explain why I didn't do that: > if (!caller) { >+ /* Eval code needs to inherit principals from the caller. */ > JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, > JSMSG_BAD_INDIRECT_CALL, js_eval_str); > return JS_FALSE; > } To be a little more explicit, one of the places obj_eval uses caller is here: > JSPrincipals *principals = js_EvalFramePrincipals(cx, callee, caller); js_EvalFramePrincipals does have some code to cope with NULL caller, but I'm not sure it does exactly what we want. So to be on the safe side I'm going to leave that fix for another day.

Landed part 1: http://hg.mozilla.org/tracemonkey/rev/8402d56a7777 and part 2: http://hg.mozilla.org/tracemonkey/rev/7598b7ab2e76 and backed it out: http://hg.mozilla.org/tracemonkey/rev/a4383c16a9b5 and relanded it: http://hg.mozilla.org/tracemonkey/rev/2e20ecdd1194 To top it off I put the wrong bug number in all four commits. Awesome. :-(