[PDF Ink Signatures] Clean up page unload preventers

Currently, entries in `PDFiumEngine::stroked_pages_unload_preventers_`
never get removed. This is because `ink_stroke_objects_map_` entries
originally never got discarded, as stated in the pdfium_engine.h
comments. With https://crrev.com/1382585, this assumption is no longer
true. Fix this discrepancy by:

1) Change `ink_stroke_objects_map_` to store the page index in addition
   to the page objects in the map's key. As a result, the key is now a
   new struct InkStrokeData, and `ink_stroke_objects_map_` gets renamed
   to `ink_stroke_data_`. Update the comments to reflect these changes.
2) Change PDFiumEngine::DiscardStroke() to check if a given page still
   has strokes after discards. If not, remove the associated page unload
   preventer.

The change in (1) will be needed in the near future to fix
https://crbug.com/395766076 as well. Since undo discards and thumbnail
redrawing are relatively infrequent operations, there is no need to make
`ink_stroke_data_` a map of maps.

Bug: 378427083, 395766076
Change-Id: I05dad30b3413eeb7569d90b23aca9bc0218460dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6270681
Reviewed-by: Alan Screen <awscreen@chromium.org>
Reviewed-by: Andy Phan <andyphan@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1420813}

pdf/pdfium/pdfium_engine.cc

@@ -602,7 +602,7 @@ PDFiumEngine::~PDFiumEngine() {
   find_results_.clear();
   selection_.clear();
 #if BUILDFLAG(ENABLE_PDF_INK2)
-  ink_stroke_objects_map_.clear();
+  ink_stroke_data_.clear();
   stroked_pages_unload_preventers_.clear();
 #endif
 
@@ -4531,12 +4531,14 @@ void PDFiumEngine::ApplyStroke(int page_index,
   ink_stroked_pages_needing_regeneration_.insert(page_index);
 
   bool inserted =
-      ink_stroke_objects_map_.insert({id, std::move(page_objects)}).second;
+      ink_stroke_data_
+          .insert({id, InkStrokeData(page_index, std::move(page_objects))})
+          .second;
   CHECK(inserted);  // Stroke IDs should be unique when added.
 
-  // Since there is now a page reference in `ink_stroke_objects_map_`, ensure
+  // Since there are now page references in `ink_stroke_data_`, ensure that this
-  // that this page has a ScopedUnloadPreventer so that the reference doesn't
+  // page has a ScopedUnloadPreventer so that the references do not become stale
-  // becomes stale if PDFiumPage::Unload() gets called.
+  // if PDFiumPage::Unload() gets called.
   if (!stroked_pages_unload_preventers_.contains(page_index)) {
     stroked_pages_unload_preventers_.insert(
         {page_index, PDFiumPage::ScopedUnloadPreventer(pdfium_page)});
@@ -4547,9 +4549,9 @@ void PDFiumEngine::UpdateStrokeActive(int page_index,
                                       InkStrokeId id,
                                       bool active) {
   CHECK(PageIndexInBounds(page_index));
-  auto it = ink_stroke_objects_map_.find(id);
+  auto it = ink_stroke_data_.find(id);
-  CHECK(it != ink_stroke_objects_map_.end());
+  CHECK(it != ink_stroke_data_.end());
-  for (FPDF_PAGEOBJECT page_object : it->second) {
+  for (FPDF_PAGEOBJECT page_object : it->second.page_objects) {
     bool result = FPDFPageObj_SetIsActive(page_object, active);
     CHECK(result);
   }
@@ -4558,18 +4560,26 @@ void PDFiumEngine::UpdateStrokeActive(int page_index,
 
 void PDFiumEngine::DiscardStroke(int page_index, InkStrokeId id) {
   CHECK(PageIndexInBounds(page_index));
-  auto it = ink_stroke_objects_map_.find(id);
+  auto it = ink_stroke_data_.find(id);
-  CHECK(it != ink_stroke_objects_map_.end());
+  CHECK(it != ink_stroke_data_.end());
-  for (FPDF_PAGEOBJECT page_object : it->second) {
+  for (FPDF_PAGEOBJECT page_object : it->second.page_objects) {
     bool result =
         FPDFPage_RemoveObject(pages_[page_index]->GetPage(), page_object);
     CHECK(result);
 
-    // After FPDFPage_RemoveObject(), ownership of `page_object` is transferred
+    // FPDFPage_RemoveObject() transferred ownership of `page_object` to the
-    // from the page to `this`, so free it.
+    // caller. Free it since `page_object` is being discarded.
     FPDFPageObj_Destroy(page_object);
   }
-  ink_stroke_objects_map_.erase(it);
+  ink_stroke_data_.erase(it);
+
+  bool page_still_has_strokes =
+      std::ranges::any_of(ink_stroke_data_, [page_index](const auto& it) {
+        return it.second.page_index == page_index;
+      });
+  if (!page_still_has_strokes) {
+    stroked_pages_unload_preventers_.erase(page_index);
+  }
 }
 
 PDFLoadedWithV2InkAnnotations PDFiumEngine::ContainsV2InkPath(
@@ -4635,6 +4645,18 @@ void PDFiumEngine::RegenerateContents() {
   }
   ink_stroked_pages_needing_regeneration_.clear();
 }
+
+PDFiumEngine::InkStrokeData::InkStrokeData(
+    int page_index,
+    std::vector<FPDF_PAGEOBJECT> page_objects)
+    : page_index(page_index), page_objects(std::move(page_objects)) {}
+
+PDFiumEngine::InkStrokeData::InkStrokeData(InkStrokeData&&) noexcept = default;
+
+PDFiumEngine::InkStrokeData& PDFiumEngine::InkStrokeData::operator=(
+    InkStrokeData&&) noexcept = default;
+
+PDFiumEngine::InkStrokeData::~InkStrokeData() = default;
 #endif  // BUILDFLAG(ENABLE_PDF_INK2)
 
 PDFiumEngine::ProgressivePaint::ProgressivePaint(int index,

pdf/pdfium/pdfium_engine.h

@@ -435,13 +435,18 @@ class PDFiumEngine : public DocumentLoader::Client, public IFSDK_PAUSE {
                                  InkModeledShapeId id,
                                  bool active);
 
+  // Regenerate contents for all pages that need it due to Ink strokes.
+  void RegenerateContents();
+
   const std::map<InkModeledShapeId, FPDF_PAGEOBJECT>&
   ink_modeled_shape_map_for_testing() const {
     return ink_modeled_shape_map_;
   }
 
-  // Regenerate contents for all pages that need it due to Ink strokes.
+  const std::map<int, PDFiumPage::ScopedUnloadPreventer>&
-  void RegenerateContents();
+  stroked_pages_unload_preventers_for_testing() const {
+    return stroked_pages_unload_preventers_;
+  }
 #endif  // BUILDFLAG(ENABLE_PDF_INK2)
 
   // DocumentLoader::Client:
@@ -1235,19 +1240,28 @@ class PDFiumEngine : public DocumentLoader::Client, public IFSDK_PAUSE {
 
 #if BUILDFLAG(ENABLE_PDF_INK2)
   // Map of zero-based page indices with Ink strokes to page unload preventers.
-  // Pages with Ink strokes have page references in `ink_stroke_objects_map_`,
+  // Pages with Ink strokes have page references in `ink_stroke_data_`, so these
-  // so these unload preventers ensure those page pointers stay valid by
+  // unload preventers ensure those page handles stay valid by keeping the page
-  // keeping the pages in memory.  Entries don't get deleted from
+  // in memory.  Use one unload preventer per page for simplicity.
-  // `ink_stroke_objects_map_`, so just need one preventer per page instead of
-  // per stroke.
   std::map<int, PDFiumPage::ScopedUnloadPreventer>
       stroked_pages_unload_preventers_;
 
-  // The handles for stroke path page objects within the PDF document, mapped
+  struct InkStrokeData {
-  // using the `InkStrokeId` provided during stroke creation.  The handles are
+    InkStrokeData(int page_index, std::vector<FPDF_PAGEOBJECT> page_objects);
-  // protected against becoming stale from page unloads by
+    InkStrokeData(InkStrokeData&&) noexcept;
-  // `stroked_pages_unload_preventers_`.
+    InkStrokeData& operator=(InkStrokeData&&) noexcept;
-  std::map<InkStrokeId, std::vector<FPDF_PAGEOBJECT>> ink_stroke_objects_map_;
+    ~InkStrokeData();
+
+    int page_index;
+
+    // The handles for stroke path page objects within the PDF document.
+    // `stroked_pages_unload_preventers_` protects these handles from going
+    // stale.
+    std::vector<FPDF_PAGEOBJECT> page_objects;
+  };
+
+  // Data associated for Ink strokes, keyed by stroke IDs.
+  std::map<InkStrokeId, InkStrokeData> ink_stroke_data_;
 
   // Tracks the pages which need to be regenerated before saving due to Ink
   // stroke changes.

pdf/pdfium/pdfium_engine_unittest.cc

@@ -2229,6 +2229,8 @@ TEST_P(PDFiumEngineInkDrawTest, StrokeData) {
   const base::FilePath kAppliedStroke2FilePath(
       GetInkTestDataFilePath("applied_stroke2.png"));
   CheckPdfRendering(page.GetPage(), kPageSizeInPoints, kAppliedStroke2FilePath);
+  EXPECT_TRUE(engine->stroked_pages_unload_preventers_for_testing().contains(
+      kPageIndex));
 
   // Getting the save data should now have the new strokes.
   // Verify visibility of strokes in that copy.  Must call GetSaveData()
@@ -2256,6 +2258,8 @@ TEST_P(PDFiumEngineInkDrawTest, StrokeData) {
   EXPECT_EQ(GetPdfMarkObjCountForTesting(engine->doc(),
                                          kInkAnnotationIdentifierKeyV2),
             1);
+  EXPECT_TRUE(engine->stroked_pages_unload_preventers_for_testing().contains(
+      kPageIndex));
 
   // Set the highlighter stroke as active again, to perform the equivalent of an
   // "redo" action. The affected stroke should be included in the saved PDF data
@@ -2269,6 +2273,8 @@ TEST_P(PDFiumEngineInkDrawTest, StrokeData) {
   EXPECT_EQ(GetPdfMarkObjCountForTesting(engine->doc(),
                                          kInkAnnotationIdentifierKeyV2),
             2);
+  EXPECT_TRUE(engine->stroked_pages_unload_preventers_for_testing().contains(
+      kPageIndex));
 }
 
 TEST_P(PDFiumEngineInkDrawTest, StrokeDiscardStroke) {
@@ -2311,6 +2317,8 @@ TEST_P(PDFiumEngineInkDrawTest, StrokeDiscardStroke) {
   const base::FilePath kAppliedStroke1FilePath(
       GetInkTestDataFilePath("applied_stroke1.png"));
   CheckPdfRendering(page.GetPage(), kPageSizeInPoints, kAppliedStroke1FilePath);
+  EXPECT_TRUE(engine->stroked_pages_unload_preventers_for_testing().contains(
+      kPageIndex));
 
   // Set the stroke as inactive, to perform the equivalent of an "undo" action.
   engine->UpdateStrokeActive(kPageIndex, kStrokeId, /*active=*/false);
@@ -2324,11 +2332,15 @@ TEST_P(PDFiumEngineInkDrawTest, StrokeDiscardStroke) {
                                          kInkAnnotationIdentifierKeyV2),
             0);
   EXPECT_EQ(FPDFPage_CountObjects(page.GetPage()), 1);
+  EXPECT_TRUE(engine->stroked_pages_unload_preventers_for_testing().contains(
+      kPageIndex));
 
   // Discard the stroke.
   engine->DiscardStroke(kPageIndex, kStrokeId);
 
   EXPECT_EQ(FPDFPage_CountObjects(page.GetPage()), 0);
+  EXPECT_FALSE(engine->stroked_pages_unload_preventers_for_testing().contains(
+      kPageIndex));
 
   // Draw a new stroke, reusing the same InkStrokeId. This can occur after an
   // undo action.
@@ -2346,6 +2358,8 @@ TEST_P(PDFiumEngineInkDrawTest, StrokeDiscardStroke) {
       GetInkTestDataFilePath("applied_stroke3.png"));
   CheckPdfRendering(page.GetPage(), kPageSizeInPoints, kAppliedStroke3FilePath);
   EXPECT_EQ(FPDFPage_CountObjects(page.GetPage()), 1);
+  EXPECT_TRUE(engine->stroked_pages_unload_preventers_for_testing().contains(
+      kPageIndex));
 }
 
 TEST_P(PDFiumEngineInkDrawTest, LoadedV2InkPathsAndUpdateShapeActive) {