Christian Holler (:decoder) Reporter
	Description • 13 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.

	Jason Orendorff [:jorendorff] Assignee
	Comment 1 • 13 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]) {}

	Jason Orendorff [:jorendorff] Assignee
	Comment 2 • 13 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.

	Jason Orendorff [:jorendorff] Assignee
	Comment 3 • 13 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.

	Jason Orendorff [:jorendorff] Assignee
	Comment 4 • 13 years ago

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

	Jason Orendorff [:jorendorff] Assignee
	Comment 5 • 13 years ago

This patch works except for the decompiler.

	Jason Orendorff [:jorendorff] Assignee
	Comment 6 • 13 years ago

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.)

	David Anderson [:dvander] - inactive, e-mail if emergency
	Comment 7 • 13 years ago

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?

	Jason Orendorff [:jorendorff] Assignee
	Comment 8 • 13 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.

	Marco Bonardo [:mak]
	Comment 9 • 13 years ago

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

	Jason Orendorff [:jorendorff] Assignee
	Comment 10 • 13 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.

	neil@parkwaycc.co.uk
	Comment 11 • 13 years ago

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?

	Jason Orendorff [:jorendorff] Assignee
	Comment 12 • 13 years ago

Quite likely, yes.

	Jason Orendorff [:jorendorff] Assignee
	Comment 13 • 12 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.

	Jim Blandy :jimb
	Comment 14 • 12 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]

	Jason Orendorff [:jorendorff] Assignee
	Comment 16 • 12 years ago

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

	Jim Blandy :jimb
	Comment 17 • 12 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.

	Jim Blandy :jimb
	Comment 18 • 12 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.

	Jason Orendorff [:jorendorff] Assignee
	Comment 19 • 12 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.

	Jim Blandy :jimb
	Comment 20 • 12 years ago

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

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 22 • 12 years ago

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

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 23 • 12 years ago

(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

	Jason Orendorff [:jorendorff] Assignee
	Comment 24 • 12 years ago

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

	Ed Morley [:emorley]
	Comment 25 • 12 years ago

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

	Christian Holler (:decoder) Reporter
	Comment 26 • 12 years ago

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