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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Jesse Ruderman]

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?

Comment 1

[Brendan Eich [:brendan]]

Due to bug 441477 AFAICT. Taking.

/be

Comment 2

[Brendan Eich [:brendan]]

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

Comment 3

[Jesse Ruderman]

You're right.  I wonder why I suddenly started hitting it so often.

Comment 4

[Bob Clary [:bc] (inactive)]

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 5

[Brendan Eich [:brendan]]

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

Comment 6

[Jesse Ruderman]

This bug is annoying because it keeps managing to trigger different assertions.

Comment 7

[Brendan Eich [:brendan]]

Attachment: fix

Jesse, could you please apply and test this? Thanks,

/be

Comment 8

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

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>

Comment 9

[Brendan Eich [:brendan]]

Thanks, Gary!

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

/be

Comment 10

[Brendan Eich [:brendan]]

(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

Comment 11

[Brendan Eich [:brendan]]

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

Comment 12

[Brendan Eich [:brendan]]

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

Comment 13

[Brendan Eich [:brendan]]

Attachment: 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

Comment 14

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

(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>

Comment 15

[Brendan Eich [:brendan]]

Attachment: 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,

/be

Comment 16

[Brendan Eich [:brendan]]

Patch is against tracemonkey repo, I forgot to add.

/be

Comment 17

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

(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>

Comment 18

[Brendan Eich [:brendan]]

Gary: thanks, I ran those tests too. Got any more? Running jsfunfuzz would be good. Thanks again,

/be

Comment 19

[Jesse Ruderman]

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.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 346857 [details] [diff] [review]
best fix

Looks good.

Comment 21

[Brendan Eich [:brendan]]

Fixed in tm:

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

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

/be

Comment 22

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

Attachment: incomplete patch for 1.9.0.x

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.

Comment 23

[Daniel Veditz [:dveditz]]

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.

Comment 24

[Bob Clary [:bc] (inactive)]

added test http://hg.mozilla.org/mozilla-central/rev/ed57e3d66597 and cvs.

Comment 25

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2