Closed Bug 458851 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b2

People

(Reporter: gal, Assigned: brendan)

References

Details

(Keywords: testcase, verified1.9.1)

Attachments

(1 file)

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();
Severity: normal → major
Status: NEW → ASSIGNED
OS: Mac OS X → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.9.1b2
Depends on: 381843
Working on this; feel free to take it back if you've got insights.
Assignee: brendan → jim
I am working on bugs I self-assign -- please don't take. See the blocking bug, which I am working on first.

/be
Assignee: jim → 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
Depends on: 441479
No longer depends on: 381843
Flags: blocking1.9.1+
Severity: major → critical
Depends on: 461248
Blocks: 460875
Duplicate of this bug: 461590
Attached patch proposed fixSplinter Review
Attachment #345023 - Flags: review?(gal)
Attachment #345023 - Flags: review?(mrbkap)
Comment on attachment 345023 [details] [diff] [review]
proposed fix

Blake gets emitter/decompiler duty ;-).

/be
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 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 on attachment 345023 [details] [diff] [review]
proposed fix

I suggest a JS_ASSERT on JSOP_CALL in snapshot if its not NEXTITER.
Attachment #345023 - Flags: review?(gal) → review+
(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.
Attachment #345023 - Flags: review?(mrbkap) → review+
Fixed in tm:

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

Will land in m-c shortly.

/be
Depends on: 461930
Depends on: 461932
(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
No longer depends on: 461930, 461932
Depends on: 461930, 461932
Depends on: 462005
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
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Duplicate of this bug: 462504
Depends on: 463997
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
Flags: in-testsuite+
Flags: in-litmus-
Depends on: 464645
Blocks: abp
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.