1416337 - Assertion failure: false (MOZ_ASSERT_UNREACHABLE: this token was previously looked up with a different modifier, potentially making tokenization non-deterministic), at js/src/frontend/TokenStream.h:444

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 864174ac0707+ (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):

x = function() 0 ? 1 : a => {}


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000000000436752 in js::frontend::TokenStreamAnyChars::verifyConsistentModifier (this=<optimized out>, modifier=js::frontend::Token::None, lookaheadToken=...) at js/src/frontend/TokenStream.h:442
#0  0x0000000000436752 in js::frontend::TokenStreamAnyChars::verifyConsistentModifier (this=<optimized out>, modifier=js::frontend::Token::None, lookaheadToken=...) at js/src/frontend/TokenStream.h:442
#1  0x00000000004c9a72 in js::frontend::TokenStreamAnyChars::verifyConsistentModifier (this=0x7fffffffcd40, modifier=js::frontend::Token::None, lookaheadToken=...) at js/src/frontend/TokenStream.h:770
#2  js::frontend::TokenStream::getToken (this=this@entry=0x7fffffffcd40, ttp=ttp@entry=0x7fffffffb84c, modifier=js::frontend::Token::None, modifier=js::frontend::Token::None) at js/src/frontend/TokenStream.h:771
#3  0x00000000005009cd in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::memberExpr (this=this@entry=0x7fffffffcd28, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, tripledotHandling=tripledotHandling@entry=js::frontend::TripledotProhibited, tt=js::frontend::TOK_FUNCTION, allowCallSyntax=allowCallSyntax@entry=true, possibleError=possibleError@entry=0x7fffffffbba0, invoked=js::frontend::ParserBase::PredictUninvoked) at js/src/frontend/Parser.cpp:8814
#4  0x0000000000501745 in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::unaryExpr (this=this@entry=0x7fffffffcd28, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, tripledotHandling=js::frontend::TripledotProhibited, possibleError=possibleError@entry=0x7fffffffbba0, invoked=js::frontend::ParserBase::PredictUninvoked) at js/src/frontend/Parser.cpp:8343
#5  0x0000000000501bb5 in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::orExpr (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=0x7fffffffbba0, invoked=invoked@entry=js::frontend::ParserBase::PredictUninvoked) at js/src/frontend/Parser.cpp:7860
#6  0x0000000000501fae in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::condExpr (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=0x7fffffffbba0, invoked=invoked@entry=js::frontend::ParserBase::PredictUninvoked) at js/src/frontend/Parser.cpp:7931
#7  0x00000000004fa407 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:8059
#8  0x00000000004fa835 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:8171
#9  0x00000000004fae8d 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:7726
#10 0x00000000004fbb8b 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:5838
#11 0x0000000000505d55 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:7591
#12 0x0000000000506288 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:4148
#13 0x00000000004d5faa in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::globalBody (this=0x7fffffffcd28, globalsc=globalsc@entry=0x7fffffffc720) at js/src/frontend/Parser.cpp:2214
#14 0x0000000000af8f08 in BytecodeCompiler::compileScript (this=this@entry=0x7fffffffc770, environment=..., environment@entry=..., sc=sc@entry=0x7fffffffc720) at js/src/frontend/BytecodeCompiler.cpp:348
[...]
#26 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:8985
rax	0x0	0
rbx	0x7fffffffcd28	140737488342312
rcx	0x7ffff6c282ad	140737333330605
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffb7e0	140737488336864
rsp	0x7fffffffb7e0	140737488336864
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9e7a0	140737332766624
r12	0x7fffffffcd40	140737488342336
r13	0x0	0
r14	0x7fffffffcd28	140737488342312
r15	0x7ffff42d0080	140737289977984
rip	0x436752 <js::frontend::TokenStreamAnyChars::verifyConsistentModifier(js::frontend::Token::Modifier, js::frontend::Token)+28>
=> 0x436752 <js::frontend::TokenStreamAnyChars::verifyConsistentModifier(js::frontend::Token::Modifier, js::frontend::Token)+28>:	movl   $0x0,0x0
   0x43675d <js::frontend::TokenStreamAnyChars::verifyConsistentModifier(js::frontend::Token::Modifier, js::frontend::Token)+39>:	ud2


For Waldo ^.^

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/8b99335365c5
user:        Jeff Walden
date:        Tue Nov 07 16:03:52 2017 -0800
summary:     Bug 1414805 - 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

This iteration took 277.192 seconds to run.

Comment 2

[Jeff Walden [:Waldo]]

I think this may be something of an issue with our function-expression-closure support, in terms of the grammar it ostensibly implements.

The basic problem is this.  In the official grammar, PrimaryExpression includes FunctionExpression.  PrimaryExpression is one subcomponent of LogicalORExpression (that is, orExpr).  So after PrimaryExpression, the next token should be parsed as None, i.e. as if it were a binary operator -- '/' would be division and not RegExp.  But when we have *our* FunctionExpression that allows for expression-closure bodies, the body can be [lookahead ∉ { '{' }] AssignmentExpression.  And the next token after AssignmentExpression should parse as Operand, i.e. '/' begins a RegExp.

So when we look at this in the parser as it exists now, if the function body is AssignmentExpression that ends with an arrow function with block body, Parser::assignExpr looks ahead at the next token (to determine it's not an assignment operator) using the Operand modifier.  But then when Parser::primaryExpr that consumes the function-expression completes, Parser::orExpr will look at the next token as None.  There is no modifier exception inserted between these two lookaheads: by orExpr's logic there *shouldn't* be, yet, to set OperandIsNone; and by Parser::condExpr's logic (called in Parser::assignExpr) there shouldn't be because '/' does not continue the AssignmentExpression and must be the start of a new Expression (permitted only if ASI intrudes).

I remember way back when SpiderMonkey's function expression closure syntax was rejected because it had some sort of shift/reduce or reduce/reduce conflict to it.  Can't find it in es-discuss archives.  Maybe this is that issue, rediscovered now that we have clearer/more consistent handling of lookahead modifiers?

I'm not immediately sure how this should be fixed.  Maybe if we brokenly add a modifier exception in Parser::assignExpr, that would make this work again.  But I think it would essentially hide *every* time we should hit this assertion verifying modifier consistency wrt trailing ArrowFunction with block body, which would make this lookahead thing often useless.

Comment 3

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] (I'm baaaaaaack...) from comment #2)
> I remember way back when SpiderMonkey's function expression closure syntax
> was rejected because it had some sort of shift/reduce or reduce/reduce
> conflict to it.  Can't find it in es-discuss archives.  Maybe this is that
> issue, rediscovered now that we have clearer/more consistent handling of
> lookahead modifiers?

Bison seems to agree with that assertion.

andre@VBdev:~/tmp/grammar$ LANG=C bison function-closure.yacc
function-closure.yacc: warning: 1 shift/reduce conflict [-Wconflicts-sr]


function-closure.yacc:
---
%start script

%error-verbose

%token ID
%token FUNCTION "function"

%%

script : statementList ;

statementList : statement
              | statementList statement
              ;

statement : assignmentExpression ';' ;

assignmentExpression : primaryExpression
                     | primaryExpression '=' assignmentExpression
                     ;

primaryExpression : ID
                  | FUNCTION '(' ')' assignmentExpression
                  ;

%%
---

Comment 4

[Jeff Walden [:Waldo]]

The reason we *do* have this problem with the function-expression-closure implementation, but do *not* have it with ArrowFunction, is that ArrowFunction is higher up in the production hierarchy.  Its final term *can* be AssignmentExpression, because it itself is AssignmentExpression.  If ArrowFunction were a PrimaryExpression, we would have this same problem there.

So, one possible thought is that *maybe* we could fix this if we extended our function-expression-closure hack such that instead of augmenting FunctionExpression as an expansion of PrimaryExpression, we reverted FunctionExpression to what it is in the standard, and we added a *new* FunctionExpressionClosure production that was a fresh expansion of AssignmentExpression.

(If you are recoiling in horror at this idea, you're not the only one.)

I am not sure how literally workable this is.  It seems like it *should* be, given that arrow functions have to work through something like this (their beginning may be indistinguishable from ConditionalExpression), using the cover-grammar idea.  I'm not sure how *much* work, or how deep an intrusion, the change would be in the parser.  But it seems like it would be the ideal/safest fix for this, if we can't simply rip out function expression closures entirely.  (And bug 1319512 suggests we can't rip out nearly soon enough for this bug's purposes.)

Tentative thoughts on implementation, are that Parser::primaryExpr would, after parsing arguments, test for '{'.  If it got that token, it would continue as normal.  If it *didn't* get that token, it would return some sort of NodeFunctionExpressionClosureStart node.  The caller of Parser::primaryExpr would propagate that node untouched if it got it, and the caller of that, and so on until Parser::assignExpr.  If Parser::assignExpr sees that token, it would proceed to parse a function body, and then do the usual Parser::functionDefinition thing.  If it *doesn't* see that token, it would do what it already does.

I may at least make a partial stab on Monday at doing this, before declaring the idea a complete failure.  If it turns too horrific, I'll have to think of something else.

Comment 5

[Jeff Walden [:Waldo]]

I filed bug 1417262 to back out in beta all the changes that led up to this bug.  I don't think the ultimate fix here is backportable, and I'm not particularly confident in some sort of hackish spot-workaround I could write -- and then only have tested on branches.  So a branch backout it is, and the real fix (which I expect will break syntax in some isolated edge cases) can happen on trunk at somewhat more leisure (given next uplift is about two months out).

Comment 6

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision f41930a869a8).

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Split FunctionSyntaxKind's Expression initializer into AssignmentExpression and PrimaryExpression flavors (even if for the moment only the latter is ever used or generated)

So the idea of these patches is to *change* our expression-closure syntax from allowing any function as PrimaryExpression to use the expression-closure syntax, to only allowing functions that are *also* the entirety of an AssignmentExpression to do so.  Then the next token after the AssignmentExpression is always gotten as Operand (potentially with modifier exceptions if the trailing AssignmentExpression produces them).  Technically this is a syntax change.  But 1) this is an extension already, and 2) the uses of this soon-to-be-broken syntax are rather silly.

Note this *doesn't* prevent expression closures from appearing in comma-separated contexts like Expression (comma expression) or argument lists.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Implement an ExpressionClosure::{Forbidden,Allowed} enum parameter in the parser, to distinguish places where our function-expression-closure syntax (normal function, just with an AssignmentExpression as body, not a braced StatementList) is permitted. (|

This just strings through the parameters but doesn't use them.  That's later.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Fix bug 1298809's test to function correctly on beta/release

I discovered this test was busted when trying to test my patchwork locally.  If I were reviewing, I think this would have been a vote for a somewhat different mechanism for disabling expression closure syntax in nightly builds.  :-\

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Limit the function expression-closure extension to apply only to function expressions that constitute an entire AssignmentExpression, so that the next token-get after the function expression closure can safely use Operand

Various iterations of this patch have done well on try, tho not this precise last/final version.  Will try-server, of course, before landing.

Comment 11

[Jeff Walden [:Waldo]]

...and remove the config/milestone.txt change before landing actually, of course.

Comment 12

[Tooru Fujisawa [:arai]]

Comment on attachment 8931087 [details] [diff] [review]
Limit the function expression-closure extension to apply only to function expressions that constitute an entire AssignmentExpression, so that the next token-get after the function expression closure can safely use Operand

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

This looks nice :D

Just a note, with this change, the affected case I can think of is following:

js> let function_or_undefined;
js> let f = function_or_undefined || function() 10;
typein:2:44 SyntaxError: missing { before function body:
typein:2:44 let f = function_or_undefined || function() 10;
typein:2:44 ............................................^

the error message suggests putting "{", that should happen anyway once expression-closure support gets removed,
so I think it's fine.

::: js/src/frontend/FullParseHandler.h
@@ +651,5 @@
>      ParseNode* newArrowFunction(const TokenPos& pos) {
>          return new_<CodeNode>(PNK_FUNCTION, JSOP_LAMBDA_ARROW, pos);
>      }
>  
> +    bool isFunctionExprBody(ParseNode* node) const {

this excludes arrow function, so the name should be isExpressionClosure

::: js/src/frontend/SyntaxParseHandler.h
@@ +55,5 @@
> +        NodeFunctionExpressionBlockBody,
> +
> +        // A non-arrow function expression with AssignmentExpression body -- a
> +        // proprietary SpiderMonkey extension.
> +        NodeFunctionExpressionExprBody,

This also should be NodeFunctionExpressionClosure or something

Comment 13

[Tom S [:evilpie]]

Do you plan to uplift all of this to beta? I would actually feel better about just disabling expression closures there instead of uplifting this. Fixing this on trunk isn't really useful, because we don't want to enable expression closures again.

Comment 14

[Jeff Walden [:Waldo]]

(In reply to Fuzzing Team from comment #6)
> JSBugMon: The testcase found in this bug no longer reproduces (tried
> revision f41930a869a8).

This would have been due to bug 1298809, I think.

Mini-try-run is going:

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/113bb57a4a4b
Split FunctionSyntaxKind's Expression initializer into AssignmentExpression and PrimaryExpression flavors (even if for the moment only the latter is ever used or generated).  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bcec4cc47cf
Implement an ExpressionClosure::{Forbidden,Allowed} enum parameter in the parser, to distinguish places where our function-expression-closure syntax (normal function, just with an AssignmentExpression as body, not a braced StatementList) is permitted.  (|function() foo| used to be supported anywhere PrimaryExpression was allowed, but ambiguity as to whether a binary operator is part of the function's body, or part of the AssignmentExpression the function-expression-closure was embedded in, led us to change this.)  Don't actually use this parameter yet -- just pass it in the right places.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/00a7aef6d3b2
Fix bug 1298809's test to function correctly on beta/release.  r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dd56cab6631
Limit the function expression-closure extension to apply only to function expressions that constitute an entire AssignmentExpression, so that the next token-get after the function expression closure can safely use Operand.  r=arai

Comment 16

[Jeff Walden [:Waldo]]

(In reply to Tom Schuster [:evilpie] from comment #13)
> Do you plan to uplift all of this to beta? I would actually feel better
> about just disabling expression closures there instead of uplifting this.
> Fixing this on trunk isn't really useful, because we don't want to enable
> expression closures again.

I wasn't planning to, as I backed out all the prior patchwork in this area on beta.  If we're really that unconcerned about this syntax extension that we would disable it on !release-or-beta -- and not by turning the feature off independent of release-cycle, to ride the trains from there -- it seems like doing something to obviate the !release-or-beta decision is a bit weird.

If we want to reland the backed-out work, and then land these on beta, I can go along with it.  But I don't intend to propose doing so.

Comment 17

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/113bb57a4a4b
https://hg.mozilla.org/mozilla-central/rev/0bcec4cc47cf
https://hg.mozilla.org/mozilla-central/rev/00a7aef6d3b2
https://hg.mozilla.org/mozilla-central/rev/8dd56cab6631