fix: avoid PumpTokenizer calls when IsStopped

Improves checks around deferred parser scheduling to make sure that
PumpTokenizer does not operate when the parser's stopped, improves
parser termination to deschedule any pending pumps, and expands test
coverage.

Bug: 901056, 1106314
Change-Id: I03787e9a9e440fc9ce92d98040b1cdce3ea1ddc5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2302612
Commit-Queue: Richard Townsend <richard.townsend@arm.com>
Reviewed-by: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792405}

third_party/blink/renderer/core/html/parser/html_document_parser.cc

@@ -144,6 +144,10 @@ class HTMLDocumentParserState
     }
   }
 
+  bool HasPendingWorkScheduled() const {
+    return IsScheduled() || IsScheduledToDelayEnd() || ShouldEndIfDelayed();
+  }
+
   void SetEndIfDelayed(bool value) { end_if_delayed_ = value; }
   void SetShouldComplete(bool value) { should_complete_ = value; }
   bool ShouldEndIfDelayed() const { return end_if_delayed_; }
@@ -351,9 +355,17 @@ void HTMLDocumentParser::Trace(Visitor* visitor) const {
   HTMLParserScriptRunnerHost::Trace(visitor);
 }
 
+bool HTMLDocumentParser::HasPendingWorkScheduledForTesting() const {
+  return task_runner_state_->HasPendingWorkScheduled();
+}
+
 void HTMLDocumentParser::Detach() {
   if (have_background_parser_)
     StopBackgroundParser();
+  // Deschedule any pending tokenizer pumps.
+  task_runner_state_->SetState(
+      HTMLDocumentParserState::DeferredParserState::kNotScheduled);
+  task_runner_state_->SetEndIfDelayed(false);
   DocumentParser::Detach();
   if (script_runner_)
     script_runner_->Detach();
@@ -382,6 +394,7 @@ void HTMLDocumentParser::StopParsing() {
   }
   task_runner_state_->SetState(
       HTMLDocumentParserState::DeferredParserState::kNotScheduled);
+  task_runner_state_->SetEndIfDelayed(false);
   if (have_background_parser_)
     StopBackgroundParser();
 }
@@ -431,13 +444,18 @@ void HTMLDocumentParser::DeferredPumpTokenizerIfPossible() {
   // This function should only be called when
   // --enable-blink-features=ForceSynchronousHTMLParsing is available.
   DCHECK(RuntimeEnabledFeatures::ForceSynchronousHTMLParsingEnabled());
+  // If we're scheduled for a tokenizer pump, then document should be attached
+  // and the parser should not be stopped, but sometimes a script completes
+  // loading (so we schedule a pump) but the Document is stopped in the meantime
+  // (e.g. fast/parser/iframe-onload-document-close-with-external-script.html).
+  DCHECK(task_runner_state_->GetState() ==
+             HTMLDocumentParserState::DeferredParserState::kNotScheduled ||
+         (!IsStopped() && !IsDetached()))
+      << "Document should be attached (" << !IsDetached()
+      << ") and not stopped (" << !IsStopped() << ")";
   TRACE_EVENT2("blink", "HTMLDocumentParser::DeferredPumpTokenizerIfPossible",
                "parser", (void*)this, "state",
                task_runner_state_->GetStateAsString());
-
-  if (IsDetached())
-    return;
-
   if (task_runner_state_->IsScheduled()) {
     HTMLDocumentParser::PumpTokenizerIfPossible();
   } else if (task_runner_state_->IsScheduledToDelayEnd()) {
@@ -456,7 +474,7 @@ void HTMLDocumentParser::PumpTokenizerIfPossible() {
   bool yielded = false;
   const bool should_call_delay_end = task_runner_state_->ShouldEndIfDelayed();
   CheckIfBlockingStylesheetAdded();
-  if ((!IsStopped() && !IsPaused()) || should_call_delay_end) {
+  if (!IsStopped() && (!IsPaused() || should_call_delay_end)) {
     yielded = PumpTokenizer();
   }
 
@@ -470,7 +488,7 @@ void HTMLDocumentParser::PumpTokenizerIfPossible() {
       if (task_runner_state_->ShouldComplete() ||
           task_runner_state_->GetMode() != kAllowDeferredParsing) {
         EndIfDelayed();  // Synchronous case
-      } else {
+      } else if (!IsStopped()) {
         ScheduleEndIfDelayed();  // async case
       }
     }
@@ -926,6 +944,7 @@ bool HTMLDocumentParser::PumpTokenizer() {
 void HTMLDocumentParser::SchedulePumpTokenizer() {
   TRACE_EVENT0("blink", "HTMLDocumentParser::SchedulePumpTokenizer");
   DCHECK(RuntimeEnabledFeatures::ForceSynchronousHTMLParsingEnabled());
+  DCHECK(!IsStopped());
   loading_task_runner_->PostTask(
       FROM_HERE, WTF::Bind(&HTMLDocumentParser::DeferredPumpTokenizerIfPossible,
                            WrapPersistent(this)));
@@ -936,6 +955,7 @@ void HTMLDocumentParser::SchedulePumpTokenizer() {
 void HTMLDocumentParser::ScheduleEndIfDelayed() {
   TRACE_EVENT0("blink", "HTMLDocumentParser::ScheduleEndIfDelayed");
   DCHECK(RuntimeEnabledFeatures::ForceSynchronousHTMLParsingEnabled());
+  DCHECK(!IsStopped());
   task_runner_state_->SetEndIfDelayed(true);
   task_runner_state_->SetState(
       HTMLDocumentParserState::DeferredParserState::kScheduledWithEndIfDelayed);

third_party/blink/renderer/core/html/parser/html_document_parser.h

@@ -101,6 +101,10 @@ class CORE_EXPORT HTMLDocumentParser : public ScriptableDocumentParser,
   HTMLParserScriptRunnerHost* AsHTMLParserScriptRunnerHostForTesting() {
     return this;
   }
+  // Returns true if any tokenizer pumps / end if delayed / asynchronous work is
+  // scheduled. Exposed so that tests can check that the parser's exited in a
+  // good state.
+  bool HasPendingWorkScheduledForTesting() const;
 
   HTMLTokenizer* Tokenizer() const { return tokenizer_.get(); }
 

third_party/blink/renderer/core/html/parser/html_document_parser_loading_test.cc

@@ -2,10 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "third_party/blink/renderer/core/html/parser/html_document_parser.h"
-
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/renderer/core/dom/document.h"
+#include "third_party/blink/renderer/core/html/parser/html_document_parser.h"
 #include "third_party/blink/renderer/core/testing/sim/sim_request.h"
 #include "third_party/blink/renderer/core/testing/sim/sim_test.h"
 #include "third_party/blink/renderer/platform/testing/unit_test_helpers.h"
@@ -20,23 +19,49 @@ class HTMLDocumentParserSimTest : public SimTest {
   }
 };
 
-class HTMLDocumentParserLoadingTest : public HTMLDocumentParserSimTest,
-                                      public testing::WithParamInterface<bool> {
+class HTMLDocumentParserLoadingTest
+    : public HTMLDocumentParserSimTest,
+      public testing::WithParamInterface<ParserSynchronizationPolicy> {
  protected:
   HTMLDocumentParserLoadingTest() {
-    Document::SetThreadedParsingEnabledForTesting(GetParam());
+    if (GetParam() == ParserSynchronizationPolicy::kForceSynchronousParsing) {
+      Document::SetThreadedParsingEnabledForTesting(false);
+    } else {
+      Document::SetThreadedParsingEnabledForTesting(true);
+    }
+
+    if (GetParam() == ParserSynchronizationPolicy::kAllowDeferredParsing) {
+      RuntimeEnabledFeatures::SetForceSynchronousHTMLParsingEnabled(true);
+    } else {
+      RuntimeEnabledFeatures::SetForceSynchronousHTMLParsingEnabled(false);
+    }
   }
   static bool SheetInHeadBlocksParser() {
     return RuntimeEnabledFeatures::BlockHTMLParserOnStyleSheetsEnabled();
   }
 };
 
-INSTANTIATE_TEST_SUITE_P(Threaded,
+INSTANTIATE_TEST_SUITE_P(HTMLDocumentParserLoadingTest,
                          HTMLDocumentParserLoadingTest,
-                         testing::Values(true));
-INSTANTIATE_TEST_SUITE_P(NotThreaded,
-                         HTMLDocumentParserLoadingTest,
-                         testing::Values(false));
+                         testing::Values(kAllowDeferredParsing,
+                                         kAllowAsynchronousParsing,
+                                         kForceSynchronousParsing));
+
+TEST_P(HTMLDocumentParserLoadingTest, IFrameDoesNotRenterParser) {
+  SimRequest main_resource("https://example.com/test.html", "text/html");
+  SimRequest::Params params;
+  params.response_http_status = 200;
+  SimSubresourceRequest js("https://example.com/non-existent.js",
+                           "application/javascript", params);
+  LoadURL("https://example.com/test.html");
+  main_resource.Complete(R"HTML(
+<script src="non-existent.js"></script>
+<iframe onload="document.write('This test passes if it does not crash'); document.close();"></iframe>
+  )HTML");
+  test::RunPendingTasks();
+  js.Complete("");
+  test::RunPendingTasks();
+}
 
 TEST_P(HTMLDocumentParserLoadingTest,
        PauseParsingForExternalStylesheetsInHead) {

third_party/blink/renderer/core/html/parser/html_document_parser_test.cc

@@ -12,6 +12,7 @@
 #include "third_party/blink/renderer/core/loader/prerenderer_client.h"
 #include "third_party/blink/renderer/core/testing/page_test_base.h"
 #include "third_party/blink/renderer/platform/heap/heap.h"
+#include "third_party/blink/renderer/platform/testing/unit_test_helpers.h"
 
 namespace blink {
 
@@ -28,16 +29,29 @@ class MockPrerendererClient : public PrerendererClient {
   bool is_prefetch_only_;
 };
 
-class HTMLDocumentParserTest : public PageTestBase {
+class HTMLDocumentParserTest
+    : public PageTestBase,
+      public testing::WithParamInterface<ParserSynchronizationPolicy> {
  protected:
   void SetUp() override {
     PageTestBase::SetUp();
     GetDocument().SetURL(KURL("https://example.test"));
+    if (GetParam() == ParserSynchronizationPolicy::kForceSynchronousParsing) {
+      Document::SetThreadedParsingEnabledForTesting(false);
+    } else {
+      Document::SetThreadedParsingEnabledForTesting(true);
+    }
+    if (GetParam() == ParserSynchronizationPolicy::kAllowDeferredParsing) {
+      RuntimeEnabledFeatures::SetForceSynchronousHTMLParsingEnabled(true);
+    } else if (GetParam() ==
+               ParserSynchronizationPolicy::kAllowAsynchronousParsing) {
+      RuntimeEnabledFeatures::SetForceSynchronousHTMLParsingEnabled(false);
+    }
   }
 
   HTMLDocumentParser* CreateParser(HTMLDocument& document) {
-    auto* parser = MakeGarbageCollected<HTMLDocumentParser>(
-        document, kForceSynchronousParsing);
+    auto* parser =
+        MakeGarbageCollected<HTMLDocumentParser>(document, GetParam());
     std::unique_ptr<TextResourceDecoder> decoder(
         BuildTextResourceDecoderFor(&document, "text/html", g_null_atom));
     parser->SetDecoder(std::move(decoder));
@@ -47,7 +61,55 @@ class HTMLDocumentParserTest : public PageTestBase {
 
 }  // namespace
 
-TEST_F(HTMLDocumentParserTest, AppendPrefetch) {
+INSTANTIATE_TEST_SUITE_P(HTMLDocumentParserTest,
+                         HTMLDocumentParserTest,
+                         testing::Values(kForceSynchronousParsing,
+                                         kAllowDeferredParsing));
+
+TEST_P(HTMLDocumentParserTest, StopThenPrepareToStopShouldNotCrash) {
+  auto& document = To<HTMLDocument>(GetDocument());
+  DocumentParser* parser = CreateParser(document);
+  const char kBytes[] = "<html>";
+  parser->AppendBytes(kBytes, sizeof(kBytes));
+  // These methods are not supposed to be called one after the other, but in
+  // practice it can happen (e.g. if navigation is aborted).
+  parser->StopParsing();
+  parser->PrepareToStopParsing();
+}
+
+TEST_P(HTMLDocumentParserTest, HasNoPendingWorkAfterStopParsing) {
+  auto& document = To<HTMLDocument>(GetDocument());
+  HTMLDocumentParser* parser = CreateParser(document);
+  DocumentParser* control_parser = static_cast<DocumentParser*>(parser);
+  const char kBytes[] = "<html>";
+  control_parser->AppendBytes(kBytes, sizeof(kBytes));
+  control_parser->StopParsing();
+  EXPECT_FALSE(parser->HasPendingWorkScheduledForTesting());
+}
+
+TEST_P(HTMLDocumentParserTest, HasNoPendingWorkAfterStopParsingThenAppend) {
+  auto& document = To<HTMLDocument>(GetDocument());
+  HTMLDocumentParser* parser = CreateParser(document);
+  DocumentParser* control_parser = static_cast<DocumentParser*>(parser);
+  const char kBytes1[] = "<html>";
+  control_parser->AppendBytes(kBytes1, sizeof(kBytes1));
+  control_parser->StopParsing();
+  const char kBytes2[] = "<head>";
+  control_parser->AppendBytes(kBytes2, sizeof(kBytes2));
+  EXPECT_FALSE(parser->HasPendingWorkScheduledForTesting());
+}
+
+TEST_P(HTMLDocumentParserTest, HasNoPendingWorkAfterDetach) {
+  auto& document = To<HTMLDocument>(GetDocument());
+  HTMLDocumentParser* parser = CreateParser(document);
+  DocumentParser* control_parser = static_cast<DocumentParser*>(parser);
+  const char kBytes[] = "<html>";
+  control_parser->AppendBytes(kBytes, sizeof(kBytes));
+  control_parser->Detach();
+  EXPECT_FALSE(parser->HasPendingWorkScheduledForTesting());
+}
+
+TEST_P(HTMLDocumentParserTest, AppendPrefetch) {
   auto& document = To<HTMLDocument>(GetDocument());
   ProvidePrerendererClientTo(
       *document.GetPage(),
@@ -66,9 +128,12 @@ TEST_F(HTMLDocumentParserTest, AppendPrefetch) {
   // DCHECK).
   static_cast<DocumentParser*>(parser)->Finish();
   EXPECT_EQ(HTMLTokenizer::kDataState, parser->Tokenizer()->GetState());
+  // Cancel any pending work to make sure that RuntimeFeatures DCHECKs do not
+  // fire.
+  (static_cast<DocumentParser*>(parser))->StopParsing();
 }
 
-TEST_F(HTMLDocumentParserTest, AppendNoPrefetch) {
+TEST_P(HTMLDocumentParserTest, AppendNoPrefetch) {
   auto& document = To<HTMLDocument>(GetDocument());
   EXPECT_FALSE(document.IsPrefetchOnly());
   // Use ForceSynchronousParsing to allow calling append().
@@ -76,11 +141,15 @@ TEST_F(HTMLDocumentParserTest, AppendNoPrefetch) {
 
   const char kBytes[] = "<ht";
   parser->AppendBytes(kBytes, sizeof(kBytes));
+  test::RunPendingTasks();
   // The bytes are forwarded to the tokenizer.
   HTMLParserScriptRunnerHost* script_runner_host =
       parser->AsHTMLParserScriptRunnerHostForTesting();
   EXPECT_FALSE(script_runner_host->HasPreloadScanner());
   EXPECT_EQ(HTMLTokenizer::kTagNameState, parser->Tokenizer()->GetState());
+  // Cancel any pending work to make sure that RuntimeFeatures DCHECKs do not
+  // fire.
+  (static_cast<DocumentParser*>(parser))->StopParsing();
 }
 
 }  // namespace blink