Fix stale find results after global find pasteboard changes (macOS)

See the updated test in FindBarPlatformHelperMacTest for repro steps.

As part of the fix, this also cleans up some logic in FindTabHelper and
FindBarView. Dealing with a find of an empty string (which causes the
search to be cleared) no longer has to be special cased at each call
site. Instead, the logic is handled in FindTabHelper::StartFinding where
it belongs. That also removes a footgun where you might supply an empty
string (from, e.g., a user) and end up unintentionally getting a no-op
or a search of the previous string. The only places that relied on that
behavior were a few tests, which now use a better way of doing that.

Change-Id: Ide3260c3c4ce122d7e0ca9a1de521b84402bce4e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2465205
Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#817563}

chrome/browser/ui/find_bar/find_bar_controller.cc

@@ -139,6 +139,7 @@ void FindBarController::ChangeWebContents(WebContents* contents) {
       content::Source<NavigationController>(&web_contents_->GetController()));
 
   MaybeSetPrepopulateText();
+  UpdateFindBarForCurrentResult();
 
   if (find_tab_helper && find_tab_helper->find_ui_active()) {
     // A tab with a visible find bar just got selected and we need to show the
@@ -147,14 +148,36 @@ void FindBarController::ChangeWebContents(WebContents* contents) {
     // we don't surprise the user by popping up to the left for no apparent
     // reason.
     find_bar_->Show(false);
+    // The condition below can be true on macOS if the global pasteboard changed
+    // while this tab was inactive (the find result will have been reset by
+    // FindBarPlatformHelperMac). In that case, we need to find the new text to
+    // update the results in the findbar. If condition is true due to the find
+    // text being empty, the call to StartFinding will be a harmless no-op.
+    if (find_tab_helper->find_result().number_of_matches() == -1) {
+      find_tab_helper->StartFinding(find_bar_->GetFindText(),
+                                    true /* forward_direction */,
+                                    false /* case_sensitive */,
+                                    false /* find_match */);
+    }
   }
 
-  UpdateFindBarForCurrentResult();
   find_bar_->UpdateFindBarForChangedWebContents();
 }
 
 void FindBarController::SetText(base::string16 text) {
   find_bar_->SetFindTextAndSelectedRange(text, find_bar_->GetSelectedRange());
+
+  if (!web_contents_)
+    return;
+  find_in_page::FindTabHelper* find_tab_helper =
+      find_in_page::FindTabHelper::FromWebContents(web_contents_);
+  if (!find_tab_helper->find_ui_active())
+    return;
+
+  find_tab_helper->StartFinding(text,
+                                true /* forward_direction */,
+                                false /* case_sensitive */,
+                                false /* find_match */);
 }
 
 void FindBarController::OnUserChangedFindText(base::string16 text) {
@@ -185,6 +208,11 @@ void FindBarController::Observe(int type,
   }
 }
 
+void FindBarController::OnFindEmptyText(content::WebContents* web_contents) {
+  DCHECK_EQ(web_contents, web_contents_);
+  UpdateFindBarForCurrentResult();
+}
+
 void FindBarController::OnFindResultAvailable(
     content::WebContents* web_contents) {
   DCHECK_EQ(web_contents, web_contents_);

chrome/browser/ui/find_bar/find_bar_controller.h

@@ -65,6 +65,7 @@ class FindBarController : public content::NotificationObserver,
                const content::NotificationDetails& details) override;
 
   // find_in_page::FindResultObserver:
+  void OnFindEmptyText(content::WebContents* web_contents) override;
   void OnFindResultAvailable(content::WebContents* web_contents) override;
 
   void SetText(base::string16 text);

chrome/browser/ui/find_bar/find_bar_host_browsertest.cc

@@ -143,6 +143,16 @@ class FindInPageControllerTest : public InProcessBrowserTest {
     EnsureFindBoxOpenForBrowser(browser());
   }
 
+  int FindNext(WebContents* web_contents, int* ordinal) {
+    browser()->GetFindBarController()->Show(true /*find_next*/);
+    ui_test_utils::FindResultWaiter observer(web_contents);
+    observer.Wait();
+    if (ordinal) {
+      *ordinal = observer.active_match_ordinal();
+    }
+    return observer.number_of_matches();
+  }
+
   int FindInPage16(WebContents* web_contents,
                    const base::string16& search_str,
                    bool forward,
@@ -1090,9 +1100,8 @@ IN_PROC_BROWSER_TEST_F(FindInPageControllerTest,
 
   // Search for 'no_match'. No matches should be found.
   int ordinal = 0;
-  WebContents* web_contents =
-      browser()->tab_strip_model()->GetActiveWebContents();
-  EXPECT_EQ(0, FindInPageASCII(web_contents, "no_match",
+  auto* tab_strip = browser()->tab_strip_model();
+  EXPECT_EQ(0, FindInPageASCII(tab_strip->GetActiveWebContents(), "no_match",
                                kFwd, kIgnoreCase, &ordinal));
   EXPECT_EQ(0, ordinal);
 
@@ -1102,8 +1111,7 @@ IN_PROC_BROWSER_TEST_F(FindInPageControllerTest,
 
   // Simulate what happens when you press F3 for FindNext. We should get a
   // response here (a hang means search was aborted).
-  EXPECT_EQ(0, ui_test_utils::FindInPage(web_contents, base::string16(), kFwd,
-                                         kIgnoreCase, &ordinal, nullptr));
+  EXPECT_EQ(0, FindNext(tab_strip->GetActiveWebContents(), &ordinal));
   EXPECT_EQ(0, ordinal);
 
   // Open another tab (tab C).
@@ -1112,8 +1120,7 @@ IN_PROC_BROWSER_TEST_F(FindInPageControllerTest,
 
   // Simulate what happens when you press F3 for FindNext. We should get a
   // response here (a hang means search was aborted).
-  EXPECT_EQ(0, ui_test_utils::FindInPage(web_contents, base::string16(), kFwd,
-                                         kIgnoreCase, &ordinal, nullptr));
+  EXPECT_EQ(0, FindNext(tab_strip->GetActiveWebContents(), &ordinal));
   EXPECT_EQ(0, ordinal);
 }
 
@@ -1158,8 +1165,7 @@ IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, RestartSearchFromF3) {
 
   // Simulate what happens when you press F3 for FindNext. Still should show
   // one match. This cleared the pre-populate string at one point (see bug).
-  EXPECT_EQ(1, ui_test_utils::FindInPage(web_contents, base::string16(), kFwd,
-                                         kIgnoreCase, &ordinal, nullptr));
+  EXPECT_EQ(1, FindNext(web_contents, &ordinal));
   EXPECT_EQ(1, ordinal);
 
   // End the Find session, thereby making the next F3 start afresh.
@@ -1167,7 +1173,7 @@ IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, RestartSearchFromF3) {
       find_in_page::SelectionAction::kKeep, find_in_page::ResultAction::kKeep);
 
   // Simulate F3 while Find box is closed. Should have 1 match.
-  EXPECT_EQ(1, FindInPageASCII(web_contents, "", kFwd, kIgnoreCase, &ordinal));
+  EXPECT_EQ(1, FindNext(web_contents, &ordinal));
   EXPECT_EQ(1, ordinal);
 }
 
@@ -1211,8 +1217,7 @@ IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, MAYBE_PreferPreviousSearch) {
   browser()->GetFindBarController()->EndFindSession(
       find_in_page::SelectionAction::kKeep, find_in_page::ResultAction::kKeep);
   // Simulate F3.
-  ui_test_utils::FindInPage(web_contents_1, base::string16(), kFwd, kIgnoreCase,
-                            &ordinal, nullptr);
+  FindNext(web_contents_1, &ordinal);
   FindBar* find_bar = browser()->GetFindBarController()->find_bar();
   if (find_bar->HasGlobalFindPasteboard()) {
     EXPECT_EQ(find_in_page::FindTabHelper::FromWebContents(web_contents_1)

chrome/browser/ui/find_bar/find_bar_platform_helper_mac_browsertest.mm

@@ -10,11 +10,31 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/find_bar/find_bar.h"
 #include "chrome/browser/ui/find_bar/find_bar_controller.h"
+#include "chrome/test/base/find_result_waiter.h"
 #include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "content/public/test/browser_test.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #import "ui/base/cocoa/find_pasteboard.h"
 
+const char kSimple[] = "simple.html";
+
+GURL GetURL(const std::string& filename) {
+  return ui_test_utils::GetTestUrl(base::FilePath().AppendASCII("find_in_page"),
+                                   base::FilePath().AppendASCII(filename));
+}
+
+int WaitForFind(content::WebContents* web_contents, int* ordinal) {
+  ui_test_utils::FindResultWaiter observer(web_contents);
+  observer.Wait();
+  if (ordinal) {
+    *ordinal = observer.active_match_ordinal();
+  }
+  return observer.number_of_matches();
+}
+
 class FindBarPlatformHelperMacTest : public InProcessBrowserTest {
  public:
   FindBarPlatformHelperMacTest() {}
@@ -51,21 +71,61 @@ IN_PROC_BROWSER_TEST_F(FindBarPlatformHelperMacTest,
             find_bar_controller->find_bar()->GetFindText());
 }
 
-// Tests that the find bar is updated as the pasteboard updates.
+// Tests that the find bar text and results are updated as the global find
+// pasteboard updates. The following bug used to exist:
+// 1) Find some text.
+// 2) Switch to another app and find something there to put it in the global
+//    find pasteboard.
+// 3) Switch back to chrome, make sure no text is selected, and open the
+//    findbar. The bug caused the match counts from the previous search to
+//    remain in the findbar and the old find results to remain highlighted.
 IN_PROC_BROWSER_TEST_F(FindBarPlatformHelperMacTest,
                        FindBarUpdatedFromPasteboard) {
+  ui_test_utils::NavigateToURL(browser(), GetURL(kSimple));
+
   FindBarController* find_bar_controller = browser()->GetFindBarController();
   ASSERT_NE(nullptr, find_bar_controller);
-
+  FindBar* find_bar = find_bar_controller->find_bar();
+  ASSERT_NE(nullptr, find_bar);
+
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_NE(nullptr, web_contents);
+  find_in_page::FindTabHelper* helper =
+      find_in_page::FindTabHelper::FromWebContents(web_contents);
+  ASSERT_NE(nullptr, helper);
+
+  int ordinal = -1;
+  int find_request_id = helper->current_find_request_id();
   NSString* update_string = @"Update String";
   [[FindPasteboard sharedInstance] setFindText:update_string];
 
-  EXPECT_EQ(base::SysNSStringToUTF16(update_string),
-            find_bar_controller->find_bar()->GetFindText());
+  // setFindText shouldn't trigger a find since the findbar isn't showing
+  EXPECT_EQ(find_request_id, helper->current_find_request_id());
+  EXPECT_EQ(base::SysNSStringToUTF16(update_string), find_bar->GetFindText());
 
-  NSString* next_string = @"Next String";
-  [[FindPasteboard sharedInstance] setFindText:next_string];
+  find_bar_controller->Show();
+  // Showing the findbar should trigger a find request
+  EXPECT_EQ(find_request_id + 1, helper->current_find_request_id());
+  EXPECT_EQ(0, WaitForFind(web_contents, &ordinal));
+  EXPECT_EQ(0, ordinal);
 
-  EXPECT_EQ(base::SysNSStringToUTF16(next_string),
-            find_bar_controller->find_bar()->GetFindText());
+  NSString* next_string = @"some text";
+  [[FindPasteboard sharedInstance] setFindText:next_string];
+  // setFindText should trigger a find since the findbar is now showing
+  EXPECT_EQ(find_request_id + 2, helper->current_find_request_id());
+  EXPECT_EQ(1, WaitForFind(web_contents, &ordinal));
+  EXPECT_EQ(0, ordinal);
+  EXPECT_EQ(base::SysNSStringToUTF16(next_string), find_bar->GetFindText());
+
+  NSString* empty_string = @"";
+  [[FindPasteboard sharedInstance] setFindText:empty_string];
+  // setting an empty string won't trigger a find, but it should clear
+  // the find results
+  EXPECT_EQ(find_request_id + 2, helper->current_find_request_id());
+  EXPECT_EQ(-1, helper->find_result().number_of_matches());
+  EXPECT_EQ(-1, helper->find_result().active_match_ordinal());
+  EXPECT_EQ(base::string16(),
+            find_bar->GetFindBarTesting()->GetMatchCountText());
+  EXPECT_EQ(base::SysNSStringToUTF16(empty_string), find_bar->GetFindText());
 }

chrome/browser/ui/find_bar/find_bar_platform_helper_mac_interactive_uitest.mm

@@ -259,8 +259,7 @@ IN_PROC_BROWSER_TEST_F(FindBarPlatformHelperMacInteractiveUITest,
   find_bar_controller->EndFindSession(find_in_page::SelectionAction::kKeep,
                                       find_in_page::ResultAction::kKeep);
   // Simulate F3.
-  ui_test_utils::FindInPage(first_active_web_contents, base::string16(), true,
-                            false, nullptr, nullptr);
+  browser()->GetFindBarController()->Show(true /*find_next*/);
   EXPECT_EQ(
       base::ASCIIToUTF16("given"),
       find_in_page::FindTabHelper::FromWebContents(first_active_web_contents)

chrome/browser/ui/views/find_bar_view.cc

@@ -423,27 +423,10 @@ void FindBarView::Find(const base::string16& search_text) {
 
   controller->OnUserChangedFindText(search_text);
 
-  // When the user changes something in the text box we check the contents and
-  // if the textbox contains something we set it as the new search string and
-  // initiate search (even though old searches might be in progress).
-  if (!search_text.empty()) {
-    find_tab_helper->StartFinding(search_text, true /* forward_direction */,
-                                  false /* case_sensitive */,
-                                  true /* find_match */);
-  } else {
-    find_tab_helper->StopFinding(find_in_page::SelectionAction::kClear);
-    UpdateForResult(find_tab_helper->find_result(), base::string16());
-    find_bar_host_->MoveWindowIfNecessary();
-
-    // Clearing the text box should clear the prepopulate state so that when
-    // we close and reopen the Find box it doesn't show the search we just
-    // deleted. We can't do this on ChromeOS yet because we get ContentsChanged
-    // sent for a lot more things than just the user nulling out the search
-    // terms. See http://crbug.com/45372.
-    FindBarState* find_bar_state = FindBarStateFactory::GetForBrowserContext(
-        web_contents->GetBrowserContext());
-    find_bar_state->SetLastSearchText(base::string16());
-  }
+  // Initiate a search (even though old searches might be in progress).
+  find_tab_helper->StartFinding(search_text, true /* forward_direction */,
+                                false /* case_sensitive */,
+                                true /* find_match */);
 }
 
 void FindBarView::FindNext(bool reverse) {

chrome/browser/ui/views/find_bar_views_interactive_uitest.cc

@@ -591,6 +591,9 @@ IN_PROC_BROWSER_TEST_F(FindInPageTest, MAYBE_CtrlEnter) {
                                GURL("data:text/html,This is some text with a "
                                     "<a href=\"about:blank\">link</a>."));
 
+  auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents();
+  auto* host = web_contents->GetRenderWidgetHostView()->GetRenderWidgetHost();
+
   browser()->GetFindBarController()->Show();
 
   // Search for "link".
@@ -602,6 +605,8 @@ IN_PROC_BROWSER_TEST_F(FindInPageTest, MAYBE_CtrlEnter) {
       browser(), ui::VKEY_N, false, false, false, false));
   ASSERT_TRUE(ui_test_utils::SendKeyPressSync(
       browser(), ui::VKEY_K, false, false, false, false));
+  content::RunUntilInputProcessed(host);
+
   EXPECT_EQ(ASCIIToUTF16("link"), GetFindBarText());
 
   ui_test_utils::UrlLoadObserver observer(

components/find_in_page/find_result_observer.h

@@ -19,6 +19,7 @@ class FindResultObserver : public base::CheckedObserver {
  public:
   virtual void OnFindResultAvailable(content::WebContents* web_contents) = 0;
 
+  virtual void OnFindEmptyText(content::WebContents* web_contents) {}
   virtual void OnFindTabHelperDestroyed(FindTabHelper* helper) {}
 };
 

components/find_in_page/find_tab_helper.cc

@@ -51,17 +51,18 @@ void FindTabHelper::StartFinding(base::string16 search_string,
   const base::char16 kInvalidChars[] = {'\r', 0};
   base::RemoveChars(search_string, kInvalidChars, &search_string);
 
-  // If search_string is empty, it means FindNext was pressed with a keyboard
-  // shortcut so unless we have something to search for we return early.
-  if (search_string.empty() && find_text_.empty()) {
-    search_string = GetInitialSearchText();
+  // Keep track of what the last search was across the tabs.
+  if (delegate_)
+    delegate_->SetLastSearchText(search_string);
 
-    if (search_string.empty())
-      return;
+  if (search_string.empty()) {
+    StopFinding(find_in_page::SelectionAction::kClear);
+    for (auto& observer : observers_)
+      observer.OnFindEmptyText(web_contents_);
+    return;
   }
 
-  // NB: search_string will be empty when using the FindNext keyboard shortcut.
-  bool new_session = (find_text_ != search_string && !search_string.empty()) ||
+  bool new_session = find_text_ != search_string ||
                      (last_search_case_sensitive_ != case_sensitive) ||
                      find_op_aborted_;
 
@@ -70,24 +71,16 @@ void FindTabHelper::StartFinding(base::string16 search_string,
   if (!new_session && !find_match)
     return;
 
-  // Keep track of the previous search.
-  previous_find_text_ = find_text_;
-
   current_find_request_id_ = find_request_id_counter_++;
   if (new_session)
     current_find_session_id_ = current_find_request_id_;
 
-  if (!search_string.empty())
-    find_text_ = search_string;
+  previous_find_text_ = find_text_;
+  find_text_ = search_string;
   last_search_case_sensitive_ = case_sensitive;
-
   find_op_aborted_ = false;
   should_find_match_ = find_match;
 
-  // Keep track of what the last search was across the tabs.
-  if (delegate_)
-    delegate_->SetLastSearchText(find_text_);
-
   auto options = blink::mojom::FindOptions::New();
   options->forward = forward_direction;
   options->match_case = case_sensitive;