458851 - TM: for-in loops skip every other value in certain cases

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Andreas Gal :gal]

Testcase:

function f() {
    var a = [1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16];
    var x = 0;
    for (var i in a) {
        i = parseInt(i);
        x++;
    }
    print(x);
}
f();

Comment 1

[Jim Blandy :jimb]

Working on this; feel free to take it back if you've got insights.

Comment 2

[Brendan Eich [:brendan]]

I am working on bugs I self-assign -- please don't take. See the blocking bug, which I am working on first.

/be

Comment 3

[Brendan Eich [:brendan]]

Er, see the revised blocking bug. After that's fixed, the patch for this bug splits JSOP_FOR* up into JSOP_NEXTITER followed by JSOP_IFNE to close the loop, with JSOP_SET* at loop header. The fun part will be the decompiler.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 345023 [details] [diff] [review]
proposed fix

Blake gets emitter/decompiler duty ;-).

/be

Comment 7

[Brendan Eich [:brendan]]

Bob, this patch removes JSOP_FORCONST, which makes code of the form

function f() { const x; for (x in y)... }

a compile-time error, just as ... for (const x in y) ... already is. Two tests need to be version-forked (sorry, I forget the details) so that the error is from now on expected:

js1_5/decompilation/regress-437288-02.js
js1_5/extensions/regress-437288-01.js

/be

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 345023 [details] [diff] [review]
proposed fix

>                     if (ss->inArrayInit || ss->inGenExp) {
>-                        lval = POP_STR();
>-                        rval = POP_STR();
>-                        if (ss->opcodes[ss->top - 1] == JSOP_FORLOCAL) {
>-                            ss->sprinter.offset -= PAREN_SLOP;
>+                        (void) PopOff(ss, JSOP_NOP);
>+                        rval = TOP_STR();
>+                        if (ss->top >= 2 &&

At this point, ss->top must be >= 2, so my mq now has this left operand of && hoisted as a LOCAL_ASSERT.

>                                             ss->opcodes[ss->top - 2] == JSOP_FORLOCAL) {

It was odd that the else clause LOCAL_ASSERT'ed ss->opocodes[ss->top - 2] == JSOP_ENTERBLOCK without checking ss->top >= 2 -- that caught my eye.

/be

Comment 9

[Andreas Gal :gal]

Comment on attachment 345023 [details] [diff] [review]
proposed fix

I suggest a JS_ASSERT on JSOP_CALL in snapshot if its not NEXTITER.

Comment 10

[Bob Clary [:bc] (inactive)]

(In reply to comment #7)

> js1_5/decompilation/regress-437288-02.js
> js1_5/extensions/regress-437288-01.js

k, thx. with any luck I'll have an update for the tests and failures this evening.

Comment 11

[Brendan Eich [:brendan]]

Fixed in tm:

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

Will land in m-c shortly.

/be

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #11)
> Will land in m-c shortly.

Make that tomorrow, after chasing down fuzz-found followups. Thanks again to Jesse and his wonder-fuzzer.

/be

Comment 13

[Brendan Eich [:brendan]]

firebot: Check-in: http://hg.mozilla.org/tracemonkey/rev/05c20a65b1df - Jason Orendorff - Backed out changeset d4fe79372140 (bug 458851) due to persistent orange on TraceMonkey tinderboxes.
[10:52am] firebot: http://hg.mozilla.org/tracemonkey/rev/64a079564908 - Jason Orendorff - Merge backout of d4fe79372140.

Debugging...

/be

Comment 14

[Brendan Eich [:brendan]]

This patch exposed a latent bug fixed here:

http://hg.mozilla.org/tracemonkey/rev/040bfff82cde

So I relanded this bug's patch:

http://hg.mozilla.org/tracemonkey/rev/08b215e43638

/be

Comment 16

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/37b3fdbb0f07
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-458851.js,v  <--  regress-458851.js
initial revision: 1.1

Comment 17

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2