493232 - Wrong variable value accessed in closure

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Dave Townsend [:mossop]]

Crappy summary but I'm not really sure how to describe this. See the following page in Shiretoko:

http://www.oxymoronical.com/experiments/mozmap/broken.html

When you click on any of the markers, the info for Hamburg pops open. If you do the same on a Minefield build then the correct info pops open for each. Gran Paradiso also only shows the Hamburg info all the time so I guess this is a bug on trunk, but I'm not sure what is wrong with the js. I basically have the following:

for (place in uniques) {
  var location = uniques[place];
  var marker = new GMarker(...);
  GEvent.addEventListener(marker, "click", function() {
    ...
  });
}

Within the anonymous function the values of location and marker look to be always the last values they had during the last iteration of the loop. I would expect them to have the values that they had at the time the function was created.

Comment 1

[Dave Townsend [:mossop]]

Looks like maybe a merge has happened so no Shiretoko is behaving the same as Minefield. Which means this behaviour will change between 3.0 and 3.5 and so we should figure out whether it was actually correct before or after before we release.

Comment 2

[Mike Shaver (:shaver emeritus)]

I expected the Gran Paradiso behaviour: each of those anonymous functions closes over the same scope, and so will see the subsequent updates to marker and location.

Is this a desired ES5 change, or a regression? blocking to find out!

Comment 3

[Brendan Eich [:brendan]]

Clearly a regression. But I can't follow the Minefield vs. Shiretoko baseball game in comment 0 and comment 1 -- it seemed Minefield was correct, Shiretoko was not, then a merge happened (trunk into 1.9.1) and both are bad? What patch did the damage?

What about tracemonkey tip?

/be

Comment 4

[Dave Townsend [:mossop]]

(In reply to comment #3)
> Clearly a regression. But I can't follow the Minefield vs. Shiretoko baseball
> game in comment 0 and comment 1 -- it seemed Minefield was correct, Shiretoko
> was not, then a merge happened (trunk into 1.9.1) and both are bad? What patch
> did the damage?

To make that hopefully a bit clear. When I filed the bug, 3.0.x and 3.5 were behaving correctly. 3.6 was broken. Now both 3.5 and 3.6 are broken, so a patch landed on the 1.9.1 branch between the 15th and 26th that broke it. We just need to narrow that down.

Comment 5

[Mike Shaver (:shaver emeritus)]

When you say "behaving correctly", do you mean that they see the same values, or that they see different values?

Comment 6

[Dave Townsend [:mossop]]

(In reply to comment #5)
> When you say "behaving correctly", do you mean that they see the same values,
> or that they see different values?

I mean correctly based on your expectation in comment 2, i.e. the same as Gran Paradiso. I'll have a narrower range in a few minutes anyway.

Comment 7

[Dave Townsend [:mossop]]

The regression on 1.9.1 occurred in this range: http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?fromchange=f9fdf276d414&tochange=11e0fd20fde9

Comment 8

[Dave Townsend [:mossop]]

Toggling the jit pref doesn't change this.

Comment 9

[Jason Orendorff [:jorendorff]]

(function () {
    var funs = [];
    for (var i = 0; i < 2; i++) {
        var v = i;
        funs[i] = function() { return v; };
    }
    assertEq(funs[0](), 3);
}());

We make a flat closure for the lambda, I guess because v is only assigned once (oops).

Comment 10

[Brendan Eich [:brendan]]

Dream-JS has Scheme-ish bindings. Meanwhile back in reality...

Talked to jorendorff on IRC, straightforward conservative fix in sight.

/be

Comment 11

[Jason Orendorff [:jorendorff]]

The conservative fix will be to deoptimize functions that appear inside loops.

However I just realized this can happen to you even if the loop is not in the immediately enclosing function:

(function() {
    var funs = [];
    for (var i = 0; i < 2; i++) {
        var v = i;
        funs[i] = (function() {
                       return function () { return v; };
                   })();
    }
    assertEq(funs[0](), 2);
}

So a function in *any* loop, at any static level, must be deoptimized. I need to hack a bit more, probably adding a bit to JSTreeContext. ETA tomorrow morning.

(Note that the assertEq in comment 9 should also say 2, not 3.)

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: v1

Heh, did I say 2?  How about 1, would you believe 1?  Here are the
really-correct-I-promise tests.

(function () {
    var funs = [];
    for (var i = 0; i < 2; i++) {
        var v = i;
        funs[i] = function() { return v; };
    }
    assertEq(funs[0](), 1);
})();

(function() {
    var funs = [];
    for (var i = 0; i < 2; i++) {
        var v = i;
        funs[i] = (function() {
                       return function () { return v; };
                   })();
    }
    assertEq(funs[0](), 1);
})();

Comment 13

[Brendan Eich [:brendan]]

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

Does the assertion in NoteLValue need to be adjusted?

        if (dn->frameLevel() != tc->staticLevel) {
            /*
             * The above condition takes advantage of the all-ones nature of
             * FREE_UPVAR_COOKIE, and the reserved frame level JS_BITMASK(16).
             * We make a stronger assertion by excluding FREE_UPVAR_COOKIE.
             */
            JS_ASSERT_IF(dn->pn_cookie != FREE_UPVAR_COOKIE,
                         dn->frameLevel() < tc->staticLevel);
            tc->flags |= TCF_FUN_SETS_OUTER_NAME;
        }

Look out for places where the fact that FREE_UPVAR_COOKIE is > any valid frame level is employed to optimize tests.

The check in SetStaticLevel uses >= FREE_STATIC_LEVEL, so that's cool. Please patch or file a followup bug if BumpStaticLevel could overflow //in extremis//.

/be

Comment 14

[Boris Zbarsky [:bzbarsky]]

So does that mean that this function will now fall off trace?  Or is that not affected?

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #11)
> However I just realized this can happen to you even if the loop is not in the
> immediately enclosing function:
> 
> (function() {
>     var funs = [];
>     for (var i = 0; i < 2; i++) {
>         var v = i;
>         funs[i] = (function() {
>                        return function () { return v; };
>                    })();
>     }
>     assertEq(funs[0](), 2);
> }
> 
> So a function in *any* loop, at any static level, must be deoptimized.

Only if the upvar crosses a funbox with inLoop true, where inLoop is set based only on that funbox's tree context. IOW, avoid the outer loop in newFunctionBox, and make use such a "local inLoop" in the dominance analysis.

BumpStaticLevel needs protection. Something like:

eval(Array((1<<14)-2).join("(function(){")+"return (i for (i in g));"+Array((1<<14)-2).join("}())"))

should cause BumpStaticLevel to increment the static level to equal FREE_STATIC_LEVEL as revised by this bug's patch. Pre-existing bug, sorry -- fix or file separately, either's good.

/be

Comment 16

[Brendan Eich [:brendan]]

To answer bz's comment 14 question, the example given should cause deoptimization to use heavyweight functions, which dmandelin is working on tracing.

/be

Comment 17

[Jason Orendorff [:jorendorff]]

Attachment: v2

Stack checking and our recursive-descent parser team up to prevent function nesting from getting anywhere near 1<<14, it turns out.  On my machine, in a DEBUG build, we get as far as 166 levels of nesting before throwing InternalError.

Fixed anyway, I think, but it's impossible to test.

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #17)
> Created an attachment (id=379951) [details]
> v2
> 
> Stack checking and our recursive-descent parser team up to prevent function
> nesting from getting anywhere near 1<<14, it turns out.

Stack limit is configurable.

> On my machine, in a
> DEBUG build, we get as far as 166 levels of nesting before throwing
> InternalError.

Cool, thanks for checking. Could certainly split cookies differently based on this -- 8 bit skip/level + 24 bit slot.

> Fixed anyway, I think, but it's impossible to test.

Try jacking the stack limit?

Reviewing now.

/be

Comment 19

[Brendan Eich [:brendan]]

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

>                         while (afunbox->level != lexdepLevel) {
>+                            if (afunbox->inLoop)
>+                                goto break2;

Could comment briefly about closure evaluating each time through the loop but capturing only the scope chain objects, so the value of the dominating var will be whatever ends up there on the last iteration. Give a brief example?

>+                             * Assert but check anyway, to check future changes
>+                             * that bind eval upvars in the parser.

(Thanks for trimming this comment!)

>                              */
>                             JS_ASSERT(afunbox);
> 
>                             /*
>                              * If this function is reaching up across an
>                              * enclosing funarg, we cannot make a flat
>                              * closure. The display stops working once the
>                              * funarg escapes.
>                              */
>                             if (!afunbox || afunbox->node->isFunArg())
>                                 goto break2;
>                         }
>+                        if (afunbox->inLoop)
>+                            goto break2;

This could just be a break, but perhaps clearer to goto break2 again. Too bad the afunbox->inLoop test has to be repeated. One idea to avoid that at the price of a redundant test (which might be optimized away) is to add !afunbox->inLoop to the while loop's condition. Then this would be the common code both for the trivial break/goto break2, and for the comment with example. YMMV, just wanted to throw the idea out.

>-                        return NULL;
>+                        return false;

Yikes -- thanks for fixing these two. Oh well, 0 is 0.

>     uint32              queued:1,
>-                        level:15,
>+                        inLoop:1,  /* in a loop in parent function */
>+                        level:JSFB_LEVEL_BITS,

Uber-nit: indent the comment more to separate the columnation of it and any future right-hand-side-of-line comments from the ending column of the longest member declarator (in this case the JSFB_LEVEL_BITS bitfield size).

r=me with above considered. Thanks,

/be

Comment 20

[Jason Orendorff [:jorendorff]]

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

Brendan agreed on IRC that we don't need to check afunbox->inLoop inside the loop at all; the new comment in the changeset I pushed explains.

Comment 21

[Robert Sayre]

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

Comment 22

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/93b3f436c099