677957 - Assertion failure: !cx->iterValue.isMagic(JS_NO_ITER_VALUE), at jsiter.cpp:1017

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following code asserts on mozilla-inbound (revision 609f37c36bd7, no options required):


function make(g) {
    var o = {f: function(a,b) { return a*b; }, g: g};
    return o;
}
var x = make(function(c) { return c*z; });
var expect = '';
test();
function test() {
  try {
    for each (new test(this, expect). test.__proto__ in x) {}
  } catch(ex) {  }
}


There's also a bug open on TI with the same assertion (bug 677032), maybe they are related.

Comment 1

[Jason Orendorff [:jorendorff]]

Sigh. I'll be astonished if a patch of mine isn't implicated in this.

Without recursion:

function test() {
    for each (var i in []) {}
}
for each (new test().p in [0]) {}

Comment 2

[Jason Orendorff [:jorendorff]]

Bisect says:

The first bad revision is:
changeset:   73021:938c1a177114
user:        Jason Orendorff <[e-mail]>
date:        Tue Jul 19 11:00:43 2011 -0500
summary:     Bug 648175 - Remove JSOP_FOR*. Second second landing, to coin a phrase. r=dvander.

Taking.

Comment 3

[Jason Orendorff [:jorendorff]]

Pretty obvious what's going on here. cx->iterValue is being overwritten.

Before 938c1a177114:

    /* Location to stash the iteration value between JSOP_MOREITER and
       JSOP_FOR*. */
    js::Value           iterValue;

After:

    /* Location to stash the iteration value between JSOP_MOREITER and
       JSOP_ITERNEXT. */
    js::Value           iterValue;

The difference is that other stuff can happen between MOREITER and ITERNEXT. Aurora is probably affected. I'll get to this Monday at the latest.

Comment 4

[Jason Orendorff [:jorendorff]]

Forgot about this one, but I'm spending today on it.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: WIP 1

This patch works except for the decompiler.

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: v2

The part of this patch that explains all the rest is:

>@@ ... @@ EmitAssignment(JSContext *cx, JSCodeGene
>     /* Now emit the right operand (it may affect the namespace). */
>     if (rhs) {
>         if (!js_EmitTree(cx, cg, rhs))
>             return false;
>     } else {
>-        /* The value to assign is the next enumeration value in a for-in loop. */
>-        if (js_Emit2(cx, cg, JSOP_ITERNEXT, offset) < 0)
>+        /*
>+         * The value to assign is the next enumeration value in a for-in loop.
>+         * That value is produced by a JSOP_ITERNEXT op, previously emitted.
>+         * If offset == 1, that slot is already at the top of the
>+         * stack. Otherwise, rearrange the stack to put that value on top.
>+         */
>+        if (offset != 1 && js_Emit2(cx, cg, JSOP_PICK, offset - 1) < 0)
>             return false;
>     }

Comment 3 above explains why this change is necessary.

(The decompiler thing was a pre-existing bug, harmless til now.)

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 557518 [details] [diff] [review]
v2

Review of attachment 557518 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, good to see that offset go.

::: js/src/jsopcode.cpp
@@ +2724,5 @@
>                  break;
>  
> +              case JSOP_PICK:
> +              {
> +                uintN i = pc[1];

GET_UINT8?

Comment 8

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/502c33ae0d81

There's no GET_UINT8, but I'll add one in a quick follow-up if you like.

Comment 9

[Marco Bonardo [:mak]]

backed out Bug 677957 and Bug 672893 since one of these caused Android mochitest 5 permaorange in test_jQuery.html and I couldn't identify the culprit at first glance.

1120 INFO TEST-PASS | /tests/dom/tests/mochitest/ajax/jquery/test_jQuery.html | fx module: JS 0 to show - Height must be reset to 0: 0px: 0
1121 INFO TEST-PASS | /tests/dom/tests/mochitest/ajax/jquery/test_jQuery.html | fx module: JS 0 to show - Opacity must be reset to 0: 0: 0
1122 INFO TEST-PASS | /tests/dom/tests/mochitest/ajax/jquery/test_jQuery.html | fx module: JS 0 to show - Make sure height is auto.
1123 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/ajax/jquery/test_jQuery.html | Test timed out.
1124 INFO TEST-END | /tests/dom/tests/mochitest/ajax/jquery/test_jQuery.html | finished in 300232ms

Comment 10

[Jason Orendorff [:jorendorff]]

No clue why this would cause a test to time out on Android. It is reproducible though (via Try Server).

I was hoping to work on this here, since I got a Galaxy Tab, but now it's dead (bug 688583). Unassigning myself from this bug for now.

This bug means for-in loops are really really broken. I am nervous about it.

Comment 11

[[e-mail]]

So, if I get this assertion, and there's a for...in loop on the JS stack (in my case, in XPCOMUtils_QueryInterface in XPCOMUtils.jsm), then it's probably this bug?

Comment 12

[Jason Orendorff [:jorendorff]]

Quite likely, yes.

Comment 13

[Jason Orendorff [:jorendorff]]

The next step in this bug is to unbitrot the patch and reproduce the problem on try server again. Then, assuming it still reproduces, beg mjrosen for help debugging. Re-taking.

Comment 14

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #13)
> The next step in this bug is to unbitrot the patch and reproduce the problem
> on try server again. Then, assuming it still reproduces, beg mjrosen for
> help debugging. Re-taking.

[Indiana Jones theme music]

Comment 15

[Jason Orendorff [:jorendorff]]

Attachment: v3

Comment 16

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=f67a07f43b09

Comment 17

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #16)
> https://tbpl.mozilla.org/?tree=Try&rev=f67a07f43b09

I restarted the Android oranges that had no summaries, to see if that would get us something more helpful.

Comment 18

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #17)
> (In reply to Jason Orendorff [:jorendorff] from comment #16)
> > https://tbpl.mozilla.org/?tree=Try&rev=f67a07f43b09
> 
> I restarted the Android oranges that had no summaries, to see if that would
> get us something more helpful.

I looked at all the failing logs for Android, and I didn't find any real failures. I've starred them, and restarted them just to see.

Comment 19

[Jason Orendorff [:jorendorff]]

Comment on attachment 646705 [details] [diff] [review]
v3

OK, this should land.

dvander, you reviewed this 10+ months ago; perhaps after that time it's worth another glance.

Comment 20

[Jim Blandy :jimb]

I've closed bug 772770 in favor of the patch in this bug. Bug 777596 is the agreed-upon long-term solution.

Comment 22

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

(In reply to Jim Blandy :jimb from comment #20)
> I've closed bug 772770 in favor of the patch in this bug. Bug 777596 is the
> agreed-upon long-term solution.

As discussed on IRC, it would be nice to check if this patch also fixes bug 677032 (one should add the moar_xml line to attempt to reproduce with recent nightly js shells).

Comment 23

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

(In reply to Jason Orendorff [:jorendorff] from comment #19)
> Comment on attachment 646705 [details] [diff] [review]
> v3

This patch bitrotted a little, when I was testing out the patch on mozilla-inbound changeset aa61e5ec5612 :

applying 677957-c19.patch
patching file js/src/jsinterp.cpp
Hunk #1 FAILED at 1735
1 out of 1 hunks FAILED -- saving rejects to file js/src/jsinterp.cpp.rej
patching file js/src/methodjit/StubCalls.cpp
Hunk #1 FAILED at 1042
1 out of 1 hunks FAILED -- saving rejects to file js/src/methodjit/StubCalls.cpp.rej
patching file js/src/vm/Xdr.h
Hunk #1 FAILED at 19
1 out of 1 hunks FAILED -- saving rejects to file js/src/vm/Xdr.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 677957-c19.patch

Comment 24

[Jason Orendorff [:jorendorff]]

Unbitrotted and pushed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/88798c5eafa9

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/88798c5eafa9

Comment 26

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug677957-1.js.