1122334 - Assertion failure: next->isKind(PNK_ARRAYPUSH), at jsreflect.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Reflect.parse("function f() { [ for(n of f()) if(n) for(m of f()) m] }")

asserts js debug shell on m-c changeset 89a190592267 with --fuzzing-safe --no-threads --no-ion at Assertion failure: next->isKind(PNK_ARRAYPUSH), at jsreflect.cpp.

Debug configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

Regression window: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7cff27cb2845&tochange=ff5ca7959511

Guessing this is related to bug 979865 (ES6 array comprehensions), so setting needinfo? from Jason and Andy.

Comment 1

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

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0xda892, 0x000000010066ff79 js-dbg-opt-64-dm-nsprBuild-darwin-89a190592267`(anonymous namespace)::ASTSerializer::expression(js::frontend::ParseNode*, JS::MutableHandle<JS::Value>) [inlined] (anonymous namespace)::ASTSerializer::comprehension(this=<unavailable>, pn=<unavailable>) + 67 at jsreflect.cpp:2642, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x000000010066ff79 js-dbg-opt-64-dm-nsprBuild-darwin-89a190592267`(anonymous namespace)::ASTSerializer::expression(js::frontend::ParseNode*, JS::MutableHandle<JS::Value>) [inlined] (anonymous namespace)::ASTSerializer::comprehension(this=<unavailable>, pn=<unavailable>) + 67 at jsreflect.cpp:2642
    frame #1: 0x000000010066ff36 js-dbg-opt-64-dm-nsprBuild-darwin-89a190592267`(anonymous namespace)::ASTSerializer::expression(this=<unavailable>, pn=<unavailable>, dst=<unavailable>) + 19350 at jsreflect.cpp:3031
    frame #2: 0x00000001006664ad js-dbg-opt-64-dm-nsprBuild-darwin-89a190592267`(anonymous namespace)::ASTSerializer::statement(this=0x00007fff5fbfdd18, pn=0x0000000102044ea8, dst=<unavailable>) + 989 at jsreflect.cpp:2376
    frame #3: 0x00000001006778ae js-dbg-opt-64-dm-nsprBuild-darwin-89a190592267`(anonymous namespace)::ASTSerializer::functionBody(js::frontend::ParseNode*, js::frontend::TokenPos*, JS::MutableHandle<JS::Value>) [inlined] (anonymous namespace)::ASTSerializer::sourceElement(this=0x00007fff5fbfcd68, this=0x0000000101c14758, this=0x00007fff5fbfdd18, root=0x0000000101c14758, v=<unavailable>, pn=0x0000000102044ea8) + 270 at jsreflect.cpp:1996
    frame #4: 0x00000001006778a0 js-dbg-opt-64-dm-nsprBuild-darwin-89a190592267`(anonymous namespace)::ASTSerializer::functionBody(this=0x00007fff5fbfdd18, pn=<unavailable>, pos=0x000000010204493c, dst=<unavailable>) + 256 at jsreflect.cpp:3405
(lldb)

Comment 2

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

This was found by combining random js tests together with jsfunfuzz, the specific file(s) is/are:

http://hg.mozilla.org/mozilla-central/file/89a190592267/js/src/tests/ecma_6/Comprehensions/nested-for-if.js

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Support ES7 comprehension syntax with multiple ComprehensionIf in Reflect.parse.

`ASTSerializer::comprehension` supports only Legacy Array Comprehension's syntax for `ComprehensionTail`, which has zero or one `ComprehensionIf`, at the end of it. But ES7 Array Comprehension can contain multiple `ComprehensionIf`s, also they could be between 2 `ComprehensionFor` nodes or `ComprehensionFor` and `ComprehensionIf`.

`ASTSerializer::comprehension` expects `PNK_IF` to be the last node before `PNK_ARRAYPUSH`, but it's not true for ES7 Array Comprehension. `ASTSerializer::generatorExpression` has same problem.

Then, `ComprehensionExpression` AST node has "filter" property, but it supports zero or one node, and there is no way to indicate the order of it in "blocks" (I guess this is similar case as omitted default parameter).

I prepared a patch which adds `ComprehensionIf`, which has "test" property corresponds to "filter" property in legacy one, and put it into "blocks" array in same order as syntax. Also, do not use "filter" property for ES7 comprehension ("style" property is "modern").
But it could cause compatibility problem for "modern" syntax, and there might be more safer approach (or more drastic one?). Any feekback is welcome :)

Try is running: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5d6d71e7037

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8550188 [details] [diff] [review]
Support ES7 comprehension syntax with multiple ComprehensionIf in Reflect.parse.

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

Thanks!

::: js/src/jsreflect.cpp
@@ +2662,5 @@
> +                    return false;
> +            }
> +            next = next->pn_kid2;
> +        } else if (next->isKind(PNK_STATEMENTLIST) && next->pn_count == 0) {
> +            /* FoldConstants optimized away the push. */

FoldConstants shouldn't have run if we got here. In a separate patch, can you remove this special case and see if anything breaks?

@@ +2698,5 @@
> +                return false;
> +            next = next->pn_right;
> +        } else if (next->isKind(PNK_IF)) {
> +            if (isLegacy) {
> +                if (!optExpression(next->pn_kid1, &filter))

Just before this line, you could MOZ_ASSERT(filter.isMagic(JS_SERIALIZE_NO_NODE));

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Remove FoldConstants checking code from ASTSerializer::comprehension in Reflect.parse.

Thank you for reviewing :)

(In reply to Jason Orendorff [:jorendorff] from comment #4)
> ::: js/src/jsreflect.cpp
> @@ +2662,5 @@
> > +                    return false;
> > +            }
> > +            next = next->pn_kid2;
> > +        } else if (next->isKind(PNK_STATEMENTLIST) && next->pn_count == 0) {
> > +            /* FoldConstants optimized away the push. */
> 
> FoldConstants shouldn't have run if we got here. In a separate patch, can
> you remove this special case and see if anything breaks?

Yeah, it works without that code.
reflect-parse.js already has a test case for legacy one (bug 632029),
so I added a test case for modern one, which gets optimized by FoldConstants in normal execution.

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 8552693 [details] [diff] [review]
Part 2: Remove FoldConstants checking code from ASTSerializer::comprehension in Reflect.parse.

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

Thank you!

Comment 7

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/86662d2c6d41
https://hg.mozilla.org/integration/mozilla-inbound/rev/e185414acea3

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/86662d2c6d41
https://hg.mozilla.org/mozilla-central/rev/e185414acea3