1319416 - 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:476

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

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

if (testcase() !== true) {}
function testcase() {
    var each = (...of) => () => {}
    /\W+/u.testcase() = 10;
}



Backtrace:

 received signal SIGSEGV, Segmentation fault.
js::frontend::TokenStream::addModifierException (this=this@entry=0x7fffffffc660, modifierException=modifierException@entry=js::frontend::Token::OperandIsNone) at js/src/frontend/TokenStream.h:475
#0  js::frontend::TokenStream::addModifierException (this=this@entry=0x7fffffffc660, modifierException=modifierException@entry=js::frontend::Token::OperandIsNone) at js/src/frontend/TokenStream.h:475
#1  0x00000000004a3d54 in js::frontend::MatchOrInsertSemicolonHelper (ts=..., modifier=js::frontend::Token::None) at js/src/frontend/Parser.cpp:2631
#2  0x00000000004e3ebc in js::frontend::MatchOrInsertSemicolonAfterExpression (ts=...) at js/src/frontend/Parser.cpp:2638
#3  js::frontend::Parser<js::frontend::FullParseHandler>::variableStatement (this=this@entry=0x7fffffffc630, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName) at js/src/frontend/Parser.cpp:6756
#4  0x00000000004e6493 in js::frontend::Parser<js::frontend::FullParseHandler>::statementListItem (this=this@entry=0x7fffffffc630, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, canHaveDirectives=<optimized out>) at js/src/frontend/Parser.cpp:6977
#5  0x00000000004e66b7 in js::frontend::Parser<js::frontend::FullParseHandler>::statementList (this=0x7fffffffc630, yieldHandling=js::frontend::YieldIsName) at js/src/frontend/Parser.cpp:3815
#6  0x00000000004e7c6f in js::frontend::Parser<js::frontend::FullParseHandler>::functionBody (this=this@entry=0x7fffffffc630, inHandling=inHandling@entry=js::frontend::InAllowed, yieldHandling=js::frontend::YieldIsName, kind=kind@entry=js::frontend::Statement, type=type@entry=js::frontend::Parser<js::frontend::FullParseHandler>::StatementListBody) at js/src/frontend/Parser.cpp:2442
#7  0x00000000004e8372 in js::frontend::Parser<js::frontend::FullParseHandler>::functionFormalParametersAndBody (this=this@entry=0x7fffffffc630, inHandling=inHandling@entry=js::frontend::InAllowed, yieldHandling=<optimized out>, pn=0x7ffff69f4020, kind=js::frontend::Statement) at js/src/frontend/Parser.cpp:3445
#8  0x00000000004ae067 in js::frontend::Parser<js::frontend::FullParseHandler>::standaloneLazyFunction (this=this@entry=0x7fffffffc630, fun=fun@entry=..., strict=<optimized out>, generatorKind=js::NotGenerator, asyncKind=js::SyncFunction) at js/src/frontend/Parser.cpp:3361
#9  0x0000000000b18f88 in js::frontend::CompileLazyFunction (cx=0x7ffff695f000, lazy=lazy@entry=..., chars=0x7ffff699e26a u"() {\n    var each = (...of) => () => {}\n    /\\W+/u.testcase() = 10;\n}\n", length=length@entry=69) at js/src/frontend/BytecodeCompiler.cpp:651
#10 0x000000000096e796 in JSFunction::createScriptForLazilyInterpretedFunction (cx=cx@entry=0x7ffff695f000, fun=fun@entry=...) at js/src/jsfun.cpp:1492
#11 0x0000000000466bfa in JSFunction::getOrCreateScript (this=<optimized out>, cx=0x7ffff695f000) at js/src/jsfun.h:408
#12 0x0000000000b64d71 in Interpret (cx=0x7ffff695f000, state=...) at js/src/vm/Interpreter.cpp:2934
[...]
#22 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:7940
rax	0x201c7c0	33671104
rbx	0x2	2
rcx	0x1182680	18359936
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffbc00	140737488337920
rsp	0x7fffffffbbf0	140737488337904
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x7fffffffc660	140737488340576
r13	0x7fffffffc660	140737488340576
r14	0x0	0
r15	0x1	1
rip	0x4af659 <js::frontend::TokenStream::addModifierException(js::frontend::Token::ModifierException)+713>
=> 0x4af659 <js::frontend::TokenStream::addModifierException(js::frontend::Token::ModifierException)+713>:	movl   $0x0,0x0
   0x4af664 <js::frontend::TokenStream::addModifierException(js::frontend::Token::ModifierException)+724>:	ud2

Comment 1

[Fuzzing Team]

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/d1bf9267ba7d
user:        Jan de Mooij
date:        Tue Sep 13 14:14:32 2016 +0200
summary:     Bug 1297706 - Syntax parse arrow functions. r=shu

This iteration took 0.849 seconds to run.

Comment 2

[Jeff Walden [:Waldo]]

Hmm.  I'd have thought bug 1302994 addressed the only instance of this.  :-(  I wonder if it's time to go back to the drawing board and rigorize/systematize how we use modifier exceptions, the way a mega-patch I had was attempting to do (but then I dropped it because it almost certainly would have been un-backportable).

Comment 3

[Andrew Overholt [:overholt]]

Jeff, did you get a chance to look into this any more? Any thoughts on how to progress?

Comment 4

[Jeff Walden [:Waldo]]

I'm attempting to resurrect my mega-patch now to see if that more-principled approach will fix this.  Hopefully that'll be reducible into something backportable, but we'll see.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Substantially rejigger token lookahead and modifier exceptions to function more like the spec

I'm not 100% sure what the problem is with this bug.  But I *think* the problem is about 90% certain to be that the peeking hack after block-bodied arrow functions is just wrong.

Currently, after a block body, we have code-smelly handling because our arrow function parsing treats block-bodied stuff differently.  That works for an ArrowFunction as an entire expression.  But it fails if the ArrowFunction appears at the end of *another* ArrowFunction.  I'm not wholly certain why.  And unless there's some greater danger to this bug than just a possible misparse, I'm not inclined to investigate it.

But in the spec, in reality, it's *always* the case that after an arrow function -- any arrow function -- an expression/Operand is expected, and no modifier should be necessary.  There's no difference between arrow functions -- within ArrowFunction the production -- in this regard.  So why in the world is a peek of a certain kind, and/or a modifier exception, required after one with a block body?

The answer is, it really shouldn't be.  Everything beneath Parser::orExpr() should be getting the next token as Operand, because '/' begins an expression in that context (only without error if the '/' is on a fresh line).  And everything that might call Parser::orExpr(), should be parsing with the None modifier, because '/' is never an Operand there, only an operator.  And if we recognize that, conceptually, only a *single* OperandIsNone exception should ever appear in the parser, I think we can treat all arrow functions identically.

This patch implements this behavior.  3 files changed, 138 insertions(+), 194 deletions(-), not including the test.  Passes all jstests/jit-tests in the shell, so it's probably reviewable now (even if a try-run is more than called for, and may well suss out other places where modifiers require changes.)

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Consolidate all semicolon-matching into a single function, now that there's no quasi-bogus need for multiple variants

The semicolon-insert logic should also have been recognized as code-smelly before now, I think.  ASI means the place where the implicit semicolon is inserted, *always* is the possible beginning of an ExpressionStatement, so interpreting '/' as regular expression should always have been the only sensible behavior.  With the previous patch (which left matchOrInsertSemicolonAfter* as being identical, to avoid this mess of extra change in a concise-ish patch), we can have only a single function to do this.

Comment 7

[Tooru Fujisawa [:arai]]

Comment on attachment 8825599 [details] [diff] [review]
Substantially rejigger token lookahead and modifier exceptions to function more like the spec

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

Great :)
This makes the purpose of modifier/exception more clear,
and it would be the time to revisit bug 1201421 to name them properly, to follow the actual use case.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f12f1db4b4f1
Substantially rejigger token lookahead and modifier exceptions to function more like the spec.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5c333fa9772
Consolidate all semicolon-matching into a single function, now that there's no quasi-bogus need for multiple variants.  r=arai

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for crashes/asserts.
https://treeherder.mozilla.org/logviewer.html#?job_id=68480345&repo=mozilla-inbound&lineNumber=3826

https://hg.mozilla.org/integration/mozilla-inbound/rev/963adce2ffbe5d8b0cd9acf8bb7fd6e85bde27d4

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Substantially rejiggered, take two

Bah.  Any context that takes expr() (so that the first peek after an ArrowFunction is Operand, when ordinarily it's None with a modifier exception added automagically) is going to be a pain to get modifiers correct.  So it goes.  :-(

Comment 11

[Tooru Fujisawa [:arai]]

Comment on attachment 8825600 [details] [diff] [review]
Consolidate all semicolon-matching into a single function, now that there's no quasi-bogus need for multiple variants

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

::: js/src/frontend/Parser.cpp
@@ +5919,3 @@
>              return null();
>      } else {
> +        if (!matchOrInsertSemicolon())

overlooked this.
if (exprNode) has the same then- and else- branches.

Comment 12

[Tooru Fujisawa [:arai]]

Comment on attachment 8826396 [details] [diff] [review]
Substantially rejiggered, take two

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

looks good except one place.

https://dxr.mozilla.org/mozilla-central/rev/048240a074e841c425a4da4707cf8e353074ec1d/js/src/frontend/Parser.cpp#5713-5716
>         // Parser::declaration consumed everything up to the closing ')'.  That
>         // token follows an {Assignment,}Expression, so the next token must be
>         // consumed as if an operator continued the expression, i.e. as None.
>         MUST_MATCH_TOKEN_MOD(TOK_RP, TokenStream::None, JSMSG_PAREN_AFTER_FOR_CTRL);

this could (maybe should?) be changed to Operand now, since it's after assignExpr()/expr()


also, maybe we could add another variant of parser test like js/src/jit-test/lib/syntax.js to test modifier after certain expression in certain context.
currently js/src/jit-test/lib/syntax.js append the codelet to the pre-defined code.
we could extend it to be able to embed the codelet to anywhere in the template, something like following
  codelet:
    () => {}
  template:
    for (; @; ) break;
  evaluated code:
    for (; () => {}; ) break;

of course in other bug.

Comment 13

[Jeff Walden [:Waldo]]

Minor update: try-servering indicates a number of additional bugs in this that require fixing, possibly (probably?) enough for a second go-around.  Still on that, amidst stabbing at request queue that briefly passed 50 recently.  :-\

Comment 14

[Tooru Fujisawa [:arai]]

(response to IRC)
I agree that it's nice to handle KeywordIsName (or KeywordIsKeyword, maybe?) in separate step

Comment 15

[Andrew Overholt [:overholt]]

Jeff, are we going to be able to do anything here for 53 (Aurora at present)?

Comment 16

[Jeff Walden [:Waldo]]

Almost certainly no.  The partial fix-work I have makes pervasive change to the parser and is certainly not backportable.  (Unless some actual security issue pops up from this, but I don't think one would.)

Comment 17

[Andrew Overholt [:overholt]]

53->wontfix given comment 16. Thanks for the clarification, Jeff.

Comment 18

[Nathan Froyd [:froydnj]]

Jeff, is this landable for 55?  I assume that even if this did land for 55, we're not going to want to uplift this to 54?

Comment 19

[Jeff Walden [:Waldo]]

It isn't landable.  At this point, it's in line behind the generally-more-important work of single-byte parsing, bug 1351107.  I'd really really like to finish this up quickly after that, but there are some scheduling issues that will make that tricky.

Comment 20

[Nathan Froyd [:froydnj]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #19)
> It isn't landable.  At this point, it's in line behind the
> generally-more-important work of single-byte parsing, bug 1351107.  I'd
> really really like to finish this up quickly after that, but there are some
> scheduling issues that will make that tricky.

Hm, OK.  Gonna say wontfix for 54, but we'll leave this open for 55.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking affected for 56 since the duplicate seems to be.

Comment 23

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.

Comment 24

[Mike Taylor [:miketaylr]]

Given comment #19, wontfixing for 55 and 56. Given that it's a fuzz bug, marking fix-optional for 57.

Comment 25

[Jeff Walden [:Waldo]]

I picked up the partial patch I had from 5+ months ago, un-depended it on bug 1351107, and *think* I have a complete patch, maybe.  Passes jit-tests, jstests running now (no failures at the 33% mark!), and then we'll see what all the JS in the tree that runs in every other test suite turns up -- but I think what I have is super-close to completion.  Will post patch once jstests run, possibly flagging for review at that point to let that go in parallel with a full try-run and fixing of anything that turns up.

As noted in earlier comments, the patch here is totally unsuitable for backporting anywhere -- far too big and finicky.

Comment 26

[Jeff Walden [:Waldo]]

Getting there getting there...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11d31b6b48950e4e2d1faf36df617b2d52a2f422

Not sure yet whether all those failures are unrelated to the patchwork, but this is definitely close.  And unlike previous times here, I doubt there remain fundamental issues to resolve.  Should have this finished tomorrow.  

Comment 27

[Jeff Walden [:Waldo]]

Attachment: Convert Parser::{cond,or}Expr1 to plain old Parser::{cond,or}Expr

Having only two functions in the entire Parser that consume a bunch of tokens *plus one extra* is bizarrely different, for no good reason.

And it turns out, if you're writing a parser that requests the next token and wants to specify which of InputElement{Div,RegExp} is desired for tokenizing, it makes a ton of sense for the end of parsing a binary expression, to add an exception so that InputElementDiv can now be requested as InputElementRegExp without conflict.

This patch makes both these functions consume exactly what you'd think, and no more.  The next token may be *examined* -- although once this bug is fixed, that's not necessarily the case! -- but if so, it's put back and a modifier exception is added to allow examination using the other production.

This patch shouldn't change any behavior, but it at least slims down the overall aggregated patch for this bug.  And given we're on the third attempt at review here :-\ any slimming that can be done (sadly, all too little) is desirable.

Comment 28

[Jeff Walden [:Waldo]]

Attachment: Use better names (and multiple variables) to replace a single |TokenKind tt|

At one time I think I thought this was necessary to the overall patch, and that I'd specifically build upon these changes.  I'm not sure I do any more.  But in any event, having multiple variables (not using the inscrutable "tt" name) for different contexts here is much more readable.  The compiler/optimizer should be able to combine the storage for the two pretty easily, given their lifetimes don't overlap.

Comment 29

[Jeff Walden [:Waldo]]

Attachment: Actually fix this bug

Okay, this actually makes the fix.  There are several different moving parts in this, all mostly intertwined and incapable of separation.

First, matchOrInsertSemicolonHelper *always* looks for Operand now.  This just makes sense -- any place where ASI is permissible, means you have the start of a new Statement after, and that could be an ExpressionStatement requiring Operand.  It's nonsense for None ever to be appropriate, because that would imply a division at the start of an expression.  However, as with the previous patches here, having just one single matchOrInsertSemicolon function (instead of a helper called by two actual functions with different modifiers) is left to a subsequent patch.

Second, modifiers for distinguishing division from regexp are *mostly* handled by a single exception added at the end of Parser::orExpr, after the end of a binary expression has been reached.  This just makes sense -- the end of a binary expression can't have division after it, because then that would require extending the binary expression.  So every caller of Parser::orExpr should be followed by looking at the next token as Operand (and the same for callers of those functions, when the call is in terminal position).  It's not clear to me whether messing this up will result in broken behavior (other than asserting); exactly how things go awry when these assertions fail, is very unclear.  (And not worth trying to figure out.)

Third, the one place where this distinguishing happens other than that spot, is the new modifier exception added in the not-an-assignment/arrow-function default case in Parser::assignExpr.  That exception is really only necessary for when Parser::assignExpr reads up a Parser::condExpr and another token, then it turns out the Parser::condExpr was the entirety of the Parser::assignExpr -- which is pretty much only ConditionalExpression that ends in an ArrowFunction (with block body) AssignmentExpression, e.g. |1 ? 2 : () => {}|.  See the numerous tests in the new test file that use exactly that sort of pattern to trigger that exact scenario.

Fourth, the modifiers passed in a ton of places need to be changed, so that the next modifier after Parser::orExpr (or Parser::condExpr that calls it, or Parser::assignExpr that calls that) is always Operand.  (Or is handled by some exception somehow.)  The changes are not always entirely obvious for this -- I could probably write a paragraph-long comment by each of these to demonstrate exactly why each particular modifier is needed.  :-(  But it would be tedious and take a lot of time, so I'm deferring that unless it's deemed necessary.  Hopefully blame in the future is enough for people to understand how this stuff should be used.

Fresh tryserver is here and looks as green as it ever does these days:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d26cf0a1a1f4a02e43c8b8303df6990560b78f73

Excluding the new tests, this is a wonderful 3 files changed, 130 insertions(+), 197 deletions(-).  And getting rid of the comments containing the phrases "ASI's infinite majesty" (so much snark, RIP :-( ), "first token-peek", and "If you think this all sounds pretty code-smelly" is easily the highlight of recent hacking for me.

Comment 30

[Tooru Fujisawa [:arai]]

Comment on attachment 8923558 [details] [diff] [review]
Convert Parser::{cond,or}Expr1 to plain old Parser::{cond,or}Expr

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

Nice cleanup :D
I'll review remaining patch tonight or tomorrow.

Comment 31

[Tooru Fujisawa [:arai]]

Comment on attachment 8923562 [details] [diff] [review]
Use better names (and multiple variables) to replace a single |TokenKind tt|

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

Yes, it's clearer :)

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1efe4c311aca
Convert Parser::{cond,or}Expr1 to plain old Parser::{cond,or}Expr.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f49d6e7e883
Use better names (and multiple variables) to replace a single |TokenKind tt|.  r=arai

Comment 33

[Tooru Fujisawa [:arai]]

Comment on attachment 8923652 [details] [diff] [review]
Actually fix this bug

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

Thanks!
r+ with one question in arrayBindingPattern answered.

::: js/src/frontend/Parser.cpp
@@ +4629,4 @@
>           }
>  
>           TokenKind tt;
> +         if (!tokenStream.getToken(&tt))

in this case, it's using TokenStream::None because there can't be RegExp ?

@@ +8185,4 @@
>          tokenStream.consumeKnownToken(TOK_ASYNC, TokenStream::Operand);
>  
>          TokenKind tokenAfterAsync;
> +        if (!tokenStream.getToken(&tokenAfterAsync, TokenStream::None))

this change has no effect.

Comment 34

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/1efe4c311aca
https://hg.mozilla.org/mozilla-central/rev/2f49d6e7e883

Comment 35

[Jeff Walden [:Waldo]]

Comment on attachment 8923652 [details] [diff] [review]
Actually fix this bug

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

::: js/src/frontend/Parser.cpp
@@ +4629,4 @@
>           }
>  
>           TokenKind tt;
> +         if (!tokenStream.getToken(&tt))

I *think* this was for identifier-parsing consistency with Parser::objectBindingPattern.  That code must use None for parsing names because Parser::propertyName gets the token as None, because property names are gotten in a spot where the closing '}' in a binding pattern could be gotten (if there were a trailing comma).  And that's gotten that way because, similar to object *literals*, the '}' is gotten as None because it could appear after the AssignmentExpression that could be the value of a property in said literal:

  var obj = { x: 3 / 5 };
                       ^

The brace is first gotten as None by binary-operator parsing, so the closing brace in general is so gotten.  (Of course if the expression were an ArrowFunction with block body, that brace would be gotten only by object-literal parsing -- but that's the exceptional case this entire bug is concerned about.)

Now as I look at this code now, maybe you *could* keep this as Operand safely, if you kept a few other things that way.  Maybe.  But I'm not at all confident of that, and indeed I'd bet it's the other way around, given pretty much every change I made in this patch was motivated by existing tests/code or the new ones in the patch.

@@ +8185,4 @@
>          tokenStream.consumeKnownToken(TOK_ASYNC, TokenStream::Operand);
>  
>          TokenKind tokenAfterAsync;
> +        if (!tokenStream.getToken(&tokenAfterAsync, TokenStream::None))

Yeah, I guess so.  Maybe in that followup we can make specifying the modifer non-optional, or built into the function call itself, or *something* so it's more explicit and you don't just use one way unthinkingly and inevitably get it wrong.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe95681caba3
Substantially rejigger token lookahead and modifier exceptions to function more like the spec.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/b63a6b43bab8
Consolidate all semicolon-matching into a single function, now that there's no quasi-bogus need for multiple variants.  r=arai

Comment 37

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/fe95681caba3
https://hg.mozilla.org/mozilla-central/rev/b63a6b43bab8