Closed
Bug 1440497
Opened 6 years ago
Closed 6 years ago
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
Categories
(Core :: JavaScript Engine, defect, P5)
Tracking
()
RESOLVED
FIXED
mozilla60
People
(Reporter: decoder, Assigned: Waldo)
References
Details
(4 keywords, Whiteboard: [jsbugmon:update,bisect])
Attachments
(1 file)
29.86 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
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•6 years ago
|
||
(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•6 years ago
|
||
Or we can keep this open and close it when we fix bug 1083458.
Depends on: 1083458
Reporter | ||
Comment 3•6 years ago
|
||
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.
Updated•6 years ago
|
Severity: critical → normal
Priority: -- → P5
Assignee | ||
Comment 4•6 years ago
|
||
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.
Attachment #8954471 -
Flags: review?(arai.unmht)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Comment 5•6 years ago
|
||
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.
Attachment #8954471 -
Flags: review?(arai.unmht) → review+
Comment 6•6 years ago
|
||
Can't we just abortIfSyntaxParser when we see an expression closure?
Pushed by jwalden@mit.edu: 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
Assignee | ||
Comment 8•6 years ago
|
||
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.
Assignee | ||
Comment 9•6 years ago
|
||
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
Attachment #8954471 -
Flags: approval-mozilla-beta?
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f68825f0495d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 11•6 years ago
|
||
(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.
Flags: needinfo?(jwalden+bmo)
Updated•6 years ago
|
status-firefox58:
--- → wontfix
status-firefox59:
--- → affected
status-firefox-esr52:
--- → wontfix
Assignee | ||
Comment 12•6 years ago
|
||
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.)
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Updated•6 years ago
|
Attachment #8954471 -
Flags: approval-mozilla-beta?
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•