From 0b4fb3cf2e4263c1778eea6f0bf803d7353e3b1a Mon Sep 17 00:00:00 2001
From: Andy Phan <[email protected]>
Date: Tue, 07 Jul 2026 14:01:37 -0700
Subject: [PATCH] [PDF] Defer page deletion and invalidate selections to prevent UAFs

During event handling (e.g., input events in XFA PDFs) or page-scanning
operations (e.g., `PDFiumEngine::LoadTextAnnotationsFromPdf()`),
JavaScript or document updates can modify the document structure and
delete pages. If these pages are destroyed immediately while still
referenced in the call stack, it can lead to UAF crashes.

To prevent this:
- Introduce `CreateScopedDeferredPageUnload()` returning a
  `base::ScopedClosureRunner` to manage the `defer_page_unload_` state
  and trigger `CleanUpDeferredPages()` deterministically on scope exit.
- Consolidate existing deferred page unload logic into the callback
  in `CreateScopedDeferredPageUnload()`.
- Defer the destruction of deleted `PDFiumPage` objects by storing them
  in `deferred_page_deletions_` if they have active unload preventers.
- Apply the scoped deferred page unload in
  `LoadTextAnnotationsFromPdf()` to prevent temporary memory bloat by
  cleaning up after each iteration.
- Invalidate some index-based state if the page count changes.

Bug: 513688690
Change-Id: Ib8a5c69fdbf6086e41e9ac0680e6987a208ccfda
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7869473
Reviewed-by: Lei Zhang <[email protected]>
Commit-Queue: Andy Phan <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1658273}
---
diff --git a/pdf/pdfium/pdfium_engine.cc b/pdf/pdfium/pdfium_engine.cc
index ee2cd319..fd008596 100644
--- a/pdf/pdfium/pdfium_engine.cc
+++ b/pdf/pdfium/pdfium_engine.cc
@@ -1482,8 +1482,7 @@
 }
 
 bool PDFiumEngine::HandleInputEvent(const blink::WebInputEvent& event) {
-  DCHECK(!defer_page_unload_);
-  defer_page_unload_ = true;
+  base::ScopedClosureRunner unload_preventer = CreateScopedDeferredPageUnload();
   bool rv = false;
   switch (event.GetType()) {
     case blink::WebInputEvent::Type::kMouseDown:
@@ -1532,18 +1531,6 @@
       break;
   }
 
-  DCHECK(defer_page_unload_);
-  defer_page_unload_ = false;
-
-  // Store the pages to unload away because the act of unloading pages can cause
-  // there to be more pages to unload. We leave those extra pages to be unloaded
-  // on the next go around.
-  std::vector<int> pages_to_unload;
-  std::swap(pages_to_unload, deferred_page_unloads_);
-  for (int page_index : pages_to_unload) {
-    pages_[page_index]->Unload();
-  }
-
   return rv;
 }
 
@@ -2623,6 +2610,20 @@
   return &find_results_[current_find_index_.value()];
 }
 
+void PDFiumEngine::ClearFindResults() {
+  FindResultChangeInvalidator find_change_invalidator(this);
+
+  find_results_.clear();
+  next_page_to_search_ = -1;
+  last_page_to_search_ = -1;
+  last_char_index_to_search_ = -1;
+  current_find_index_.reset();
+  current_find_text_.clear();
+
+  UpdateTickMarks();
+  find_weak_factory_.InvalidateWeakPtrs();
+}
+
 bool PDFiumEngine::SelectFindResult(bool forward) {
   if (find_results_.empty()) {
     return false;
@@ -2669,17 +2670,7 @@
 }
 
 void PDFiumEngine::StopFind() {
-  FindResultChangeInvalidator find_change_invalidator(this);
-
-  find_results_.clear();
-  next_page_to_search_ = -1;
-  last_page_to_search_ = -1;
-  last_char_index_to_search_ = -1;
-  current_find_index_.reset();
-  current_find_text_.clear();
-
-  UpdateTickMarks();
-  find_weak_factory_.InvalidateWeakPtrs();
+  ClearFindResults();
 }
 
 std::vector<gfx::Rect> PDFiumEngine::GetAllScreenRectsUnion(
@@ -3138,11 +3129,11 @@
     return *in_flight_visible_page_;
   }
 
-  // We can call GetMostVisiblePage through a callback from PDFium. We have
-  // to defer the page deletion otherwise we could potentially delete the page
-  // that originated the calling JS request and destroy the objects that are
+  // GetMostVisiblePage() can be called through a callback from PDFium. Defer
+  // the page deletion, otherwise the page that originated the calling JS
+  // request could potentially be deleted and destroy the objects that are
   // currently being used.
-  base::AutoReset<bool> defer_page_unload_guard(&defer_page_unload_, true);
+  base::ScopedClosureRunner unload_preventer = CreateScopedDeferredPageUnload();
   CalculateVisiblePages();
   return most_visible_page_;
 }
@@ -3467,16 +3458,81 @@
 
   // Remove pages that do not exist anymore.
   if (pages_.size() > new_page_count) {
+    const size_t deferred_count_before = deferred_page_deletions_.size();
     for (size_t i = new_page_count; i < pages_.size(); ++i) {
-      pages_[i]->Unload();
+      if (defer_page_unload_ || !pages_[i]->Unload()) {
+        deferred_page_deletions_.push_back(std::move(pages_[i]));
+      }
     }
 
     pages_.resize(new_page_count);
+
+    // Reset index-based state that is now out of bounds.
+    if (last_focused_page_ >= static_cast<int>(new_page_count)) {
+      last_focused_page_ = -1;
+    }
+    if (most_visible_page_ >= static_cast<int>(new_page_count)) {
+      most_visible_page_ = -1;
+    }
+    // Clear deferred unloads to prevent stale, out-of-bounds indices. This is
+    // guaranteed to be repopulated by the subsequent CalculateVisiblePages()
+    // call.
+    deferred_page_unloads_.clear();
+
+    // Clear selections and highlights that might now point to invalid page
+    // indices.
+    selection_.clear();
+    saved_selection_.clear();
+    RemoveTextFragments();
+    ClearFindResults();
+
+    if (deferred_page_deletions_.size() > deferred_count_before) {
+      // Post a task to clean up the deferred page deletions. This ensures that
+      // even if no `CreateScopedDeferredPageUnload()` scope is active to
+      // trigger synchronous cleanup, or if the page was deferred due to
+      // temporary stack-allocated preventers that outlive the active scope,
+      // destruction of the pages will be attempted once the stack unwinds.
+      base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(
+          FROM_HERE, base::BindOnce(&PDFiumEngine::CleanUpDeferredPages,
+                                    weak_factory_.GetWeakPtr()));
+    }
   }
 
   return page_sizes;
 }
 
+void PDFiumEngine::CleanUpDeferredPages() {
+  if (defer_page_unload_) {
+    return;
+  }
+
+  // Unload pages that were deferred from unloading.
+  std::vector<int> pages_to_unload;
+  std::swap(pages_to_unload, deferred_page_unloads_);
+  for (int page_index : pages_to_unload) {
+    if (page_index < static_cast<int>(pages_.size())) {
+      pages_[page_index]->Unload();
+    }
+  }
+
+  // Destroy pages that were deleted.
+  std::erase_if(deferred_page_deletions_,
+                [](const auto& page) { return page->Unload(); });
+}
+
+base::ScopedClosureRunner PDFiumEngine::CreateScopedDeferredPageUnload() {
+  bool prev_defer_page_unload = defer_page_unload_;
+  defer_page_unload_ = true;
+  return base::ScopedClosureRunner(base::BindOnce(
+      [](base::WeakPtr<PDFiumEngine> engine, bool prev_defer_page_unload) {
+        if (engine) {
+          engine->defer_page_unload_ = prev_defer_page_unload;
+          engine->CleanUpDeferredPages();
+        }
+      },
+      weak_factory_.GetWeakPtr(), prev_defer_page_unload));
+}
+
 void PDFiumEngine::LoadBody() {
   DCHECK(doc());
   if (doc_loader_->IsDocumentComplete()) {
@@ -5583,6 +5639,8 @@
 DocumentInkTextBoxesMap PDFiumEngine::LoadTextAnnotationsFromPdf() {
   DocumentInkTextBoxesMap document_textboxes;
   for (size_t i = 0; i < pages_.size(); ++i) {
+    base::ScopedClosureRunner unload_preventer =
+        CreateScopedDeferredPageUnload();
     PDFiumPage* page = pages_[i].get();
     std::vector<ReadInkTextResult> page_results =
         ReadInkTextAnnotationsFromPage(page->GetPage());
diff --git a/pdf/pdfium/pdfium_engine.h b/pdf/pdfium/pdfium_engine.h
index b9d342e0..bbad2d9 100644
--- a/pdf/pdfium/pdfium_engine.h
+++ b/pdf/pdfium/pdfium_engine.h
@@ -19,6 +19,7 @@
 #include "base/containers/flat_map.h"
 #include "base/containers/span.h"
 #include "base/functional/callback.h"
+#include "base/functional/callback_helpers.h"
 #include "base/memory/raw_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
@@ -769,6 +770,7 @@
 
   friend class FormFillerTest;
   friend class PDFiumDrawSelectionTestBase;
+  friend class PDFiumEnginePageMutationTest;
   friend class PDFiumEngineTabbingTest;
   friend class PDFiumEngineTest;
   friend class PDFiumFormFiller;
@@ -830,6 +832,15 @@
   std::vector<gfx::Size> LoadPageSizes(
       const DocumentLayout::Options& layout_options);
 
+  // Cleans up active pages that were deferred from unloading. This is a
+  // best-effort cleanup; pages with active unload preventers will remain
+  // deferred.
+  void CleanUpDeferredPages();
+
+  // Defers page unloading and triggers CleanUpDeferredPages() when the returned
+  // runner goes out of scope.
+  base::ScopedClosureRunner CreateScopedDeferredPageUnload();
+
   void LoadBody();
 
   void LoadPages();
@@ -890,6 +901,9 @@
   // no find selection.
   const PDFiumRange* GetFindSelection() const;
 
+  // Clears find results and resets the search state variables.
+  void ClearFindResults();
+
   // Search a page ourself using ICU.
   void SearchUsingICU(const std::u16string& term,
                       bool case_sensitive,
@@ -1249,12 +1263,20 @@
   // The indexes of the pages pending download.
   std::vector<uint32_t> pending_pages_;
 
-  // During handling of input events we don't want to unload any pages in
-  // callbacks to us from PDFium, since the current page can change while PDFium
-  // code still has a pointer to it.
+  // Set to true to prevent unloading of pages during operations where the stack
+  // may hold raw pointers to them (e.g. during input event handling or text
+  // annotation loading). Managed via `CreateScopedDeferredPageUnload()`.
   bool defer_page_unload_ = false;
+
+  // Page indices that are deferred from unloading.
   std::vector<int> deferred_page_unloads_;
 
+  // If `defer_page_unload_` is true, or if there is an active page unload
+  // preventer, pages deleted in `LoadPageSizes()` cannot be destroyed
+  // immediately. They are moved here to defer their destruction until the
+  // deferrals/preventers are cleared.
+  std::vector<std::unique_ptr<PDFiumPage>> deferred_page_deletions_;
+
   // Used for text selection, but does not include text within form text areas.
   // There could be more than one range if selection spans more than one page.
   std::vector<PDFiumRange> selection_;
diff --git a/pdf/pdfium/pdfium_engine_unittest.cc b/pdf/pdfium/pdfium_engine_unittest.cc
index 4fe9232..4dd228e 100644
--- a/pdf/pdfium/pdfium_engine_unittest.cc
+++ b/pdf/pdfium/pdfium_engine_unittest.cc
@@ -83,6 +83,7 @@
 #include "pdf/test/pdf_ink_test_helpers.h"
 #include "third_party/ink/src/ink/strokes/input/stroke_input_batch.h"
 #include "third_party/ink/src/ink/strokes/stroke.h"
+#include "third_party/pdfium/public/fpdf_edit.h"
 #endif
 
 namespace chrome_pdf {
@@ -1812,6 +1813,101 @@
 
 INSTANTIATE_TEST_SUITE_P(All, PDFiumEngineDeathTest, testing::Bool());
 
+class PDFiumEnginePageMutationTest : public PDFiumEngineTest {
+ protected:
+  FPDF_FORMHANDLE GetFormHandle(PDFiumEngine* engine) { return engine->form(); }
+
+  FPDF_DOCUMENT GetDoc(PDFiumEngine* engine) { return engine->doc(); }
+
+  void InvalidateAllPages(PDFiumEngine* engine) {
+    engine->InvalidateAllPages();
+  }
+
+  void SetLastFocusedPage(PDFiumEngine* engine, int page_index) {
+    engine->last_focused_page_ = page_index;
+  }
+};
+
+// Simulates an XFA page deletion when handling a char event.
+TEST_P(PDFiumEnginePageMutationTest, PageCountShrinkOnHandleInputEvent) {
+  NiceMock<MockTestClient> client(/*use_skia_renderer=*/GetParam());
+  std::unique_ptr<PDFiumEngine> engine = InitializeEngine(
+      &client, FILE_PATH_LITERAL("annotation_form_fields.pdf"));
+  ASSERT_TRUE(engine);
+  ASSERT_EQ(2, engine->GetNumberOfPages());
+  ASSERT_TRUE(GetFormHandle(engine.get()));
+
+  bool test_triggered = false;
+  bool in_on_char = false;
+
+  // Invalidate() is called during form changes.
+  EXPECT_CALL(client, Invalidate(_)).WillRepeatedly([&](const gfx::Rect& rect) {
+    if (in_on_char && !test_triggered) {
+      test_triggered = true;
+      FPDFPage_Delete(GetDoc(engine.get()), 1);
+      InvalidateAllPages(engine.get());
+    }
+  });
+
+  engine->PluginSizeUpdated({1024, 4096});
+
+  // Put focus on an annotation on page 2.
+  {
+    constexpr int kPageIndex = 1;
+    constexpr int kAnnotIndex = 0;
+    PDFiumPage& page = GetPDFiumPage(*engine, kPageIndex);
+    ScopedFPDFAnnotation annot(FPDFPage_GetAnnot(page.GetPage(), kAnnotIndex));
+    ASSERT_TRUE(annot);
+    engine->UpdateFocus(/*has_focus=*/true);
+    ASSERT_TRUE(FORM_SetFocusedAnnot(GetFormHandle(engine.get()), annot.get()));
+    SetLastFocusedPage(engine.get(), kPageIndex);
+  }
+
+  // Trigger OnChar() on page 2. This executes FORM_OnChar().
+  blink::WebKeyboardEvent char_event(
+      blink::WebInputEvent::Type::kChar, blink::WebInputEvent::kNoModifiers,
+      blink::WebInputEvent::GetStaticTimeStampForTests());
+  char_event.text[0] = 'a';
+
+  // HandleInputEvent() should complete without crashing.
+  in_on_char = true;
+  engine->HandleInputEvent(char_event);
+  in_on_char = false;
+
+  EXPECT_TRUE(test_triggered);
+}
+
+TEST_P(PDFiumEnginePageMutationTest, DeferPageDestructionWithPreventer) {
+  NiceMock<MockTestClient> client(/*use_skia_renderer=*/GetParam());
+  std::unique_ptr<PDFiumEngine> engine = InitializeEngine(
+      &client, FILE_PATH_LITERAL("annotation_form_fields.pdf"));
+  ASSERT_TRUE(engine);
+  ASSERT_EQ(2, engine->GetNumberOfPages());
+
+  {
+    // Get page 2.
+    constexpr int kPageIndex = 1;
+    PDFiumPage& page = GetPDFiumPage(*engine, kPageIndex);
+    PDFiumPage::ScopedPageUnloadPreventer preventer(&page);
+
+    // Delete page 2 in the document.
+    FPDFPage_Delete(GetDoc(engine.get()), kPageIndex);
+
+    // Trigger a layout update. Since the preventer is active, page 2's
+    // destruction must be deferred.
+    InvalidateAllPages(engine.get());
+
+    // Page 2 is removed from engine's active pages list.
+    EXPECT_EQ(1, engine->GetNumberOfPages());
+
+    // `preventer` is still holding a raw pointer to `page`. `page` should be
+    // kept alive, and its destruction should be deferred. This should complete
+    // without crashing.
+  }
+}
+
+INSTANTIATE_TEST_SUITE_P(All, PDFiumEnginePageMutationTest, testing::Bool());
+
 class PDFiumEngineTabbingTest : public PDFiumTestBase {
  public:
   PDFiumEngineTabbingTest() = default;

diff --git a/pdf/pdfium/pdfium_engine_unittest.cc b/pdf/pdfium/pdfium_engine_unittest.cc
index 4fe9232..4dd228e 100644
--- a/pdf/pdfium/pdfium_engine_unittest.cc
+++ b/pdf/pdfium/pdfium_engine_unittest.cc
@@ -83,6 +83,7 @@
 #include "pdf/test/pdf_ink_test_helpers.h"
 #include "third_party/ink/src/ink/strokes/input/stroke_input_batch.h"
 #include "third_party/ink/src/ink/strokes/stroke.h"
+#include "third_party/pdfium/public/fpdf_edit.h"
 #endif
 
 namespace chrome_pdf {
@@ -1812,6 +1813,101 @@
 
 INSTANTIATE_TEST_SUITE_P(All, PDFiumEngineDeathTest, testing::Bool());
 
+class PDFiumEnginePageMutationTest : public PDFiumEngineTest {
+ protected:
+  FPDF_FORMHANDLE GetFormHandle(PDFiumEngine* engine) { return engine->form(); }
+
+  FPDF_DOCUMENT GetDoc(PDFiumEngine* engine) { return engine->doc(); }
+
+  void InvalidateAllPages(PDFiumEngine* engine) {
+    engine->InvalidateAllPages();
+  }
+
+  void SetLastFocusedPage(PDFiumEngine* engine, int page_index) {
+    engine->last_focused_page_ = page_index;
+  }
+};
+
+// Simulates an XFA page deletion when handling a char event.
+TEST_P(PDFiumEnginePageMutationTest, PageCountShrinkOnHandleInputEvent) {
+  NiceMock<MockTestClient> client(/*use_skia_renderer=*/GetParam());
+  std::unique_ptr<PDFiumEngine> engine = InitializeEngine(
+      &client, FILE_PATH_LITERAL("annotation_form_fields.pdf"));
+  ASSERT_TRUE(engine);
+  ASSERT_EQ(2, engine->GetNumberOfPages());
+  ASSERT_TRUE(GetFormHandle(engine.get()));
+
+  bool test_triggered = false;
+  bool in_on_char = false;
+
+  // Invalidate() is called during form changes.
+  EXPECT_CALL(client, Invalidate(_)).WillRepeatedly([&](const gfx::Rect& rect) {
+    if (in_on_char && !test_triggered) {
+      test_triggered = true;
+      FPDFPage_Delete(GetDoc(engine.get()), 1);
+      InvalidateAllPages(engine.get());
+    }
+  });
+
+  engine->PluginSizeUpdated({1024, 4096});
+
+  // Put focus on an annotation on page 2.
+  {
+    constexpr int kPageIndex = 1;
+    constexpr int kAnnotIndex = 0;
+    PDFiumPage& page = GetPDFiumPage(*engine, kPageIndex);
+    ScopedFPDFAnnotation annot(FPDFPage_GetAnnot(page.GetPage(), kAnnotIndex));
+    ASSERT_TRUE(annot);
+    engine->UpdateFocus(/*has_focus=*/true);
+    ASSERT_TRUE(FORM_SetFocusedAnnot(GetFormHandle(engine.get()), annot.get()));
+    SetLastFocusedPage(engine.get(), kPageIndex);
+  }
+
+  // Trigger OnChar() on page 2. This executes FORM_OnChar().
+  blink::WebKeyboardEvent char_event(
+      blink::WebInputEvent::Type::kChar, blink::WebInputEvent::kNoModifiers,
+      blink::WebInputEvent::GetStaticTimeStampForTests());
+  char_event.text[0] = 'a';
+
+  // HandleInputEvent() should complete without crashing.
+  in_on_char = true;
+  engine->HandleInputEvent(char_event);
+  in_on_char = false;
+
+  EXPECT_TRUE(test_triggered);
+}
+
+TEST_P(PDFiumEnginePageMutationTest, DeferPageDestructionWithPreventer) {
+  NiceMock<MockTestClient> client(/*use_skia_renderer=*/GetParam());
+  std::unique_ptr<PDFiumEngine> engine = InitializeEngine(
+      &client, FILE_PATH_LITERAL("annotation_form_fields.pdf"));
+  ASSERT_TRUE(engine);
+  ASSERT_EQ(2, engine->GetNumberOfPages());
+
+  {
+    // Get page 2.
+    constexpr int kPageIndex = 1;
+    PDFiumPage& page = GetPDFiumPage(*engine, kPageIndex);
+    PDFiumPage::ScopedPageUnloadPreventer preventer(&page);
+
+    // Delete page 2 in the document.
+    FPDFPage_Delete(GetDoc(engine.get()), kPageIndex);
+
+    // Trigger a layout update. Since the preventer is active, page 2's
+    // destruction must be deferred.
+    InvalidateAllPages(engine.get());
+
+    // Page 2 is removed from engine's active pages list.
+    EXPECT_EQ(1, engine->GetNumberOfPages());
+
+    // `preventer` is still holding a raw pointer to `page`. `page` should be
+    // kept alive, and its destruction should be deferred. This should complete
+    // without crashing.
+  }
+}
+
+INSTANTIATE_TEST_SUITE_P(All, PDFiumEnginePageMutationTest, testing::Bool());
+
 class PDFiumEngineTabbingTest : public PDFiumTestBase {
  public:
   PDFiumEngineTabbingTest() = default;