Closed Bug 963641 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: gkw, Assigned: Waldo)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update])

Attachments

(8 files, 3 obsolete files)

Attached file 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?
Flags: needinfo?(jwalden+bmo)
Reflect.parse("({__proto__: window})");

causes Assertion failure: false, at jsreflect.cpp

and has the same regression window.
Summary: Assertion failure: pair->isKind(PNK_COLON), at frontend/Parser.cpp → Assertion failure: pair->isKind(PNK_COLON), at frontend/Parser.cpp or Assertion failure: false, at jsreflect.cpp
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).
Flags: needinfo?(jwalden+bmo)
Okay, I *think* I've tracked down all the places where I failed to split JSOP_INITPROP correctly.  Patch sequence upload commencing now.
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.
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Attachment #8365497 - Flags: review?(jorendorff)
Attachment #8365498 - Flags: review?(jorendorff)
The exception for __proto__ as module export should be added to the asm.js spec -- it's not there now.
Attachment #8365499 - Flags: review?(jorendorff)
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.
Attachment #8365500 - Flags: review?(jorendorff)
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).
Attachment #8365501 - Flags: review?(jorendorff)
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.  :-)
Attachment #8365502 - Flags: review?(jdemooij)
(function() {
    "use asm"
    return ({
        __proto__: 0
    })
})()

asserts bfe4ed6d47ce without any CLI arguments at Assertion failure: pn->isKind(PNK_COLON), at jit/AsmJS.cpp
Summary: Assertion failure: pair->isKind(PNK_COLON), at frontend/Parser.cpp or Assertion failure: false, at jsreflect.cpp → 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
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..
Attachment #8365502 - Flags: review?(jdemooij) → review+
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, …);
Attachment #8365497 - Flags: review?(jorendorff) → review+
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.
Attachment #8365498 - Flags: review?(jorendorff) → review+
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 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
Attachment #8365499 - Flags: review?(jorendorff) → review+
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.
Attachment #8365497 - Flags: review+
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`.
Attachment #8365500 - Flags: review?(jorendorff)
Attachment #8365501 - Flags: review?(jorendorff)
More tests seem good, even if the underlying reason for them doesn't quite exist any more.
Attachment #8365498 - Attachment is obsolete: true
Attachment #8365499 - Attachment is obsolete: true
Attachment #8366765 - Flags: review?(jorendorff)
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.
Attachment #8365500 - Attachment is obsolete: true
Attachment #8366766 - Flags: review?(jorendorff)
Should have folded this into the test-only patch earlier, oh well, too late now.  Just an extra test.
Attachment #8366767 - Flags: review?(jorendorff)
Attachment #8366765 - Flags: review?(jorendorff) → review+
Duplicate of this bug: 965150
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!
Attachment #8366766 - Flags: review?(jorendorff) → review+
Attachment #8366767 - Flags: review?(jorendorff) → review+
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.