Closed Bug 443071 Opened 12 years ago Closed 11 years ago

Assertion failure with "for (;;[]=[])"

Categories

(Core :: JavaScript Engine, defect, P2, critical)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1

People

(Reporter: jruderman, Assigned: brendan)

References

(Blocks 1 open bug)

Details

(4 keywords)

Attachments

(2 files, 2 obsolete files)

js> print(function() { for (;;[]=[]) { } })  
Assertion failure: top < ss->printer->script->depth, at jsopcode.cpp:894

This bug also shows up as:
Assertion failure: top != 0, at jsopcode.cpp:919

jsfunfuzz is hitting this a lot.

Is this a regression from bug 441477 or one of its followups?
Due to bug 441477 AFAICT. Taking.

/be
Assignee: general → brendan
Blocks: 441477
OS: Mac OS X → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.9.1
OS: All → Mac OS X
Priority: P1 → --
Hardware: All → PC
Target Milestone: mozilla1.9.1 → ---
OS: Mac OS X → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.9.1
I take it back -- this is old, I get the same assertbotch in my 1.8 branch and cvs 1.9.x trunk debug shell builds.

/be
No longer blocks: 441477
Status: NEW → ASSIGNED
Priority: P1 → P2
You're right.  I wonder why I suddenly started hitting it so often.
from bug 371802 comment 6

1.9.1

js> function f(a,b,n){for(var [i,j]=[a,b];i<n;[i,j]=[a,b])print(i)}
js> f
Assertion failure: top != 0, at jsopcode.cpp:957

but 1.8.1/1.9.0

js> function f(a,b,n){for(var [i,j]=[a,b];i<n;[i,j]=[a,b])print(i)}
js> f
function f(a, b, n) {
    for (var [i, j] = [a, b]; (i < n); [i, j] = [a, b]) {
        print(i);
    }
}

Comment 4 shows how this bug's particular testcase (comment 0) failed due to an older flaw, one that predates the 1.9.1 work, but that the 1.9.1 work regressed other cases that used to succeed. I'd like to fix everything here, soon.

/be
This bug is annoying because it keeps managing to trigger different assertions.
Flags: blocking1.9.1?
Attached patch fix (obsolete) — Splinter Review
Jesse, could you please apply and test this? Thanks,

/be
Attachment #346628 - Flags: review?(mrbkap)
I had time to apply the patch to test this just now, seems to fix the assert in 1.9.1:

$ ./js
js> print(function() { for (;;[]=[]) { } })
function () {
    for (;; [] = []) {
    }
}
js>

$ ./js
js> function f(a,b,n){for(var [i,j]=[a,b];i<n;[i,j]=[a,b])print(i)}
js> f
function f(a, b, n) {
    for (var [i, j] = [a, b]; (i < n);    [i, j] = [a, b];
 ) {
        print(i);
    }
}
js>
Thanks, Gary!

Blake, if you r+ soon I will get this into tm and it can ride from there into m-c.

/be
(In reply to comment #8)
> $ ./js
> js> function f(a,b,n){for(var [i,j]=[a,b];i<n;[i,j]=[a,b])print(i)}
> js> f
> function f(a, b, n) {
>     for (var [i, j] = [a, b]; (i < n);    [i, j] = [a, b];
>  ) {
>         print(i);
>     }
> }

Er, wait -- that's not right! :-(

/be
Attachment #346628 - Attachment is obsolete: true
Attachment #346628 - Flags: review?(mrbkap)
The JSOP_POPN decompiler code is out of date. It thinks a POPN followed by a GOTO must be the third part of a for(;;) loop head, but since bug 441477 was fixed the for loop (like all other loops now) has a backward IFNE as its loop-closing jump. Wherefore this bug! Citing the dependency.

/be
Blocks: 441477
Forgot to say that of course the condition precedes the IFNE, so there is no dedicated opcode after the POPN in the update part of the for(;;) head.

for (init; cond; update) body

turns into

init; GOTO L1; L2: body; update; L1: cond; IFNE L2

New approach needed here.

/be
Attached patch better fix (obsolete) — Splinter Review
Lots of comments. The cases to walk through invole for (init; cond; update) body with init, cond, update, and body variously empty or not, and group assignments or not. Also let ([]=[]) {}, your old fave!

/be
Attachment #346829 - Flags: review?(mrbkap)
(In reply to comment #13)
> Created an attachment (id=346829) [details]
> better fix
> 
> Lots of comments. The cases to walk through invole for (init; cond; update)
> body with init, cond, update, and body variously empty or not, and group
> assignments or not. Also let ([]=[]) {}, your old fave!
> 
> /be

Here's my results from testing this patch:

$ ./js
js> print(function() { for (;;[]=[]) { } })
function () {
    for (;; [] = []) {
    }
}
js> 

$ ./js
js> function f(a,b,n){for(var [i,j]=[a,b];i<n;[i,j]=[a,b])print(i)}
js> f
js>
Attached patch best fixSplinter Review
Trivial fix to last-minute LOCAL_ASSERT (which is why opt silently decompiles nothing). Got the operands of - reversed. Gary, can you pound on this? Thanks,

/be
Attachment #346829 - Attachment is obsolete: true
Attachment #346857 - Flags: review?(mrbkap)
Attachment #346829 - Flags: review?(mrbkap)
Patch is against tracemonkey repo, I forgot to add.

/be
(In reply to comment #15)
> Created an attachment (id=346857) [details]
> best fix
> 
> Trivial fix to last-minute LOCAL_ASSERT (which is why opt silently decompiles
> nothing). Got the operands of - reversed. Gary, can you pound on this? Thanks,

Yup, have always been testing against TM repo, results for this third patch here:

$ ./js
js> print(function() { for (;;[]=[]) { } })
function () {
    for (;; [] = []) {
    }
}
js>

$ ./js
js> function f(a,b,n){for(var [i,j]=[a,b];i<n;[i,j]=[a,b])print(i)}
js> f
function f(a, b, n) {
    for (var [i, j] = [a, b]; i < n; [i, j] = [a, b]) {
        print(i);
    }
}
js>
Gary: thanks, I ran those tests too. Got any more? Running jsfunfuzz would be good. Thanks again,

/be
This patch fixes the testcase I had in my whenfixed directory for this bug.  Running jsfunfuzz now with this bug's assertions in known_assertions commented out.
Flags: blocking1.9.1? → blocking1.9.1+
Comment on attachment 346857 [details] [diff] [review]
best fix

Looks good.
Attachment #346857 - Flags: review?(mrbkap) → review+
Fixed in tm:

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

Closing since I really BELIEVE that we will merge with m-c today!

/be
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Here's an incomplete patch for branch -- it's a tad too complex for a straight forward trivial adaptation.

This patch only tackles jsemit.c and while most lines were quite simple to adapt, I didn't find the botch below in jsemit.c to change:

             /* Set the first note offset so we can find the loop condition. */
             if (!js_SetSrcNoteOffset(cx, cg, (uintN)noteIndex, 0,
-                                     CG_OFFSET(cg) - top)) {
+                                     CG_OFFSET(cg) - tmp)) {
                 return JS_FALSE;
             }


Also, jsopcode.c is very different such that adapting the patch for 1.9.0.x will take more than just trivial-ness.

Any help is welcomed as this issue still occurs in 1.9.0.x but fully adapting Brendan's patch is still above my level.
Flags: wanted1.9.0.x?
Not sure we can't live with this for 1.9.0. Unless it's absolutely necessary to fix a security bug (or maybe a topcrash) we don't like mucking with the js engine.
Flags: wanted1.9.0.x?
added test http://hg.mozilla.org/mozilla-central/rev/ed57e3d66597 and cvs.
Flags: in-testsuite+
Flags: in-litmus-
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.