eval cache ignores the static depth differences

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

({regression})

unspecified
regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +

Firefox Tracking Flags

(status1.9.2 final-fixed)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
Nominating for 1.9.2 as the fix will alter eval(cx, scope) semantics.
Flags: blocking1.9.2?
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.
Flags: blocking1.9.2? → blocking1.9.2-
(Assignee)

Comment 3

9 years ago
(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?
Flags: blocking1.9.2- → blocking1.9.2?
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.
Uhm, should this bug be closed?

Updated

9 years ago
Group: core-security
Flags: blocking1.9.2? → blocking1.9.2+
(Assignee)

Comment 7

9 years ago
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)
(Assignee)

Comment 8

9 years ago
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

9 years ago
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.
(Assignee)

Updated

9 years ago
Blocks: 454184
(Assignee)

Comment 10

9 years ago
I change the title of this bug to reflect the exact problem that exists on trunk and 192, 191 branches.
Summary: fixing eval(source, scope) → eval cache ignores the static depth differences
(Assignee)

Comment 11

9 years ago
Created attachment 415207 [details] [diff] [review]
v1

Here is a minimal fix that enables eval caching only when the eval(one_argument) form is used.
Attachment #415207 - Flags: review?(brendan)
(Assignee)

Comment 12

9 years ago
(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.
(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 on attachment 415207 [details] [diff] [review]
v1

Great, thanks for finding and fixing.

Please nominate for 1.9.2.

/be
Attachment #415207 - Flags: review?(brendan) → review+
(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
(Assignee)

Comment 16

9 years ago
https://hg.mozilla.org/tracemonkey/rev/57a6ad20eae9
Keywords: regression
Whiteboard: fixed-in-tracemonkey

Comment 17

9 years ago
http://hg.mozilla.org/mozilla-central/rev/57a6ad20eae9
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Depends on: 532491

Comment 18

9 years ago
Igor, js1_4/Eval/regress-531037.js needs to be listed in js1_4/Eval/jstests.list
(Assignee)

Comment 19

9 years ago
changing js1_4/Eval/jstests.list to include bug's test - https://hg.mozilla.org/tracemonkey/rev/796924e6ee80
Group: core-security
You need to log in before you can comment on or make changes to this bug.