531037 - eval cache ignores the static depth differences

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Currently eval(source, null_or_undefined) is buggy, see bug 528116 comment 17 and must be fixed as it leads, for example, to false eval cache hits. But to fix the problems we have to clarify the meaning eval(source, scope) especially when scope is not an object. There are at least 3 ways to deal with this.

1. Ignore the scope parameter to follow ES5 and other browsers. This is the best solution from code simplicity point of view but we maybe way to late into 3.6 cycle to do this.

2. Treat eval(source, scope) as an alias to eval.call(scope, source). Although this does not match the current semantics in all the cases, it is 100% compatible with the current code with usage like eval(source, anotherWindowReferrence) while still reducing complexity of obj_eval implementation.

3. Decide what to do about eval(source, not_an_object) especially when not_an_object is null or undefined and fix obj_eval accordingly.

Comment 1

[Igor Bukanov]

Nominating for 1.9.2 as the fix will alter eval(cx, scope) semantics.

Comment 2

[Mike Shaver (:shaver emeritus)]

Where is |eval(cx, scope)| used?  I'm not familiar with it.  I would rather change eval (again) on trunk only, and have some time with 3.7 betas to find and resolve site and extension dependencies; we don't have any betas left for 3.6, likely, and making this change without a beta seems like a recipe for pain.

Blocking- for it, therefore, but renom if I'm missing something.

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> Blocking- for it, therefore, but renom if I'm missing something.

I renominate this for 1.9.2 as the false positives for eval cache may have bad consequences. In the comment 0 I was asking about what route to take regarding fixing eval(source, obj). I suppose the comment 1 means that the suggestion 1 and 2 is out. But then the question is what to do about eval(cx, null_or_undefined). Right now it is broken leading to that cache bug. 

The safest route would be to treat eval(cx, null_or_undefined) just as eval(cx) as this is closest to what the code is doing right now and fix the cache management accordingly. Is it ok?

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Igor: I'd keep running with a fix regardless of the blocking nomination; that flag only dictates whether or not we'd hold the release of Firefox 3.6 for this fix.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Uhm, should this bug be closed?

Comment 7

[Igor Bukanov]

Here is the test case that demonstrates the problem of false positives with the eval cache:

(function() {
    var b = 10;
    var fff = function() { return --b >= 0; }; 
    var src = "while (fff());";
    eval(src, null);
    b = 10;
    try {
        eval(src, {fff: function() {throw 0;}});
        throw new Error("Unexpected success of eval");
    } catch (e) {
        if (e !== 0)
            throw e;
    }
})();

Without jit the test case throws Error: Unexpected success of eval. With the jit a debug build asserts:

Assertion failure: ti->nStackTypes == NativeStackSlots(cx, 0), at /home/igor/m/tm/js/src/jstracer.cpp:6314
Aborted (core dumped)

Comment 8

[Igor Bukanov]

To bc: where the test case from the comment 7 should go? The problem exists without the jit, but with the jit it is worse. So should I duplicate the test both under tests and trace-test directories?

Comment 9

[Bob Clary [:bc] (inactive)]

currently on the unittest machines, tests placed in js/src/trace-test are only tested against the shell while tests placed in js/src/tests are tested in the browser with content and chrome jit enabled by default. In the future, the js/src/tests will be run on the unittest machines in the shell using jstests.py.

considering that this bug is about eval and scopes I would expect the browser tests to be equally important if not more so. I hate to ask for duplication, but in this case I think it is best. Note you can run your test with jit on and off in the same test file under js/src/tests if you wish without having to have a jit version and a non-jit version.

I realize the separate trace-tests were created out of frustration with the old-style jstests, but going forward I think it might be a good idea to think about combining them or adding a way to run the trace-tests in the browser.

Comment 10

[Igor Bukanov]

I change the title of this bug to reflect the exact problem that exists on trunk and 192, 191 branches.

Comment 11

[Igor Bukanov]

Attachment: v1

Here is a minimal fix that enables eval caching only when the eval(one_argument) form is used.

Comment 12

[Igor Bukanov]

(In reply to comment #9)
> considering that this bug is about eval and scopes I would expect the browser
> tests to be equally important if not more so. I hate to ask for duplication,
> but in this case I think it is best.

What is a suggested directory for the test under js/src/tests? The bug is about a regression in SM extension to eval.

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> (In reply to comment #9)
> > considering that this bug is about eval and scopes I would expect the browser
> > tests to be equally important if not more so. I hate to ask for duplication,
> > but in this case I think it is best.
> 
> What is a suggested directory for the test under js/src/tests? The bug is about
> a regression in SM extension to eval.

I see possibly relevant ancient history under tests/js1_4/Eval/. The indirect and two-arg eval forms are very old, from around ES3 (so 1.4 or 1.5), IIRC.

/be

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 415207 [details] [diff] [review]
v1

Great, thanks for finding and fixing.

Please nominate for 1.9.2.

/be

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #14)
> (From update of attachment 415207 [details] [diff] [review])
> Great, thanks for finding and fixing.

Further work to kill two-arg eval is overdue, thanks for that too.

> Please nominate for 1.9.2.

Argh, missed the blocking1.9.2+ set already by sayrer.

/be

Comment 16

[Igor Bukanov]

https://hg.mozilla.org/tracemonkey/rev/57a6ad20eae9

Comment 17

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/57a6ad20eae9

Comment 18

[Bob Clary [:bc] (inactive)]

Igor, js1_4/Eval/regress-531037.js needs to be listed in js1_4/Eval/jstests.list

Comment 19

[Igor Bukanov]

changing js1_4/Eval/jstests.list to include bug's test - https://hg.mozilla.org/tracemonkey/rev/796924e6ee80

Comment 20

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fd0745f78d42