963641 - Assertion failure: pair->isKind(PNK_COLON), at frontend/Parser.cpp or Assertion failure: false, at jsreflect.cpp or Assertion failure: pn->isKind(PNK_COLON), at jit/AsmJS.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

({__proto__:x}=

asserts js debug shell on m-c changeset bfe4ed6d47ce without any CLI arguments at Assertion failure: pair->isKind(PNK_COLON), at frontend/Parser.cpp

My configure flags are:

CC="clang -Qunused-arguments" AR=ar CXX="clang++ -Qunused-arguments" sh ./configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-exact-rooting --with-ccache --enable-threadsafe <other NSPR options>

=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20140123130637" and the hash "1435517d890c".
The "bad" changeset has the timestamp "20140123134019" and the hash "f2e86f6fef07".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1435517d890c&tochange=f2e86f6fef07

Waldo, is bug 948583 a likely regressor?

Comment 1

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

Reflect.parse("({__proto__: window})");

causes Assertion failure: false, at jsreflect.cpp

and has the same regression window.

Comment 2

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

Attachment: stack for testcase in comment 1

Comment 3

[Jeff Walden [:Waldo]]

So, I fail at fully distinguishing all the existing JSOP_INITPROP instances into that plus JSOP_MUTATEPROTO whenever necessary.  Patches and tests coming up for all the places where I failed to do that, as soon as I can get a build complete against which to write said tests (and then check the tests fail without patch, pass with, of course).

Comment 4

[Jeff Walden [:Waldo]]

Okay, I *think* I've tracked down all the places where I failed to split JSOP_INITPROP correctly.  Patch sequence upload commencing now.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: 1 - Miscellaneous non-bug-fixing changes

These are miscellaneous minor changes I ended up making while patching, that don't actually fix anything but probably should still happen.  Comment fixes, asserting harder, eliminating some JSOP_INITPROP special-cases when __proto__ (which is impossible now), structuring some code so it's more obviously case-based-by-op, &c.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: 2 - Update Reflect code for PNK_MUTATEPROTO

Comment 7

[Jeff Walden [:Waldo]]

Attachment: 3 - Make asm.js module exports aware of PNK_MUTATEPROTO

The exception for __proto__ as module export should be added to the asm.js spec -- it's not there now.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: 4 - Make destructuring patterns containing __proto__ work

Because checkDestructuring (which does a syntax pass on destructuring patterns) and EmitDestructuringDecls go approximately hand in hand, other than maybe by doing a syntax-only parse, I don't think I can subdivide this patch any further with more narrowly-targeted tests, unfortunately.  Hopefully this should be good enough as far as size minimization goes.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: 5 - Fix cloneLeftHandSide for PNK_MUTATEPROTO

Cloning only happens for certain particular cases -- for (var/let/const ___ in/of expr), and our weird array comprehensions in similar cases -- so wasn't triggered by previous tests.  Fix that up, add tests for the for-loop case (I'm in the office way too late today to have any interest in also adding array-comprehension tests too, now).

Comment 10

[Jeff Walden [:Waldo]]

Attachment: 6 - Update analyzeSSA for JSOP_MUTATEPROTO

I briefly tried writing a test that fails without this, but it's way too late here, I need to get home and sleep.  Punting to you to tell me what to do here to make one.  :-)

Comment 11

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

(function() {
    "use asm"
    return ({
        __proto__: 0
    })
})()

asserts bfe4ed6d47ce without any CLI arguments at Assertion failure: pn->isKind(PNK_COLON), at jit/AsmJS.cpp

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8365502 [details] [diff] [review]
6 - Update analyzeSSA for JSOP_MUTATEPROTO

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

ScriptAnalysis is only used for the arguments-analysis these days, so try a function containing both |arguments| and MUTATEPROTO. I don't know if you'll get assertion failures without MUTATEPROTO there though..

Comment 13

[Jason Orendorff [:jorendorff]]

Comment on attachment 8365497 [details] [diff] [review]
1 - Miscellaneous non-bug-fixing changes

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

::: js/src/jit/BaselineIC.cpp
@@ +7261,2 @@
>      } else {
> +        if (script->strict()) {

MOZ_ASSERT(op == JSOP_SETPROP, …);

Comment 14

[Jason Orendorff [:jorendorff]]

Comment on attachment 8365498 [details] [diff] [review]
2 - Update Reflect code for PNK_MUTATEPROTO

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

::: js/src/jsreflect.cpp
@@ +1218,5 @@
>      RootedValue kindName(cx);
>      if (!atomValue(kind == PROP_INIT
>                     ? "init"
> +                   : kind == PROP_MUTATEPROTO
> +                   ? "proto"

Keep this as "init" for compatibility.

Comment 15

[Jason Orendorff [:jorendorff]]

Comment on attachment 8365498 [details] [diff] [review]
2 - Update Reflect code for PNK_MUTATEPROTO

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

::: js/src/jsreflect.cpp
@@ +1218,5 @@
>      RootedValue kindName(cx);
>      if (!atomValue(kind == PROP_INIT
>                     ? "init"
> +                   : kind == PROP_MUTATEPROTO
> +                   ? "proto"

Actually I think you can get rid of PROP_MUTATEPROTO with that change.

Comment 16

[Jason Orendorff [:jorendorff]]

Comment on attachment 8365499 [details] [diff] [review]
3 - Make asm.js module exports aware of PNK_MUTATEPROTO

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

::: js/src/jit-test/tests/asm.js/testBasic.js
@@ +75,4 @@
>  assertEq(Object.keys(exp).join(), 'f');
>  
>  assertAsmTypeFail(USE_ASM + "function f() { return 3 } return {1:f}");
> +assertAsmTypeFail(USE_ASM + "function f() { return 3 } return {__proto__:f}");

:-D

Comment 17

[Jason Orendorff [:jorendorff]]

Comment on attachment 8365497 [details] [diff] [review]
1 - Miscellaneous non-bug-fixing changes

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

Clearing review --- on a little more thought, I think it's better not to have PNK_MUTATEPROTO, and detect this special case in the emitter, not the parser.

Comment 18

[Jason Orendorff [:jorendorff]]

Comment on attachment 8365500 [details] [diff] [review]
4 - Make destructuring patterns containing __proto__ work

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

::: js/src/frontend/Parser.cpp
@@ +3129,2 @@
>                          return false;
>                  }

If you don't mind, throw in a test anyway:

    function f(obj) {
        {__proto__} = obj;
        var __proto__;
        assertEq(__proto__ === obj);
    }
    var x = {};
    f(x);
    assertEq(this.__proto__ !== x, true);

and maybe the same for `let`.

Comment 19

[Jeff Walden [:Waldo]]

Attachment: 1 - Reflect.parse+__proto__ and asm.js __proto__ tests

More tests seem good, even if the underlying reason for them doesn't quite exist any more.

Comment 20

[Jeff Walden [:Waldo]]

Attachment: 2 - Remove PNK_MUTATEPROTO, distinguish at emit time

There's still some of the earlier-patch cleanup here, seems worth having regardless.  Also the tests I wrote earlier are still here, same reason as with the previous patch.

Comment 21

[Jeff Walden [:Waldo]]

Attachment: 3 - Test for abortive cloneLeftHandSide changes

Should have folded this into the test-only patch earlier, oh well, too late now.  Just an extra test.

Comment 23

[Jason Orendorff [:jorendorff]]

Comment on attachment 8366766 [details] [diff] [review]
2 - Remove PNK_MUTATEPROTO, distinguish at emit time

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

Great. r=me.

::: js/src/frontend/Parser.cpp
@@ +3662,2 @@
>          pn = letDeclaration();
> +    }

Thank you TextWrangler!

Comment 24

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f53d87699bc2
https://hg.mozilla.org/integration/mozilla-inbound/rev/513791fc2754
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e170276a033
https://hg.mozilla.org/integration/mozilla-inbound/rev/78d5cf8a1ac6
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7566746035f

I tried briefly on the TI test, then punted to not rathole.  Not worth the time, sadly.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f53d87699bc2
https://hg.mozilla.org/mozilla-central/rev/513791fc2754
https://hg.mozilla.org/mozilla-central/rev/1e170276a033
https://hg.mozilla.org/mozilla-central/rev/78d5cf8a1ac6
https://hg.mozilla.org/mozilla-central/rev/a7566746035f