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

VERIFIED FIXED in mozilla1.9.1b2

Status

()

defect
P1
critical
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: gal, Assigned: brendan)

Tracking

(Blocks 1 bug, {testcase, verified1.9.1})

unspecified
mozilla1.9.1b2
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Reporter

Description

11 years ago
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();
Assignee

Updated

11 years ago
Severity: normal → major
Status: NEW → ASSIGNED
OS: Mac OS X → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.9.1b2
Assignee

Updated

11 years ago
Depends on: 381843

Comment 1

11 years ago
Working on this; feel free to take it back if you've got insights.
Assignee: brendan → jim
Assignee

Comment 2

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

Comment 3

11 years ago
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

Updated

11 years ago
Flags: blocking1.9.1+
Assignee

Updated

11 years ago
Severity: major → critical
Assignee

Updated

11 years ago
Depends on: 461248
Assignee

Updated

11 years ago
Blocks: 460875
Assignee

Updated

11 years ago
Duplicate of this bug: 461590
Assignee

Comment 5

11 years ago
Posted patch proposed fixSplinter Review
Attachment #345023 - Flags: review?(gal)
Assignee

Updated

11 years ago
Attachment #345023 - Flags: review?(mrbkap)
Assignee

Comment 6

11 years ago
Comment on attachment 345023 [details] [diff] [review]
proposed fix

Blake gets emitter/decompiler duty ;-).

/be
Assignee

Comment 7

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

Comment 8

11 years ago
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
Reporter

Comment 9

11 years ago
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+

Comment 10

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

Comment 11

11 years ago
Fixed in tm:

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

Will land in m-c shortly.

/be

Updated

11 years ago
Depends on: 461930

Updated

11 years ago
Depends on: 461932
Assignee

Comment 12

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

Updated

11 years ago
Depends on: 461930, 461932

Updated

11 years ago
Depends on: 462005
Assignee

Comment 13

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

Comment 14

11 years ago
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

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee

Updated

11 years ago
Duplicate of this bug: 462504

Updated

11 years ago
Depends on: 463997

Comment 16

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

Updated

11 years ago
Depends on: 464645

Updated

11 years ago
Blocks: abp

Comment 17

11 years ago
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.