1302994 - Assertion failure: hasLookahead(), at js/src/frontend/TokenStream.h:1001

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

function main() {
    for (var zeros = "0, ", each = get => (yield); i < repetitions.length; ++i) {}
}
main();


Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x00000000004a7314 in js::frontend::TokenStream::nextToken (this=<optimized out>) at js/src/frontend/TokenStream.h:1001
#0  0x00000000004a7314 in js::frontend::TokenStream::nextToken (this=<optimized out>) at js/src/frontend/TokenStream.h:1001
#1  0x00000000004a7341 in js::frontend::TokenStream::addModifierException (this=this@entry=0x7fffffffd220, modifierException=modifierException@entry=js::frontend::Token::OperandIsNone) at js/src/frontend/TokenStream.h:456
#2  0x00000000004c3add in js::frontend::Parser<js::frontend::FullParseHandler>::initializerInNameDeclaration (this=this@entry=0x7fffffffd1f0, decl=decl@entry=0x7ffff69fd138, binding=binding@entry=0x7ffff69fd1c8, name=..., name@entry=..., declKind=declKind@entry=js::frontend::DeclarationKind::Var, initialDeclaration=initialDeclaration@entry=false, yieldHandling=js::frontend::YieldIsName, forHeadKind=0x7fffffffc784, forInOrOfExpression=0x7fffffffc7a0) at js/src/frontend/Parser.cpp:4164
#3  0x00000000004c3d74 in js::frontend::Parser<js::frontend::FullParseHandler>::declarationName (this=this@entry=0x7fffffffd1f0, decl=decl@entry=0x7ffff69fd138, declKind=declKind@entry=js::frontend::DeclarationKind::Var, tt=<optimized out>, initialDeclaration=initialDeclaration@entry=false, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, forHeadKind=0x7fffffffc784, forInOrOfExpression=0x7fffffffc7a0) at js/src/frontend/Parser.cpp:4204
#4  0x00000000004ca15b in js::frontend::Parser<js::frontend::FullParseHandler>::declarationList (this=this@entry=0x7fffffffd1f0, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, kind=kind@entry=js::frontend::PNK_VAR, forHeadKind=forHeadKind@entry=0x7fffffffc784, forInOrOfExpression=forInOrOfExpression@entry=0x7fffffffc7a0) at js/src/frontend/Parser.cpp:4298
#5  0x00000000004ca5b1 in js::frontend::Parser<js::frontend::FullParseHandler>::forHeadStart (this=this@entry=0x7fffffffd1f0, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, forHeadKind=forHeadKind@entry=0x7fffffffc784, forInitialPart=forInitialPart@entry=0x7fffffffc798, forLoopLexicalScope=..., forInOrOfExpression=forInOrOfExpression@entry=0x7fffffffc7a0) at js/src/frontend/Parser.cpp:5126
#6  0x00000000004cbb08 in js::frontend::Parser<js::frontend::FullParseHandler>::forStatement (this=this@entry=0x7fffffffd1f0, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName) at js/src/frontend/Parser.cpp:5288
#7  0x00000000004ccfeb in js::frontend::Parser<js::frontend::FullParseHandler>::statementListItem (this=this@entry=0x7fffffffd1f0, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, canHaveDirectives=<optimized out>) at js/src/frontend/Parser.cpp:6803
#8  0x00000000004cd2bd in js::frontend::Parser<js::frontend::FullParseHandler>::statementList (this=this@entry=0x7fffffffd1f0, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName) at js/src/frontend/Parser.cpp:3613
#9  0x00000000004cee13 in js::frontend::Parser<js::frontend::FullParseHandler>::functionBody (this=this@entry=0x7fffffffd1f0, inHandling=inHandling@entry=js::frontend::InAllowed, yieldHandling=yieldHandling@entry=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:2333
#10 0x00000000004cf2dd in js::frontend::Parser<js::frontend::FullParseHandler>::functionFormalParametersAndBody (this=this@entry=0x7fffffffd1f0, inHandling=inHandling@entry=js::frontend::InAllowed, yieldHandling=js::frontend::YieldIsName, pn=0x7ffff69fd020, kind=js::frontend::Statement) at js/src/frontend/Parser.cpp:3298
#11 0x00000000004a608e in js::frontend::Parser<js::frontend::FullParseHandler>::standaloneLazyFunction (this=this@entry=0x7fffffffd1f0, fun=fun@entry=..., strict=<optimized out>, generatorKind=js::NotGenerator) at js/src/frontend/Parser.cpp:3221
#12 0x0000000000cd72ef in js::frontend::CompileLazyFunction (cx=cx@entry=0x7ffff6944000, lazy=lazy@entry=..., chars=0x7ffff0216022 u"() {\n        for (var zeros = \"0, \", each = get => (yield); i < repetitions.length; ++i) {}\n    }\n    main();", length=97) at js/src/frontend/BytecodeCompiler.cpp:649
#13 0x000000000090c1d8 in JSFunction::createScriptForLazilyInterpretedFunction (cx=cx@entry=0x7ffff6944000, fun=fun@entry=...) at js/src/jsfun.cpp:1505
#14 0x0000000000464e5a in JSFunction::getOrCreateScript (this=<optimized out>, cx=0x7ffff6944000) at js/src/jsfun.h:396
#15 0x0000000000ad3157 in Interpret (cx=0x7ffff6944000, state=...) at js/src/vm/Interpreter.cpp:2928
[...]
#25 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:7663
rax	0x0	0
rbx	0x2	2
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffc4e0	140737488340192
rsp	0x7fffffffc4e0	140737488340192
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x7fffffffd220	140737488343584
r13	0x7ffff69fd1c8	140737331057096
r14	0x2	2
r15	0x7ffff69fd228	140737331057192
rip	0x4a7314 <js::frontend::TokenStream::nextToken() const+68>
=> 0x4a7314 <js::frontend::TokenStream::nextToken() const+68>:	movl   $0x0,0x0
   0x4a731f <js::frontend::TokenStream::nextToken() const+79>:	ud2

Comment 1

[Jeff Walden [:Waldo]]

Pretty recent regression -- doesn't repro in my tree with the identifier-parsing/keyword changes of a few days ago, does reproduce with tip.  I'm investigating.  Looks like some sort of modifier mistake, tho I'm still moving around in rr to figure it out.

Interestingly, the crash stack/info aren't wholly informative here -- FullParseHandler seems to be going off the rails due to bad info cached by SyntaxParseHandler.  That's a failure mode I've not seen before.

Comment 2

[Jeff Walden [:Waldo]]

Oh, actually -- this is fallout from syntax-parsing of arrow functions.  CCing the usual suspects, 'cause I'm going to a lunchtime ultimate game and shouldn't monopolize this in case the problem/fix is immediately obvious -- will investigate on return if it's not dealt with by then.

Comment 3

[Jeff Walden [:Waldo]]

Arrow function syntax-parsing just exposed this.  It's really a pre-existing problem with arrow functions with AssignmentExpression body in this one weird place.  I think.

Worst.  Language.  Ever.

Comment 4

[Jeff Walden [:Waldo]]

Ugh, this is a mess.

Basically, different TokenStream modifiers are required to examine the next token after an ArrowFunction that has a {} body (modifier should be Operand) and one that has an AssignmentExpression body (modifier should be None).  And in certain contexts, the next token after is additionally gotten as Operand, regardless.  If the ArrowFunction appears at the end of the init-component of a for(;;) loop, the next token has to be gettable as Operand, because |for(;| gets it as Operand.

I'm spinning wheels a bit now, so calling it a night.  I've been trying to avoid introducing separate arrow-function nodes for this purpose, but it might be unavoidable.  :-(

Comment 5

[Jeff Walden [:Waldo]]

I have a +82,-142 patch that's some nice simplification following up on comment 4.  But after enough investigation, I don't think it's the fundamental issue here, and it doesn't actually fix this bug.  The fundamental issue is in Parser<FullParseHandler>::skipLazyInnerFunction.

Suppose we have a syntax-parsed function that contains lazy-parsable inner functions.  The first go-around, the outer function is syntax-parsed and we save coordinates for its various inner functions.  Now suppose we call that outer function.  This triggers full parsing.  During that full parse, when a lazy-parsable function is encountered, we don't full-parse it.  Instead, excepting a couple bits of info-tracking, we fast-forward the token stream over the inner function and resume parsing afterward.

    // When a lazily-parsed function is called, we only fully parse (and emit)
    // that function, not any of its nested children. The initial syntax-only
    // parse recorded the free variables of nested functions and their extents,
    // so we can skip over them after accounting for their free variables.

The fast-forwarding is accomplished here:

    return tokenStream.advance(fun->lazyScript()->end() - userbufBase);

by a function that does these important bits:

    const char16_t* end = userbuf.rawCharPtrAt(position);
    while (userbuf.addressOfNextRawChar() < end)
        getChar();

    Token* cur = &tokens[cursor];
    cur->pos.begin = userbuf.offset();
    MOZ_MAKE_MEM_UNDEFINED(&cur->type, sizeof(cur->type));
    lookahead = 0;

Now consider a simplified version of the testcase here:

  function main() {
    for (var x = 0, y = get => q; ; )
    {}
  }
  main();

When we skip over the arrow function, the next token to get is the ';' after the init-parse of the for(;;)-head.  That token has this come into play:

    // Per Parser::forHeadStart, the semicolon in |for (;| is ultimately
    // gotten as Operand.  But initializer expressions terminate with the
    // absence of an operator gotten as None, so we need an exception.
    tokenStream.addModifierException(TokenStream::OperandIsNone);

And that triggers the assertion, because we don't have any lookahead yet.

My +82,-142 patch simplifies modifier exception handling such that we don't need an explicit lookahead modifier there -- pushing modifier exception handling downward to the exact point in the Expression sub-productions where Operand again becomes permissible.  It's a good change.  But it incidentally makes the lookahead handling here, and possibly elsewhere, even worse -- because then the first token-get after the inner function, with no lookahead available, might *assume* the existence of that added modifier exception.  And so the next token would be initially gotten, on this second full-parse, with the *wrong* modifier.

I'm not sure what we want to do about this.

I *think*, given all the above, that the initial parse would use the correct modifier, so '/' would be interpreted correctly initially -- and then, because it would have to be glommed up in the AssignmentExpression *within* the ArrowFunction, could never be the token just after an ArrowFunction.  So this is just an assertion failure, not actual buggy behavior -- if I'm evaluating correctly.

We could fix this by saving a token of lookahead in LazyScript.  But shoving stuff into LazyScript (*especially* DEBUG-only) is hard because of sizing/padding requirements, so I'd really rather not do that.

Maybe there's some sort of hack we should set on the TokenStream, to indicate "don't care about lookahead checking for the next token", and we should use that here?  That's the best I've got, and I'm super-unsure of it.  arai, you have any better ideas?

Oh, and in other funtimes, this stuff may have migrated to a branch either yesterday or today or tomorrow, so a small fix for this is highly preferable.  Either that, or we revert syntax-parsing of arrow functions, which of course none of us would like to happen.

Comment 6

[Tooru Fujisawa [:arai]]

iiuc, the actual issue is that we're adding a modifier exception to a token that is not peeked in the same function,
expecting that certain method called there will keep a token in lookahead buffer.  and skipLazyInnerFunction breaks the assumption.

your patch removes all those case that adds modifier exception to unknown token, so I think we can just go with your patch, but keeping the exception for isBlock case for arrow function with block body.
(and not touching skipLazyInnerFunction)

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Super-minimal patch, but code-smelly

I'm running my mind in circles trying to figure out how to fix this fully cleanly, and it's at the point where I'm just spinning my wheels without getting anywhere.  But this needs to be fixed.  So how about a spot-patch, in this one weird case that presumes lookahead, and just get on with life for now?

This also has the strong advantage of being backportable, where a +82,-142 patch plus whatever other changes would be necessary to fix completely, would disputably not be.

Comment 8

[Tooru Fujisawa [:arai]]

Comment on attachment 8793886 [details] [diff] [review]
Super-minimal patch, but code-smelly

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

looks good :)

please add a testcase from comment #0.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddfe1d1657b3
Precautionarily peek at the next token after the AssignmentExpression in a for-loop head's declaration, when searching for a for(;;)'s first semicolon, in case the init-component ends in a lazy inner function that, when skipped during full-parsing, clears lookahead.  r=arai

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ddfe1d1657b3

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ddfe1d1657b3

Comment 12

[Gerry Chang [:gchang]]

Hi :Waldo,
Since this bug is a regression and also affects 51, do you consider to uplift this for 51 if this patch is not too risky?

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Aurora backport

Oops, yeah, meant to request this when landing but forgot.

Approval Request Comment
[Feature/regressing bug #]: bug 1297706, the part that made arrow functions syntax-parse
[User impact if declined]: weird syntax errors at runtime, *after* the relevant script has been syntax-parsed; assertion failures in debug builds; uncertain of exact fallout, don't care to think about it for such a brief fix (and so quickly after the initial bustage)
[Describe test coverage new/current, TreeHerder]: test in patch
[Risks and why]: pretty low -- this just makes one arm of an |if| consistent with what the other does, in terms of lookahead, so the existing arm provides some coverage for what this addition does
[String/UUID change made/needed]: N/A

Comment 14

[Gerry Chang [:gchang]]

Comment on attachment 8795595 [details] [diff] [review]
Aurora backport

Fix a regression. Take it in 51 aurora.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0f7d8aba3267