JSOP_NAME inside null closure leads to bogus ReferenceError on trace

RESOLVED FIXED in mozilla1.9.3a5

Status

()

defect
P1
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jorendorff, Assigned: brendan)

Tracking

Other Branch
mozilla1.9.3a5
Points:
---

Firefox Tracking Flags

(blocking2.0 beta1+)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 3 obsolete attachments)

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.)

Updated

9 years ago
blocking2.0: --- → beta1+
Assignee: general → dmandelin
Posted patch Patch (obsolete) — Splinter Review
Attachment #440828 - Flags: review?(jorendorff)
Reporter

Comment 2

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

Comment 3

9 years ago
Posted patch front end fix (obsolete) — Splinter Review
Assignee: dmandelin → brendan
Attachment #440828 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #441213 - Flags: review?(jorendorff)
Attachment #440828 - Flags: review?(jorendorff)
Reporter

Comment 4

9 years ago
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();
Attachment #441213 - Flags: review?(jorendorff) → review-
Assignee

Updated

9 years ago
Priority: -- → P1
Target Milestone: --- → mozilla1.9.3a5
Assignee

Comment 5

9 years ago
Posted patch hack fix, not for committing (obsolete) — Splinter Review
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
Attachment #441213 - Attachment is obsolete: true
Attachment #441228 - Flags: feedback?(jorendorff)
Assignee

Comment 6

9 years ago
Posted patch proposed fixSplinter Review
Would TCF_FUN_NEEDS_FULL_SCOPE_CHAIN be a better name? It would certainly be longer :-/.

/be
Attachment #441228 - Attachment is obsolete: true
Attachment #441700 - Flags: review?(jorendorff)
Attachment #441228 - Flags: feedback?(jorendorff)
Assignee

Comment 7

9 years ago
TCF_FUN_ENTRAINS_SCOPES... better, shorter.

/be
Reporter

Comment 8

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

Updated

9 years ago
Attachment #441700 - Flags: review?(jorendorff) → review+
Assignee

Comment 9

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

Comment 10

9 years ago
http://hg.mozilla.org/tracemonkey/rev/3a9506850292

/be
Whiteboard: fixed-in-tracemonkey
Reporter

Comment 11

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

Comment 12

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

Comment 13

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

Comment 14

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

9 years ago
http://hg.mozilla.org/mozilla-central/rev/3a9506850292
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.