Closed Bug 1414805 Opened 2 years ago Closed 2 years ago

Assertion failure: next.type != TOK_DIV && next.type != TOK_REGEXP (next token requires contextual specifier to be parsed unambiguously), at js/src/frontend/TokenStream.h:414

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 --- fixed

People

(Reporter: decoder, Assigned: Waldo)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Attachments

(4 files)

The following testcase crashes on mozilla-central revision 4e6df5159df3 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

1 ? (yield) : (yield) ^= (...target) => {} /Q/;


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x00000000004d6ca8 in js::frontend::TokenStreamAnyChars::addModifierException (this=this@entry=0x7fffffffcd40, modifierException=modifierException@entry=js::frontend::Token::OperandIsNone) at js/src/frontend/TokenStream.h:413
#0  0x00000000004d6ca8 in js::frontend::TokenStreamAnyChars::addModifierException (this=this@entry=0x7fffffffcd40, modifierException=modifierException@entry=js::frontend::Token::OperandIsNone) at js/src/frontend/TokenStream.h:413
#1  0x00000000004f9995 in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::assignExpr (this=this@entry=0x7fffffffcd28, inHandling=inHandling@entry=js::frontend::InAllowed, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, tripledotHandling=tripledotHandling@entry=js::frontend::TripledotProhibited, possibleError=possibleError@entry=0x0, invoked=invoked@entry=js::frontend::ParserBase::PredictUninvoked) at js/src/frontend/Parser.cpp:8126
#2  0x00000000004f9e1d in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::expr (this=this@entry=0x7fffffffcd28, inHandling=inHandling@entry=js::frontend::InAllowed, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, tripledotHandling=tripledotHandling@entry=js::frontend::TripledotProhibited, possibleError=possibleError@entry=0x0, invoked=invoked@entry=js::frontend::ParserBase::PredictUninvoked) at js/src/frontend/Parser.cpp:7715
#3  0x00000000004fab1b in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::expressionStatement (this=this@entry=0x7fffffffcd28, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, invoked=invoked@entry=js::frontend::ParserBase::PredictUninvoked) at js/src/frontend/Parser.cpp:5827
#4  0x0000000000504d35 in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::statementListItem (this=this@entry=0x7fffffffcd28, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, canHaveDirectives=<optimized out>) at js/src/frontend/Parser.cpp:7580
#5  0x0000000000505268 in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::statementList (this=this@entry=0x7fffffffcd28, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName) at js/src/frontend/Parser.cpp:4140
#6  0x00000000004d507a in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::globalBody (this=0x7fffffffcd28, globalsc=globalsc@entry=0x7fffffffc720) at js/src/frontend/Parser.cpp:2214
#7  0x0000000000af6878 in BytecodeCompiler::compileScript (this=this@entry=0x7fffffffc770, environment=..., environment@entry=..., sc=sc@entry=0x7fffffffc720) at js/src/frontend/BytecodeCompiler.cpp:348
#8  0x0000000000af71e1 in BytecodeCompiler::compileGlobalScript (scopeKind=<optimized out>, this=0x7fffffffc770) at js/src/frontend/BytecodeCompiler.cpp:394
#9  js::frontend::CompileGlobalScript (cx=cx@entry=0x7ffff6948000, alloc=..., scopeKind=scopeKind@entry=js::ScopeKind::Global, options=..., srcBuf=..., sourceObjectOut=sourceObjectOut@entry=0x0) at js/src/frontend/BytecodeCompiler.cpp:592
#10 0x000000000098484e in Compile (cx=cx@entry=0x7ffff6948000, options=..., scopeKind=scopeKind@entry=js::ScopeKind::Global, srcBuf=..., script=...) at js/src/jsapi.cpp:4136
#11 0x0000000000984a1a in Compile (script=..., length=<optimized out>, chars=0x7ffff4360630 u"1 ? (yield) : (yield) ^= (...target) => {} /Q/;\n", scopeKind=js::ScopeKind::Global, options=..., cx=0x7ffff6948000) at js/src/jsapi.cpp:4145
#12 Compile (cx=cx@entry=0x7ffff6948000, options=..., scopeKind=scopeKind@entry=js::ScopeKind::Global, bytes=<optimized out>, length=48, script=..., script@entry=...) at js/src/jsapi.cpp:4160
#13 0x000000000098a07d in Compile (cx=cx@entry=0x7ffff6948000, options=..., scopeKind=scopeKind@entry=js::ScopeKind::Global, fp=fp@entry=0x7ffff4351400, script=...) at js/src/jsapi.cpp:4171
#14 0x000000000098a0e5 in JS::Compile (cx=cx@entry=0x7ffff6948000, options=..., file=file@entry=0x7ffff4351400, script=..., script@entry=...) at js/src/jsapi.cpp:4211
#15 0x0000000000434d8d in RunFile (compileOnly=false, file=0x7ffff4351400, filename=<optimized out>, cx=0x7ffff6948000) at js/src/shell/js.cpp:685
#16 Process (cx=0x7ffff6948000, filename=<optimized out>, forceTTY=forceTTY@entry=false, kind=kind@entry=FileScript) at js/src/shell/js.cpp:1048
#17 0x0000000000445eac in ProcessArgs (op=0x7fffffffd9e0, cx=0x7ffff6948000) at js/src/shell/js.cpp:8184
#18 Shell (envp=<optimized out>, op=0x7fffffffd9e0, cx=0x7ffff6948000) at js/src/shell/js.cpp:8556
#19 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:8962
rax	0x0	0
rbx	0x7fffffffcd28	140737488342312
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffbcb0	140737488338096
rsp	0x7fffffffbcb0	140737488338096
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x7fffffffcd40	140737488342336
r13	0x7ffff4305200	140737290195456
r14	0x0	0
r15	0x0	0
rip	0x4d6ca8 <js::frontend::TokenStreamAnyChars::addModifierException(js::frontend::Token::ModifierException)+488>
=> 0x4d6ca8 <js::frontend::TokenStreamAnyChars::addModifierException(js::frontend::Token::ModifierException)+488>:	movl   $0x0,0x0
   0x4d6cb3 <js::frontend::TokenStreamAnyChars::addModifierException(js::frontend::Token::ModifierException)+499>:	ud2
I believe Waldo is familiar with this one. Sorry in advance ;)
Flags: needinfo?(jwalden+bmo)
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/fe95681caba3
user:        Jeff Walden
date:        Mon Oct 30 12:47:57 2017 -0700
summary:     Bug 1319416 - Substantially rejigger token lookahead and modifier exceptions to function more like the spec.  r=arai

This iteration took 277.512 seconds to run.
This is not horribly unexpected.  :-)  The patchwork in bug 1319416 leaves us in a fundamentally better place than before -- but it was likely to regress something in the short run.  On it.
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Flags: needinfo?(jwalden+bmo)
I have a patch for this.  Unfortunately, in the process of fixing, on a lark I started adding more testcases entirely unrelated to anything pointed out by this.  And they broke left and right.  So I started auditing everything in the parser, and that picked up more issues, and I'm not even done finding them (or fixing them).  The latest issue, that I'm giving up fixing today far too late into it, involves changing modifiers in Parser::functionArguments -- and if that doesn't scare you in the complexity of any changes made there, you don't understand how we parse function arguments at all.  (Answer: all in a single function, for every single possible flavor of function syntax in the language.  Super-gag.)

Back tomorrow to hopefully fix the rest of this.  I'm as yet undecided how many separate patches I'm going to split this up into, because it probably could be half a dozen or more for all the distinct issues I've found now.  But it's extra work, and testing all the intermediate states is super-slow and arguably not worth it if only the final state's correctness ultimately matters.
This fixes the original problem reported here.  Gist of the fix is that it's just wrong for the token after condExpr() in assignExpr() to be gotten as None, so we fix that.  But that requires ensuring several places where assignExpr() can appear -- within array/object literals/binding patterns -- also need to get commas/closing-brac(es|kets) as Operand for consistency.  And you sometimes need modifier exceptions to accord as well.  So this change overall spiraled large.  :-(  Unfortunately, I don't think I can trim away any of it (and I tried).
Attachment #8926162 - Flags: review?(arai.unmht)
This does not *appear* to fix any observable bugs, or at least I couldn't find one that triggered.  But it is consistent with how tokens after AssignmentExpression should be accessed generally.
Attachment #8926164 - Flags: review?(arai.unmht)
Some of these, I think I broke in the intermediate state of writing this patch.  But I don't think the patches you're seeing break/fix any of these tests.  It's just good hygiene for us to have more of these.
Attachment #8926165 - Flags: review?(arai.unmht)
The usual thing in this bug of ensuring that the token after assignExpr() is gotten as Operand, and thus that closing-parens/commas are consistently gotten that way.

Parser::functionArguments() is a royal mess.  Fortunately I was able to patch this without having to fundamentally change it.  But we *really* need to fix function parsing so that there's some sort of helper class you can use to parse the umpteen different varieties of function, without commingling all the myriad ways their arguments present (and require particular modifiers as well).
Attachment #8926168 - Flags: review?(arai.unmht)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2d1f253e95f537b215711951a3a0145c44ca24e

is all these patches (minus two or so little tweaks that seem obvious in very-minimal context to be correct -- specifically, removing a couple unnecessary addModifierExceptions followed by breaks to MUST_MATCH_TOKEN_MODs that don't require the exception any more).  I'm pretty optimistic, at least enough to get a review now.  But I think this is going to have to demand one final all-push given how gnarl it is.

Also, I audited all the assignExpr()/expr() callers to ensure the next token is always gotten with the correct modifier, and I think everything is correct now.  Odds on that analysis being correct are...not great, but they seem like they should be better than before this bug, or before bug 1319416.  :-|
Blocks: 1319416
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f8462f8650211c80f4cf530f5763ff4764fc51d is a rebased version of these patches, atop inbound ~5h ago.  Everything appears good to go.
I'll review this tonight.
Comment on attachment 8926162 [details] [diff] [review]
Don't mishandle ASI after a ternary expression ending in an arrow function with block body, or such ternary appearing in a computed property name expression

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

Looks good :)

I'm wondering if we should force using same/fixed modifier for some token kinds (like, TOK_COMMA/TOK_ASSIGN always need Operand),
inside matchToken, consumeKnownToken, and some helper macros that receives expected token.
Attachment #8926162 - Flags: review?(arai.unmht) → review+
Attachment #8926164 - Flags: review?(arai.unmht) → review+
Attachment #8926165 - Flags: review?(arai.unmht) → review+
Attachment #8926168 - Flags: review?(arai.unmht) → review+
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b99335365c5
Don't mishandle ASI after a ternary expression ending in an arrow function with block body, or such ternary appearing in a computed property name expression.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8441c69c083
Parse the colon in a ?: expression using the Operand modifier, because it follows after an AssignmentExpression.  This does not *appear* to fix any observable bugs, but it is consistent with how tokens after AssignmentExpression should be accessed generally.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/eed73ebe0266
Add more tests for other cases of parsing ternary expressions ending in an arrow function with block body, that aren't already covered in the existing test.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/af596055be21
Use proper modifiers when parsing default expressions that end in an arrow function with block body, in function parameters.  r=arai
You need to log in before you can comment on or make changes to this bug.