532491 - "Assertion failure: staticLevel == script->staticLevel, at ../jsobj.cpp" with eval

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

function f(foo) {
  if (a % 2 == 1) {
    try {
      eval(foo);
    } catch(e) {}
  }
}
a = 1;
f("eval(\"x\")");
f("x");


asserts js debug shell on TM tip without -j at Assertion failure: staticLevel == script->staticLevel, at ../jsobj.cpp:1491

Nominating blocking1.9.2? because possibly-related bug 531037 is marked blocking1.9.2+. Security-sensitive because bug 531037 is also marked security-sensitive.

autoBisect shows this is probably related to bug 531037:

The first bad revision is:
changeset:   35345:57a6ad20eae9
user:        Igor Bukanov
date:        Tue Dec 01 00:01:11 2009 +0300
summary:     bug 531037 - eval fixes. r=brendan

Comment 1

[Andreas Gal :gal]

(gdb) list
1488	                                i = -1;
1489	                            }
1490	                        }
1491	                        if (i < 0 ||
1492	                            STOBJ_GET_PARENT(objarray->vector[i]) == scopeobj) {
1493	                            JS_ASSERT(staticLevel == script->staticLevel);
1494	                            EVAL_CACHE_METER(hit);
1495	                            *scriptp = script->u.nextToGC;
1496	                            script->u.nextToGC = NULL;
1497	                            break;
(gdb) p staticLevel
$1 = 2
(gdb) p script->staticLevel
$2 = 3
(gdb) bt

Comment 2

[Andreas Gal :gal]

Attachment: qualify eval cache hit on staticLevel

Comment 3

[Igor Bukanov]

Comment on attachment 415767 [details] [diff] [review]
qualify eval cache hit on staticLevel

>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp
>--- a/js/src/jsobj.cpp
>+++ b/js/src/jsobj.cpp
>@@ -1446,16 +1446,17 @@ obj_eval(JSContext *cx, JSObject *obj, u
>     bucket = EvalCacheHash(cx, str);
>     if (!indirectCall && argc == 1 && caller->fun) {

Drop the check for argc == 1 here. In the bug 531037 I thought that adding that check was equivalent to checking for the static depth. But I have missed the nested eval case obviously.

Comment 4

[Andreas Gal :gal]

Pushed with test case.

http://hg.mozilla.org/tracemonkey/rev/c3cc71461953

Comment 5

[Andreas Gal :gal]

As usual, lots of kudos to gary for finding this and reducing it.

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/c3cc71461953

Comment 7

[Bob Clary [:bc] (inactive)]

Gal, js1_8/regress/regress-532491.js needs to be listed in js1_8/regress/jstests.list

Comment 8

[Andreas Gal :gal]

Thanks bc.

http://hg.mozilla.org/tracemonkey/rev/54f375249f8e

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/54f375249f8e

Comment 10

[Robert Sayre]

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

Comment 11

[Robert Sayre]

Attachment: how the branch ended up.

Getting this bug and bug 531037 landed on branch turned out to be a little tricky, because we're not taking blake's indirect-eval patch there. I think I got it right by skipping the eval cache on for indirect eval and when there's a scopeobj.

Comment 12

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/tests/js1_8/regress/regress-532491.js.

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.