Closed Bug 1233120 Opened 8 years ago Closed 8 years ago

Assertion failure: ltok != TOK_RC, at js/src/frontend/Parser.cpp:9059 with ES6 Classes

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: decoder, Assigned: arai)

References

Details

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

Attachments

(1 file, 1 obsolete file)

The following testcase crashes on mozilla-central revision 749f9328dd76 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --thread-count=2):

eval("(function() { class a { constructor() { } static });");



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000503308 in js::frontend::Parser<js::frontend::FullParseHandler>::propertyName (this=this@entry=0x7fffffffbbf0, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, propList=propList@entry=0x7ffff69cf220, propType=propType@entry=0x7fffffff9ba0, propAtom=..., propAtom@entry=...) at js/src/frontend/Parser.cpp:9059
#0  0x0000000000503308 in js::frontend::Parser<js::frontend::FullParseHandler>::propertyName (this=this@entry=0x7fffffffbbf0, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, propList=propList@entry=0x7ffff69cf220, propType=propType@entry=0x7fffffff9ba0, propAtom=..., propAtom@entry=...) at js/src/frontend/Parser.cpp:9059
#1  0x00000000004d9009 in js::frontend::Parser<js::frontend::FullParseHandler>::classDefinition (this=this@entry=0x7fffffffbbf0, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, classContext=classContext@entry=js::frontend::Parser<js::frontend::FullParseHandler>::ClassStatement, defaultHandling=defaultHandling@entry=js::frontend::NameRequired) at js/src/frontend/Parser.cpp:6598
#2  0x00000000004fec0a in js::frontend::Parser<js::frontend::FullParseHandler>::statement (this=this@entry=0x7fffffffbbf0, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, canHaveDirectives=<optimized out>) at js/src/frontend/Parser.cpp:6914
#3  0x00000000004ff141 in js::frontend::Parser<js::frontend::FullParseHandler>::statements (this=this@entry=0x7fffffffbbf0, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName) at js/src/frontend/Parser.cpp:3306
#4  0x00000000004ff50b in js::frontend::Parser<js::frontend::FullParseHandler>::functionBody (this=this@entry=0x7fffffffbbf0, inHandling=inHandling@entry=js::frontend::InAllowed, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, kind=kind@entry=js::frontend::Expression, type=type@entry=js::frontend::Parser<js::frontend::FullParseHandler>::StatementListBody) at js/src/frontend/Parser.cpp:1325
#5  0x00000000004ff9f0 in js::frontend::Parser<js::frontend::FullParseHandler>::functionArgsAndBodyGeneric (this=this@entry=0x7fffffffbbf0, inHandling=inHandling@entry=js::frontend::InAllowed, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, pn=pn@entry=0x7ffff69cf0a8, fun=fun@entry=..., kind=kind@entry=js::frontend::Expression) at js/src/frontend/Parser.cpp:2999
#6  0x00000000004d7335 in js::frontend::Parser<js::frontend::FullParseHandler>::functionArgsAndBody (this=this@entry=0x7fffffffbbf0, inHandling=inHandling@entry=js::frontend::InAllowed, pn=0x7ffff69cf0a8, fun=..., fun@entry=..., kind=kind@entry=js::frontend::Expression, generatorKind=generatorKind@entry=js::NotGenerator, inheritedDirectives=..., newDirectives=newDirectives@entry=0x7fffffffa3a0) at js/src/frontend/Parser.cpp:2802
#7  0x00000000004ffdda in js::frontend::Parser<js::frontend::FullParseHandler>::functionDef (this=this@entry=0x7fffffffbbf0, inHandling=inHandling@entry=js::frontend::InAllowed, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, funName=..., funName@entry=..., kind=kind@entry=js::frontend::Expression, generatorKind=js::NotGenerator, invoked=invoked@entry=js::frontend::Parser<js::frontend::FullParseHandler>::PredictInvoked) at js/src/frontend/Parser.cpp:2634
#8  0x00000000005003a3 in js::frontend::Parser<js::frontend::FullParseHandler>::functionExpr (this=this@entry=0x7fffffffbbf0, invoked=js::frontend::Parser<js::frontend::FullParseHandler>::PredictInvoked) at js/src/frontend/Parser.cpp:3115
[...]
#42 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:6877
rax	0x0	0
rbx	0x7fffffffbbf0	140737488337904
rcx	0x7ffff6ca53cd	140737333842893
rdx	0x0	0
rsi	0x7ffff6f7a9d0	140737336814032
rdi	0x7ffff6f791c0	140737336807872
rbp	0x7fffffff9ac0	140737488329408
rsp	0x7fffffff9a10	140737488329232
r8	0x7ffff7fe0780	140737354008448
r9	0x6372732f736a2f6c	7165916604736876396
r10	0x7fffffff97d0	140737488328656
r11	0x7ffff6c27960	140737333328224
r12	0x7fffffff9ba0	140737488329632
r13	0x7fffffffbc20	140737488337952
r14	0x0	0
r15	0x7ffff69cf220	140737330868768
rip	0x503308 <js::frontend::Parser<js::frontend::FullParseHandler>::propertyName(js::frontend::YieldHandling, js::frontend::ParseNode*, js::frontend::PropertyType*, JS::MutableHandle<JSAtom*>)+2056>
=> 0x503308 <js::frontend::Parser<js::frontend::FullParseHandler>::propertyName(js::frontend::YieldHandling, js::frontend::ParseNode*, js::frontend::PropertyType*, JS::MutableHandle<JSAtom*>)+2056>:	movl   $0x2363,0x0
   0x503313 <js::frontend::Parser<js::frontend::FullParseHandler>::propertyName(js::frontend::YieldHandling, js::frontend::ParseNode*, js::frontend::PropertyType*, JS::MutableHandle<JSAtom*>)+2067>:	callq  0x4a3db0 <abort()>
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/5ad7d2771c73
user:        Tooru Fujisawa
date:        Tue Aug 25 05:42:50 2015 +0900
summary:     Bug 1192412 - Part 0: Refactor property list parsing. r=efaust

This iteration took 258.989 seconds to run.
Arai-san, is bug 1192412 a likely regressor?
Blocks: 1192412
Flags: needinfo?(arai.unmht)
Thanks :)

Parser<ParseHandler>::propertyName expects TOK_RC to be handled in caller, so, if we found |static| in class declaration, we should check token after that, and if it's TOK_RC, we should throw error there.
Assignee: nobody → arai.unmht
Flags: needinfo?(arai.unmht)
Attachment #8699240 - Flags: review?(jwalden+bmo)
Comment on attachment 8699240 [details] [diff] [review]
Check token after static in class declaration.

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

Forry for the fly-by. Wanted to make the comment on the test, and got carried away.

Setting f+, I guess.

::: js/src/frontend/Parser.cpp
@@ +6580,5 @@
>          bool isStatic = false;
>          if (tt == TOK_NAME && tokenStream.currentName() == context->names().static_) {
>              if (!tokenStream.peekToken(&tt, TokenStream::KeywordIsName))
>                  return null();
> +            if (tt == TOK_RC) {

What about TOK_EOF? Can we do something more restrictive? The only valid tokens are TOK_NAME, TOK_YIELD, and TOK_LP, right?

::: js/src/tests/ecma_6/Class/staticMethods.js
@@ +12,5 @@
>  assertEq(X.count.bind({hits: 77})(), 78);
>  
> +// static requires property id.
> +assertThrowsInstanceOf(() => eval("class a { static }"), SyntaxError);
> +assertThrowsInstanceOf(() => eval("class a { static ; }"), SyntaxError);

I would prefer that this went into js1_8_5/reflect-parse/classes.js, if possible.
Attachment #8699240 - Flags: feedback+
As discussed in IRC, keeping the change in Parser.cpp, as all other tokens are handled in propertyName.  only TOK_RC is not handled there, because TOK_RC at the first token should be handled differently. (in this case TOK_RC is not the first token, but second, so it should be SyntaxError)

Moved the test to js1_8_5/reflect-parse/classes.js.
Attachment #8699240 - Attachment is obsolete: true
Attachment #8699240 - Flags: review?(jwalden+bmo)
Attachment #8699263 - Flags: review?(jwalden+bmo)
Comment on attachment 8699263 [details] [diff] [review]
Check token after static in class declaration.

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

Please convert

    // TOK_RC should be handled in caller.
    MOZ_ASSERT(ltok != TOK_RC);

to

    MOZ_ASSERT(ltok != TOK_RC, "caller should have handled TOK_RC");

which would make immediately clear -- in the bug summary, even! -- what the problem is.

::: js/src/frontend/Parser.cpp
@@ +6583,5 @@
>                  return null();
> +            if (tt == TOK_RC) {
> +                tokenStream.consumeKnownToken(tt, TokenStream::KeywordIsName);
> +                report(ParseError, false, null(), JSMSG_UNEXPECTED_TOKEN,
> +                       "property id", TokenKindToDesc(tt));

"id" is our own internal terminology -- use "property name".

::: js/src/tests/js1_8_5/reflect-parse/classes.js
@@ +479,5 @@
>      assertClassError("class NAME { static *y", SyntaxError);
>      assertClassError("class NAME { static get", SyntaxError);
>      assertClassError("class NAME { static get y", SyntaxError);
> +    assertClassError("class NAME { static }", SyntaxError);
> +    assertClassError("class NAME { static ;", SyntaxError);

We have invalid-character testing of this elsewhere, right?  Can't remember the file, you probably have it closer to mind than I do.  Definitely want "class NAME { static @" or something like it tested somewhere.
Attachment #8699263 - Flags: review?(jwalden+bmo) → review+
Thank you for reviewing :)

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> ::: js/src/tests/js1_8_5/reflect-parse/classes.js
> @@ +479,5 @@
> >      assertClassError("class NAME { static *y", SyntaxError);
> >      assertClassError("class NAME { static get", SyntaxError);
> >      assertClassError("class NAME { static get y", SyntaxError);
> > +    assertClassError("class NAME { static }", SyntaxError);
> > +    assertClassError("class NAME { static ;", SyntaxError);
> 
> We have invalid-character testing of this elsewhere, right?  Can't remember
> the file, you probably have it closer to mind than I do.  Definitely want
> "class NAME { static @" or something like it tested somewhere.

Yes, it's tested in syntax-error-illegal-character.js.

https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/js/src/jit-test/lib/syntax.js#1014
>   test("class a { constructor() { } static ");

https://dxr.mozilla.org/mozilla-central/rev/388bdc46ba51ee31da8b8abe977e0ca38d117434/js/src/jit-test/tests/parser/syntax-error-illegal-character.js#6
> var postfixes = [
>   "@",
> ];
> ...
> test_syntax(postfixes, check_syntax_error, false);
https://hg.mozilla.org/mozilla-central/rev/d0ce7ffeecfc
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: