[PDF Ink Signatures] Fix PDF load slowdown from V2 Ink load metric

The existing implementation for recording PDF loads with V2 Ink
annotations is causing a slowdown and large memory usage for large PDFs
with many PDF objects. It loops through every page in the PDF and every
PDF object, stopping only if encountering a V2 Ink annotation object.

There is no other way to check if a PDF has a V2 Ink annotation object,
but the metric is still useful to have. Add a timeout for the PDF
scanning and set it to a value that will capture over 90% of PDFs. The
timeout limit must be kept low, as all PDF loads can be affected by this
limit.

Avoid excessive memory usage by unloading the PDF page immediately after
loading and scanning for V2 Ink annotation objects.

The original metric must be deprecated, since the new metric now uses an
enum to indicate when the PDF scan times out.

Fixed: 393557667
Change-Id: I30a5fa6460ac3e196f44e26702cf64972e5a5929
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6249669
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Andy Phan <andyphan@chromium.org>
Reviewed-by: Alan Screen <awscreen@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1418463}

pdf/pdf_ink_metrics_handler.cc

@@ -4,6 +4,8 @@
 
 #include "pdf/pdf_ink_metrics_handler.h"
 
+#include <optional>
+
 #include "base/containers/fixed_flat_map.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/notreached.h"
@@ -158,8 +160,10 @@ void ReportEraseStroke(float size, ink::StrokeInput::ToolType tool_type) {
   ReportStrokeInputDeviceType(tool_type);
 }
 
-void RecordPdfLoadedWithV2InkAnnotations(bool has_annotations) {
-  base::UmaHistogramBoolean("PDF.LoadedWithV2InkAnnotations", has_annotations);
+void RecordPdfLoadedWithV2InkAnnotations(
+    PDFLoadedWithV2InkAnnotations loaded_with_annotations) {
+  base::UmaHistogramEnumeration("PDF.LoadedWithV2InkAnnotations2",
+                                loaded_with_annotations);
 }
 
 }  // namespace chrome_pdf

pdf/pdf_ink_metrics_handler.h

@@ -99,13 +99,26 @@ enum class StrokeMetricPenColor {
 };
 // LINT.ThenChange(//tools/metrics/histograms/metadata/pdf/enums.xml:PDFInk2StrokePenColor)
 
+// These values are persisted to logs. Entries should not be renumbered and
+// numeric values should never be reused.
+//
+// LINT.IfChange(PDFLoadedWithV2InkAnnotations)
+enum class PDFLoadedWithV2InkAnnotations {
+  kUnknown = 0,
+  kTrue = 1,
+  kFalse = 2,
+  kMaxValue = kFalse,
+};
+// LINT.ThenChange(//tools/metrics/histograms/metadata/pdf/enums.xml:PDFLoadedWithV2InkAnnotations)
+
 void ReportDrawStroke(PdfInkBrush::Type type,
                       const ink::Brush& brush,
                       ink::StrokeInput::ToolType tool_type);
 
 void ReportEraseStroke(float size, ink::StrokeInput::ToolType tool_type);
 
-void RecordPdfLoadedWithV2InkAnnotations(bool has_annotations);
+void RecordPdfLoadedWithV2InkAnnotations(
+    PDFLoadedWithV2InkAnnotations loaded_with_annotations);
 
 }  // namespace chrome_pdf
 

pdf/pdf_view_web_plugin.cc

@@ -2467,7 +2467,10 @@ void PdfViewWebPlugin::RecordDocumentMetrics() {
   // `metrics_handler_` is only initialized when not in Print Preview, so the
   // V2 ink annotations load metric will not count Print Preview loads.
   if (ink_module_) {
-    RecordPdfLoadedWithV2InkAnnotations(engine_->ContainsV2InkPath());
+    // Use a timeout limit of 100ms, which will capture over 90 percent of PDFs
+    // without increasing the PDF load time a significant amount.
+    RecordPdfLoadedWithV2InkAnnotations(
+        engine_->ContainsV2InkPath(base::Milliseconds(100)));
   }
 #endif  // BUILDFLAG(ENABLE_PDF_INK2)
 }

pdf/pdf_view_web_plugin_unittest.cc

@@ -89,6 +89,7 @@
 
 #if BUILDFLAG(ENABLE_PDF_INK2)
 #include "pdf/pdf_ink_brush.h"
+#include "pdf/pdf_ink_metrics_handler.h"
 #include "pdf/pdf_ink_module_client.h"
 #include "pdf/test/pdf_ink_test_helpers.h"
 #include "pdf/test/test_helpers.h"
@@ -118,7 +119,7 @@ using ::testing::SizeIs;
 
 #if BUILDFLAG(ENABLE_PDF_INK2)
 constexpr char kPdfLoadedWithV2InkAnnotationsMetric[] =
-    "PDF.LoadedWithV2InkAnnotations";
+    "PDF.LoadedWithV2InkAnnotations2";
 #endif  // BUILDFLAG(ENABLE_PDF_INK2)
 
 // `kCanvasSize` needs to be big enough to hold plugin's snapshots during
@@ -3039,19 +3040,33 @@ using PdfViewWebPluginInkMetricTest = PdfViewWebPluginInkTest;
 TEST_F(PdfViewWebPluginInkMetricTest, LoadedWithoutV2InkAnnotations) {
   base::HistogramTester histograms;
 
-  EXPECT_CALL(*engine_ptr_, ContainsV2InkPath()).WillOnce(Return(false));
+  EXPECT_CALL(*engine_ptr_, ContainsV2InkPath(_))
+      .WillOnce(Return(PDFLoadedWithV2InkAnnotations::kFalse));
   plugin_->DocumentLoadComplete();
 
-  histograms.ExpectUniqueSample(kPdfLoadedWithV2InkAnnotationsMetric, false, 1);
+  histograms.ExpectUniqueSample(kPdfLoadedWithV2InkAnnotationsMetric,
+                                PDFLoadedWithV2InkAnnotations::kFalse, 1);
 }
 
 TEST_F(PdfViewWebPluginInkMetricTest, LoadedWithV2InkAnnotations) {
   base::HistogramTester histograms;
 
-  EXPECT_CALL(*engine_ptr_, ContainsV2InkPath()).WillOnce(Return(true));
+  EXPECT_CALL(*engine_ptr_, ContainsV2InkPath(_))
+      .WillOnce(Return(PDFLoadedWithV2InkAnnotations::kTrue));
   plugin_->DocumentLoadComplete();
 
-  histograms.ExpectUniqueSample(kPdfLoadedWithV2InkAnnotationsMetric, true, 1);
+  histograms.ExpectUniqueSample(kPdfLoadedWithV2InkAnnotationsMetric,
+                                PDFLoadedWithV2InkAnnotations::kTrue, 1);
+}
+
+TEST_F(PdfViewWebPluginInkMetricTest, LoadedWithV2InkAnnotationsTimeout) {
+  base::HistogramTester histograms;
+  EXPECT_CALL(*engine_ptr_, ContainsV2InkPath(_))
+      .WillOnce(Return(PDFLoadedWithV2InkAnnotations::kUnknown));
+  plugin_->DocumentLoadComplete();
+
+  histograms.ExpectUniqueSample(kPdfLoadedWithV2InkAnnotationsMetric,
+                                PDFLoadedWithV2InkAnnotations::kUnknown, 1);
 }
 
 class PdfViewWebPluginPrintPreviewInkMetricTest
@@ -3071,7 +3086,7 @@ TEST_F(PdfViewWebPluginPrintPreviewInkMetricTest,
     "pageCount": 1,
   })"));
 
-  EXPECT_CALL(*engine_ptr_, ContainsV2InkPath()).Times(0);
+  EXPECT_CALL(*engine_ptr_, ContainsV2InkPath(_)).Times(0);
   plugin_->DocumentLoadComplete();
 
   // The V2 ink annotations PDF load metric should not increment for Print

pdf/pdfium/pdfium_engine.cc

@@ -98,6 +98,7 @@
 #endif
 
 #if BUILDFLAG(ENABLE_PDF_INK2)
+#include "pdf/pdf_ink_metrics_handler.h"
 #include "pdf/pdfium/pdfium_ink_reader.h"
 #include "pdf/pdfium/pdfium_ink_writer.h"
 #include "third_party/ink/src/ink/strokes/stroke.h"
@@ -4565,13 +4566,25 @@ void PDFiumEngine::DiscardStroke(int page_index, InkStrokeId id) {
   ink_stroke_objects_map_.erase(it);
 }
 
-bool PDFiumEngine::ContainsV2InkPath() const {
+PDFLoadedWithV2InkAnnotations PDFiumEngine::ContainsV2InkPath(
+    const base::TimeDelta& timeout) const {
+  base::TimeTicks start_time = base::TimeTicks::Now();
   for (const auto& page : pages_) {
-    if (PageContainsV2InkPath(page->GetPage())) {
-      return true;
+    if (base::TimeTicks::Now() - start_time >= timeout) {
+      return PDFLoadedWithV2InkAnnotations::kUnknown;
+    }
+
+    bool page_already_loaded = !!page->page();
+    bool contains_v2_ink_path = PageContainsV2InkPath(page->GetPage());
+    if (!page_already_loaded) {
+      // Unload the page to avoid loading all pages into memory.
+      page->Unload();
+    }
+    if (contains_v2_ink_path) {
+      return PDFLoadedWithV2InkAnnotations::kTrue;
     }
   }
-  return false;
+  return PDFLoadedWithV2InkAnnotations::kFalse;
 }
 
 std::map<InkModeledShapeId, ink::PartitionedMesh>

pdf/pdfium/pdfium_engine.h

@@ -63,6 +63,7 @@
 
 #if BUILDFLAG(ENABLE_PDF_INK2)
 #include "pdf/pdf_ink_ids.h"
+#include "pdf/pdf_ink_metrics_handler.h"
 #include "third_party/ink/src/ink/geometry/partitioned_mesh.h"
 #endif
 
@@ -80,6 +81,10 @@ struct WebPrintParams;
 }  // namespace blink
 
 #if BUILDFLAG(ENABLE_PDF_INK2)
+namespace base {
+class TimeDelta;
+}  // namespace base
+
 namespace ink {
 class Stroke;
 }  // namespace ink
@@ -395,9 +400,11 @@ class PDFiumEngine : public DocumentLoader::Client, public IFSDK_PAUSE {
   // `ApplyStroke()`. Virtual to support testing.
   virtual void DiscardStroke(int page_index, InkStrokeId id);
 
-  // Returns whether any of the pages contains a "V2" path created by Ink.
-  // Virtual to support testing.
-  virtual bool ContainsV2InkPath() const;
+  // Returns whether any of the pages contains a "V2" path created by Ink or
+  // unknown if unable to find any "V2" paths within `timeout`. Virtual to
+  // support testing.
+  virtual PDFLoadedWithV2InkAnnotations ContainsV2InkPath(
+      const base::TimeDelta& timeout) const;
 
   // Loads "V2" Ink paths from a page in the PDF identified by `page_index`. The
   // `page_index` must be in bounds.

pdf/pdfium/pdfium_engine_unittest.cc

@@ -59,6 +59,7 @@
 
 #include "pdf/pdf_ink_brush.h"
 #include "pdf/pdf_ink_constants.h"
+#include "pdf/pdf_ink_metrics_handler.h"
 #include "pdf/pdfium/pdfium_test_helpers.h"
 #include "pdf/test/pdf_ink_test_helpers.h"
 #include "third_party/ink/src/ink/strokes/input/stroke_input_batch.h"
@@ -2116,12 +2117,23 @@ TEST_P(PDFiumEngineInkTest, ContainsV2InkPath) {
       InitializeEngine(&client, FILE_PATH_LITERAL("blank.pdf"));
   ASSERT_TRUE(engine);
   ASSERT_EQ(1, engine->GetNumberOfPages());
-  EXPECT_FALSE(engine->ContainsV2InkPath());
+  constexpr base::TimeDelta kContainsV2InkPathTimeout =
+      base::Milliseconds(5000);
+  EXPECT_EQ(engine->ContainsV2InkPath(kContainsV2InkPathTimeout),
+            PDFLoadedWithV2InkAnnotations::kFalse);
 
   engine = InitializeEngine(&client, FILE_PATH_LITERAL("ink_v2.pdf"));
   ASSERT_TRUE(engine);
   ASSERT_EQ(1, engine->GetNumberOfPages());
-  EXPECT_TRUE(engine->ContainsV2InkPath());
+  EXPECT_EQ(engine->ContainsV2InkPath(kContainsV2InkPathTimeout),
+            PDFLoadedWithV2InkAnnotations::kTrue);
+
+  // Test timeout.
+  engine = InitializeEngine(&client, FILE_PATH_LITERAL("ink_v2.pdf"));
+  ASSERT_TRUE(engine);
+  ASSERT_EQ(1, engine->GetNumberOfPages());
+  EXPECT_EQ(engine->ContainsV2InkPath(base::Milliseconds(0)),
+            PDFLoadedWithV2InkAnnotations::kUnknown);
 }
 
 TEST_P(PDFiumEngineInkTest, LoadV2InkPathsForPage) {

pdf/test/test_pdfium_engine.h

@@ -109,7 +109,10 @@ class TestPDFiumEngine : public PDFiumEngine {
 
   MOCK_METHOD(void, DiscardStroke, (int, InkStrokeId), (override));
 
-  MOCK_METHOD(bool, ContainsV2InkPath, (), (const override));
+  MOCK_METHOD(PDFLoadedWithV2InkAnnotations,
+              ContainsV2InkPath,
+              (const base::TimeDelta&),
+              (const override));
 
   MOCK_METHOD((std::map<InkModeledShapeId, ink::PartitionedMesh>),
               LoadV2InkPathsForPage,

tools/metrics/histograms/metadata/pdf/enums.xml

@@ -222,6 +222,16 @@ chromium-metrics-reviews@google.com.
 
 <!-- LINT.ThenChange(//pdf/pdf_ink_metrics_handler.h:PDFInk2StrokePenColor) -->
 
+<!-- LINT.IfChange(PDFLoadedWithV2InkAnnotations) -->
+
+<enum name="PDFLoadedWithV2InkAnnotations">
+  <int value="0" label="Unknown"/>
+  <int value="1" label="True"/>
+  <int value="2" label="False"/>
+</enum>
+
+<!-- LINT.ThenChange(//pdf/pdf_ink_metrics_handler.h:PDFLoadedWithV2InkAnnotations) -->
+
 <enum name="PDFPostMessageDataType">
 <!-- This must be kept current with PostMessageDataType in
      chrome/browser/resources/pdf/pdf_viewer.ts. -->

tools/metrics/histograms/metadata/pdf/histograms.xml

@@ -146,13 +146,14 @@ chromium-metrics-reviews@google.com.
   </token>
 </histogram>
 
-<histogram name="PDF.LoadedWithV2InkAnnotations" enum="Boolean"
-    expires_after="2025-12-01">
+<histogram name="PDF.LoadedWithV2InkAnnotations2"
+    enum="PDFLoadedWithV2InkAnnotations" expires_after="2025-12-01">
   <owner>andyphan@chromium.org</owner>
   <owner>thestig@chromium.org</owner>
   <summary>
     Tracks the number of PDF documents loaded with existing V2 ink annotations
-    in the PDF viewer.
+    in the PDF viewer. Records &quot;Unknown&quot; if there is a timeout failure
+    when scanning the PDF for existing V2 ink annotations.
   </summary>
 </histogram>