Changes to how FindRequestManager reports find results.

I've noticed some problematic edge cases with the reporting of find-in-page results from FindRequestManager. While trying to write some new tests in find_request_manager_browsertest.cc, I noticed that the new tests sometimes fail because of these edge cases. The existing tests in there could definitely fail from this as well, though much less likely. I have seen them fail very rarely before though and this is probably why.

The details of the issue, edge cases, and my solution are explained in this design doc: https://docs.google.com/a/google.com/document/d/1BCvuSW9XSBH7GMx5VC0TgBh6XmJJDOzz_XZKItjF6oM/edit?usp=sharing

UPDATE: Anther find-in-page CL I landed recently (https://codereview.chromium.org/2186113002/) was reverted because of a test that began timing out. I believe that failure is caused by the problems fixed here, so that bugfix is now blocked on this CL.
Review-Url: https://codereview.chromium.org/2249133002
Cr-Commit-Position: refs/heads/master@{#415353}

content/browser/find_request_manager.cc

@@ -91,11 +91,13 @@ FindRequestManager::FindRequestManager(WebContentsImpl* web_contents)
     : WebContentsObserver(web_contents),
       contents_(web_contents),
       current_session_id_(kInvalidId),
+      pending_find_next_reply_(nullptr),
       pending_active_match_ordinal_(false),
       number_of_matches_(0),
       active_frame_(nullptr),
       relative_active_match_ordinal_(0),
-      active_match_ordinal_(0) {}
+      active_match_ordinal_(0),
+      last_reported_id_(kInvalidId) {}
 
 FindRequestManager::~FindRequestManager() {}
 
@@ -190,16 +192,21 @@ void FindRequestManager::OnFindReply(RenderFrameHost* rfh,
 
   // This is the final update for this frame for the current find operation.
 
-  pending_replies_.erase(rfh);
-  if (request_id == current_session_id_ && !pending_replies_.empty()) {
+  pending_initial_replies_.erase(rfh);
+  if (request_id == current_session_id_ && !pending_initial_replies_.empty()) {
     NotifyFindReply(request_id, false /* final_update */);
     return;
   }
-  DCHECK(request_id == current_session_id_ ||
-         current_request_.options.findNext);
 
   // This is the final update for the current find operation.
-  FinalUpdate(request_id, rfh);
+
+  if (request_id == current_request_.id && request_id != current_session_id_) {
+    DCHECK(current_request_.options.findNext);
+    DCHECK_EQ(pending_find_next_reply_, rfh);
+    pending_find_next_reply_ = nullptr;
+  }
+
+  FinalUpdateReceived(request_id, rfh);
 }
 
 void FindRequestManager::RemoveFrame(RenderFrameHost* rfh) {
@@ -218,6 +225,7 @@ void FindRequestManager::RemoveFrame(RenderFrameHost* rfh) {
   if (active_frame_ == rfh) {
     active_frame_ = nullptr;
     relative_active_match_ordinal_ = 0;
+    selection_rect_ = gfx::Rect();
   }
   UpdateActiveMatchOrdinal();
 
@@ -239,17 +247,28 @@ void FindRequestManager::RemoveFrame(RenderFrameHost* rfh) {
   RemoveFindMatchRectsPendingReply(rfh);
 #endif
 
-  if (pending_replies_.count(rfh)) {
+  // If no pending find replies are expected for the removed frame, then just
+  // report the updated results.
+  if (!pending_initial_replies_.count(rfh) && pending_find_next_reply_ != rfh) {
+    bool final_update =
+        pending_initial_replies_.empty() && !pending_find_next_reply_;
+    NotifyFindReply(current_session_id_, final_update);
+    return;
+  }
+
+  if (pending_initial_replies_.count(rfh)) {
     // A reply should not be expected from the removed frame.
-    pending_replies_.erase(rfh);
-    if (pending_replies_.empty()) {
-      FinalUpdate(current_request_.id, rfh);
-      return;
+    pending_initial_replies_.erase(rfh);
+    if (pending_initial_replies_.empty()) {
+      FinalUpdateReceived(current_session_id_, rfh);
     }
   }
 
-  NotifyFindReply(current_session_id_,
-                  pending_replies_.empty() /* final_update */);
+  if (pending_find_next_reply_ == rfh) {
+    // A reply should not be expected from the removed frame.
+    pending_find_next_reply_ = nullptr;
+    FinalUpdateReceived(current_request_.id, rfh);
+  }
 }
 
 #if defined(OS_ANDROID)
@@ -264,7 +283,7 @@ void FindRequestManager::ActivateNearestFindResult(float x, float y) {
   for (FrameTreeNode* node : contents_->GetFrameTree()->Nodes()) {
     RenderFrameHost* rfh = node->current_frame_host();
 
-    if (!CheckFrame(rfh))
+    if (!CheckFrame(rfh) || !rfh->IsRenderFrameLive())
       continue;
 
     activate_.pending_replies.insert(rfh);
@@ -299,7 +318,7 @@ void FindRequestManager::RequestFindMatchRects(int current_version) {
   for (FrameTreeNode* node : contents_->GetFrameTree()->Nodes()) {
     RenderFrameHost* rfh = node->current_frame_host();
 
-    if (!CheckFrame(rfh))
+    if (!CheckFrame(rfh) || !rfh->IsRenderFrameLive())
       continue;
 
     match_rects_.pending_replies.insert(rfh);
@@ -343,7 +362,8 @@ void FindRequestManager::FrameDeleted(RenderFrameHost* rfh) {
 void FindRequestManager::Reset(const FindRequest& initial_request) {
   current_session_id_ = initial_request.id;
   current_request_ = initial_request;
-  pending_replies_.clear();
+  pending_initial_replies_.clear();
+  pending_find_next_reply_ = nullptr;
   pending_active_match_ordinal_ = true;
   matches_per_frame_.clear();
   number_of_matches_ = 0;
@@ -351,6 +371,7 @@ void FindRequestManager::Reset(const FindRequest& initial_request) {
   relative_active_match_ordinal_ = 0;
   active_match_ordinal_ = 0;
   selection_rect_ = gfx::Rect();
+  last_reported_id_ = kInvalidId;
 #if defined(OS_ANDROID)
   activate_ = ActivateNearestFindResultState();
   match_rects_.pending_replies.clear();
@@ -400,18 +421,31 @@ void FindRequestManager::AdvanceQueue(int request_id) {
 
 void FindRequestManager::SendFindIPC(const FindRequest& request,
                                      RenderFrameHost* rfh) {
-  pending_replies_.insert(rfh);
+  DCHECK(CheckFrame(rfh));
+  DCHECK(rfh->IsRenderFrameLive());
+
+  if (request.options.findNext)
+    pending_find_next_reply_ = rfh;
+  else
+    pending_initial_replies_.insert(rfh);
+
   rfh->Send(new FrameMsg_Find(rfh->GetRoutingID(), request.id,
                               request.search_text, request.options));
 }
 
-void FindRequestManager::NotifyFindReply(int request_id,
-                                         bool final_update) const {
+void FindRequestManager::NotifyFindReply(int request_id, bool final_update) {
   if (request_id == kInvalidId) {
     NOTREACHED();
     return;
   }
 
+  // Ensure that replies are not reported with IDs lower than the ID of the
+  // latest request we have results from.
+  if (request_id < last_reported_id_)
+    request_id = last_reported_id_;
+  else
+    last_reported_id_ = request_id;
+
   contents_->NotifyFindReply(request_id, number_of_matches_, selection_rect_,
                              active_match_ordinal_, final_update);
 }
@@ -437,7 +471,7 @@ RenderFrameHost* FindRequestManager::Traverse(RenderFrameHost* from_rfh,
       continue;
     RenderFrameHost* current_rfh = node->current_frame_host();
     if (!matches_only || matches_per_frame_.find(current_rfh)->second ||
-        pending_replies_.count(current_rfh)) {
+        pending_initial_replies_.count(current_rfh)) {
       // Note that if there is still a pending reply expected for this frame,
       // then it may have unaccounted matches and will not be skipped via
       // |matches_only|.
@@ -466,7 +500,7 @@ void FindRequestManager::AddFrame(RenderFrameHost* rfh) {
 }
 
 bool FindRequestManager::CheckFrame(RenderFrameHost* rfh) const {
-  return rfh && rfh->IsRenderFrameLive() && matches_per_frame_.count(rfh);
+  return rfh && matches_per_frame_.count(rfh);
 }
 
 void FindRequestManager::UpdateActiveMatchOrdinal() {
@@ -490,11 +524,16 @@ void FindRequestManager::UpdateActiveMatchOrdinal() {
   active_match_ordinal_ += relative_active_match_ordinal_;
 }
 
-void FindRequestManager::FinalUpdate(int request_id, RenderFrameHost* rfh) {
+void FindRequestManager::FinalUpdateReceived(int request_id,
+                                             RenderFrameHost* rfh) {
   if (!number_of_matches_ ||
-      !pending_active_match_ordinal_ ||
+      (active_match_ordinal_ && !pending_active_match_ordinal_) ||
       request_id != current_request_.id) {
-    NotifyFindReply(request_id, true /* final_update */);
+    // All the find results for |request_id| are in and ready to report. Note
+    // that |final_update| will be set to false if there are still pending
+    // replies expected from the initial find request.
+    NotifyFindReply(request_id,
+                    pending_initial_replies_.empty() /* final_update */);
     AdvanceQueue(request_id);
     return;
   }

content/browser/find_request_manager.h

@@ -124,7 +124,7 @@ class CONTENT_EXPORT FindRequestManager : public WebContentsObserver {
   void SendFindIPC(const FindRequest& request, RenderFrameHost* rfh);
 
   // Sends the find results (as they currently are) to the WebContents.
-  void NotifyFindReply(int request_id, bool final_update) const;
+  void NotifyFindReply(int request_id, bool final_update);
 
   // Returns the initial frame in search order. This will be either the first
   // frame, if searching forward, or the last frame, if searching backward.
@@ -158,7 +158,12 @@ class CONTENT_EXPORT FindRequestManager : public WebContentsObserver {
 
   // Called when all pending find replies have been received for the find
   // request with ID |request_id|. The final update was received from |rfh|.
-  void FinalUpdate(int request_id, RenderFrameHost* rfh);
+  //
+  // Note that this is the final update for this particular find request, but
+  // not necessarily for all issued requests. If there are still pending replies
+  // expected for a previous find request, then the outgoing find reply issued
+  // from this function will not be marked final.
+  void FinalUpdateReceived(int request_id, RenderFrameHost* rfh);
 
 #if defined(OS_ANDROID)
   // Called when a nearest find result reply is no longer pending for a frame.
@@ -252,10 +257,15 @@ class CONTENT_EXPORT FindRequestManager : public WebContentsObserver {
   // The current find request.
   FindRequest current_request_;
 
-  // The set of frames that are still expected to reply to a pending find
-  // request. Frames are removed from |pending_replies_| when their reply with
-  // |final_update| set to true is received.
-  std::unordered_set<RenderFrameHost*> pending_replies_;
+  // The set of frames that are still expected to reply to a pending initial
+  // find request. Frames are removed from |pending_initial_replies_| when their
+  // reply to the initial find request is received with |final_update| set to
+  // true.
+  std::unordered_set<RenderFrameHost*> pending_initial_replies_;
+
+  // The frame (if any) that is still expected to reply to the last pending
+  // "find next" request.
+  RenderFrameHost* pending_find_next_reply_;
 
   // Indicates whether an update to the active match ordinal is expected. Once
   // set, |pending_active_match_ordinal_| will not reset until an update to the
@@ -288,6 +298,10 @@ class CONTENT_EXPORT FindRequestManager : public WebContentsObserver {
   // Find requests are queued here when previous requests need to be handled
   // before these ones can be properly routed.
   std::queue<FindRequest> find_request_queue_;
+
+  // Keeps track of the find request ID of the last find reply reported via
+  // NotifyFindReply().
+  int last_reported_id_;
 };
 
 }  // namespace content