The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: decoder, Assigned: jorendorff)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Other Branch
mozilla17
x86_64
Linux
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:p2])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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]) {}
(Assignee)

Comment 2

6 years ago
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.
Assignee: general → jorendorff
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
Forgot about this one, but I'm spending today on it.
(Assignee)

Comment 5

6 years ago
Created attachment 557359 [details] [diff] [review]
WIP 1

This patch works except for the decompiler.
(Assignee)

Comment 6

6 years ago
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.)
Attachment #557359 - Attachment is obsolete: true
Attachment #557518 - Flags: review?(dvander)
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?
Attachment #557518 - Flags: review?(dvander) → review+
(Assignee)

Comment 8

6 years ago
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.
Whiteboard: js-triage-needed → [inbound]
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
Whiteboard: [inbound]
Blocks: 683694
(Assignee)

Comment 10

6 years ago
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.
Assignee: jorendorff → general
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?
(Assignee)

Comment 12

5 years ago
Quite likely, yes.
Whiteboard: [js:p2]
(Assignee)

Comment 13

5 years ago
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.
Assignee: general → jorendorff

Comment 14

5 years ago
(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]
(Assignee)

Comment 15

5 years ago
Created attachment 646705 [details] [diff] [review]
v3
Attachment #557518 - Attachment is obsolete: true
(Assignee)

Comment 16

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

Comment 17

5 years ago
(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

5 years ago
(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.
(Assignee)

Comment 19

5 years ago
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.
Attachment #646705 - Flags: review?(dvander)

Comment 20

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

Updated

5 years ago
Duplicate of this bug: 772770
(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).
Attachment #646705 - Flags: review?(dvander) → review+
Blocks: 349611
(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
(Assignee)

Comment 24

5 years ago
Unbitrotted and pushed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/88798c5eafa9
https://hg.mozilla.org/mozilla-central/rev/88798c5eafa9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Depends on: 779850
(Reporter)

Comment 26

4 years ago
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug677957-1.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.