Closed Bug 1254335 Opened 4 years ago Closed 4 years ago

Assertion failure: !possibleError->hasError(), at js/src/frontend/Parser.cpp:7831

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- unaffected
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: gkw, Assigned: mrrrgn)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update])

Attachments

(1 file, 1 obsolete file)

The following testcase crashes on mozilla-central revision be593a64d7c6 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

({e=[]}==(

Backtrace:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010005d6d3 js::frontend::Parser<js::frontend::FullParseHandler>::assignExpr(js::frontend::InHandling, js::frontend::YieldHandling, js::frontend::TripledotHandling, js::frontend::Parser<js::frontend::FullParseHandler>::PossibleError*, js::frontend::Parser<js::frontend::FullParseHandler>::InvokedPrediction) + 1843 (Parser.cpp:7831)
1   js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010005ccc5 js::frontend::Parser<js::frontend::FullParseHandler>::expr(js::frontend::InHandling, js::frontend::YieldHandling, js::frontend::TripledotHandling, js::frontend::Parser<js::frontend::FullParseHandler>::PossibleError*, js::frontend::Parser<js::frontend::FullParseHandler>::InvokedPrediction) + 37 (Parser.cpp:7540)
2   js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010005c435 js::frontend::Parser<js::frontend::FullParseHandler>::primaryExpr(js::frontend::YieldHandling, js::frontend::TripledotHandling, js::frontend::Parser<js::frontend::FullParseHandler>::PossibleError*, js::frontend::TokenKind, js::frontend::Parser<js::frontend::FullParseHandler>::InvokedPrediction) + 1637 (Parser.cpp:9576)
3   js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010005f2d3 js::frontend::Parser<js::frontend::FullParseHandler>::memberExpr(js::frontend::YieldHandling, js::frontend::TripledotHandling, js::frontend::Parser<js::frontend::FullParseHandler>::PossibleError*, js::frontend::TokenKind, bool, js::frontend::Parser<js::frontend::FullParseHandler>::InvokedPrediction) + 803 (Parser.cpp:8684)
4   js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010005ebbd js::frontend::Parser<js::frontend::FullParseHandler>::unaryExpr(js::frontend::YieldHandling, js::frontend::TripledotHandling, js::frontend::Parser<js::frontend::FullParseHandler>::PossibleError*, js::frontend::Parser<js::frontend::FullParseHandler>::InvokedPrediction) + 749 (Parser.cpp:8212)
5   js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010005e55b js::frontend::Parser<js::frontend::FullParseHandler>::orExpr1(js::frontend::InHandling, js::frontend::YieldHandling, js::frontend::TripledotHandling, js::frontend::Parser<js::frontend::FullParseHandler>::PossibleError*, js::frontend::Parser<js::frontend::FullParseHandler>::InvokedPrediction) + 107 (Parser.cpp:7701)
6   js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010005e312 js::frontend::Parser<js::frontend::FullParseHandler>::condExpr1(js::frontend::InHandling, js::frontend::YieldHandling, js::frontend::TripledotHandling, js::frontend::Parser<js::frontend::FullParseHandler>::PossibleError*, js::frontend::Parser<js::frontend::FullParseHandler>::InvokedPrediction) + 34 (Parser.cpp:7755)
7   js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010005d29d js::frontend::Parser<js::frontend::FullParseHandler>::assignExpr(js::frontend::InHandling, js::frontend::YieldHandling, js::frontend::TripledotHandling, js::frontend::Parser<js::frontend::FullParseHandler>::PossibleError*, js::frontend::Parser<js::frontend::FullParseHandler>::InvokedPrediction) + 765 (Parser.cpp:7882)
8   js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010005ccc5 js::frontend::Parser<js::frontend::FullParseHandler>::expr(js::frontend::InHandling, js::frontend::YieldHandling, js::frontend::TripledotHandling, js::frontend::Parser<js::frontend::FullParseHandler>::PossibleError*, js::frontend::Parser<js::frontend::FullParseHandler>::InvokedPrediction) + 37 (Parser.cpp:7540)
9   js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010005c435 js::frontend::Parser<js::frontend::FullParseHandler>::primaryExpr(js::frontend::YieldHandling, js::frontend::TripledotHandling, js::frontend::Parser<js::frontend::FullParseHandler>::PossibleError*, js::frontend::TokenKind, js::frontend::Parser<js::frontend::FullParseHandler>::InvokedPrediction) + 1637 (Parser.cpp:9576)
10  js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010005f2d3 js::frontend::Parser<js::frontend::FullParseHandler>::memberExpr(js::frontend::YieldHandling, js::frontend::TripledotHandling, js::frontend::Parser<js::frontend::FullParseHandler>::PossibleError*, js::frontend::TokenKind, bool, js::frontend::Parser<js::frontend::FullParseHandler>::InvokedPrediction) + 803 (Parser.cpp:8684)
11  js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010005ebbd js::frontend::Parser<js::frontend::FullParseHandler>::unaryExpr(js::frontend::YieldHandling, js::frontend::TripledotHandling, js::frontend::Parser<js::frontend::FullParseHandler>::PossibleError*, js::frontend::Parser<js::frontend::FullParseHandler>::InvokedPrediction) + 749 (Parser.cpp:8212)
12  js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010005e55b js::frontend::Parser<js::frontend::FullParseHandler>::orExpr1(js::frontend::InHandling, js::frontend::YieldHandling, js::frontend::TripledotHandling, js::frontend::Parser<js::frontend::FullParseHandler>::PossibleError*, js::frontend::Parser<js::frontend::FullParseHandler>::InvokedPrediction) + 107 (Parser.cpp:7701)
13  js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010005e312 js::frontend::Parser<js::frontend::FullParseHandler>::condExpr1(js::frontend::InHandling, js::frontend::YieldHandling, js::frontend::TripledotHandling, js::frontend::Parser<js::frontend::FullParseHandler>::PossibleError*, js::frontend::Parser<js::frontend::FullParseHandler>::InvokedPrediction) + 34 (Parser.cpp:7755)
14  js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010005d29d js::frontend::Parser<js::frontend::FullParseHandler>::assignExpr(js::frontend::InHandling, js::frontend::YieldHandling, js::frontend::TripledotHandling, js::frontend::Parser<js::frontend::FullParseHandler>::PossibleError*, js::frontend::Parser<js::frontend::FullParseHandler>::InvokedPrediction) + 765 (Parser.cpp:7882)
15  js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010005ccc5 js::frontend::Parser<js::frontend::FullParseHandler>::expr(js::frontend::InHandling, js::frontend::YieldHandling, js::frontend::TripledotHandling, js::frontend::Parser<js::frontend::FullParseHandler>::PossibleError*, js::frontend::Parser<js::frontend::FullParseHandler>::InvokedPrediction) + 37 (Parser.cpp:7540)
16  js-dbg-64-dm-clang-darwin-be593a64d7c6	0x00000001000505d3 js::frontend::Parser<js::frontend::FullParseHandler>::expressionStatement(js::frontend::YieldHandling, js::frontend::Parser<js::frontend::FullParseHandler>::InvokedPrediction) + 99 (Parser.cpp:7593)
17  js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010004fc2c js::frontend::Parser<js::frontend::FullParseHandler>::statement(js::frontend::YieldHandling, bool) + 1580 (Parser.cpp:7378)
18  js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010004d91c js::frontend::Parser<js::frontend::FullParseHandler>::statements(js::frontend::YieldHandling) + 572 (Parser.cpp:3530)
19  js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010004715d js::frontend::Parser<js::frontend::FullParseHandler>::globalBody() + 77 (Parser.cpp:1105)
20  js-dbg-64-dm-clang-darwin-be593a64d7c6	0x0000000100956bed BytecodeCompiler::compileScript(JS::Handle<JSObject*>, JS::Handle<JSScript*>) + 733 (BytecodeCompiler.cpp:527)
21  js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010095896d js::frontend::CompileScript(js::ExclusiveContext*, js::LifoAlloc*, JS::Handle<JSObject*>, JS::Handle<js::StaticScope*>, JS::Handle<JSScript*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JSString*, js::SourceCompressionTask*, js::ScriptSourceObject**) + 189 (BytecodeCompiler.cpp:738)
22  js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010054a754 Compile(JSContext*, JS::ReadOnlyCompileOptions const&, SyntacticScopeOption, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) + 404 (RootingAPI.h:481)
23  js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010054aadb Compile(JSContext*, JS::ReadOnlyCompileOptions const&, SyntacticScopeOption, char const*, unsigned long, JS::MutableHandle<JSScript*>) + 267 (jsapi.cpp:3981)
24  js-dbg-64-dm-clang-darwin-be593a64d7c6	0x000000010054ac2c JS::Compile(JSContext*, JS::ReadOnlyCompileOptions const&, __sFILE*, JS::MutableHandle<JSScript*>) + 108 (jsapi.cpp:4007)
25  js-dbg-64-dm-clang-darwin-be593a64d7c6	0x00000001000208b6 Process(JSContext*, char const*, bool, FileKind) + 3286 (js.cpp:514)
26  js-dbg-64-dm-clang-darwin-be593a64d7c6	0x0000000100006742 main + 11778 (js.cpp:6563)
27  js-dbg-64-dm-clang-darwin-be593a64d7c6	0x00000001000018b4 start + 52
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/bbab21ac3b8c
user:        Morgan Phillips
date:        Sat Mar 05 12:51:38 2016 -0800
summary:     Bug 932080 - Support default values in destructuring; r=jorendorff

Morgan, is bug 932080 a likely regressor?
Blocks: 932080
Flags: needinfo?(winter2718)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #1)
> autoBisect shows this is probably related to the following changeset:
> 
> The first bad revision is:
> changeset:   https://hg.mozilla.org/mozilla-central/rev/bbab21ac3b8c
> user:        Morgan Phillips
> date:        Sat Mar 05 12:51:38 2016 -0800
> summary:     Bug 932080 - Support default values in destructuring;
> r=jorendorff
> 
> Morgan, is bug 932080 a likely regressor?

It is, taking the bug. I'd been hesitant about this assertion from the start. Will consider removing it altogether after a short investigation. Thanks !
Assignee: nobody → winter2718
Flags: needinfo?(winter2718)
So, not having a pending error when we enter an assignExpr is not an invariant. Instead, it's a sign that we're in a bad state and we should raise the pending error right away. I'll attempt to distill this into a convincing comment. Patch coming soon.
Attached patch assertion.diff (obsolete) — Splinter Review
Sorry for the delay here. So, now we know that !pendingError->hasError() isn't an invariant (in assignExpr) but I do believe its violation will always accompany an existing syntax error. That said, after going back and forth about this all day, I decided to remove the assertion altogether rather than replace it with an error check.

My primary reason for this is that the intent of the original assertion was to guarantee that pending errors are not overwritten. I believe a better way of checking this is to just assert inside of PossibleError::setPending. The intent is more clear and violations of the original assertion will still produce errors.

I may disagree with myself after a nights sleep, but for now it's my position.
Attachment #8728323 - Flags: review?(jorendorff)
Attachment #8728323 - Flags: review?(jorendorff)
Attached patch assertion.diffSplinter Review
Attachment #8728323 - Attachment is obsolete: true
Attachment #8728346 - Flags: review?(jorendorff)
I moved from an assertion to just failing to set a pending error (and returning false). This seems more sensible.
Comment on attachment 8728346 [details] [diff] [review]
assertion.diff

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

All right, land it - but I don't feel very confident that it's correct this time. If any more bugs are found, it'll be time to take a step back and figure out a way to cover all the cases systematically. Hopefully something simple that we can trust.
Attachment #8728346 - Flags: review?(jorendorff) → review+
https://hg.mozilla.org/mozilla-central/rev/73f748c3b24b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Let's get this onto Aurora as well. A small testcase is pretty easy to trigger the issue on that branch.
Flags: needinfo?(winter2718)
Comment on attachment 8728346 [details] [diff] [review]
assertion.diff

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: May cause crashes.
[Describe test coverage new/current, TreeHerder]: A unit test case is included in the patch.
[Risks and why]: 
[String/UUID change made/needed]:
Flags: needinfo?(winter2718)
Attachment #8728346 - Flags: approval-mozilla-aurora?
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #12)
> Let's get this onto Aurora as well. A small testcase is pretty easy to
> trigger the issue on that branch.

Agreed. There was another fuzzbug that we may need to flag. Checking my history.
s/fuzzbug/similar bug/
I do have crashes that occur at js::jit::MConstant::toObjectOrNull which reduce to this assertion on mozilla-aurora.
Comment on attachment 8728346 [details] [diff] [review]
assertion.diff

Potential Crash fix, Aurora47+
Attachment #8728346 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.