[modules] Disallow HTML comments in modules

Bug: v8:5045
Change-Id: I1d8b6be8a65595dc357c4f721b1a03425e025e6e
Reviewed-on: https://chromium-review.googlesource.com/463811
Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
Reviewed-by: Daniel Ehrenberg <littledan@chromium.org>
Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45107}

src/messages.h

@@ -590,6 +590,7 @@ class ErrorUtils : public AllStatic {
   T(MalformedRegExp, "Invalid regular expression: /%/: %")                     \
   T(MalformedRegExpFlags, "Invalid regular expression flags")                  \
   T(ModuleExportUndefined, "Export '%' is not defined in module")              \
+  T(HtmlCommentInModule, "HTML comments are not allowed in modules")           \
   T(MultipleDefaultsInSwitch,                                                  \
     "More than one default clause in switch statement")                        \
   T(NewlineAfterThrow, "Illegal newline after throw")                          \

src/parsing/parser.cc

@@ -631,7 +631,7 @@ FunctionLiteral* Parser::ParseProgram(Isolate* isolate, ParseInfo* info) {
 
   {
     std::unique_ptr<Utf16CharacterStream> stream(ScannerStream::For(source));
-    scanner_.Initialize(stream.get());
+    scanner_.Initialize(stream.get(), info->is_module());
     result = DoParseProgram(info);
   }
   if (result != NULL) {
@@ -805,7 +805,7 @@ FunctionLiteral* Parser::ParseFunction(Isolate* isolate, ParseInfo* info) {
     std::unique_ptr<Utf16CharacterStream> stream(ScannerStream::For(
         source, shared_info->start_position(), shared_info->end_position()));
     Handle<String> name(String::cast(shared_info->name()));
-    scanner_.Initialize(stream.get());
+    scanner_.Initialize(stream.get(), info->is_module());
     info->set_function_name(ast_value_factory()->GetString(name));
     result = DoParseFunction(info);
     if (result != nullptr) {
@@ -3548,7 +3548,7 @@ void Parser::ParseOnBackground(ParseInfo* info) {
                                     runtime_call_stats_));
     stream_ptr = stream.get();
   }
-  scanner_.Initialize(stream_ptr);
+  scanner_.Initialize(stream_ptr, info->is_module());
   DCHECK(info->maybe_outer_scope_info().is_null());
 
   DCHECK(original_scope_);

src/parsing/scanner.cc

@@ -182,16 +182,14 @@ Scanner::Scanner(UnicodeCache* unicode_cache)
       octal_message_(MessageTemplate::kNone),
       found_html_comment_(false) {}
 
-void Scanner::Initialize(Utf16CharacterStream* source) {
+void Scanner::Initialize(Utf16CharacterStream* source, bool is_module) {
   DCHECK_NOT_NULL(source);
   source_ = source;
+  is_module_ = is_module;
   // Need to capture identifiers in order to recognize "get" and "set"
   // in object literals.
   Init();
-  // Skip initial whitespace allowing HTML comment ends just like
-  // after a newline and scan first token.
   has_line_terminator_before_next_ = true;
-  SkipWhiteSpace();
   Scan();
 }
 
@@ -443,7 +441,7 @@ static inline bool IsLittleEndianByteOrderMark(uc32 c) {
   return c == 0xFFFE;
 }
 
-bool Scanner::SkipWhiteSpace() {
+Token::Value Scanner::SkipWhiteSpace() {
   int start_position = source_pos();
 
   while (true) {
@@ -481,11 +479,26 @@ bool Scanner::SkipWhiteSpace() {
     }
 
     // Treat the rest of the line as a comment.
-    SkipSingleLineComment();
+    Token::Value token = SkipSingleHTMLComment();
+    if (token == Token::ILLEGAL) {
+      return token;
+    }
   }
 
   // Return whether or not we skipped any characters.
-  return source_pos() != start_position;
+  if (source_pos() == start_position) {
+    return Token::ILLEGAL;
+  }
+
+  return Token::WHITESPACE;
+}
+
+Token::Value Scanner::SkipSingleHTMLComment() {
+  if (is_module_) {
+    ReportScannerError(source_pos(), MessageTemplate::kHtmlCommentInModule);
+    return Token::ILLEGAL;
+  }
+  return SkipSingleLineComment();
 }
 
 Token::Value Scanner::SkipSingleLineComment() {
@@ -606,7 +619,7 @@ Token::Value Scanner::ScanHtmlComment() {
   }
 
   found_html_comment_ = true;
-  return SkipSingleLineComment();
+  return SkipSingleHTMLComment();
 }
 
 void Scanner::Scan() {
@@ -712,7 +725,7 @@ void Scanner::Scan() {
           if (c0_ == '>' && HasAnyLineTerminatorBeforeNext()) {
             // For compatibility with SpiderMonkey, we skip lines that
             // start with an HTML comment end '-->'.
-            token = SkipSingleLineComment();
+            token = SkipSingleHTMLComment();
           } else {
             token = Token::DEC;
           }
@@ -864,10 +877,11 @@ void Scanner::Scan() {
           token = ScanIdentifierOrKeyword();
         } else if (IsDecimalDigit(c0_)) {
           token = ScanNumber(false);
-        } else if (SkipWhiteSpace()) {
-          token = Token::WHITESPACE;
         } else {
-          token = Select(Token::ILLEGAL);
+          token = SkipWhiteSpace();
+          if (token == Token::ILLEGAL) {
+            Advance();
+          }
         }
         break;
     }

src/parsing/scanner.h

@@ -197,7 +197,7 @@ class Scanner {
 
   explicit Scanner(UnicodeCache* scanner_contants);
 
-  void Initialize(Utf16CharacterStream* source);
+  void Initialize(Utf16CharacterStream* source, bool is_module);
 
   // Returns the next token and advances input.
   Token::Value Next();
@@ -689,7 +689,8 @@ class Scanner {
   // Scans a single JavaScript token.
   void Scan();
 
-  bool SkipWhiteSpace();
+  Token::Value SkipWhiteSpace();
+  Token::Value SkipSingleHTMLComment();
   Token::Value SkipSingleLineComment();
   Token::Value SkipSourceURLComment();
   void TryToParseSourceURLComment();
@@ -717,6 +718,8 @@ class Scanner {
   template <bool capture_raw>
   uc32 ScanUnicodeEscape();
 
+  bool is_module_;
+
   Token::Value ScanTemplateSpan();
 
   // Return the current source position.

test/cctest/parsing/test-scanner.cc

@@ -39,7 +39,7 @@ ScannerTestHelper make_scanner(const char* src) {
   helper.stream = ScannerStream::ForTesting(src);
   helper.scanner =
       std::unique_ptr<Scanner>(new Scanner(helper.unicode_cache.get()));
-  helper.scanner->Initialize(helper.stream.get());
+  helper.scanner->Initialize(helper.stream.get(), false);
   return helper;
 }
 

test/cctest/test-parsing.cc

@@ -78,7 +78,7 @@ TEST(ScanKeywords) {
     {
       auto stream = i::ScannerStream::ForTesting(keyword, length);
       i::Scanner scanner(&unicode_cache);
-      scanner.Initialize(stream.get());
+      scanner.Initialize(stream.get(), false);
       CHECK_EQ(key_token.token, scanner.Next());
       CHECK_EQ(i::Token::EOS, scanner.Next());
     }
@@ -86,7 +86,7 @@ TEST(ScanKeywords) {
     {
       auto stream = i::ScannerStream::ForTesting(keyword, length - 1);
       i::Scanner scanner(&unicode_cache);
-      scanner.Initialize(stream.get());
+      scanner.Initialize(stream.get(), false);
       CHECK_EQ(i::Token::IDENTIFIER, scanner.Next());
       CHECK_EQ(i::Token::EOS, scanner.Next());
     }
@@ -97,7 +97,7 @@ TEST(ScanKeywords) {
       buffer[length] = chars_to_append[j];
       auto stream = i::ScannerStream::ForTesting(buffer, length + 1);
       i::Scanner scanner(&unicode_cache);
-      scanner.Initialize(stream.get());
+      scanner.Initialize(stream.get(), false);
       CHECK_EQ(i::Token::IDENTIFIER, scanner.Next());
       CHECK_EQ(i::Token::EOS, scanner.Next());
     }
@@ -107,7 +107,7 @@ TEST(ScanKeywords) {
       buffer[length - 1] = '_';
       auto stream = i::ScannerStream::ForTesting(buffer, length);
       i::Scanner scanner(&unicode_cache);
-      scanner.Initialize(stream.get());
+      scanner.Initialize(stream.get(), false);
       CHECK_EQ(i::Token::IDENTIFIER, scanner.Next());
       CHECK_EQ(i::Token::EOS, scanner.Next());
     }
@@ -173,7 +173,7 @@ TEST(ScanHTMLEndComments) {
     const char* source = tests[i];
     auto stream = i::ScannerStream::ForTesting(source);
     i::Scanner scanner(CcTest::i_isolate()->unicode_cache());
-    scanner.Initialize(stream.get());
+    scanner.Initialize(stream.get(), false);
     i::Zone zone(CcTest::i_isolate()->allocator(), ZONE_NAME);
     i::AstValueFactory ast_value_factory(
         &zone, CcTest::i_isolate()->ast_string_constants(),
@@ -192,7 +192,7 @@ TEST(ScanHTMLEndComments) {
     const char* source = fail_tests[i];
     auto stream = i::ScannerStream::ForTesting(source);
     i::Scanner scanner(CcTest::i_isolate()->unicode_cache());
-    scanner.Initialize(stream.get());
+    scanner.Initialize(stream.get(), false);
     i::Zone zone(CcTest::i_isolate()->allocator(), ZONE_NAME);
     i::AstValueFactory ast_value_factory(
         &zone, CcTest::i_isolate()->ast_string_constants(),
@@ -209,6 +209,28 @@ TEST(ScanHTMLEndComments) {
   }
 }
 
+TEST(ScanHtmlComments) {
+  const char* src = "a <!-- b --> c";
+  i::UnicodeCache unicode_cache;
+
+  // Disallow HTML comments.
+  {
+    auto stream = i::ScannerStream::ForTesting(src);
+    i::Scanner scanner(&unicode_cache);
+    scanner.Initialize(stream.get(), true);
+    CHECK_EQ(i::Token::IDENTIFIER, scanner.Next());
+    CHECK_EQ(i::Token::ILLEGAL, scanner.Next());
+  }
+
+  // Skip HTML comments:
+  {
+    auto stream = i::ScannerStream::ForTesting(src);
+    i::Scanner scanner(&unicode_cache);
+    scanner.Initialize(stream.get(), false);
+    CHECK_EQ(i::Token::IDENTIFIER, scanner.Next());
+    CHECK_EQ(i::Token::EOS, scanner.Next());
+  }
+}
 
 class ScriptResource : public v8::String::ExternalOneByteStringResource {
  public:
@@ -365,7 +387,7 @@ TEST(StandAlonePreParser) {
   for (int i = 0; programs[i]; i++) {
     auto stream = i::ScannerStream::ForTesting(programs[i]);
     i::Scanner scanner(CcTest::i_isolate()->unicode_cache());
-    scanner.Initialize(stream.get());
+    scanner.Initialize(stream.get(), false);
 
     i::Zone zone(CcTest::i_isolate()->allocator(), ZONE_NAME);
     i::AstValueFactory ast_value_factory(
@@ -401,7 +423,7 @@ TEST(StandAlonePreParserNoNatives) {
   for (int i = 0; programs[i]; i++) {
     auto stream = i::ScannerStream::ForTesting(programs[i]);
     i::Scanner scanner(isolate->unicode_cache());
-    scanner.Initialize(stream.get());
+    scanner.Initialize(stream.get(), false);
 
     // Preparser defaults to disallowing natives syntax.
     i::Zone zone(CcTest::i_isolate()->allocator(), ZONE_NAME);
@@ -471,7 +493,7 @@ TEST(RegressChromium62639) {
 
   auto stream = i::ScannerStream::ForTesting(program);
   i::Scanner scanner(CcTest::i_isolate()->unicode_cache());
-  scanner.Initialize(stream.get());
+  scanner.Initialize(stream.get(), false);
   i::Zone zone(CcTest::i_isolate()->allocator(), ZONE_NAME);
   i::AstValueFactory ast_value_factory(
       &zone, CcTest::i_isolate()->ast_string_constants(),
@@ -548,7 +570,7 @@ TEST(PreParseOverflow) {
 
   auto stream = i::ScannerStream::ForTesting(program.get(), kProgramSize);
   i::Scanner scanner(isolate->unicode_cache());
-  scanner.Initialize(stream.get());
+  scanner.Initialize(stream.get(), false);
 
   i::Zone zone(CcTest::i_isolate()->allocator(), ZONE_NAME);
   i::AstValueFactory ast_value_factory(
@@ -568,7 +590,7 @@ void TestStreamScanner(i::Utf16CharacterStream* stream,
                        int skip_pos = 0,  // Zero means not skipping.
                        int skip_to = 0) {
   i::Scanner scanner(CcTest::i_isolate()->unicode_cache());
-  scanner.Initialize(stream);
+  scanner.Initialize(stream, false);
 
   int i = 0;
   do {
@@ -646,7 +668,7 @@ void TestScanRegExp(const char* re_source, const char* expected) {
   auto stream = i::ScannerStream::ForTesting(re_source);
   i::HandleScope scope(CcTest::i_isolate());
   i::Scanner scanner(CcTest::i_isolate()->unicode_cache());
-  scanner.Initialize(stream.get());
+  scanner.Initialize(stream.get(), false);
 
   i::Token::Value start = scanner.peek();
   CHECK(start == i::Token::DIV || start == i::Token::ASSIGN_DIV);
@@ -1334,7 +1356,7 @@ void TestParserSyncWithFlags(i::Handle<i::String> source,
                            &pending_error_handler,
                            isolate->counters()->runtime_call_stats());
     SetParserFlags(&preparser, flags);
-    scanner.Initialize(stream.get());
+    scanner.Initialize(stream.get(), is_module);
     i::PreParser::PreParseResult result = preparser.PreParseProgram(is_module);
     CHECK_EQ(i::PreParser::kPreParseSuccess, result);
   }

test/test262/local-tests/test/language/module-code/comment-single-line-html-close-comment-before-function.js

@@ -0,0 +1,16 @@
+// Copyright (C) 2017 the V8 project authors. All rights reserved.
+// This code is governed by the BSD license found in the LICENSE file.
+/*---
+description: >
+    HTML-like comments are not available in module code
+    (SingleLineHTMLCloseComment)
+esid: sec-html-like-comments
+es6id: B1.3
+negative:
+  phase: early
+  type: SyntaxError
+flags: [module]
+---*/
+
+-->
+function f(){}

test/test262/local-tests/test/language/module-code/comment-single-line-html-close-comment-newline-before-function.js

@@ -0,0 +1,15 @@
+// Copyright (C) 2017 the V8 project authors. All rights reserved.
+// This code is governed by the BSD license found in the LICENSE file.
+/*---
+description: >
+    HTML-like comments are not available in module code
+    (SingleLineHTMLCloseComment)
+esid: sec-html-like-comments
+es6id: B1.3
+negative:
+  phase: early
+  type: SyntaxError
+flags: [module]
+---*/
+-->
+function f(){}

test/test262/test262.status

@@ -369,9 +369,6 @@
   'language/statements/async-function/early-errors-declaration-formals-body-duplicate': [FAIL],
 
   # Module-related tests
-  # v8:5485
-  'language/module-code/comment-multi-line-html*': [FAIL],
-  'language/module-code/comment-single-line-html*': [FAIL],
   # v8:5487
   'language/module-code/namespace/internals/get-own-property-str-found-uninit': [FAIL],
   # https://github.com/tc39/ecma262/pull/858