Fix find-in-page for the PDF Viewer.

Send the correct "final update" value, with a test case to trigger the
failing scenario.

While trying to reduce the number of messages sent while performing
find-in-page in crrev.com/1021347, the "final update" bit got set
incorrectly and got sent twice in some cases. This causes the browser to
"hang up" after receiving the first "final update" message and ignore
the second one. Developer builds also trigger a DCHECK() failure when
this happens.

Fix this by giving the plugin more control over the "final update" bit
in the blink::WebPluginContainer::ReportFindInPageSelection() interface,
and make the simple change in WebPluginContainerImpl() to pass the value
along. Then add all the plumbing necessary to the PDF plugin code to
communicate with WebPluginContainer, and let
PDFiumEngine::SelectFindResult() send the appropriate values.

Bug: 1352097
Change-Id: I91c1433792f0f931258fb893954180f8af1c5f5b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3828901
Reviewed-by: K. Moon <kmoon@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1037259}

chrome/browser/pdf/pdf_find_request_manager_browsertest.cc

@@ -350,4 +350,26 @@ IN_PROC_BROWSER_TEST_F(PdfFindRequestManagerTest, DoesNotSearchPdfViewerUi) {
   EXPECT_EQ(1, results.number_of_matches);
 }
 
+// Regression test for crbug.com/1352097.
+IN_PROC_BROWSER_TEST_F(PdfFindRequestManagerTest, SingleResultFindNext) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  LoadAndWait("/find_in_pdf_page.pdf");
+  ASSERT_TRUE(pdf_extension_test_util::EnsurePDFHasLoaded(contents()));
+
+  auto options = blink::mojom::FindOptions::New();
+  Find("pdf", options.Clone());
+  delegate()->MarkNextReply();
+  delegate()->WaitForNextReply();
+
+  options->new_session = false;
+  Find("pdf", options.Clone());
+  delegate()->MarkNextReply();
+  delegate()->WaitForNextReply();
+
+  FindResults results = delegate()->GetFindResults();
+  EXPECT_EQ(last_request_id(), results.request_id);
+  EXPECT_EQ(1, results.number_of_matches);
+  EXPECT_EQ(1, results.active_match_ordinal);
+}
+
 }  // namespace content

components/pdf/renderer/pdf_view_web_plugin_client.cc

@@ -115,8 +115,9 @@ void PdfViewWebPluginClient::ReportFindInPageMatchCount(int identifier,
 }
 
 void PdfViewWebPluginClient::ReportFindInPageSelection(int identifier,
-                                                       int index) {
-  plugin_container_->ReportFindInPageSelection(identifier, index);
+                                                       int index,
+                                                       bool final_update) {
+  plugin_container_->ReportFindInPageSelection(identifier, index, final_update);
 }
 
 void PdfViewWebPluginClient::ReportFindInPageTickmarks(

components/pdf/renderer/pdf_view_web_plugin_client.h

@@ -49,7 +49,9 @@ class PdfViewWebPluginClient : public chrome_pdf::PdfViewWebPlugin::Client {
   void ReportFindInPageMatchCount(int identifier,
                                   int total,
                                   bool final_update) override;
-  void ReportFindInPageSelection(int identifier, int index) override;
+  void ReportFindInPageSelection(int identifier,
+                                 int index,
+                                 bool final_update) override;
   void ReportFindInPageTickmarks(
       const std::vector<gfx::Rect>& tickmarks) override;
   float DeviceScaleFactor() override;

pdf/pdf_engine.h

@@ -171,13 +171,16 @@ class PDFEngine {
 
     // Updates the number of find results for the current search term.  If
     // there are no matches 0 should be passed in.  Only when the plugin has
-    // finished searching should it pass in the final count with final_result
+    // finished searching should it pass in the final count with `final_result`
     // set to true.
     virtual void NotifyNumberOfFindResultsChanged(int total,
                                                   bool final_result) {}
 
-    // Updates the index of the currently selected search item.
-    virtual void NotifySelectedFindResultChanged(int current_find_index) {}
+    // Updates the index of the currently selected search item. Set
+    // `final_result` to true only when there is no subsequent
+    // `NotifyNumberOfFindResultsChanged()` call.
+    virtual void NotifySelectedFindResultChanged(int current_find_index,
+                                                 bool final_result) {}
 
     virtual void NotifyTouchSelectionOccurred() {}
 

pdf/pdf_view_web_plugin.cc

@@ -841,12 +841,14 @@ void PdfViewWebPlugin::NotifyNumberOfFindResultsChanged(int total,
       kFindResultCooldown);
 }
 
-void PdfViewWebPlugin::NotifySelectedFindResultChanged(int current_find_index) {
+void PdfViewWebPlugin::NotifySelectedFindResultChanged(int current_find_index,
+                                                       bool final_result) {
   if (find_identifier_ == -1 || !client_->PluginContainer())
     return;
 
   DCHECK_GE(current_find_index, -1);
-  client_->ReportFindInPageSelection(find_identifier_, current_find_index + 1);
+  client_->ReportFindInPageSelection(find_identifier_, current_find_index + 1,
+                                     final_result);
 }
 
 void PdfViewWebPlugin::CaretChanged(const gfx::Rect& caret_rect) {

pdf/pdf_view_web_plugin.h

@@ -133,7 +133,9 @@ class PdfViewWebPlugin final : public PdfViewPluginBase,
                                             bool final_update) = 0;
 
     // Notify the web plugin container about the selected find result in plugin.
-    virtual void ReportFindInPageSelection(int identifier, int index) = 0;
+    virtual void ReportFindInPageSelection(int identifier,
+                                           int index,
+                                           bool final_update) = 0;
 
     // Notify the web plugin container about find result tickmarks.
     virtual void ReportFindInPageTickmarks(
@@ -278,7 +280,8 @@ class PdfViewWebPlugin final : public PdfViewPluginBase,
   void UpdateCursor(ui::mojom::CursorType new_cursor_type) override;
   void UpdateTickMarks(const std::vector<gfx::Rect>& tickmarks) override;
   void NotifyNumberOfFindResultsChanged(int total, bool final_result) override;
-  void NotifySelectedFindResultChanged(int current_find_index) override;
+  void NotifySelectedFindResultChanged(int current_find_index,
+                                       bool final_result) override;
   void GetDocumentPassword(
       base::OnceCallback<void(const std::string&)> callback) override;
   void Beep() override;

pdf/pdf_view_web_plugin_unittest.cc

@@ -254,7 +254,7 @@ class FakePdfViewWebPluginClient : public PdfViewWebPlugin::Client {
 
   MOCK_METHOD(void, ReportFindInPageMatchCount, (int, int, bool), (override));
 
-  MOCK_METHOD(void, ReportFindInPageSelection, (int, int), (override));
+  MOCK_METHOD(void, ReportFindInPageSelection, (int, int, bool), (override));
 
   MOCK_METHOD(void,
               ReportFindInPageTickmarks,

pdf/pdfium/findtext_unittest.cc

@@ -28,7 +28,7 @@ class FindTextTestClient : public TestClient {
 
   // PDFEngine::Client:
   MOCK_METHOD(void, NotifyNumberOfFindResultsChanged, (int, bool), (override));
-  MOCK_METHOD(void, NotifySelectedFindResultChanged, (int), (override));
+  MOCK_METHOD(void, NotifySelectedFindResultChanged, (int, bool), (override));
 
   std::vector<SearchStringResult> SearchString(const char16_t* string,
                                                const char16_t* term,
@@ -73,7 +73,8 @@ void ExpectInitialSearchResults(FindTextTestClient& client, int count) {
 
   EXPECT_CALL(client,
               NotifyNumberOfFindResultsChanged(1, /*final_result=*/false));
-  EXPECT_CALL(client, NotifySelectedFindResultChanged(0));
+  EXPECT_CALL(client,
+              NotifySelectedFindResultChanged(0, /*final_result=*/false));
   for (int i = 2; i < count + 1; ++i) {
     EXPECT_CALL(client,
                 NotifyNumberOfFindResultsChanged(i, /*final_result=*/false));
@@ -185,14 +186,17 @@ TEST_F(FindTextTest, SelectFindResult) {
   engine->StartFind("world", /*case_sensitive=*/true);
 
   EXPECT_CALL(client, NotifyNumberOfFindResultsChanged(_, _)).Times(0);
-  EXPECT_CALL(client, NotifySelectedFindResultChanged(1));
+  EXPECT_CALL(client,
+              NotifySelectedFindResultChanged(1, /*final_result=*/true));
 
   ASSERT_TRUE(engine->SelectFindResult(/*forward=*/true));
 
-  EXPECT_CALL(client, NotifySelectedFindResultChanged(2));
+  EXPECT_CALL(client,
+              NotifySelectedFindResultChanged(2, /*final_result=*/true));
   ASSERT_TRUE(engine->SelectFindResult(/*forward=*/true));
 
-  EXPECT_CALL(client, NotifySelectedFindResultChanged(1));
+  EXPECT_CALL(client,
+              NotifySelectedFindResultChanged(1, /*final_result=*/true));
   ASSERT_TRUE(engine->SelectFindResult(/*forward=*/false));
 }
 
@@ -208,8 +212,10 @@ TEST_F(FindTextTest, SelectFindResultAndSwitchToTwoUpView) {
   {
     InSequence sequence;
 
-    EXPECT_CALL(client, NotifySelectedFindResultChanged(1));
-    EXPECT_CALL(client, NotifySelectedFindResultChanged(2));
+    EXPECT_CALL(client,
+                NotifySelectedFindResultChanged(1, /*final_result=*/true));
+    EXPECT_CALL(client,
+                NotifySelectedFindResultChanged(2, /*final_result=*/true));
   }
   ASSERT_TRUE(engine->SelectFindResult(/*forward=*/true));
   ASSERT_TRUE(engine->SelectFindResult(/*forward=*/true));
@@ -229,7 +235,8 @@ TEST_F(FindTextTest, SelectFindResultAndSwitchToTwoUpView) {
   {
     InSequence sequence;
 
-    EXPECT_CALL(client, NotifySelectedFindResultChanged(2));
+    EXPECT_CALL(client,
+                NotifySelectedFindResultChanged(2, /*final_result=*/true));
   }
   ASSERT_TRUE(engine->SelectFindResult(/*forward=*/true));
 }

pdf/pdfium/pdfium_engine.cc

@@ -1761,6 +1761,7 @@ void PDFiumEngine::StartFind(const std::string& text, bool case_sensitive) {
       character_to_start_searching_from = old_selection[0].char_index();
       last_page_to_search_ = next_page_to_search_;
     }
+    search_in_progress_ = true;
   }
 
   int current_page = next_page_to_search_;
@@ -1801,6 +1802,8 @@ void PDFiumEngine::StartFind(const std::string& text, bool case_sensitive) {
        (pages_.size() > 1 && current_page == next_page_to_search_));
 
   if (end_of_search) {
+    search_in_progress_ = false;
+
     // Send the final notification.
     client_->NotifyNumberOfFindResultsChanged(find_results_.size(), true);
     return;
@@ -2023,8 +2026,9 @@ bool PDFiumEngine::SelectFindResult(bool forward) {
     if ((forward && current_index >= last_index) ||
         (!forward && current_index == 0)) {
       current_find_index_.reset();
-      client_->NotifySelectedFindResultChanged(-1);
-      client_->NotifyNumberOfFindResultsChanged(find_results_.size(), true);
+      client_->NotifySelectedFindResultChanged(-1, /*final_result=*/false);
+      client_->NotifyNumberOfFindResultsChanged(find_results_.size(),
+                                                /*final_result=*/true);
       return true;
     }
     int increment = forward ? 1 : -1;
@@ -2070,7 +2074,8 @@ bool PDFiumEngine::SelectFindResult(bool forward) {
     }
   }
 
-  client_->NotifySelectedFindResultChanged(current_find_index_.value());
+  client_->NotifySelectedFindResultChanged(
+      current_find_index_.value(), /*final_result=*/!search_in_progress_);
   return true;
 }
 

pdf/pdfium/pdfium_engine.h

@@ -726,6 +726,8 @@ class PDFiumEngine : public PDFEngine,
   std::string current_find_text_;
   // The results found.
   std::vector<PDFiumRange> find_results_;
+  // Whether a search is in progress.
+  bool search_in_progress_ = false;
   // Which page to search next.
   int next_page_to_search_ = -1;
   // Where to stop searching.

pdf/preview_mode_client.cc

@@ -72,8 +72,8 @@ void PreviewModeClient::NotifyNumberOfFindResultsChanged(int total,
   NOTREACHED();
 }
 
-void PreviewModeClient::NotifySelectedFindResultChanged(
-    int current_find_index) {
+void PreviewModeClient::NotifySelectedFindResultChanged(int current_find_index,
+                                                        bool final_result) {
   NOTREACHED();
 }
 

pdf/preview_mode_client.h

@@ -41,7 +41,8 @@ class PreviewModeClient : public PDFEngine::Client {
   void UpdateCursor(ui::mojom::CursorType cursor_type) override;
   void UpdateTickMarks(const std::vector<gfx::Rect>& tickmarks) override;
   void NotifyNumberOfFindResultsChanged(int total, bool final_result) override;
-  void NotifySelectedFindResultChanged(int current_find_index) override;
+  void NotifySelectedFindResultChanged(int current_find_index,
+                                       bool final_result) override;
   void GetDocumentPassword(
       base::OnceCallback<void(const std::string&)> callback) override;
   void Alert(const std::string& message) override;

third_party/blink/public/web/web_plugin_container.h

@@ -133,7 +133,9 @@ class WebPluginContainer {
   virtual void ReportFindInPageMatchCount(int identifier,
                                           int total,
                                           bool final_update) = 0;
-  virtual void ReportFindInPageSelection(int identifier, int index) = 0;
+  virtual void ReportFindInPageSelection(int identifier,
+                                         int index,
+                                         bool final_update) = 0;
 
   virtual float PageScaleFactor() = 0;
   virtual float PageZoomFactor() = 0;

third_party/blink/renderer/core/exported/web_plugin_container_impl.cc

@@ -320,19 +320,20 @@ void WebPluginContainerImpl::ReportFindInPageMatchCount(int identifier,
   if (!frame)
     return;
 
-  final_find_update_ = final_update;
   frame->GetFindInPage()->ReportFindInPageMatchCount(identifier, total,
                                                      final_update);
 }
 
 void WebPluginContainerImpl::ReportFindInPageSelection(int identifier,
-                                                       int index) {
+                                                       int index,
+                                                       bool final_update) {
   WebLocalFrameImpl* frame =
       WebLocalFrameImpl::FromFrame(element_->GetDocument().GetFrame());
   if (!frame)
     return;
-  frame->GetFindInPage()->ReportFindInPageSelection(
-      identifier, index, gfx::Rect(), final_find_update_);
+
+  frame->GetFindInPage()->ReportFindInPageSelection(identifier, index,
+                                                    gfx::Rect(), final_update);
 }
 
 float WebPluginContainerImpl::PageScaleFactor() {

third_party/blink/renderer/core/exported/web_plugin_container_impl.h

@@ -137,7 +137,9 @@ class CORE_EXPORT WebPluginContainerImpl final
   void ReportFindInPageMatchCount(int identifier,
                                   int total,
                                   bool final_update) override;
-  void ReportFindInPageSelection(int identifier, int index) override;
+  void ReportFindInPageSelection(int identifier,
+                                 int index,
+                                 bool final_update) override;
   float PageScaleFactor() override;
   float PageZoomFactor() override;
   void SetCcLayer(cc::Layer*) override;
@@ -234,10 +236,6 @@ class CORE_EXPORT WebPluginContainerImpl final
   cc::Layer* layer_ = nullptr;
   TouchEventRequestType touch_event_request_type_ = kTouchEventRequestTypeNone;
   bool wants_wheel_events_ = false;
-
-  // Whether the most recent `ReportFindInPageMatchCount()` call was the final
-  // update or not.
-  bool final_find_update_ = false;
 };
 
 template <>