620130 - Calls to eval with same code + varying strict mode of script containing eval == fail

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

In my tree with bug 514568 at least partially fixed (no bugs yet, must be doing something wrong):

[jwalden@find-waldo-now src]$ dbg/js
js> function t(code) { return eval(code); }
js> t("'use strict'; eval('var x = 2;'); typeof x")
"undefined"
js> t("eval('var x = 2;'); typeof x")
"undefined"

In the first case the 'var x = 2;' eval code is strict mode, so the binding doesn't leak into the outside eval, and "undefined" is the correct result.  But in the second case, 'var x = 2;' is not in strict mode code, so it should leak, and the result should be "number".

Now try the opposite order:

[jwalden@find-waldo-now src]$ dbg/js
js> function t(code) { return eval(code); }
js> t("eval('var x = 2;'); typeof x")
"number"
js> t("'use strict'; eval('var x = 2;'); typeof x")
"number"

The first result is correct, and now the second one is wrong!

I'm pretty sure this bug isn't due to my patch.  When I look at the eval-cache-implementation code, it appears to condition cache hits based on same eval code, same caller function, and same scope object.  But behavior of the eval here depends on the calling *script*'s strictness, which does not come into play here.  This dodgy comment at the top of EvalCacheLookup illuminates the logic error:

     * An eval cache entry should never be considered a hit unless its
     * strictness matches that of the new eval code. The existing code takes
     * care of this, because hits are qualified by the function from which
     * eval was called, whose strictness doesn't change. Scripts produced by
     * calls to eval from global code are not cached.

The key is "hits are qualified by the function from which eval was called, whose strictness doesn't change".  The code's qualifying on the wrong thing here -- it should qualify on the caller script, not the caller function.  (I think if we did this we wouldn't need to not cache evals in global code.)

It's hard to make a testcase that fails without using the functionality added by my patch, for reasons that escape me.  I would have thought this or its reversal would fail:

[jwalden@find-waldo-now src]$ dbg/js
js> function t(code) { return eval(code); }
js> t("'use strict'; eval('typeof Object.getOwnPropertyDescriptor((function(){return arguments;})(), \\'caller\\')');");
"object"
js> t("eval('typeof Object.getOwnPropertyDescriptor((function(){return arguments;})(), \\'caller\\')');");
"undefined"

The only testcase I've found that demonstrates this failure pre-patch is this one (first result is right, second result also right because the eval cache wasn't seeded when parsing |with| in strict mode failed, third result uses the bad cache hit and is wrong):

[jwalden@find-waldo-now src]$ dbg/js
js> function t(code) { return eval(code); }
js> t("'use strict'; try { eval('with (5) 17'); } catch (e) { 'threw'; }");
"threw"
js> t("try { eval('with (5) 17'); } catch (e) { 'threw'; }");
17
js> t("'use strict'; try { eval('with (5) 17'); } catch (e) { 'threw'; }");
17

As you might have guessed, I discovered this bug while writing tests for bug 514568.

Comment 1

[Jeff Walden [:Waldo]]

Replacing the "evalType == DIRECT_EVAL && caller->isFunctionFrame()" check in EvalKernel with a check for "evalType == DIRECT_EVAL && caller->isFunctionFrame() && !caller->isEvalFrame()" does the trick for the testcase here that works without needing bug 514568 fixed.  But this seems unsatisfying versus keying based on the calling script as we *should* be doing, so I'm going to take a quick look and see what makes that not possible.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Patch and test

Keying based on calling script is possible but probably requires adding onto JSScript, and ideally we'd rip out JSScript::savedCallerFun and related hacking at the same time.  Basically, it isn't immediately trivial, so let's punt it to bug 620141.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 498539 [details] [diff] [review]
Patch and test

Good minimal fix with FIXME -- thanks.

/be

Comment 4

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/126cd4bf58e0

Comment 5

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/126cd4bf58e0