From 957b9a0ea9b0d793c223818a2aefb84b9f9e84c9 Mon Sep 17 00:00:00 2001
From: Daniel Cheng <[email protected]>
Date: Mon, 15 Jun 2026 21:28:36 -0700
Subject: [PATCH] Fix reentrancy handling when committing data in DocumentLoader

Commit 254aa1df5bc94dd33b82b639f9c34306f432e645 fixed DocumentLoader to
handle reentrancy while committing data by queueing reentrantly-received
data, and committing that data when control returns to the top-leveli
call to `ProcessDataBody()` (originally named `dataReceived()`).

Commit 666b298327b61521dea4a7c63163b2228501e267 refactored Blink to use
range-based for loops when iterating through a `SharedBuffer`. However,
this is unsafe when dealing with reentrancy because:
- a range-based for loop is just syntactic shorthand; internally, the
  compiler transforms the loop to an equivalent version that uses
  iterators
- appending segments to a `SharedBuffer` logically invalidates the
  iterator, as the backing `Vector` may need to be resized

Fix this by moving ownership of the data buffers onto the stack when
draining the buffers; this avoids reentrant mutation while iterating
through the buffers. Also improve the tests to cover this missed case
as well: the original tests only triggered reentrant calls inside the
initial `CommitData()` call; the updated tests also trigger reentrancy
in the subsequent `CommitData()` calls inside the loop.

An alternate fix could take advantage of the fact that vector iterators
are random-access and index off the iterator. This would save some heap
allocations, since `SharedBuffer` is refcounted. However, appending to
a `Vector` is (intentionally) not amortized O(1) cost, so it's not
clearly better either.

Bug: 497451790
Change-Id: I7c8967bc3617ff13bbf221b031c0238c61ac19f9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7859663
Reviewed-by: Nate Chapin <[email protected]>
Commit-Queue: Daniel Cheng <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1647313}
---

diff --git a/third_party/blink/renderer/core/loader/document_loader.cc b/third_party/blink/renderer/core/loader/document_loader.cc
index 37eb120f..8e22a445 100644
--- a/third_party/blink/renderer/core/loader/document_loader.cc
+++ b/third_party/blink/renderer/core/loader/document_loader.cc
@@ -1936,20 +1936,26 @@
   if (data)
     CommitData(*data);
 
-  // Process data received in reentrant invocations. Note that the invocations
-  // of CommitData() may queue more data in reentrant invocations, so iterate
-  // until it's empty.
-  DCHECK(data_buffer_->empty() || decoded_data_buffer_.empty());
-  for (const auto& span : *data_buffer_) {
-    EncodedBodyData body_data(span);
-    CommitData(body_data);
+  // Process data received in reentrant invocations. Note that
+  // - invocations of `CommitData()` may queue more data in reentrant
+  //   invocations, so iterate until the buffers are completely consumed
+  // - for any given instance of `DocumentLoader`, only one of `data_buffer_`
+  //   or `decoded_data_buffer_` will ever be used.
+  while (!data_buffer_->empty()) {
+    scoped_refptr<SharedBuffer> data_buffer =
+        std::exchange(data_buffer_, SharedBuffer::Create());
+    for (const auto& span : *data_buffer) {
+      EncodedBodyData body_data(span);
+      CommitData(body_data);
+    }
   }
-  for (auto& decoded_data : decoded_data_buffer_)
-    CommitData(decoded_data);
-
-  // All data has been consumed, so flush the buffer.
-  data_buffer_->Clear();
-  decoded_data_buffer_.clear();
+  while (!decoded_data_buffer_.empty()) {
+    Vector<DecodedBodyData> decoded_data_buffer =
+        std::move(decoded_data_buffer_);
+    for (const auto& decoded_data : decoded_data_buffer) {
+      CommitData(const_cast<DecodedBodyData&>(decoded_data));
+    }
+  }
 }
 
 void DocumentLoader::StopLoading() {
diff --git a/third_party/blink/renderer/core/loader/document_loader.h b/third_party/blink/renderer/core/loader/document_loader.h
index 8d6a7f16..fce802d 100644
--- a/third_party/blink/renderer/core/loader/document_loader.h
+++ b/third_party/blink/renderer/core/loader/document_loader.h
@@ -501,6 +501,8 @@
 
   void ReportTotalTakenTimeToUpdateSubresourceLoadMetrics();
 
+  bool IsInCommitDataForTesting() const { return in_commit_data_; }
+
  protected:
   // Based on its MIME type, if the main document's response corresponds to an
   // MHTML archive, then every resources will be loaded from this archive.
diff --git a/third_party/blink/renderer/core/loader/document_loader_test.cc b/third_party/blink/renderer/core/loader/document_loader_test.cc
index 7573f10..133aa03 100644
--- a/third_party/blink/renderer/core/loader/document_loader_test.cc
+++ b/third_party/blink/renderer/core/loader/document_loader_test.cc
@@ -13,6 +13,7 @@
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/unguessable_token.h"
+#include "gin/public/gin_embedders.h"
 #include "net/base/features.h"
 #include "net/storage_access_api/status.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -29,6 +30,7 @@
 #include "third_party/blink/renderer/core/html/html_iframe_element.h"
 #include "third_party/blink/renderer/core/inspector/console_message.h"
 #include "third_party/blink/renderer/core/page/page.h"
+#include "third_party/blink/renderer/core/page/scoped_page_pauser.h"
 #include "third_party/blink/renderer/core/testing/scoped_fake_plugin_registry.h"
 #include "third_party/blink/renderer/core/testing/sim/sim_request.h"
 #include "third_party/blink/renderer/core/testing/sim/sim_test.h"
@@ -340,143 +342,6 @@
                     TestMode::kPartitionedStorageUnpartitionedLinks,
                     TestMode::kPartitionedStorageAndLinksWithSelfLinks));
 
-TEST_P(DocumentLoaderTest, SingleChunk) {
-  class TestDelegate : public URLLoaderTestDelegate {
-   public:
-    void DidReceiveData(URLLoaderClient* original_client,
-                        base::span<const char> data) override {
-      EXPECT_EQ(34u, data.size())
-          << "foo.html was not served in a single chunk";
-      original_client->DidReceiveDataForTesting(data);
-    }
-  } delegate;
-
-  ScopedLoaderDelegate loader_delegate(&delegate);
-  frame_test_helpers::LoadFrame(MainFrame(), "https://example.com/foo.html");
-
-  // TODO(dcheng): How should the test verify that the original callback is
-  // invoked? The test currently still passes even if the test delegate
-  // forgets to invoke the callback.
-}
-
-// Test normal case of DocumentLoader::dataReceived(): data in multiple chunks,
-// with no reentrancy.
-TEST_P(DocumentLoaderTest, MultiChunkNoReentrancy) {
-  class TestDelegate : public URLLoaderTestDelegate {
-   public:
-    void DidReceiveData(URLLoaderClient* original_client,
-                        base::span<const char> data) override {
-      EXPECT_EQ(34u, data.size())
-          << "foo.html was not served in a single chunk";
-      // Chunk the reply into one byte chunks.
-      for (; !data.empty(); data = data.subspan<1>()) {
-        original_client->DidReceiveDataForTesting(data.first<1>());
-      }
-    }
-  } delegate;
-
-  ScopedLoaderDelegate loader_delegate(&delegate);
-  frame_test_helpers::LoadFrame(MainFrame(), "https://example.com/foo.html");
-}
-
-// Finally, test reentrant callbacks to DocumentLoader::BodyDataReceived().
-TEST_P(DocumentLoaderTest, MultiChunkWithReentrancy) {
-  // This test delegate chunks the response stage into three distinct stages:
-  // 1. The first BodyDataReceived() callback, which triggers frame detach
-  //    due to committing a provisional load.
-  // 2. The middle part of the response, which is dispatched to
-  //    BodyDataReceived() reentrantly.
-  // 3. The final chunk, which is dispatched normally at the top-level.
-  class MainFrameClient : public URLLoaderTestDelegate,
-                          public frame_test_helpers::TestWebFrameClient {
-   public:
-    // URLLoaderTestDelegate overrides:
-    bool FillNavigationParamsResponse(WebNavigationParams* params) override {
-      params->response = WebURLResponse(params->url);
-      params->response.SetMimeType("application/x-webkit-test-webplugin");
-      params->response.SetHttpStatusCode(200);
-
-      String data("<html><body>foo</body></html>");
-      for (wtf_size_t i = 0; i < data.length(); i++)
-        data_.push_back(data[i]);
-
-      auto body_loader = std::make_unique<StaticDataNavigationBodyLoader>();
-      body_loader_ = body_loader.get();
-      params->body_loader = std::move(body_loader);
-      return true;
-    }
-
-    void Serve() {
-      {
-        // Serve the first byte to the real URLLoaderClient, which should
-        // trigger frameDetach() due to committing a provisional load.
-        base::AutoReset<bool> dispatching(&dispatching_did_receive_data_, true);
-        DispatchOneByte();
-      }
-
-      // Serve the remaining bytes to complete the load.
-      EXPECT_FALSE(data_.empty());
-      while (!data_.empty())
-        DispatchOneByte();
-

diff --git a/third_party/blink/renderer/core/loader/document_loader_test.cc b/third_party/blink/renderer/core/loader/document_loader_test.cc
index 7573f10..133aa03 100644
--- a/third_party/blink/renderer/core/loader/document_loader_test.cc
+++ b/third_party/blink/renderer/core/loader/document_loader_test.cc
@@ -13,6 +13,7 @@
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/unguessable_token.h"
+#include "gin/public/gin_embedders.h"
 #include "net/base/features.h"
 #include "net/storage_access_api/status.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -29,6 +30,7 @@
 #include "third_party/blink/renderer/core/html/html_iframe_element.h"
 #include "third_party/blink/renderer/core/inspector/console_message.h"
 #include "third_party/blink/renderer/core/page/page.h"
+#include "third_party/blink/renderer/core/page/scoped_page_pauser.h"
 #include "third_party/blink/renderer/core/testing/scoped_fake_plugin_registry.h"
 #include "third_party/blink/renderer/core/testing/sim/sim_request.h"
 #include "third_party/blink/renderer/core/testing/sim/sim_test.h"
@@ -340,143 +342,6 @@
                     TestMode::kPartitionedStorageUnpartitionedLinks,
                     TestMode::kPartitionedStorageAndLinksWithSelfLinks));
 
-TEST_P(DocumentLoaderTest, SingleChunk) {
-  class TestDelegate : public URLLoaderTestDelegate {
-   public:
-    void DidReceiveData(URLLoaderClient* original_client,
-                        base::span<const char> data) override {
-      EXPECT_EQ(34u, data.size())
-          << "foo.html was not served in a single chunk";
-      original_client->DidReceiveDataForTesting(data);
-    }
-  } delegate;
-
-  ScopedLoaderDelegate loader_delegate(&delegate);
-  frame_test_helpers::LoadFrame(MainFrame(), "https://example.com/foo.html");
-
-  // TODO(dcheng): How should the test verify that the original callback is
-  // invoked? The test currently still passes even if the test delegate
-  // forgets to invoke the callback.
-}
-
-// Test normal case of DocumentLoader::dataReceived(): data in multiple chunks,
-// with no reentrancy.
-TEST_P(DocumentLoaderTest, MultiChunkNoReentrancy) {
-  class TestDelegate : public URLLoaderTestDelegate {
-   public:
-    void DidReceiveData(URLLoaderClient* original_client,
-                        base::span<const char> data) override {
-      EXPECT_EQ(34u, data.size())
-          << "foo.html was not served in a single chunk";
-      // Chunk the reply into one byte chunks.
-      for (; !data.empty(); data = data.subspan<1>()) {
-        original_client->DidReceiveDataForTesting(data.first<1>());
-      }
-    }
-  } delegate;
-
-  ScopedLoaderDelegate loader_delegate(&delegate);
-  frame_test_helpers::LoadFrame(MainFrame(), "https://example.com/foo.html");
-}
-
-// Finally, test reentrant callbacks to DocumentLoader::BodyDataReceived().
-TEST_P(DocumentLoaderTest, MultiChunkWithReentrancy) {
-  // This test delegate chunks the response stage into three distinct stages:
-  // 1. The first BodyDataReceived() callback, which triggers frame detach
-  //    due to committing a provisional load.
-  // 2. The middle part of the response, which is dispatched to
-  //    BodyDataReceived() reentrantly.
-  // 3. The final chunk, which is dispatched normally at the top-level.
-  class MainFrameClient : public URLLoaderTestDelegate,
-                          public frame_test_helpers::TestWebFrameClient {
-   public:
-    // URLLoaderTestDelegate overrides:
-    bool FillNavigationParamsResponse(WebNavigationParams* params) override {
-      params->response = WebURLResponse(params->url);
-      params->response.SetMimeType("application/x-webkit-test-webplugin");
-      params->response.SetHttpStatusCode(200);
-
-      String data("<html><body>foo</body></html>");
-      for (wtf_size_t i = 0; i < data.length(); i++)
-        data_.push_back(data[i]);
-
-      auto body_loader = std::make_unique<StaticDataNavigationBodyLoader>();
-      body_loader_ = body_loader.get();
-      params->body_loader = std::move(body_loader);
-      return true;
-    }
-
-    void Serve() {
-      {
-        // Serve the first byte to the real URLLoaderClient, which should
-        // trigger frameDetach() due to committing a provisional load.
-        base::AutoReset<bool> dispatching(&dispatching_did_receive_data_, true);
-        DispatchOneByte();
-      }
-
-      // Serve the remaining bytes to complete the load.
-      EXPECT_FALSE(data_.empty());
-      while (!data_.empty())
-        DispatchOneByte();
-
-      body_loader_->Finish();
-      body_loader_ = nullptr;
-    }
-
-    // WebLocalFrameClient overrides:
-    void RunScriptsAtDocumentElementAvailable() override {
-      if (dispatching_did_receive_data_) {
-        // This should be called by the first BodyDataReceived() call, since
-        // it should create a plugin document structure and trigger this.
-        EXPECT_GT(data_.size(), 10u);
-        // Dispatch BodyDataReceived() callbacks for part of the remaining
-        // data, saving the rest to be dispatched at the top-level as
-        // normal.
-        while (data_.size() > 10)
-          DispatchOneByte();
-        served_reentrantly_ = true;
-      }
-      TestWebFrameClient::RunScriptsAtDocumentElementAvailable();
-    }
-
-    void DispatchOneByte() {
-      char c = data_.TakeFirst();
-      body_loader_->Write(base::span_from_ref(c));
-    }
-
-    bool ServedReentrantly() const { return served_reentrantly_; }
-
-   private:
-    Deque<char> data_;
-    bool dispatching_did_receive_data_ = false;
-    bool served_reentrantly_ = false;
-    StaticDataNavigationBodyLoader* body_loader_ = nullptr;
-  };
-
-  // We use a plugin document triggered by "application/x-webkit-test-webplugin"
-  // mime type, because that gives us reliable way to get a WebLocalFrameClient
-  // callback from inside BodyDataReceived() call.
-  ScopedFakePluginRegistry fake_plugins;
-  MainFrameClient main_frame_client;
-  web_view_helper_.Initialize(&main_frame_client);
-  web_view_helper_.GetWebView()->GetPage()->GetSettings().SetPluginsEnabled(
-      true);
-
-  {
-    ScopedLoaderDelegate loader_delegate(&main_frame_client);
-    frame_test_helpers::LoadFrameDontWait(
-        MainFrame(), url_test_helpers::ToKURL("https://example.com/foo.html"));
-    main_frame_client.Serve();
-    frame_test_helpers::PumpPendingRequestsForFrameToLoad(MainFrame());
-  }
-
-  // Sanity check that we did actually test reeentrancy.
-  EXPECT_TRUE(main_frame_client.ServedReentrantly());
-
-  // MainFrameClient is stack-allocated, so manually Reset to avoid UAF.
-  web_view_helper_.Reset();
-}
-
 TEST_P(DocumentLoaderTest, isCommittedButEmpty) {
   WebViewImpl* web_view_impl =
       web_view_helper_.InitializeAndLoad("about:blank");
@@ -486,7 +351,159 @@
                   ->IsCommittedButEmpty());
 }
 
-class DocumentLoaderSimTest : public SimTest {};
+class DocumentLoaderSimTest : public SimTest {
+ protected:
+  void InstallReenterHelper(WebLocalFrameImpl& frame) {
+    v8::Isolate* isolate = frame.GetAgentGroupScheduler()->Isolate();
+    v8::HandleScope handle_scope(isolate);
+    v8::Local<v8::Context> context = frame.MainWorldScriptContext();
+    v8::Context::Scope context_scope(context);
+    v8::MicrotasksScope microtasks_scope(
+        isolate, context->GetMicrotaskQueue(),
+        v8::MicrotasksScope::kDoNotRunMicrotasks);
+
+    v8::Local<v8::External> external_this = v8::External::New(
+        isolate, this, gin::kExternalPointerTypeTagDefaultTag);
+
+    context->Global()
+        ->Set(context,
+              v8::String::NewFromUtf8(isolate, "reenter").ToLocalChecked(),
+              v8::Function::New(context, &DocumentLoaderSimTest::ReenterThunk,
+                                external_this)
+                  .ToLocalChecked())
+        .ToChecked();
+  }
+
+  SimRequest* main_resource_for_reenter_ = nullptr;
+  int reenter_call_count_ = 0;
+
+ private:
+  static void ReenterThunk(const v8::FunctionCallbackInfo<v8::Value>& info) {
+    v8::Local<v8::External> external_that = info.Data().As<v8::External>();
+    DocumentLoaderSimTest* that = static_cast<DocumentLoaderSimTest*>(
+        external_that->Value(gin::kExternalPointerTypeTagDefaultTag));
+    that->Reenter();
+  }
+
+  void Reenter() {
+    ++reenter_call_count_;
+    LocalFrame* frame = GetDocument().GetFrame();
+    DocumentLoader* loader = frame->Loader().GetDocumentLoader();
+
+    EXPECT_TRUE(loader->IsInCommitDataForTesting());
+
+    // Operations like print preview or a devtools debugger breakpoint
+    // instantiate a `ScopedPagePauser` to prevent loading and other work from
+    // making forward progress inside a nested loop.
+    ScopedPagePauser pauser;
+
+    if (main_resource_for_reenter_) {
+      main_resource_for_reenter_->Write("<div id='reentered'></div>");
+
+      // The reentered chunk should be buffered, not processed yet.
+      EXPECT_FALSE(
+          frame->GetDocument()->getElementById(AtomicString("reentered")));
+    }
+
+    // If any writes to the main resource were queued above, destroying the
+    // ScopedPagePauser will undefer loading–which will immediately flush any
+    // pending received data to DocumentLoader while DocumentLoader is still
+    // in the `CommitData()` call.
+  }
+};
+
+// Standard case: each chunk arrives and is processed immediately in its own
+// top-level commit call.
+TEST_F(DocumentLoaderSimTest, ProcessDataBuffer_Streaming) {
+  SimRequest main_resource("https://example.com", "text/html");
+  LoadURL("https://example.com");
+
+  main_resource.Write("<html><body><div id='a'></div>");
+  EXPECT_TRUE(GetDocument().getElementById(AtomicString("a")));
+
+  main_resource.Write("<div id='b'></div>");
+  EXPECT_TRUE(GetDocument().getElementById(AtomicString("b")));
+
+  main_resource.Write("</body></html>");
+  main_resource.Finish();
+}
+
+// Test the case where multiple chunks arrive while the parser is blocked.
+// They should be accumulated and then processed in a single 'drain' iteration.
+TEST_F(DocumentLoaderSimTest, ProcessDataBuffer_Buffered) {
+  SimRequest main_resource("https://example.com", "text/html");
+  LoadURL("https://example.com");
+
+  // BlockParser() ensures chunks are accumulated in DocumentLoader's buffer.
+  GetDocument().Loader()->BlockParser();
+
+  main_resource.Write("<html><body><div id='a'></div>");
+  main_resource.Write("<div id='b'></div>");
+  main_resource.Finish();
+
+  // Chunks should be buffered, not processed yet.
+  EXPECT_FALSE(GetDocument().getElementById(AtomicString("a")));
+  EXPECT_FALSE(GetDocument().getElementById(AtomicString("b")));
+
+  GetDocument().Loader()->ResumeParser();
+
+  // All chunks should have been processed now.
+  EXPECT_TRUE(GetDocument().getElementById(AtomicString("a")));
+  EXPECT_TRUE(GetDocument().getElementById(AtomicString("b")));
+}
+
+// Test reentrancy into DocumentLoader during the initial `CommitData()` call.
+TEST_F(DocumentLoaderSimTest, ProcessDataBuffer_ReentrancyFromInitialCommit) {
+  SimRequest main_resource("https://example.com", "text/html");
+  base::AutoReset<SimRequest*> main_resource_reset(&main_resource_for_reenter_,
+                                                   &main_resource);
+  LoadURL("https://example.com");
+
+  InstallReenterHelper(MainFrame());
+
+  main_resource.Write("<html><body><script>reenter();</script>");
+  EXPECT_EQ(1, reenter_call_count_);
+
+  main_resource.Finish();
+
+  EXPECT_TRUE(GetDocument().getElementById(AtomicString("reentered")));
+}
+
+// Test reentrancy into DocumentLoader from a `CommitData()` call while draining
+// buffered data.
+TEST_F(DocumentLoaderSimTest, ProcessDataBuffer_ReentrancyDuringIteration) {
+  SimRequest main_resource("https://example.com", "text/html");
+  base::AutoReset<SimRequest*> main_resource_reset(&main_resource_for_reenter_,
+                                                   &main_resource);
+  LoadURL("https://example.com");
+
+  // `BlockParser()` ensures DocumentLoader buffers data, which is necessary to
+  // trigger reentrancy in a `CommitData()` call while draining buffered data:
+  // this is somewhat of an edge case, but can happen if an OOPIF local root
+  // hasn't received its size yet.
+  //
... (truncated)