Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.

The repro case is:

foo.js:
export default (   )

Ran like this:

d8 --module foo.js

And the bug is that we reach this code:

void AstNumberingVisitor::VisitEmptyParentheses(EmptyParentheses* node) {
  UNREACHABLE();
}

Update:

So, we need to somehow disallow "export default ()", which uses "()" as an expression (ParseAssignmentExpression returns EmptyParenthesis). "()" is already disallowed as an expression in other places, and the checks are in various places in the code too:

let x = (); << ValidateExpression called from ParseVariableDeclarations

if (()) { ... } << ValidateExpression called from ParseExpression, called from ParseIfStatement


I thought the fix would be:

ParserBase<Impl>::ParseAssignmentExpression(bool accept_IN, bool* ok) {
  ...
  ExpressionT expression;
  if (IsTrivialExpression()) {
    expression = ParsePrimaryExpression(&is_async, CHECK_OK);
  } else {
    expression = ParseConditionalExpression(accept_IN, CHECK_OK);
  }
  ...

  if (peek() == Token::ARROW) {
   ...
   return expression;
+  } else {
+    ValidateExpression(CHECK_OK);
   }


But that's not the correct fix - and I don't understand why.

Apparently in many cases, e.g., here:

((x, {y = x}) => assertEquals(42, y))(42, {});
      ^^^^^

is_valid_expression() is false (because of "Invalid shorthand property initializer").

I don't understand what is_valid_expression() means, since "y = x" is a valid expression...

---------------


However, a less generic fix https://chromium-review.googlesource.com/#/c/v8/v8/+/856937 seems to work. But still, I'd like to understand why the more generic fix is not correct...

The "invalid expression" here isn't "y = x", but "{y = x}" (which is indeed an invalid expression). The trouble with the proposed generic fix is that it's not required that the left-hand-side of an AssignmentPattern be an expression (indeed, "{y = x}" is one such valid LHS but invalid expression).

As it happens, "()" is both an invalid LHS _and_ an invalid expression, but I think the fix in the linked CL (which does the checking just outside ParseAssignmentExpression) is the right fix.

Ahh, this makes perfect sense. Thanks, adamk@.

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/15eb10b571a9c08c79abf30cd149fd6d75d7cf3f

commit 15eb10b571a9c08c79abf30cd149fd6d75d7cf3f
Author: Marja Hölttä <marja@chromium.org>
Date: Wed Jan 10 09:32:50 2018

[parser] Fix: disallow "export default ()".

BUG= chromium:797581 

Change-Id: I08f880a907f122480a014763975ecc07e2c49f7d
Reviewed-on: https://chromium-review.googlesource.com/856937
Reviewed-by: Adam Klein <adamk@chromium.org>
Commit-Queue: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50471}
[modify] https://crrev.com/15eb10b571a9c08c79abf30cd149fd6d75d7cf3f/src/parsing/parser.cc
[modify] https://crrev.com/15eb10b571a9c08c79abf30cd149fd6d75d7cf3f/test/mjsunit/mjsunit.status
[add] https://crrev.com/15eb10b571a9c08c79abf30cd149fd6d75d7cf3f/test/mjsunit/regress/modules-skip-regress-797581-1.js
[add] https://crrev.com/15eb10b571a9c08c79abf30cd149fd6d75d7cf3f/test/mjsunit/regress/modules-skip-regress-797581-2.js
[add] https://crrev.com/15eb10b571a9c08c79abf30cd149fd6d75d7cf3f/test/mjsunit/regress/modules-skip-regress-797581-3.js
[add] https://crrev.com/15eb10b571a9c08c79abf30cd149fd6d75d7cf3f/test/mjsunit/regress/modules-skip-regress-797581-4.js
[add] https://crrev.com/15eb10b571a9c08c79abf30cd149fd6d75d7cf3f/test/mjsunit/regress/modules-skip-regress-797581-5.js
[add] https://crrev.com/15eb10b571a9c08c79abf30cd149fd6d75d7cf3f/test/mjsunit/regress/regress-797581.js

ClusterFuzz has detected this issue as fixed in range 528344:528345.

Detailed report: https://clusterfuzz.com/testcase?key=4570742481223680

Fuzzer: libFuzzer_javascript_parser_proto_fuzzer
Job Type: libfuzzer_chrome_asan
Platform Id: linux

Crash Type: Ill
Crash Address: 0x000002510aa8
Crash State:
  v8::internal::AstNumberingVisitor::VisitEmptyParentheses
  v8::internal::AstNumberingVisitor::VisitNoStackOverflowCheck
  v8::internal::AstNumberingVisitor::VisitStatements
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan&range=524603:524604
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan&range=528344:528345

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=4570742481223680

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

ClusterFuzz testcase 4570742481223680 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.