Avoid touching ThreatDetails's ref-count before it's fully constructed

ThreatDetails is a ref counted object, and its first reference can be
made by base::BindRepeating through ThreadDetails::StartCollection.
The ref count is released when the callback instance is destroyed.

As the RepeatingCallback instance created in StartCollection is
destroyed after the scope out, `new ThreatDetails` may have returned
a stale pointer.

This CL moves the problematic StartCollection() call out of the
constructor to avoid touching the ref count before its first proper
reference is made.

Bug: 866456
Change-Id: I82b83b9bb30cdc95cff60e3e22e79f0af17645a2
Reviewed-on: https://chromium-review.googlesource.com/1149777Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Commit-Queue: Taiju Tsuiki <tzik@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577962}

chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc

@@ -247,7 +247,7 @@ class TestThreatDetailsFactory : public ThreatDetailsFactory {
   TestThreatDetailsFactory() : details_() {}
   ~TestThreatDetailsFactory() override {}
 
-  ThreatDetails* CreateThreatDetails(
+  scoped_refptr<ThreatDetails> CreateThreatDetails(
       BaseUIManager* delegate,
       WebContents* web_contents,
       const security_interstitials::UnsafeResource& unsafe_resource,
@@ -256,11 +256,13 @@ class TestThreatDetailsFactory : public ThreatDetailsFactory {
       ReferrerChainProvider* referrer_chain_provider,
       bool trim_to_ad_tags,
       ThreatDetailsDoneCallback done_callback) override {
-    details_ = new ThreatDetails(delegate, web_contents, unsafe_resource,
-                                 url_loader_factory, history_service,
-                                 referrer_chain_provider, trim_to_ad_tags,
-                                 done_callback);
-    return details_;
+    auto details = base::WrapRefCounted(new ThreatDetails(
+        delegate, web_contents, unsafe_resource, url_loader_factory,
+        history_service, referrer_chain_provider, trim_to_ad_tags,
+        done_callback));
+    details_ = details.get();
+    details->StartCollection();
+    return details;
   }
 
   ThreatDetails* get_details() { return details_; }

chrome/browser/safe_browsing/threat_details_unittest.cc

@@ -142,6 +142,8 @@ class ThreatDetailsWrap : public ThreatDetails {
 
   size_t done_callback_count() { return done_callback_count_; }
 
+  void StartCollection() { ThreatDetails::StartCollection(); }
+
  private:
   ~ThreatDetailsWrap() override {}
 
@@ -174,7 +176,7 @@ class MockSafeBrowsingUIManager : public SafeBrowsingUIManager {
 
 class MockReferrerChainProvider : public ReferrerChainProvider {
  public:
-  virtual ~MockReferrerChainProvider(){};
+  virtual ~MockReferrerChainProvider() {}
   MOCK_METHOD3(IdentifyReferrerChainByWebContents,
                AttributionResult(content::WebContents* web_contents,
                                  int user_gesture_count_limit,
@@ -402,6 +404,7 @@ TEST_F(ThreatDetailsTest, ThreatSubResource) {
   scoped_refptr<ThreatDetailsWrap> report =
       new ThreatDetailsWrap(ui_manager_.get(), web_contents(), resource, NULL,
                             history_service(), referrer_chain_provider_.get());
+  report->StartCollection();
 
   std::string serialized = WaitForThreatDetailsDone(
       report.get(), true /* did_proceed*/, 1 /* num_visit */);
@@ -457,6 +460,7 @@ TEST_F(ThreatDetailsTest, SuspiciousSiteWithReferrerChain) {
   scoped_refptr<ThreatDetailsWrap> report =
       new ThreatDetailsWrap(ui_manager_.get(), web_contents(), resource, NULL,
                             history_service(), referrer_chain_provider_.get());
+  report->StartCollection();
 
   std::string serialized = WaitForThreatDetailsDone(
       report.get(), true /* did_proceed*/, 1 /* num_visit */);
@@ -507,6 +511,7 @@ TEST_F(ThreatDetailsTest, ThreatSubResourceWithOriginalUrl) {
   scoped_refptr<ThreatDetailsWrap> report =
       new ThreatDetailsWrap(ui_manager_.get(), web_contents(), resource, NULL,
                             history_service(), referrer_chain_provider_.get());
+  report->StartCollection();
 
   std::string serialized = WaitForThreatDetailsDone(
       report.get(), false /* did_proceed*/, 1 /* num_visit */);
@@ -555,6 +560,7 @@ TEST_F(ThreatDetailsTest, ThreatDOMDetails) {
   scoped_refptr<ThreatDetailsWrap> report =
       new ThreatDetailsWrap(ui_manager_.get(), web_contents(), resource, NULL,
                             history_service(), referrer_chain_provider_.get());
+  report->StartCollection();
 
   // Send a message from the DOM, with 2 nodes, a parent and a child.
   std::vector<mojom::ThreatDOMDetailsNodePtr> params;
@@ -777,6 +783,7 @@ TEST_F(ThreatDetailsTest, ThreatDOMDetails_MultipleFrames) {
     scoped_refptr<ThreatDetailsWrap> report = new ThreatDetailsWrap(
         ui_manager_.get(), web_contents(), resource, NULL, history_service(),
         referrer_chain_provider_.get());
+    report->StartCollection();
 
     std::vector<mojom::ThreatDOMDetailsNodePtr> outer_params_copy;
     for (auto& node : outer_params) {
@@ -833,6 +840,7 @@ TEST_F(ThreatDetailsTest, ThreatDOMDetails_MultipleFrames) {
     scoped_refptr<ThreatDetailsWrap> report = new ThreatDetailsWrap(
         ui_manager_.get(), web_contents(), resource, NULL, history_service(),
         referrer_chain_provider_.get());
+    report->StartCollection();
 
     // Send both sets of nodes from different render frames.
     report->OnReceivedThreatDOMDetails(nullptr, child_rfh,
@@ -959,6 +967,8 @@ TEST_F(ThreatDetailsTest, ThreatDOMDetails_AmbiguousDOM) {
   scoped_refptr<ThreatDetailsWrap> report =
       new ThreatDetailsWrap(ui_manager_.get(), web_contents(), resource, NULL,
                             history_service(), referrer_chain_provider_.get());
+  report->StartCollection();
+
   base::HistogramTester histograms;
 
   // Send both sets of nodes from different render frames.
@@ -1214,6 +1224,7 @@ TEST_F(ThreatDetailsTest, ThreatDOMDetails_TrimToAdTags) {
       new ThreatDetailsWrap(ui_manager_.get(), web_contents(), resource, NULL,
                             history_service(), referrer_chain_provider_.get(),
                             /*trim_to_ad_tags=*/true);
+  trimmed_report->StartCollection();
 
   // Send both sets of nodes from different render frames.
   trimmed_report->OnReceivedThreatDOMDetails(nullptr, child_rfh,
@@ -1287,6 +1298,7 @@ TEST_F(ThreatDetailsTest, ThreatDOMDetails_EmptyReportNotSent) {
       new ThreatDetailsWrap(ui_manager_.get(), web_contents(), resource, NULL,
                             history_service(), referrer_chain_provider_.get(),
                             /*trim_to_ad_tags=*/true);
+  trimmed_report->StartCollection();
 
   // Send both sets of nodes from different render frames.
   trimmed_report->OnReceivedThreatDOMDetails(nullptr, child_rfh,
@@ -1318,6 +1330,7 @@ TEST_F(ThreatDetailsTest, ThreatWithRedirectUrl) {
   scoped_refptr<ThreatDetailsWrap> report =
       new ThreatDetailsWrap(ui_manager_.get(), web_contents(), resource, NULL,
                             history_service(), referrer_chain_provider_.get());
+  report->StartCollection();
 
   std::string serialized = WaitForThreatDetailsDone(
       report.get(), true /* did_proceed*/, 0 /* num_visit */);
@@ -1390,6 +1403,7 @@ TEST_F(ThreatDetailsTest, ThreatOnMainPageLoadBlocked) {
   scoped_refptr<ThreatDetailsWrap> report =
       new ThreatDetailsWrap(ui_manager_.get(), web_contents(), resource, NULL,
                             history_service(), referrer_chain_provider_.get());
+  report->StartCollection();
 
   // Simulate clicking don't proceed.
   controller().DiscardNonCommittedEntries();
@@ -1450,6 +1464,8 @@ TEST_F(ThreatDetailsTest, ThreatWithPendingLoad) {
   scoped_refptr<ThreatDetailsWrap> report =
       new ThreatDetailsWrap(ui_manager_.get(), web_contents(), resource, NULL,
                             history_service(), referrer_chain_provider_.get());
+  report->StartCollection();
+
   std::string serialized = WaitForThreatDetailsDone(
       report.get(), true /* did_proceed*/, 1 /* num_visit */);
 
@@ -1498,6 +1514,8 @@ TEST_F(ThreatDetailsTest, ThreatOnFreshTab) {
   scoped_refptr<ThreatDetailsWrap> report =
       new ThreatDetailsWrap(ui_manager_.get(), web_contents(), resource, NULL,
                             history_service(), referrer_chain_provider_.get());
+  report->StartCollection();
+
   std::string serialized = WaitForThreatDetailsDone(
       report.get(), true /* did_proceed*/, 1 /* num_visit */);
 
@@ -1531,6 +1549,7 @@ TEST_F(ThreatDetailsTest, HTTPCache) {
   scoped_refptr<ThreatDetailsWrap> report = new ThreatDetailsWrap(
       ui_manager_.get(), web_contents(), resource, test_shared_loader_factory_,
       history_service(), referrer_chain_provider_.get());
+  report->StartCollection();
 
   SimulateFillCache(kThreatURL);
 
@@ -1609,6 +1628,7 @@ TEST_F(ThreatDetailsTest, HttpsResourceSanitization) {
   scoped_refptr<ThreatDetailsWrap> report = new ThreatDetailsWrap(
       ui_manager_.get(), web_contents(), resource, test_shared_loader_factory_,
       history_service(), referrer_chain_provider_.get());
+  report->StartCollection();
 
   SimulateFillCache(kThreatURLHttps);
 
@@ -1684,6 +1704,7 @@ TEST_F(ThreatDetailsTest, HTTPCacheNoEntries) {
   scoped_refptr<ThreatDetailsWrap> report = new ThreatDetailsWrap(
       ui_manager_.get(), web_contents(), resource, test_shared_loader_factory_,
       history_service(), referrer_chain_provider_.get());
+  report->StartCollection();
 
   // Simulate no cache entry found.
   test_url_loader_factory_.AddResponse(
@@ -1749,6 +1770,7 @@ TEST_F(ThreatDetailsTest, HistoryServiceUrls) {
   scoped_refptr<ThreatDetailsWrap> report =
       new ThreatDetailsWrap(ui_manager_.get(), web_contents(), resource, NULL,
                             history_service(), referrer_chain_provider_.get());
+  report->StartCollection();
 
   // The redirects collection starts after the IPC from the DOM is fired.
   std::vector<mojom::ThreatDOMDetailsNodePtr> params;

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:
-  ThreatDetails* CreateThreatDetails(
+  scoped_refptr<ThreatDetails> CreateThreatDetails(
       BaseUIManager* ui_manager,
       WebContents* web_contents,
       const security_interstitials::UnsafeResource& unsafe_resource,
@@ -280,10 +280,12 @@ class ThreatDetailsFactoryImpl : public ThreatDetailsFactory {
       ReferrerChainProvider* referrer_chain_provider,
       bool trim_to_ad_tags,
       ThreatDetailsDoneCallback done_callback) override {
-    return new ThreatDetails(ui_manager, web_contents, unsafe_resource,
-                             url_loader_factory, history_service,
-                             referrer_chain_provider, trim_to_ad_tags,
-                             done_callback);
+    auto threat_details = base::WrapRefCounted(new ThreatDetails(
+        ui_manager, web_contents, unsafe_resource, url_loader_factory,
+        history_service, referrer_chain_provider, trim_to_ad_tags,
+        done_callback));
+    threat_details->StartCollection();
+    return threat_details;
   }
 
  private:
@@ -299,7 +301,7 @@ static base::LazyInstance<ThreatDetailsFactoryImpl>::DestructorAtExit
 
 // Create a ThreatDetails for the given tab.
 /* static */
-ThreatDetails* ThreatDetails::NewThreatDetails(
+scoped_refptr<ThreatDetails> ThreatDetails::NewThreatDetails(
     BaseUIManager* ui_manager,
     WebContents* web_contents,
     const UnsafeResource& resource,
@@ -344,7 +346,6 @@ ThreatDetails::ThreatDetails(
   redirects_collector_ = new ThreatDetailsRedirectsCollector(
       history_service ? history_service->AsWeakPtr()
                       : base::WeakPtr<history::HistoryService>());
-  StartCollection();
 }
 
 // TODO(lpz): Consider making this constructor delegate to the parameterized one

components/safe_browsing/browser/threat_details.h

@@ -73,7 +73,7 @@ class ThreatDetails : public base::RefCounted<ThreatDetails>,
   typedef security_interstitials::UnsafeResource UnsafeResource;
 
   // Constructs a new ThreatDetails instance, using the factory.
-  static ThreatDetails* NewThreatDetails(
+  static scoped_refptr<ThreatDetails> NewThreatDetails(
       BaseUIManager* ui_manager,
       content::WebContents* web_contents,
       const UnsafeResource& resource,
@@ -107,6 +107,7 @@ class ThreatDetails : public base::RefCounted<ThreatDetails>,
  protected:
   friend class ThreatDetailsFactoryImpl;
   friend class TestThreatDetailsFactory;
+  friend class ThreatDetailsTest;
 
   ThreatDetails(
       BaseUIManager* ui_manager,
@@ -133,12 +134,12 @@ class ThreatDetails : public base::RefCounted<ThreatDetails>,
   // Used to get a pointer to the HTTP cache.
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
 
- private:
-  friend class base::RefCounted<ThreatDetails>;
-
   // Starts the collection of the report.
   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;
 
@@ -286,7 +287,7 @@ class ThreatDetailsFactory {
  public:
   virtual ~ThreatDetailsFactory() {}
 
-  virtual ThreatDetails* CreateThreatDetails(
+  virtual scoped_refptr<ThreatDetails> CreateThreatDetails(
       BaseUIManager* ui_manager,
       content::WebContents* web_contents,
       const security_interstitials::UnsafeResource& unsafe_resource,

components/safe_browsing/triggers/trigger_manager_unittest.cc

@@ -40,7 +40,7 @@ class MockThreatDetailsFactory : public ThreatDetailsFactory {
  public:
   ~MockThreatDetailsFactory() override {}
 
-  ThreatDetails* CreateThreatDetails(
+  scoped_refptr<ThreatDetails> CreateThreatDetails(
       BaseUIManager* ui_manager,
       content::WebContents* web_contents,
       const security_interstitials::UnsafeResource& unsafe_resource,
@@ -49,8 +49,7 @@ class MockThreatDetailsFactory : public ThreatDetailsFactory {
       ReferrerChainProvider* referrer_chain_provider,
       bool trim_to_ad_tags,
       ThreatDetailsDoneCallback done_callback) override {
-    MockThreatDetails* threat_details = new MockThreatDetails();
-    return threat_details;
+    return base::MakeRefCounted<MockThreatDetails>();
   }
 };