510642 - TM: Traced JSOP_BINDNAME is totally busted

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

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

(function () {
    var x;
    (eval("\
    (function () {\
        for (y in [0, 0]) let(a)((function () {\
            for (w in [0, 0])\
                x = 0\
        })());\
        with({}) {}\
    })\
    "))()
})()

crashes js opt shell from TM branch (31571:fc0bfd8bf9e9) with -j at js_CallIteratorNext near null and asserts dbg shell at Assertion failure: i < fun->u.i.nvars, at ../jsfun.cpp:1000

autoBisect shows this is probably related to bug 495329 :

The first bad revision is:
changeset:   30697:60a9ef4e1a3d
user:        David Mandelin
date:        Mon Jul 27 18:13:53 2009 -0700
summary:     Bug 495329: Trace JSOP_BINDNAME/JSOP_SETNAME for closures, r=brendan

Comment 1

[David Mandelin [:dmandelin]]

Here is an easier-to-understand test case of the same bug (it prints the wrong value instead of asserting):

function f() {
  var x = 11;

  function g() {
    var y = 12;

    function h() {
      for (var i = 0; i < 5; ++i) {
	y = 4;
	x = i * 2;
      }
    }
    h();

    print('y = ' + y);
  }
  g();

  print('x = ' + x);
} 

f();

The problem is that the setname tracing code is missing the part that navigates up N frames to set the right variable--instead we are always setting things inside the immediately enclosing function g(). Logically, it seems like the traced code for bindname should do this.

Comment 2

[David Mandelin [:dmandelin]]

Attachment: Patch

This also fixes bug 510437.

Comment 3

[David Mandelin [:dmandelin]]

Comment on attachment 394938 [details] [diff] [review]
Patch

I think it might crash a jstest, so no need to review before I check that.

Comment 4

[Jim Blandy :jimb]

For what it's worth, this patch looks like the right thing to me.  I explain more in bug 510437 comment 9 and its referents.

Comment 5

[Jim Blandy :jimb]

Actually, the patch aborts when js_FindIdentifierBase chooses the global object; this shouldn't be necessary.  Test case:

var x = 1;
function f() {
  function g() { 
    var t=0;
    for (var i=0; i<3; i++)
      x = i;
  };
  g();
  eval("var x = 2");
  g();
  print("x = " + uneval(x));
}
f();
print ("x = " + x);

aborts with:

abort: 11616: Can only search through call objects for JSOP_BINDNAME

because obj is the global object.

Comment 6

[David Mandelin [:dmandelin]]

Good catch, although that particular example doesn't trace anyway, because of the same "special setter" issue. I did update the patch so that it does trace jsop_bindname when the search reaches the global.

Comment 7

[David Mandelin [:dmandelin]]

Attachment: Patch

Comment 8

[Jason Orendorff [:jorendorff]]

I think this is a test of the "foreshadowing" issue we discussed on IRC. Before tracing begins, this creates two closures whose scopes have the same shape, but different-shaped enclosing scopes along the scope chain.

The test dutifully fails, but I'm not totally convinced. It might not be testing what I think it's testing.

var d = 1;
function heavy(x) {
    eval(x);
    return function lite() {
        var s = 0;
        for (var i = 0; i < 9; i++)
            s += d;
        return s;
    };
}

var f1 = heavy("1");
var f2 = heavy("var d = 100;");
assertEq(f1(), 9);
assertEq(f2(), 900);

Comment 11

[David Mandelin [:dmandelin]]

Attachment: Test cases

This collects all the test cases for the bugs that got folded into this one, as well as from IRC, etc. The spec is roughly to make these tests pass.

Comment 12

[David Mandelin [:dmandelin]]

Attachment: Test cases

Missed a test last time.

Comment 13

[David Mandelin [:dmandelin]]

The test case in comment 8 does in fact fail, but it's not part of this bug; it doesn't use SETNAME at all. I think it's a related bug in JSOP_NAME.

Comment 14

[David Mandelin [:dmandelin]]

Attachment: Patch 3

This is going to run on try overnight. It passes all the shell tests.

Comment 15

[David Mandelin [:dmandelin]]

Patch 3 passed try server last night.

Comment 16

[Jim Blandy :jimb]

Comment on attachment 395216 [details] [diff] [review]
Patch 3

Could we drop the whitespace changes?

Comment 17

[David Mandelin [:dmandelin]]

Attachment: Patch 3 without whitespace cleanup

Comment 18

[David Mandelin [:dmandelin]]

Attachment: Patch 4 (patch 3 rebased to tip)

Please review promptly--I have 2 other patches pending that go on top of this, and these patches also fix a couple of other outstanding bugs.

Comment 19

[Jim Blandy :jimb]

Almost done looking this over; should be done tomorrow morning.  Nit for today: the comments atop traverseScopeChain should mention obj_ins and obj2_ins by name, and explain what they are.

Comment 20

[Jim Blandy :jimb]

Comment on attachment 395877 [details] [diff] [review]
Patch 4 (patch 3 rebased to tip)

Woot!

Comment 21

[David Mandelin [:dmandelin]]

Pushed to TM as 080548cb428d.

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/080548cb428d

Comment 24

[Robert Sayre]

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

Comment 25

[Bob Clary [:bc] (inactive)]

js/src/trace-test/tests/basic/bug510437-2.js
js/src/trace-test/tests/basic/bug510437.js
js/src/trace-test/tests/basic/bug511214.js
js/src/trace-test/tests/basic/setCall.js
js/src/trace-test/tests/basic/setCallEvalMiddle.js
js/src/trace-test/tests/basic/setCallEvalMiddle2.js
js/src/trace-test/tests/basic/setCallGlobal.js

Comment 26

[Bob Clary [:bc] (inactive)]

v 1.9.3, 1.9.2

Comment 27

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

Bug 513038 has been duped against this bug. The crashes reported there affects 1.9.1. There are a couple of DeepBail crashes out there. Take as example bp-ad84b5fa-7463-4f44-8a26-784fe2091120. Considering for 1.9.1?