Closed Bug 1257053 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

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

People

(Reporter: gkw, Assigned: mrrrgn)

References

Details

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

Attachments

(1 file, 3 obsolete files)

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

((({w = x} >(-9)

Backtrace:

0   js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010004cdde js::frontend::Parser<js::frontend::FullParseHandler>::PossibleError::transferErrorTo(js::frontend::Parser<js::frontend::FullParseHandler>::PossibleError*) + 174 (Parser.cpp:3997)
1   js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005d6b7 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) + 823 (TokenStream.h:557)
2   js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005d0a5 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:7546)
3   js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005c815 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:9586)
4   js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005f673 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:8689)
5   js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005ef5d 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:8217)
6   js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005e8fb 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:7707)
7   js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005e6b2 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:7761)
8   js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005d66e 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) + 750 (Parser.cpp:7887)
9   js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005d0a5 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:7546)
10  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005c815 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:9586)
11  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005f673 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:8689)
12  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005ef5d 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:8217)
13  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005e8fb 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:7707)
14  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005e6b2 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:7761)
15  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005d66e 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) + 750 (Parser.cpp:7887)
16  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005d0a5 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:7546)
17  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005c815 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:9586)
18  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005f673 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:8689)
19  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005ef5d 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:8217)
20  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005e8fb 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:7707)
21  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005e6b2 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:7761)
22  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005d66e 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) + 750 (Parser.cpp:7887)
23  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005d0a5 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:7546)
24  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005c815 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:9586)
25  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005f673 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:8689)
26  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005ef5d 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:8217)
27  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005e8fb 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:7707)
28  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005e6b2 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:7761)
29  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005d66e 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) + 750 (Parser.cpp:7887)
30  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010005d0a5 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:7546)
31  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x0000000100050933 js::frontend::Parser<js::frontend::FullParseHandler>::expressionStatement(js::frontend::YieldHandling, js::frontend::Parser<js::frontend::FullParseHandler>::InvokedPrediction) + 99 (Parser.cpp:7599)
32  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010004ff8c js::frontend::Parser<js::frontend::FullParseHandler>::statement(js::frontend::YieldHandling, bool) + 1580 (Parser.cpp:7384)
33  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010004dc7c js::frontend::Parser<js::frontend::FullParseHandler>::statements(js::frontend::YieldHandling) + 572 (Parser.cpp:3531)
34  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x00000001000474bd js::frontend::Parser<js::frontend::FullParseHandler>::globalBody() + 77 (Parser.cpp:1106)
35  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x0000000100965f4d BytecodeCompiler::compileScript(JS::Handle<JSObject*>, JS::Handle<JSScript*>) + 733 (BytecodeCompiler.cpp:527)
36  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010096700d 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)
37  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x00000001005512dc Compile(JSContext*, JS::ReadOnlyCompileOptions const&, SyntacticScopeOption, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) + 396 (RootingAPI.h:481)
38  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010055161b Compile(JSContext*, JS::ReadOnlyCompileOptions const&, SyntacticScopeOption, char const*, unsigned long, JS::MutableHandle<JSScript*>) + 267 (jsapi.cpp:3986)
39  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x000000010055176c JS::Compile(JSContext*, JS::ReadOnlyCompileOptions const&, __sFILE*, JS::MutableHandle<JSScript*>) + 108 (jsapi.cpp:4012)
40  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x0000000100020876 Process(JSContext*, char const*, bool, FileKind) + 3270 (js.cpp:514)
41  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x0000000100005f92 main + 12370 (js.cpp:6612)
42  js-dbg-64-dm-clang-darwin-f0c0480732d3	0x0000000100000ff4 start + 52
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160305111220" and the hash "bd154748b83a56ee103c21a324b5bd1c4cc28492".
The "bad" changeset has the timestamp "20160305125420" and the hash "bbab21ac3b8cce6a8748fad28c9b4d42e156216d".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=bd154748b83a56ee103c21a324b5bd1c4cc28492&tochange=bbab21ac3b8cce6a8748fad28c9b4d42e156216d
Assignee: nobody → winter2718
Guessing bug 932080 is likely at fault here.
Blocks: 932080
This, as well as the last bug, are symptoms of the  fact that we aren't expressing these "possible errors" as early as we can. Holding on to them too long causes them to pile up and triggers this assertion.

Here are other cases where failures would be expected:

({x=1}[-1]);
true ? {x=1} : 1
({x=y}[-9])
({x=y}.x.z[-9])
({x=y}`${-9}`)
(new {x=y}(-9))

The way forward seems clear now. It's a matter of identifying sections of the parser where possible errors can't possible recover, and blowing them up as soon as they are encountered.
Attached patch possibleerrorfix.diff (obsolete) — Splinter Review
A much needed paring back of "possible error" usage, along with 24 new test cases (including the expression listed in the bug description).
Attachment #8732042 - Flags: review?(jorendorff)
Attached patch possibleerrorfix.diff (obsolete) — Splinter Review
Caught a wee little logic error.
Attachment #8732042 - Attachment is obsolete: true
Attachment #8732042 - Flags: review?(jorendorff)
Attachment #8732043 - Flags: review?(jorendorff)
Comment on attachment 8732043 [details] [diff] [review]
possibleerrorfix.diff

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

r=me. Yeah, this is a lot easier to think about. Thanks for all the new tests.

::: js/src/frontend/Parser.cpp
@@ +7717,5 @@
>              return pn;
>  
> +        // From this point onward no errors will be recoverable, setting
> +        // possibleError to null enforces this.
> +        possibleError = nullptr;

The comment should say:

    // From this point on, destructuring defaults are definitely an error.

I still think orExpr1() needs to check as soon as a binary operator is found:

         if (tok == TOK_IN ? inHandling == InAllowed : TokenKindIsBinaryOp(tok)) {
             pnk = BinaryOpTokenKindToParseNodeKind(tok);
+            if (possibleError) {
+                if (!possibleError.checkForExprErrors())
+                    return false;
+            }
         } else {

I think the behavior is correct without this, but only barely. For example:

    ({x=1} + 2) = 3;

As soon as we see `+` we should throw. The way it is right now, I think the possibleError is never checked - fortunately, the destructuring code catches the bug.

@@ +7782,2 @@
>      Node thenExpr = assignExpr(InAllowed, yieldHandling, TripledotProhibited,
>                                 possibleError);

Style nit: Instead of setting possibleError to null, explicitly pass nullptr to assignExpr here and below.

@@ +9336,5 @@
>                  seenCoverInitializedName = true;
> +
> +                // If there is no "possibleError" available for stashing the error
> +                // we must report it now.
> +                if (!possibleError) {

Comment nit: This makes it sound like the problem is finding room to store the error. :)

Also, a trick is, sometimes you can avoid an English "If" clause by putting the comment inside the if-statement:

    if (!possibleError) {
        // Destructuring defaults are definitely not allowed in this object literal,
        // because of something the caller knows about the preceding code.
        // For example, maybe the preceding token is an operator: `x + {y=z}`.

@@ +9344,5 @@
> +
> +                // Here we set a pending error so that later in the parse, once we've
> +                // determined whether or not we're destructuring, the error can be
> +                // reported or ignored appropriately.
> +                if (!possibleError->setPending(ParseError, JSMSG_BAD_PROP_ID, false)) {

I think JSMSG_COLON_AFTER_ID is a better message here:

    missing : after property id

because I think this error will mostly happen to people who just thoughtlessly typed something like

    let obj = {name="morgan", rank="admiral"};

accidentally substituting equal signs for the colons.
Attachment #8732043 - Flags: review?(jorendorff) → review+
https://hg.mozilla.org/mozilla-central/rev/5b73e9893546
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Attached patch possibleerrorfix.diff (obsolete) — Splinter Review
Attachment #8732043 - Attachment is obsolete: true
Is comment 10 due for backport to mozilla-aurora?
Flags: needinfo?(winter2718)
Attachment #8741671 - Flags: approval-mozilla-aurora?
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #11)
> Is comment 10 due for backport to mozilla-aurora?

Requesting uplift for this and Bug 1260620
Flags: needinfo?(winter2718)
Comment on attachment 8741671 [details] [diff] [review]
possibleerrorfix.diff

Hi Morgan, please renominate and fill out the aurora uplift template.
Flags: needinfo?(winter2718)
Attachment #8741671 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Approval Request Comment
[Feature/regressing bug #]: 1257053
[User impact if declined]: Crashes when impacted JS statements are executed.
[Describe test coverage new/current, TreeHerder]: SpiderMonkey (jit tests) are included with the patch. 
[Risks and why]: 
[String/UUID change made/needed]
Attachment #8741671 - Attachment is obsolete: true
Flags: needinfo?(winter2718)
Attachment #8744080 - Flags: approval-mozilla-aurora?
Comment on attachment 8744080 [details] [diff] [review]
possibleerrorfix.diff

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