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)
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)
2.70 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
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()>
Updated•8 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 1•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
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+
Assignee | ||
Comment 5•8 years ago
|
||
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 6•8 years ago
|
||
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+
Assignee | ||
Comment 7•8 years ago
|
||
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);
Assignee | ||
Comment 8•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0ce7ffeecfceac94219f2786578341541243ac6 Bug 1233120 - Check token after static in class declaration. r=jwalden
Comment 9•8 years ago
|
||
bugherder |
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.
Description
•