function test() { var a = ['x', '', '', '', '', '', '', 'x']; var b = ''; for (var i = 0; i < a.length; i++) { (function() { a[i].replace(/x/, function() { return b; }); }()); } } test(); Expected: no output Observed: test.js:6: ReferenceError: b is not defined The outer lambda (function() { a[i].replace(...); }) is being compiled to a null closure, but the nested lambda loads b using a JSOP_NAME instruction. The interpreter conceals the bug: JSOP_LAMBDA always parents the new lambda to the real scope chain, so when we get to JSOP_NAME, we have a correct scope chain to search. But on trace, we parent the outer lambda to the global, so we can't later resolve 'b' correctly. (In this test case, we are trying to resolve 'b' after deep-bailing in str_replace, but that isn't really relevant. By the time we deep-bail, the bug has already struck, as evidenced by the wrong-parented lambda we've made.)

Hmm. I was expecting a compiler fix. This would fix the test case at hand, though, and it would JIT code behave more like the interpreter. Brendan, it is a compiler bug for JSOP_NAME to be emitted for that `b` when the outer lambda is a null closure, right? If so, I'd rather see us go the opposite direction -- fix the compiler bug, then make the interpreter behave like the JIT.

Comment on attachment 441213 [details] [diff] [review] front end fix No, this patch flunks the following varation: function test() { var a = ['x', '', '', '', '', '', '', 'x']; var b = ''; for (var i = 0; i < a.length; i++) { (function(s) { s.replace(/x/, function() { return b; }); }(a[i])); } } test();

We used to make intervening functions heavyweight. The upvar2 bug tried to flag only the home of the upvar as heavyweight. That introduced this bug. Here I abuse TCF_FUN_SETS_OUTER_NAME to fix the bug. Have to think of a better way... /be

Would TCF_FUN_NEEDS_FULL_SCOPE_CHAIN be a better name? It would certainly be longer :-/. /be

TCF_FUN_ENTRAINS_SCOPES... better, shorter. /be

Comment on attachment 441700 [details] [diff] [review] proposed fix >- } else if (!mutation && !(funbox->tcflags & TCF_FUN_IS_GENERATOR)) { >+ } else if (!mutation && >+ !(~funbox->tcflags >+ & (TCF_FUN_IS_GENERATOR | TCF_FUN_NOT_NULL_CLOSURE))) { The ~ in there seems wrong. It hurts my brain a little that this bit is only checked in the one case (of about 6) where we really need it. A comment wouldn't help though, so don't worry about it. All three names for TCF_FUN_NOT_NULL_CLOSURE seem fine to me, so take your pick. This needs tests, of course. r=me with those addressed.

(In reply to comment #8) > (From update of attachment 441700 [details] [diff] [review]) > >- } else if (!mutation && !(funbox->tcflags & TCF_FUN_IS_GENERATOR)) { > >+ } else if (!mutation && > >+ !(~funbox->tcflags > >+ & (TCF_FUN_IS_GENERATOR | TCF_FUN_NOT_NULL_CLOSURE))) { > > The ~ in there seems wrong. We want both bits to be set. So the ~ inverts tcflags so only if both are set, will both be clear in the complement. Then masking with both and insisting on 0 via the outer ! finishes the job. We use this idiom elsewhere but with == 0 on the right, although it does tend to get lost. Going with TCF_FUN_ENTRAINS_SCOPES. Tests, hmm. Where were they? /be

http://hg.mozilla.org/tracemonkey/rev/3a9506850292 /be

(In reply to comment #9) > (In reply to comment #8) > > The ~ in there seems wrong. > > We want both bits to be set. [...] I follow everything after that; but I think we want both bits to be clear. function test(a) { function g() { // a generator that entrains scopes yield function () a; } a = 3; } dis("-r", test); // shows that g is a NULL_CLOSURE

Here's a better test, suitable for trace-tests. function f(a) { function g() { yield function () a; } if (a == 8) return g(); a = 3; } var x; for (var i = 0; i < 9; i++) x = f(i); x.next()(); // ReferenceError: a is not defined.

We certainly do want both bits clear! Inverted brain yesterday. Thanks for the save here. Followup fix in a hurry: http://hg.mozilla.org/tracemonkey/rev/90112fdfaa8f /be

Beware patterns, the left brain loves the seize on them like a plastic trap. The right brain must remain in charge. Many right brain thanks to jorendorff. /be

Comment-fix followup: http://hg.mozilla.org/tracemonkey/rev/b0ccac9de72a /be