1440497 - 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:488

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P5)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision dd6caba14142 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu, run with --fuzzing-safe):

enableExpressionClosures();
eval(`
function f() {
        let c = function () 0 ? 1 : a => {};
`);


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x08072efe in js::frontend::TokenStreamShared::verifyConsistentModifier (modifier=js::frontend::Token::None, lookaheadToken=...) at js/src/frontend/TokenStream.h:486
#0  0x08072efe in js::frontend::TokenStreamShared::verifyConsistentModifier (modifier=js::frontend::Token::None, lookaheadToken=...) at js/src/frontend/TokenStream.h:486
#1  0x081230e7 in js::frontend::TokenStreamShared::verifyConsistentModifier (modifier=js::frontend::Token::None, lookaheadToken=...) at js/src/frontend/TokenStream.h:1289
#2  js::frontend::TokenStreamSpecific<char16_t, js::frontend::ParserAnyCharsAccess<js::frontend::GeneralParser<js::frontend::SyntaxParseHandler, char16_t> > >::getToken (this=0xffffc480, ttp=0xffffb080, modifier=js::frontend::Token::None, modifier=js::frontend::Token::None) at js/src/frontend/TokenStream.h:1290
#3  0x08147a90 in js::frontend::GeneralParser<js::frontend::SyntaxParseHandler, char16_t>::memberExpr (this=0xffffc0dc, yieldHandling=js::frontend::YieldIsName, tripledotHandling=js::frontend::TripledotProhibited, expressionClosureHandling=js::frontend::ExpressionClosure::Allowed, tt=js::frontend::TokenKind::Function, allowCallSyntax=true, possibleError=0xffffb21c, invoked=js::frontend::ParserBase::PredictUninvoked) at js/src/frontend/Parser.cpp:8594
#4  0x081482a9 in js::frontend::GeneralParser<js::frontend::SyntaxParseHandler, char16_t>::unaryExpr (this=0xffffc0dc, yieldHandling=js::frontend::YieldIsName, tripledotHandling=js::frontend::TripledotProhibited, expressionClosureHandling=js::frontend::ExpressionClosure::Allowed, possibleError=0xffffb21c, invoked=js::frontend::ParserBase::PredictUninvoked) at js/src/frontend/Parser.cpp:8412
#5  0x08148497 in js::frontend::GeneralParser<js::frontend::SyntaxParseHandler, char16_t>::orExpr (this=0xffffc0dc, inHandling=js::frontend::InAllowed, yieldHandling=js::frontend::YieldIsName, tripledotHandling=js::frontend::TripledotProhibited, expressionClosureHandling=<optimized out>, possibleError=0xffffb21c, invoked=js::frontend::ParserBase::PredictUninvoked) at js/src/frontend/Parser.cpp:7900
#6  0x081499df in js::frontend::GeneralParser<js::frontend::SyntaxParseHandler, char16_t>::condExpr (this=0xffffc0dc, inHandling=js::frontend::InAllowed, yieldHandling=js::frontend::YieldIsName, tripledotHandling=js::frontend::TripledotProhibited, expressionClosureHandling=js::frontend::ExpressionClosure::Allowed, possibleError=0xffffb21c, invoked=js::frontend::ParserBase::PredictUninvoked) at js/src/frontend/Parser.cpp:7979
#7  0x081443ad in js::frontend::GeneralParser<js::frontend::SyntaxParseHandler, char16_t>::assignExpr (this=0xffffc0dc, inHandling=js::frontend::InAllowed, yieldHandling=js::frontend::YieldIsName, tripledotHandling=js::frontend::TripledotProhibited, possibleError=0x0, invoked=js::frontend::ParserBase::PredictUninvoked) at js/src/frontend/Parser.cpp:8110
#8  0x08149c47 in js::frontend::GeneralParser<js::frontend::SyntaxParseHandler, char16_t>::initializerInNameDeclaration (this=0xffffc0dc, binding=js::frontend::SyntaxParseHandler::NodeName, declKind=js::frontend::DeclarationKind::Let, initialDeclaration=true, yieldHandling=js::frontend::YieldIsName, forHeadKind=0x0, forInOrOfExpression=0x0) at js/src/frontend/Parser.cpp:4855
#9  0x08149f80 in js::frontend::GeneralParser<js::frontend::SyntaxParseHandler, char16_t>::declarationName (this=0xffffc0dc, declKind=js::frontend::DeclarationKind::Let, tt=js::frontend::TokenKind::Name, initialDeclaration=true, yieldHandling=js::frontend::YieldIsName, forHeadKind=0x0, forInOrOfExpression=0x0) at js/src/frontend/Parser.cpp:4936
#10 0x0814a34e in js::frontend::GeneralParser<js::frontend::SyntaxParseHandler, char16_t>::declarationList (this=0xffffc0dc, yieldHandling=js::frontend::YieldIsName, kind=js::frontend::ParseNodeKind::Let, forHeadKind=0x0, forInOrOfExpression=0x0) at js/src/frontend/Parser.cpp:5026
#11 0x0814a4ae in js::frontend::GeneralParser<js::frontend::SyntaxParseHandler, char16_t>::lexicalDeclaration (this=0xffffc0dc, yieldHandling=js::frontend::YieldIsName, kind=js::frontend::DeclarationKind::Let) at js/src/frontend/Parser.cpp:5067
#12 0x0814b27d in js::frontend::GeneralParser<js::frontend::SyntaxParseHandler, char16_t>::statementListItem (this=0xffffc0dc, yieldHandling=js::frontend::YieldIsName, canHaveDirectives=true) at js/src/frontend/Parser.cpp:7635
#13 0x0814b6bc in js::frontend::GeneralParser<js::frontend::SyntaxParseHandler, char16_t>::statementList (this=0xffffc0dc, yieldHandling=js::frontend::YieldIsName) at js/src/frontend/Parser.cpp:4209
#14 0x0814d3f8 in js::frontend::GeneralParser<js::frontend::SyntaxParseHandler, char16_t>::functionBody (this=0xffffc0dc, inHandling=js::frontend::InAllowed, yieldHandling=js::frontend::YieldIsName, kind=js::frontend::Statement, type=js::frontend::GeneralParser<js::frontend::SyntaxParseHandler, char16_t>::StatementListBody) at js/src/frontend/Parser.cpp:2713
#15 0x0814db5a in js::frontend::GeneralParser<js::frontend::SyntaxParseHandler, char16_t>::functionFormalParametersAndBody (this=0xffffc0dc, inHandling=js::frontend::InAllowed, yieldHandling=js::frontend::YieldIsName, pn=js::frontend::SyntaxParseHandler::NodeGeneric, kind=js::frontend::Statement, parameterListEnd=..., isStandaloneFunction=false) at js/src/frontend/Parser.cpp:3807
#16 0x0814df0d in js::frontend::GeneralParser<js::frontend::SyntaxParseHandler, char16_t>::innerFunctionForFunctionBox (this=0xffffc0dc, funcNode=js::frontend::SyntaxParseHandler::NodeGeneric, outerpc=0xffffbbc0, funbox=0xf6e5f050, inHandling=js::frontend::InAllowed, yieldHandling=js::frontend::YieldIsName, kind=js::frontend::Statement, newDirectives=0xffffb918) at js/src/frontend/Parser.cpp:3587
#17 0x0815d732 in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::trySyntaxParseInnerFunction (this=0xffffc4ec, funcNode=0xf6e5f030, fun=..., toStringStart=1, inHandling=js::frontend::InAllowed, yieldHandling=js::frontend::YieldIsName, kind=js::frontend::Statement, generatorKind=js::GeneratorKind::NotGenerator, asyncKind=js::FunctionAsyncKind::SyncFunction, tryAnnexB=false, inheritedDirectives=..., newDirectives=0xffffb918) at js/src/frontend/Parser.cpp:3491
#18 0x0814e344 in js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::trySyntaxParseInnerFunction (newDirectives=0xffffb918, inheritedDirectives=..., tryAnnexB=<optimized out>, asyncKind=js::FunctionAsyncKind::SyncFunction, generatorKind=js::GeneratorKind::NotGenerator, kind=js::frontend::Statement, yieldHandling=js::frontend::YieldIsName, inHandling=js::frontend::InAllowed, toStringStart=1, fun=..., funcNode=0xf6e5f030, this=0xffffc4ec) at js/src/frontend/Parser.cpp:3564
#19 js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::functionDefinition (this=0xffffc4ec, funcNode=0xf6e5f030, toStringStart=1, inHandling=js::frontend::InAllowed, yieldHandling=js::frontend::YieldIsName, funName=..., kind=js::frontend::Statement, generatorKind=js::GeneratorKind::NotGenerator, asyncKind=js::FunctionAsyncKind::SyncFunction, tryAnnexB=false) at js/src/frontend/Parser.cpp:3417
#20 0x0814e808 in js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::functionStmt (this=0xffffc4ec, toStringStart=1, yieldHandling=js::frontend::YieldIsName, defaultHandling=js::frontend::NameRequired, asyncKind=js::FunctionAsyncKind::SyncFunction) at js/src/frontend/Parser.cpp:3957
#21 0x081596b0 in js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::statementListItem (this=0xffffc4ec, yieldHandling=js::frontend::YieldIsName, canHaveDirectives=true) at js/src/frontend/Parser.cpp:7721
#22 0x0815a158 in js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::statementList (this=0xffffc4ec, yieldHandling=js::frontend::YieldIsName) at js/src/frontend/Parser.cpp:4209
#23 0x0815e983 in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::evalBody (this=0xffffc4ec, evalsc=0xffffc018) at js/src/frontend/Parser.cpp:2162
#24 0x08a3b5ec in BytecodeCompiler::compileScript (this=0xffffc04c, environment=..., sc=0xffffc018) at js/src/frontend/BytecodeCompiler.cpp:329
[...]
eax	0x0	0
ebx	0x8e16ff4	148991988
ecx	0xf7d9f864	-136710044
edx	0x0	0
esi	0xffffb080	-20352
edi	0xa	10
ebp	0xffffaff8	4294946808
esp	0xffffaff0	4294946800
eip	0x8072efe <js::frontend::TokenStreamShared::verifyConsistentModifier(js::frontend::Token::Modifier, js::frontend::Token)+38>
=> 0x8072efe <js::frontend::TokenStreamShared::verifyConsistentModifier(js::frontend::Token::Modifier, js::frontend::Token)+38>:	movl   $0x0,0x0
   0x8072f08 <js::frontend::TokenStreamShared::verifyConsistentModifier(js::frontend::Token::Modifier, js::frontend::Token)+48>:	ud2

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Christian Holler (:decoder) from comment #0)
> enableExpressionClosures();

If we're not going to backport anything here, we should WONTFIX this IMO. Expression closures are disabled on Nightly and will be removed when FF 60 hits release (unless we have to re-enable to fix the web, but I really don't expect it).

Comment 2

[Jan de Mooij [:jandem]]

Or we can keep this open and close it when we fix bug 1083458.

Comment 3

[Christian Holler (:decoder)]

Please keep this either open or remove the shell function as a fix, otherwise these crashes will keep popping up in fuzzing and we have to deal with them.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Patch

Ugh, the prior patch to fix this issue, only fixed it if the full-parser took a look at it.  Gotta update the syntax parser to convert a normal function expression node to an expression-closure node, for the existing handling of this to work.

This patch should be backportable, tho might need to do some individual hunks manually.  Kind of a shame it has to futz with a bunch of function signatures to implement it, but that can't be helped now.

Comment 5

[Tooru Fujisawa [:arai]]

Comment on attachment 8954471 [details] [diff] [review]
Patch

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

Thanks :)
it would be nice to add note to bug 1083458 or somewhere to backout this patch when removing expression closure.

Comment 6

[Jan de Mooij [:jandem]]

Can't we just abortIfSyntaxParser when we see an expression closure?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f68825f0495d
When syntax-parsing, properly handle an ArrowFunction at the end of a ConditionalExpression that forms the body of a SpiderMonkey-proprietary expression closure.  r=arai

Comment 8

[Jeff Walden [:Waldo]]

We could abort, yes.  But I had to fix this previously for bug 1416337, and an unintended oversight writing that patch (plus tests inadvertently not exercising this) is the only reason full-parsing was still buggy.  It's a bad idea to have half-baked mechanism in the tree, nor to add a second layer of hack to hack around the half-bakedness of that bug's patch that itself hacked around issues with the historical expression closure "grammar" we implemented being not LR(k) or whatever.

Plus -- while syntax-aborting is a smaller change locally, it changes control flow in many, many more places globally.  This patch keeps us on the same basic path of execution, which -- despite the number of places being touched being larger -- actually seems *safer* and *better* for backporting, IMO.

This patch exactly applies without even fuzz on beta.  Will request approval shortly.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 8954471 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1416337's patch for this was half-baked, but the issue goes back years.

[User impact if declined]:
Unclear -- we're in a state that's difficult to reason about all possibilities.  *Maybe* there's no observable difference in behavior, because the ECMAScript grammar conspires such that this assertion indicates an internal inconsistency that can't *actually* change behavior.  But I don't know, and the effort it would take to know is non-trivial.

[Is this code covered by automated tests?]:
[Needs manual test from QE? If yes, steps to reproduce]:
Automated tests in the patch, nothing more needed.

[Has the fix been verified in Nightly?]:
Landed now, so should be shortly.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]: [Why is the change risky/not risky?]:
Not notably risky.  There's a *single* interesting line here:

+                handler.noteExpressionClosure(pn);

plus two definitions of that function that do obvious things within bug 1416337's approach to the overall problem.  The rest of the patch is mechanical, uninteresting change allowing ^^^ to mutate |*pn|.  The overall effect is to shift us to code paths that already handle this construct when full-parsing and are well-tested by the test this patch augments.

[String changes made/needed]: N/A

Comment 10

[Andreea Pavel [:apavel]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jeff Walden [:Waldo] from comment #8) 
> This patch exactly applies without even fuzz on beta.  Will request approval
> shortly.

That's because Beta and m-c are both tracking Gecko 60 at the moment (see dev-platform). It does *not* graft cleanly to release, which is where 59 now resides. I'm not feeling strongly convinced this is worth taking at this point in the 59 cycle anyway given that the next build we create is going to be the RC1. If you feel strongly that it should be, though, it's at a minimum going to require a rebased patch for uplift.

Comment 12

[Jeff Walden [:Waldo]]

Mm.  We shipped with this problem for some time (see history of bug 1416337), we can ship with it in syntax-parse cases for briefly longer.  Agreed that ext build as RC1 is mildly pushy, fine holding off.  (I have moderate curiosity what the release-delta looks like, but not enough to clone a tree to figure it out.)