Observe RenderFrameHostChanged in ThreatDetails to fix race condition

RenderFrameHosts can be deleted when a navigation commits, and this
does not call FrameDeleted(). This can trigger the same race condition
as https://chromium-review.googlesource.com/1146997. If
GetThreatDomDetails completes, then the navigation commits (and the
old RenderFrameHosts is deleted), then OnReceivedThreatDOMDetails runs,
we will crash.

Bug: 817724, 860445
Change-Id: Iae81a1c3dc724113d30e17652b75a88bcd7f4de5
Reviewed-on: https://chromium-review.googlesource.com/1162422
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581079}

components/safe_browsing/browser/threat_details.cc

@@ -580,7 +580,7 @@ void ThreatDetails::RequestThreatDOMDetails(content::RenderFrameHost* frame) {
   frame->GetRemoteInterfaces()->GetInterface(&threat_reporter);
   safe_browsing::mojom::ThreatReporter* raw_threat_report =
       threat_reporter.get();
-  pending_render_frame_hosts_.insert(frame);
+  pending_render_frame_hosts_.push_back(frame);
   raw_threat_report->GetThreatDOMDetails(
       base::BindOnce(&ThreatDetails::OnReceivedThreatDOMDetails, this,
                      std::move(threat_reporter), frame));
@@ -593,8 +593,13 @@ void ThreatDetails::OnReceivedThreatDOMDetails(
     std::vector<mojom::ThreatDOMDetailsNodePtr> params) {
   // If the RenderFrameHost was closed between sending the IPC and this callback
   // running, |sender| will be invalid.
-  if (pending_render_frame_hosts_.erase(sender) == 0)
+  const auto sender_it = std::find(pending_render_frame_hosts_.begin(),
+                                   pending_render_frame_hosts_.end(), sender);
+  if (sender_it == pending_render_frame_hosts_.end()) {
     return;
+  }
+
+  pending_render_frame_hosts_.erase(sender_it);
 
   // Lookup the FrameTreeNode ID of any child frames in the list of DOM nodes.
   const int sender_process_id = sender->GetProcess()->GetID();
@@ -815,7 +820,17 @@ void ThreatDetails::AllDone() {
 }
 
 void ThreatDetails::FrameDeleted(RenderFrameHost* render_frame_host) {
-  pending_render_frame_hosts_.erase(render_frame_host);
+  auto render_frame_host_it =
+      std::find(pending_render_frame_hosts_.begin(),
+                pending_render_frame_hosts_.end(), render_frame_host);
+  if (render_frame_host_it != pending_render_frame_hosts_.end()) {
+    pending_render_frame_hosts_.erase(render_frame_host_it);
+  }
+}
+
+void ThreatDetails::RenderFrameHostChanged(RenderFrameHost* old_host,
+                                           RenderFrameHost* new_host) {
+  FrameDeleted(old_host);
 }
 
 }  // namespace safe_browsing

components/safe_browsing/browser/threat_details.h

@@ -103,6 +103,8 @@ class ThreatDetails : public base::RefCounted<ThreatDetails>,
 
   // WebContentsObserver implementation:
   void FrameDeleted(content::RenderFrameHost* render_frame_host) override;
+  void RenderFrameHostChanged(content::RenderFrameHost* old_host,
+                              content::RenderFrameHost* new_host) override;
 
  protected:
   friend class ThreatDetailsFactoryImpl;
@@ -266,7 +268,7 @@ class ThreatDetails : public base::RefCounted<ThreatDetails>,
 
   // The set of RenderFrameHosts that have pending requests and haven't been
   // deleted.
-  std::set<content::RenderFrameHost*> pending_render_frame_hosts_;
+  std::vector<content::RenderFrameHost*> pending_render_frame_hosts_;
 
   FRIEND_TEST_ALL_PREFIXES(ThreatDetailsTest, HistoryServiceUrls);
   FRIEND_TEST_ALL_PREFIXES(ThreatDetailsTest, HttpsResourceSanitization);