Last Comment Bug 677957 - Assertion failure: !cx->iterValue.isMagic(JS_NO_ITER_VALUE), at jsiter.cpp:1017
: Assertion failure: !cx->iterValue.isMagic(JS_NO_ITER_VALUE), at jsiter.cpp:1017
Status: RESOLVED FIXED
[js:p2]
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86_64 Linux
: -- critical (vote)
: mozilla17
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
: 772770 (view as bug list)
Depends on: 779850
Blocks: jsfunfuzz langfuzz 683694
  Show dependency treegraph
 
Reported: 2011-08-10 10:29 PDT by Christian Holler (:decoder)
Modified: 2013-01-14 07:51 PST (History)
9 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP 1 (15.25 KB, patch)
2011-08-31 16:18 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Review
v2 (17.02 KB, patch)
2011-09-01 09:06 PDT, Jason Orendorff [:jorendorff]
dvander: review+
Details | Diff | Review
v3 (16.84 KB, patch)
2012-07-27 13:47 PDT, Jason Orendorff [:jorendorff]
dvander: review+
Details | Diff | Review

Description Christian Holler (:decoder) 2011-08-10 10:29:09 PDT
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] 2011-08-10 10:41:43 PDT
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] 2011-08-18 11:41:57 PDT
Bisect says:

The first bad revision is:
changeset:   73021:938c1a177114
user:        Jason Orendorff <jorendorff@mozilla.com>
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] 2011-08-18 14:28:56 PDT
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] 2011-08-31 07:58:12 PDT
Forgot about this one, but I'm spending today on it.
Comment 5 Jason Orendorff [:jorendorff] 2011-08-31 16:18:45 PDT
Created attachment 557359 [details] [diff] [review]
WIP 1

This patch works except for the decompiler.
Comment 6 Jason Orendorff [:jorendorff] 2011-09-01 09:06:41 PDT
Created attachment 557518 [details] [diff] [review]
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] 2011-09-01 13:55:56 PDT
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] 2011-09-02 16:25:19 PDT
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] 2011-09-03 03:26:41 PDT
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] 2011-09-22 13:05:26 PDT
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 neil@parkwaycc.co.uk 2012-01-25 02:19:19 PST
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] 2012-01-25 08:10:21 PST
Quite likely, yes.
Comment 13 Jason Orendorff [:jorendorff] 2012-07-12 13:06:01 PDT
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 2012-07-12 13:12:03 PDT
(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] 2012-07-27 13:47:45 PDT
Created attachment 646705 [details] [diff] [review]
v3
Comment 16 Jason Orendorff [:jorendorff] 2012-07-27 16:01:16 PDT
https://tbpl.mozilla.org/?tree=Try&rev=f67a07f43b09
Comment 17 Jim Blandy :jimb 2012-07-31 15:20:48 PDT
(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 2012-07-31 15:29:34 PDT
(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] 2012-07-31 15:45:24 PDT
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 2012-07-31 15:48:26 PDT
I've closed bug 772770 in favor of the patch in this bug. Bug 777596 is the agreed-upon long-term solution.
Comment 21 Jim Blandy :jimb 2012-07-31 15:50:53 PDT
*** Bug 772770 has been marked as a duplicate of this bug. ***
Comment 22 Gary Kwong [:gkw] [:nth10sd] 2012-07-31 16:28:44 PDT
(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] 2012-08-01 14:59:32 PDT
(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] 2012-08-01 15:09:11 PDT
Unbitrotted and pushed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/88798c5eafa9
Comment 25 Ed Morley [:emorley] 2012-08-02 06:23:30 PDT
https://hg.mozilla.org/mozilla-central/rev/88798c5eafa9
Comment 26 Christian Holler (:decoder) 2013-01-14 07:51:23 PST
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug677957-1.js.

Note You need to log in before you can comment on or make changes to this bug.