488029 - Wrong assumption about read-only scope chain in js_FindIdentifierBase

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

When I implemented the changes for the bug 462734, I have missed that with upvar2 changes from the bug 452598 the parent chain for a named function expression can change in the resolution hook called from js_LookupPropertyWithFlags.

Thus after js_FindIdentifierBase calls js_LookupPropertyWithFlags, it must update the parent variable. 

Due to this regression the following example throws an exception when executed in js shell:

(function x() { eval('x = 1'); })();

if (this.hasOwnProperty('x'))
    throw "bug, unexpected x="+x;

Comment 1

[Igor Bukanov]

The last time when I looked Rhino treated names of function expressions as a synthetic variable inside the function body. After the function was parsed it added code equivalent to var function_name = callee at the beginning of the function if function_name were not hidden with argument or var names. I wonder what would prevent this optimization to work for SpiderMonkey.

Comment 2

[Igor Bukanov]

(In reply to comment #1)
> The last time when I looked Rhino treated names of function expressions as a
> synthetic variable inside the function body. After the function was parsed it
> added code equivalent to var function_name = callee at the beginning of the
> function if function_name were not hidden with argument or var names. I wonder
> what would prevent this optimization to work for SpiderMonkey.

Hm, the problem with this is that function_name must be permanent yet eval('var function_name') should be able to add mutable variable. So that Rhino's hack would not work in SM reality.

Still, instead of changing the parent scope chain, the code could add a property to the Call object itself with a special runtime support to overwrite it in JSOP_DEFVAR called from the eval. The latter may bring more code, but I just does not like the idea of relaxing that invariant that parent is always readonly.

Comment 3

[Brendan Eich [:brendan]]

ES5 formalizes the bugfix to ES3 where the named function expression's name is bound in a declarative environment record above ("up" the parent chain sense in SpiderMonkey) the activation environment record. As noted, you can tell via at least eval + delete, probably other tests (will try to think of some), that shadowing of the lambda's name is possible.

If we can reload parent for now, think on how to redo this for later, avoid too much new code (given all the newish code that came in with upvar2), that seems best for now.

/be

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> ES5 formalizes the bugfix to ES3 where the named function expression's name is
> bound in a declarative environment record above ("up" the parent chain sense in
> SpiderMonkey) the activation environment record. As noted, you can tell via at
> least eval + delete, probably other tests (will try to think of some), that
> shadowing of the lambda's name is possible.

Since the Call object is not exposed to scripts, it is possible to add the callee property to the Call object using custom getter/setter/delete hook so script would not see the difference.

Comment 5

[Igor Bukanov]

fixing the dependency bug number

Comment 6

[Igor Bukanov]

Attachment: v1

It turned out it is not possible to use the Call object to store the callee for named functions. The problem comes from the code like:

(function f() {
    eval('var f = 1;');
    f = (eval('delete f'), 2);
    if (f != 2) throw "Bad: too smart optimization";
})();

Here f should be assigned 2 since at the bind stage it will be found in the var object. So the set should store the result there. But if the declaration environment is optimized away, the code would not know without considerable runtime support if the assignment is applied to the original f pointing to the caller or to the var named f. 

So the patch does the following things:

1. It fixes the loop in js_FindIdentifierBase not to assume read-only parent. There I also added js_DeclEnvClass to the list of classes with enabled caching.

2. It creates js_DeclEnvClass instance eagerly when creating the Call object. I done this for two reasons. First, it ensures compatibility so that any potential debugging extension that may cache the depth of the scope chain for whatever reasons will not have surprises with FF 3.5. Second, with lazy js_DeclEnvClass creation the resolve hook for each and every function's call object must check if it needs to create the instance, not only Calls for named lambdas. The check is tiny but it still there for all Calls.

Only the first item is necessary to address the bug. So if the second one is not compelling, I will remove those changes.

Comment 7

[Igor Bukanov]

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

The patch contains a bad typo.

Comment 8

[Igor Bukanov]

Attachment: v2

In the patch I fixed not one but two bad typos. Now it passes the shell tests.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 372461 [details] [diff] [review]
v2

>+    JS_ASSERT(OBJ_GET_PARENT(BOGUS_CX, obj));
>+
>+    JSClass *clasp = OBJ_GET_CLASS(BOGUS_CX, obj);

Rather than OBJ_GET_{CLASS,PARENT}(BOGUS_CX, ...) why not use STOBJ_GET_{CLASS,PARENT}? Not great, to imply any single-thread vs. multi-thread issue here. Really want something better (inline methods on struct JSObject maybe).

r=me with this finessed, seems BOGUS ;-).

/be

Comment 10

[Igor Bukanov]

Attachment: v3

The new patch uses STOBJ_ macros to avoid using BOGUS_CX.

Comment 11

[Igor Bukanov]

landed to TM - http://hg.mozilla.org/tracemonkey/rev/ad4433ce75c8

Comment 12

[Igor Bukanov]

Here is a regression test for the bug that also asserts various aspects of interaction of the callee reference stored in an instance of js_DeclEnvClass and a hiding name added to the var object via eval.

(function g() {   
    eval('g = 1');
})();

assertEq('g' in this, false);

(function f() {
    var callee = f, tmp;
    eval('f = 1');
    assertEq(f, callee);
    eval('var f = 1');
    assertEq(f, 1);
    assertEq(eval('delete f'), true);
    assertEq(eval('delete f'), false);
    assertEq(delete f, false);
    f = 2;
    assertEq(f, callee);
    f = (eval('var f = 2'), 3);
    assertEq(f, 2);
    f = ((tmp = eval('delete f')), 4);
    assertEq(f, 4);
    assertEq(tmp, true);
    eval('var f = 2');
    assertEq(f, 2);
    assertEq(delete f, true);
    assertEq(eval('delete f'), false);
})();

assertEq('f' in this, false);

Comment 13

[Robert Sayre]

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

Comment 14

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/52f12b8b7d5f

Comment 15

[Jeff Walden [:Waldo]]

(In reply to comment #9)
> Really want something better (inline methods on struct JSObject maybe).

+Infinity

I've wanted to do this for months, but it'd touch enough lines of code that now's not the best time.  When we get 3.5 done, tho, lemme at 'em!

http://www.youtube.com/watch?v=S8kmv5S6G38#t=7m42s

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with testcase given in comment 0 on trunk and 1.9.1 with the
following debug builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090422 Minefield/3.6a1pre ID:20090422224452

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre)
Gecko/20090422 Shiretoko/3.5b4pre ID:20090422122043