Change ThreatDetails to be uniquely owned

Looks like most of the work on this bug was already done. ThreatDetails
is only on the UI thread, and a callback is used after
FinishCollection. These mitigated the immediate problems with
ThreatDetails lifetime. This CL just does the final switch to owning
ThreatDetails with a unique_ptr.

Bug: 777915
Change-Id: Id89b34bb1ee2dbb7a5305822751780ee4ac2e862
Reviewed-on: https://chromium-review.googlesource.com/1183782Reviewed-by: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Nathan Parker <nparker@chromium.org>
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585217}

chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc

@@ -235,7 +235,7 @@ class TestThreatDetailsFactory : public ThreatDetailsFactory {
   TestThreatDetailsFactory() : details_() {}
   ~TestThreatDetailsFactory() override {}
 
-  scoped_refptr<ThreatDetails> CreateThreatDetails(
+  std::unique_ptr<ThreatDetails> CreateThreatDetails(
       BaseUIManager* delegate,
       WebContents* web_contents,
       const security_interstitials::UnsafeResource& unsafe_resource,
@@ -244,7 +244,7 @@ class TestThreatDetailsFactory : public ThreatDetailsFactory {
       ReferrerChainProvider* referrer_chain_provider,
       bool trim_to_ad_tags,
       ThreatDetailsDoneCallback done_callback) override {
-    auto details = base::WrapRefCounted(new ThreatDetails(
+    auto details = base::WrapUnique(new ThreatDetails(
         delegate, web_contents, unsafe_resource, url_loader_factory,
         history_service, referrer_chain_provider, trim_to_ad_tags,
         done_callback));

components/safe_browsing/browser/threat_details.cc

@@ -271,7 +271,7 @@ void TrimElements(const std::set<int> target_ids,
 // don't leak it.
 class ThreatDetailsFactoryImpl : public ThreatDetailsFactory {
  public:
-  scoped_refptr<ThreatDetails> CreateThreatDetails(
+  std::unique_ptr<ThreatDetails> CreateThreatDetails(
       BaseUIManager* ui_manager,
       WebContents* web_contents,
       const security_interstitials::UnsafeResource& unsafe_resource,
@@ -280,7 +280,10 @@ class ThreatDetailsFactoryImpl : public ThreatDetailsFactory {
       ReferrerChainProvider* referrer_chain_provider,
       bool trim_to_ad_tags,
       ThreatDetailsDoneCallback done_callback) override {
-    auto threat_details = base::WrapRefCounted(new ThreatDetails(
+    // We can't use make_unique due to the protected constructor. We can't
+    // directly use std::unique_ptr<ThreatDetails>(new ThreatDetails(...))
+    // due to presubmit errors. So we use base::WrapUnique:
+    auto threat_details = base::WrapUnique(new ThreatDetails(
         ui_manager, web_contents, unsafe_resource, url_loader_factory,
         history_service, referrer_chain_provider, trim_to_ad_tags,
         done_callback));
@@ -301,7 +304,7 @@ static base::LazyInstance<ThreatDetailsFactoryImpl>::DestructorAtExit
 
 // Create a ThreatDetails for the given tab.
 /* static */
-scoped_refptr<ThreatDetails> ThreatDetails::NewThreatDetails(
+std::unique_ptr<ThreatDetails> ThreatDetails::NewThreatDetails(
     BaseUIManager* ui_manager,
     WebContents* web_contents,
     const UnsafeResource& resource,
@@ -342,7 +345,8 @@ ThreatDetails::ThreatDetails(
       cache_collector_(new ThreatDetailsCacheCollector),
       done_callback_(done_callback),
       all_done_expected_(false),
-      is_all_done_(false) {
+      is_all_done_(false),
+      weak_factory_(this) {
   redirects_collector_ = new ThreatDetailsRedirectsCollector(
       history_service ? history_service->AsWeakPtr()
                       : base::WeakPtr<history::HistoryService>());
@@ -357,7 +361,8 @@ ThreatDetails::ThreatDetails()
       ambiguous_dom_(false),
       trim_to_ad_tags_(false),
       all_done_expected_(false),
-      is_all_done_(false) {}
+      is_all_done_(false),
+      weak_factory_(this) {}
 
 ThreatDetails::~ThreatDetails() {
   DCHECK(all_done_expected_ == is_all_done_);
@@ -570,8 +575,8 @@ void ThreatDetails::StartCollection() {
     // OnReceivedThreatDOMDetails will be called when the renderer replies.
     // TODO(mattm): In theory, if the user proceeds through the warning DOM
     // detail collection could be started once the page loads.
-    web_contents()->ForEachFrame(
-        base::BindRepeating(&ThreatDetails::RequestThreatDOMDetails, this));
+    web_contents()->ForEachFrame(base::BindRepeating(
+        &ThreatDetails::RequestThreatDOMDetails, GetWeakPtr()));
   }
 }
 
@@ -582,7 +587,7 @@ void ThreatDetails::RequestThreatDOMDetails(content::RenderFrameHost* frame) {
       threat_reporter.get();
   pending_render_frame_hosts_.push_back(frame);
   raw_threat_report->GetThreatDOMDetails(
-      base::BindOnce(&ThreatDetails::OnReceivedThreatDOMDetails, this,
+      base::BindOnce(&ThreatDetails::OnReceivedThreatDOMDetails, GetWeakPtr(),
                      std::move(threat_reporter), frame));
 }
 
@@ -704,7 +709,8 @@ void ThreatDetails::FinishCollection(bool did_proceed, int num_visit) {
     urls.push_back(GURL(it->first));
   }
   redirects_collector_->StartHistoryCollection(
-      urls, base::Bind(&ThreatDetails::OnRedirectionCollectionReady, this));
+      urls,
+      base::Bind(&ThreatDetails::OnRedirectionCollectionReady, GetWeakPtr()));
 }
 
 void ThreatDetails::OnRedirectionCollectionReady() {
@@ -718,7 +724,7 @@ void ThreatDetails::OnRedirectionCollectionReady() {
   // Call the cache collector
   cache_collector_->StartCacheCollection(
       url_loader_factory_, &resources_, &cache_result_,
-      base::Bind(&ThreatDetails::OnCacheCollectionReady, this));
+      base::Bind(&ThreatDetails::OnCacheCollectionReady, GetWeakPtr()));
 }
 
 void ThreatDetails::AddRedirectUrlList(const std::vector<GURL>& urls) {
@@ -833,4 +839,8 @@ void ThreatDetails::RenderFrameHostChanged(RenderFrameHost* old_host,
   FrameDeleted(old_host);
 }
 
+base::WeakPtr<ThreatDetails> ThreatDetails::GetWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
 }  // namespace safe_browsing

components/safe_browsing/browser/threat_details.h

@@ -67,13 +67,14 @@ using FrameTreeIdToChildIdsMap = base::hash_map<int, std::unordered_set<int>>;
 // sending a report.
 using ThreatDetailsDoneCallback = base::Callback<void(content::WebContents*)>;
 
-class ThreatDetails : public base::RefCounted<ThreatDetails>,
-                      public content::WebContentsObserver {
+class ThreatDetails : public content::WebContentsObserver {
  public:
   typedef security_interstitials::UnsafeResource UnsafeResource;
 
+  ~ThreatDetails() override;
+
   // Constructs a new ThreatDetails instance, using the factory.
-  static scoped_refptr<ThreatDetails> NewThreatDetails(
+  static std::unique_ptr<ThreatDetails> NewThreatDetails(
       BaseUIManager* ui_manager,
       content::WebContents* web_contents,
       const UnsafeResource& resource,
@@ -106,6 +107,8 @@ class ThreatDetails : public base::RefCounted<ThreatDetails>,
   void RenderFrameHostChanged(content::RenderFrameHost* old_host,
                               content::RenderFrameHost* new_host) override;
 
+  base::WeakPtr<ThreatDetails> GetWeakPtr();
+
  protected:
   friend class ThreatDetailsFactoryImpl;
   friend class TestThreatDetailsFactory;
@@ -124,8 +127,6 @@ class ThreatDetails : public base::RefCounted<ThreatDetails>,
   // Default constructor for testing only.
   ThreatDetails();
 
-  ~ThreatDetails() override;
-
   virtual void AddDOMDetails(const int frame_tree_node_id,
                              std::vector<mojom::ThreatDOMDetailsNodePtr> params,
                              const KeyToFrameTreeIdMap& child_frame_tree_map);
@@ -140,8 +141,6 @@ class ThreatDetails : public base::RefCounted<ThreatDetails>,
   void StartCollection();
 
  private:
-  friend class base::RefCounted<ThreatDetails>;
-
   // Whether the url is "public" so we can add it to the report.
   bool IsReportableUrl(const GURL& url) const;
 
@@ -270,6 +269,9 @@ class ThreatDetails : public base::RefCounted<ThreatDetails>,
   // deleted.
   std::vector<content::RenderFrameHost*> pending_render_frame_hosts_;
 
+  // Used for references to |this| bound in callbacks.
+  base::WeakPtrFactory<ThreatDetails> weak_factory_;
+
   FRIEND_TEST_ALL_PREFIXES(ThreatDetailsTest, HistoryServiceUrls);
   FRIEND_TEST_ALL_PREFIXES(ThreatDetailsTest, HttpsResourceSanitization);
   FRIEND_TEST_ALL_PREFIXES(ThreatDetailsTest, HTTPCacheNoEntries);
@@ -289,7 +291,7 @@ class ThreatDetailsFactory {
  public:
   virtual ~ThreatDetailsFactory() {}
 
-  virtual scoped_refptr<ThreatDetails> CreateThreatDetails(
+  virtual std::unique_ptr<ThreatDetails> CreateThreatDetails(
       BaseUIManager* ui_manager,
       content::WebContents* web_contents,
       const security_interstitials::UnsafeResource& unsafe_resource,

components/safe_browsing/triggers/trigger_manager.cc

@@ -186,12 +186,11 @@ bool TriggerManager::StartCollectingThreatDetailsWithReason(
     return false;
 
   bool should_trim_threat_details = trigger_type == TriggerType::AD_SAMPLE;
-  collectors->threat_details =
-      scoped_refptr<ThreatDetails>(ThreatDetails::NewThreatDetails(
-          ui_manager_, web_contents, resource, url_loader_factory,
-          history_service, referrer_chain_provider_, should_trim_threat_details,
-          base::Bind(&TriggerManager::ThreatDetailsDone,
-                     weak_factory_.GetWeakPtr())));
+  collectors->threat_details = ThreatDetails::NewThreatDetails(
+      ui_manager_, web_contents, resource, url_loader_factory, history_service,
+      referrer_chain_provider_, should_trim_threat_details,
+      base::Bind(&TriggerManager::ThreatDetailsDone,
+                 weak_factory_.GetWeakPtr()));
   return true;
 }
 
@@ -220,7 +219,8 @@ bool TriggerManager::FinishCollectingThreatDetails(
     base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
         FROM_HERE,
         base::BindOnce(&ThreatDetails::FinishCollection,
-                       collectors->threat_details, did_proceed, num_visits),
+                       collectors->threat_details->GetWeakPtr(), did_proceed,
+                       num_visits),
         delay);
 
     // Record that this trigger fired and collected data.

components/safe_browsing/triggers/trigger_manager.h

@@ -43,7 +43,7 @@ struct DataCollectorsContainer {
   // Note: new data collection types should be added below as additional fields.
 
   // Collects ThreatDetails which contains resource URLs and partial DOM.
-  scoped_refptr<ThreatDetails> threat_details;
+  std::unique_ptr<ThreatDetails> threat_details;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(DataCollectorsContainer);

components/safe_browsing/triggers/trigger_manager_unittest.cc

@@ -29,10 +29,10 @@ namespace safe_browsing {
 class MockThreatDetails : public ThreatDetails {
  public:
   MockThreatDetails() {}
+  ~MockThreatDetails() override {}
   MOCK_METHOD2(FinishCollection, void(bool did_proceed, int num_visits));
 
  private:
-  ~MockThreatDetails() override {}
   DISALLOW_COPY_AND_ASSIGN(MockThreatDetails);
 };
 
@@ -40,7 +40,7 @@ class MockThreatDetailsFactory : public ThreatDetailsFactory {
  public:
   ~MockThreatDetailsFactory() override {}
 
-  scoped_refptr<ThreatDetails> CreateThreatDetails(
+  std::unique_ptr<ThreatDetails> CreateThreatDetails(
       BaseUIManager* ui_manager,
       content::WebContents* web_contents,
       const security_interstitials::UnsafeResource& unsafe_resource,
@@ -49,7 +49,7 @@ class MockThreatDetailsFactory : public ThreatDetailsFactory {
       ReferrerChainProvider* referrer_chain_provider,
       bool trim_to_ad_tags,
       ThreatDetailsDoneCallback done_callback) override {
-    return base::MakeRefCounted<MockThreatDetails>();
+    return std::make_unique<MockThreatDetails>();
   }
 };
 
@@ -135,8 +135,11 @@ class TriggerManagerTest : public ::testing::Test {
         trigger_type, web_contents, base::TimeDelta(), false, 0, options);
 
     // Invoke the callback if the report was to be sent.
-    if (expect_report_sent)
+    if (expect_report_sent) {
+      // Allow the ThreatDetails to complete, then remove it.
+      base::RunLoop().RunUntilIdle();
       trigger_manager_.ThreatDetailsDone(web_contents);
+    }
 
     return result;
   }