[Merge M108][ThreadedBodyLoader] Send data to preload scanner in the …

…background

When ThreadedBodyLoader is enabled we have the decoded body data
available on a background thread. Previously the data was still sent to
the main thread before going to the preload scanner, even when the
preload scanner was also running on a background thread.

This patch allows sending data directly from
NavigationBodyLoader::OffThreadBodyReader to the preload scanner when
they are both on background threads. This avoids potential delays from
having to bounce to the main thread before preload scanning can happen.
On local traces, there could be >100ms delay between when the body data
has been read and when the preload scanner receives that data.

The flow of data to the preload scanner will go like this:
  1. NavigationBodyLoader calls DecodedBodyDataReceived, which will
     end up getting forwarded to HTMLDocumentParser::Append
  2. HTMLDocumentParser::Append will cause the background preload
     scanner to be created and scan the initial data passed to Append.
  3. NavigationBodyLoader calls TakeProcessBackgroundDataCallback()
     which tells HTMLDocumentParser to stop sending data to the
     preload scanner in Append.
  4. NavigationBodyLoader will pass data directly to the callback
     taken from TakeProcessBackgroundDataCallback(), which avoids
     hitting the main thread at all. HTMLDocumentParser will still
     receive data through Append() calls.

A couple notes:
  - Had to add a few *ForTesting() methods to make sure tests weren't
    flaky with the threading logic going on.
  - HTMLPreloadScanner is no longer SequenceBound but instead is a
    unique_ptr with a deleter on the sequence because I needed to grab
    a WeakPtr to it on the main thread.

(cherry picked from commit 0f0ddbd)

Bug: 1373213
Change-Id: If67df50f66e2ffb09e9fd75cbae0b78da66e31d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3953138
Reviewed-by: Mason Freed <masonf@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1059543}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3961308
Cr-Commit-Position: refs/branch-heads/5359@{#48}
Cr-Branched-From: 27d3765-refs/heads/main@{#1058933}

third_party/blink/public/platform/web_navigation_body_loader.h

@@ -14,6 +14,7 @@
 #include "third_party/blink/public/platform/web_common.h"
 #include "third_party/blink/public/platform/web_loader_freeze_mode.h"
 #include "third_party/blink/public/platform/web_url_error.h"
+#include "third_party/blink/renderer/platform/wtf/functional.h"
 
 namespace blink {
 
@@ -54,6 +55,16 @@ class BLINK_EXPORT WebNavigationBodyLoader {
         int64_t total_decoded_body_length,
         bool should_report_corb_blocking,
         const absl::optional<WebURLError>& error) = 0;
+
+    // The client can return a ProcessBackgroundDataCallback which will be
+    // called on a background thread with the decoded data. The returned
+    // callback will be called on a background thread with the same decoded data
+    // which will be given to DecodedBodyDataReceived().
+    using ProcessBackgroundDataCallback =
+        WTF::CrossThreadRepeatingFunction<void(const WebString&)>;
+    virtual ProcessBackgroundDataCallback TakeProcessBackgroundDataCallback() {
+      return ProcessBackgroundDataCallback();
+    }
   };
 
   // This method fills navigation params related to the navigation request,

third_party/blink/renderer/core/dom/document_encoding_data.h

@@ -31,14 +31,15 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_CORE_DOM_DOCUMENT_ENCODING_DATA_H_
 #define THIRD_PARTY_BLINK_RENDERER_CORE_DOM_DOCUMENT_ENCODING_DATA_H_
 
+#include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/platform/wtf/allocator/allocator.h"
 #include "third_party/blink/renderer/platform/wtf/text/text_encoding.h"
 
 namespace blink {
 class TextResourceDecoder;
 struct WebEncodingData;
 
-class DocumentEncodingData {
+class CORE_EXPORT DocumentEncodingData {
   DISALLOW_NEW();
 
  public:

third_party/blink/renderer/core/dom/document_parser.h

@@ -25,12 +25,14 @@
 #define THIRD_PARTY_BLINK_RENDERER_CORE_DOM_DOCUMENT_PARSER_H_
 
 #include <memory>
+#include "base/callback.h"
 #include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/core/dom/document_encoding_data.h"
 #include "third_party/blink/renderer/platform/bindings/name_client.h"
 #include "third_party/blink/renderer/platform/heap/collection_support/heap_hash_set.h"
 #include "third_party/blink/renderer/platform/heap/garbage_collected.h"
 #include "third_party/blink/renderer/platform/wtf/forward.h"
+#include "third_party/blink/renderer/platform/wtf/functional.h"
 
 namespace blink {
 
@@ -63,6 +65,11 @@ class CORE_EXPORT DocumentParser : public GarbageCollected<DocumentParser>,
   virtual void SetHasAppendedData() {}
   virtual void AppendDecodedData(const String& data,
                                  const DocumentEncodingData& encoding_data) {}
+  using BackgroundScanCallback =
+      WTF::CrossThreadRepeatingFunction<void(const String&)>;
+  virtual BackgroundScanCallback TakeBackgroundScanCallback() {
+    return BackgroundScanCallback();
+  }
 
   // FIXME: append() should be private, but DocumentLoader and DOMPatchSupport
   // uses it for now.

third_party/blink/renderer/core/dom/scriptable_document_parser.cc

@@ -67,6 +67,12 @@ InlineScriptStreamer* ScriptableDocumentParser::TakeInlineScriptStreamer(
   return nullptr;
 }
 
+bool ScriptableDocumentParser::HasInlineScriptStreamerForTesting(
+    const String& source) {
+  base::AutoLock lock(streamers_lock_);
+  return inline_script_streamers_.Contains(source);
+}
+
 void ScriptableDocumentParser::AddCSSTokenizer(
     const String& source,
     std::unique_ptr<CachedCSSTokenizer> tokenizer) {

third_party/blink/renderer/core/dom/scriptable_document_parser.h

@@ -78,6 +78,7 @@ class CORE_EXPORT ScriptableDocumentParser : public DecodedDataDocumentParser {
   // The returned streamer is guaranteed to be correct for script text that
   // matches the passed in |source|.
   InlineScriptStreamer* TakeInlineScriptStreamer(const String& source);
+  bool HasInlineScriptStreamerForTesting(const String& source);
 
   // Adds a tokenizer for |source| which can be later retrieved with
   // TakeCSSTokenizer(). This may be called on any thread.

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

@@ -114,10 +114,10 @@ TEST_F(BackgroundHTMLScannerTest, InsideHTMLPreloadScanner) {
               .task_runner = task_runner_, .min_size = 0u, .enabled = true},
           /*pretokenize_css_params=*/
           OptimizationParams{
-              .task_runner = task_runner_, .min_size = 0u, .enabled = true}));
-  preload_scanner.ScanInBackground(
-      "<script>foo</script>", GetDocument().ValidBaseElementURL(),
+              .task_runner = task_runner_, .min_size = 0u, .enabled = true}),
       CrossThreadBindRepeating([](std::unique_ptr<PendingPreloadData>) {}));
+  preload_scanner.ScanInBackground("<script>foo</script>",
+                                   GetDocument().ValidBaseElementURL());
   FlushTaskRunner();
   EXPECT_NE(parser->TakeInlineScriptStreamer("foo"), nullptr);
 }

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

@@ -90,11 +90,19 @@ class EndIfDelayedForbiddenScope;
 class ShouldCompleteScope;
 class AttemptToEndForbiddenScope;
 
-bool ThreadedPreloadScannerEnabled() {
+enum class FeatureResetMode {
+  kUseCached,
+  kResetForTesting,
+};
+
+bool ThreadedPreloadScannerEnabled(
+    FeatureResetMode reset_mode = FeatureResetMode::kUseCached) {
   // Cache the feature value since checking for each parser regresses some micro
   // benchmarks.
-  static const bool kEnabled =
+  static bool kEnabled =
       base::FeatureList::IsEnabled(features::kThreadedPreloadScanner);
+  if (reset_mode == FeatureResetMode::kResetForTesting)
+    kEnabled = base::FeatureList::IsEnabled(features::kThreadedPreloadScanner);
   return kEnabled;
 }
 
@@ -106,11 +114,14 @@ bool TimedParserBudgetEnabled() {
   return kEnabled;
 }
 
-bool PrecompileInlineScriptsEnabled() {
+bool PrecompileInlineScriptsEnabled(
+    FeatureResetMode reset_mode = FeatureResetMode::kUseCached) {
   // Cache the feature value since checking for each parser regresses some micro
   // benchmarks.
-  static const bool kEnabled =
+  static bool kEnabled =
       base::FeatureList::IsEnabled(features::kPrecompileInlineScripts);
+  if (reset_mode == FeatureResetMode::kResetForTesting)
+    kEnabled = base::FeatureList::IsEnabled(features::kPrecompileInlineScripts);
   return kEnabled;
 }
 
@@ -627,7 +638,7 @@ void HTMLDocumentParser::Detach() {
   preload_scanner_.reset();
   insertion_preload_scanner_.reset();
   background_script_scanner_.Reset();
-  background_scanner_.Reset();
+  background_scanner_.reset();
   // Oilpan: HTMLTokenProducer may allocate a fair amount of memory. Destroy
   // it to ensure that memory is released.
   token_producer_.reset();
@@ -1364,6 +1375,20 @@ void HTMLDocumentParser::NotifyNoRemainingAsyncScripts() {
     AttemptToRunDeferredScriptsAndEnd();
 }
 
+// static
+void HTMLDocumentParser::ResetCachedFeaturesForTesting() {
+  ThreadedPreloadScannerEnabled(FeatureResetMode::kResetForTesting);
+  PrecompileInlineScriptsEnabled(FeatureResetMode::kResetForTesting);
+}
+
+// static
+void HTMLDocumentParser::FlushPreloadScannerThreadForTesting() {
+  base::RunLoop run_loop;
+  GetPreloadScannerThread()->GetTaskRunner()->PostTask(FROM_HERE,
+                                                       run_loop.QuitClosure());
+  run_loop.Run();
+}
+
 void HTMLDocumentParser::ExecuteScriptsWaitingForResources() {
   TRACE_EVENT0("blink",
                "HTMLDocumentParser::ExecuteScriptsWaitingForResources");
@@ -1567,6 +1592,13 @@ std::string HTMLDocumentParser::GetPreloadHistogramSuffix() {
                        have_seen_first_byte ? ".NonInitial" : ".Initial"});
 }
 
+DocumentParser::BackgroundScanCallback
+HTMLDocumentParser::TakeBackgroundScanCallback() {
+  if (!background_scan_fn_)
+    return BackgroundScanCallback();
+  return CrossThreadBindRepeating(std::move(background_scan_fn_), KURL());
+}
+
 void HTMLDocumentParser::ScanInBackground(const String& source) {
   if (task_runner_state_->IsSynchronous() || !GetDocument()->Url().IsValid())
     return;
@@ -1580,17 +1612,31 @@ void HTMLDocumentParser::ScanInBackground(const String& source) {
     // is already available.
     DCHECK(!preload_scanner_);
     if (!background_scanner_) {
+      // See comment on NavigationBodyLoader::StartLoadingBodyInBackground() for
+      // details on how the preload scanner flow works when the body data is
+      // being loaded in the background.
       background_scanner_ = HTMLPreloadScanner::CreateBackground(
-          this, options_, GetPreloadScannerThread()->GetTaskRunner());
+          this, options_, GetPreloadScannerThread()->GetTaskRunner(),
+          CrossThreadBindRepeating(
+              &HTMLDocumentParser::AddPreloadDataOnBackgroundThread,
+              WrapCrossThreadWeakPersistent(this),
+              GetDocument()->GetTaskRunner(TaskType::kInternalLoading)));
+
+      background_scan_fn_ = CrossThreadBindRepeating(
+          [](base::WeakPtr<HTMLPreloadScanner> scanner,
+             scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+             const KURL& url, const String& data) {
+            PostCrossThreadTask(
+                *task_runner, FROM_HERE,
+                CrossThreadBindOnce(&HTMLPreloadScanner::ScanInBackground,
+                                    std::move(scanner), data, url));
+          },
+          background_scanner_->AsWeakPtr(),
+          GetPreloadScannerThread()->GetTaskRunner());
     }
 
-    background_scanner_.AsyncCall(&HTMLPreloadScanner::ScanInBackground)
-        .WithArgs(
-            source, GetDocument()->ValidBaseElementURL(),
-            CrossThreadBindRepeating(
-                &HTMLDocumentParser::AddPreloadDataOnBackgroundThread,
-                WrapCrossThreadPersistent(this),
-                GetDocument()->GetTaskRunner(TaskType::kInternalLoading)));
+    if (background_scan_fn_)
+      background_scan_fn_.Run(GetDocument()->ValidBaseElementURL(), source);
     return;
   }
 

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

@@ -126,6 +126,9 @@ class CORE_EXPORT HTMLDocumentParser : public ScriptableDocumentParser,
   void SetDecoder(std::unique_ptr<TextResourceDecoder>) final;
   void NotifyNoRemainingAsyncScripts() final;
 
+  static void ResetCachedFeaturesForTesting();
+  static void FlushPreloadScannerThreadForTesting();
+
  protected:
   HTMLDocumentParser(HTMLDocument&,
                      ParserSynchronizationPolicy,
@@ -167,6 +170,7 @@ class CORE_EXPORT HTMLDocumentParser : public ScriptableDocumentParser,
   void DocumentElementAvailable() override;
   void CommitPreloadedData() override;
   void FlushPendingPreloads() override;
+  BackgroundScanCallback TakeBackgroundScanCallback() override;
 
   // HTMLParserScriptRunnerHost
   void NotifyScriptLoaded() final;
@@ -250,7 +254,10 @@ class CORE_EXPORT HTMLDocumentParser : public ScriptableDocumentParser,
   // A scanner used only for input provided to the insert() method.
   std::unique_ptr<HTMLPreloadScanner> insertion_preload_scanner_;
   WTF::SequenceBound<BackgroundHTMLScanner> background_script_scanner_;
-  WTF::SequenceBound<HTMLPreloadScanner> background_scanner_;
+  HTMLPreloadScanner::BackgroundPtr background_scanner_;
+  using BackgroundScanFn =
+      WTF::CrossThreadRepeatingFunction<void(const KURL&, const String&)>;
+  BackgroundScanFn background_scan_fn_;
 
   scoped_refptr<base::SingleThreadTaskRunner> loading_task_runner_;
 

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

@@ -189,6 +189,62 @@ TEST_P(HTMLDocumentParserTest, AppendNoPrefetch) {
   static_cast<DocumentParser*>(parser)->StopParsing();
 }
 
+class HTMLDocumentParserThreadedPreloadScannerTest : public PageTestBase {
+ protected:
+  HTMLDocumentParserThreadedPreloadScannerTest() {
+    scoped_feature_list_.InitWithFeatures(
+        {features::kThreadedPreloadScanner, features::kPrecompileInlineScripts},
+        {});
+    HTMLDocumentParser::ResetCachedFeaturesForTesting();
+  }
+
+  ~HTMLDocumentParserThreadedPreloadScannerTest() override {
+    scoped_feature_list_.Reset();
+    HTMLDocumentParser::ResetCachedFeaturesForTesting();
+  }
+
+  void SetUp() override {
+    PageTestBase::SetUp();
+    GetDocument().SetURL(KURL("https://example.test"));
+  }
+
+  HTMLDocumentParser* CreateParser(HTMLDocument& document) {
+    return MakeGarbageCollected<HTMLDocumentParser>(document,
+                                                    kAllowDeferredParsing);
+  }
+
+ private:
+  base::test::ScopedFeatureList scoped_feature_list_;
+};
+
+TEST_F(HTMLDocumentParserThreadedPreloadScannerTest,
+       TakeBackgroundScanCallback) {
+  auto& document = To<HTMLDocument>(GetDocument());
+  HTMLDocumentParser* parser = CreateParser(document);
+  ScopedParserDetacher detacher(parser);
+
+  // First append "foo" script which should be passed through to the scanner.
+  parser->AppendDecodedData("<script>foo</script>", DocumentEncodingData());
+  HTMLDocumentParser::FlushPreloadScannerThreadForTesting();
+  EXPECT_TRUE(parser->HasInlineScriptStreamerForTesting("foo"));
+
+  // Now take the callback.
+  auto callback =
+      static_cast<DocumentParser*>(parser)->TakeBackgroundScanCallback();
+
+  // Append "bar" script which should not be passed to the scanner.
+  parser->AppendDecodedData("<script>bar</script>", DocumentEncodingData());
+  HTMLDocumentParser::FlushPreloadScannerThreadForTesting();
+  EXPECT_FALSE(parser->HasInlineScriptStreamerForTesting("bar"));
+
+  // Append "baz" script to the callback which should be passed to the scanner.
+  callback.Run("<script>baz</script>");
+  HTMLDocumentParser::FlushPreloadScannerThreadForTesting();
+  EXPECT_TRUE(parser->HasInlineScriptStreamerForTesting("baz"));
+
+  static_cast<DocumentParser*>(parser)->StopParsing();
+}
+
 class HTMLDocumentParserProcessImmediatelyTest : public PageTestBase {
  protected:
   void SetUp() override {