Add a callback from ThreatDetails to TriggerManager.

It indicates that the report is finished and TriggerManager can release
references to ThreatDetails. This will ensure ThreatDetails lives long
enough to finish its work.

Bug: 776149,777915
Change-Id: I1a1a5876edb51420889a91aa48833bf99d81a217
Reviewed-on: https://chromium-review.googlesource.com/738290
Commit-Queue: Luke Z <lpz@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512917}

chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc

@@ -248,10 +248,11 @@ class TestThreatDetailsFactory : public ThreatDetailsFactory {
       const security_interstitials::UnsafeResource& unsafe_resource,
       net::URLRequestContextGetter* request_context_getter,
       history::HistoryService* history_service,
-      bool trim_to_ad_tags) override {
+      bool trim_to_ad_tags,
+      ThreatDetailsDoneCallback done_callback) override {
     details_ = new ThreatDetails(delegate, web_contents, unsafe_resource,
                                  request_context_getter, history_service,
-                                 trim_to_ad_tags);
+                                 trim_to_ad_tags, done_callback);
     return details_;
   }
 

chrome/browser/safe_browsing/trigger_creator.cc

@@ -30,13 +30,19 @@ void TriggerCreator::MaybeCreateTriggersForWebContents(
     return;
   }
 
+  TriggerManager* trigger_manager =
+      g_browser_process->safe_browsing_service()->trigger_manager();
+
+  // Create the helper for listening to events for this webcontents, created for
+  // all tabs since reports could be triggered from other places besides here.
+  safe_browsing::TriggerManagerWebContentsHelper::CreateForWebContents(
+      web_contents, trigger_manager);
+
   // We only start triggers for this tab if they are eligible to collect data
   // (eg: because of opt-ins, available quota, etc). If we skip a trigger but
   // later opt-in changes or quota becomes available, the trigger won't be
   // running on old tabs, but that's acceptable. The trigger will be started for
   // new tabs.
-  TriggerManager* trigger_manager =
-      g_browser_process->safe_browsing_service()->trigger_manager();
   SBErrorOptions options = TriggerManager::GetSBErrorDisplayOptions(
       *profile->GetPrefs(), *web_contents);
   if (trigger_manager->CanStartDataCollection(options,

components/safe_browsing/browser/threat_details.cc

@@ -269,10 +269,11 @@ class ThreatDetailsFactoryImpl : public ThreatDetailsFactory {
       const security_interstitials::UnsafeResource& unsafe_resource,
       net::URLRequestContextGetter* request_context_getter,
       history::HistoryService* history_service,
-      bool trim_to_ad_tags) override {
+      bool trim_to_ad_tags,
+      ThreatDetailsDoneCallback done_callback) override {
     return new ThreatDetails(ui_manager, web_contents, unsafe_resource,
                              request_context_getter, history_service,
-                             trim_to_ad_tags);
+                             trim_to_ad_tags, done_callback);
   }
 
  private:
@@ -294,14 +295,15 @@ ThreatDetails* ThreatDetails::NewThreatDetails(
     const UnsafeResource& resource,
     net::URLRequestContextGetter* request_context_getter,
     history::HistoryService* history_service,
-    bool trim_to_ad_tags) {
+    bool trim_to_ad_tags,
+    ThreatDetailsDoneCallback done_callback) {
   // Set up the factory if this has not been done already (tests do that
   // before this method is called).
   if (!factory_)
     factory_ = g_threat_details_factory_impl.Pointer();
   return factory_->CreateThreatDetails(ui_manager, web_contents, resource,
                                        request_context_getter, history_service,
-                                       trim_to_ad_tags);
+                                       trim_to_ad_tags, done_callback);
 }
 
 // Create a ThreatDetails for the given tab. Runs in the UI thread.
@@ -311,7 +313,8 @@ ThreatDetails::ThreatDetails(
     const UnsafeResource& resource,
     net::URLRequestContextGetter* request_context_getter,
     history::HistoryService* history_service,
-    bool trim_to_ad_tags)
+    bool trim_to_ad_tags,
+    ThreatDetailsDoneCallback done_callback)
     : content::WebContentsObserver(web_contents),
       request_context_getter_(request_context_getter),
       ui_manager_(ui_manager),
@@ -321,7 +324,10 @@ ThreatDetails::ThreatDetails(
       num_visits_(0),
       ambiguous_dom_(false),
       trim_to_ad_tags_(trim_to_ad_tags),
-      cache_collector_(new ThreatDetailsCacheCollector) {
+      cache_collector_(new ThreatDetailsCacheCollector),
+      done_callback_(done_callback),
+      all_done_expected_(false),
+      is_all_done_(false) {
   redirects_collector_ = new ThreatDetailsRedirectsCollector(
       history_service ? history_service->AsWeakPtr()
                       : base::WeakPtr<history::HistoryService>());
@@ -335,9 +341,14 @@ ThreatDetails::ThreatDetails()
       did_proceed_(false),
       num_visits_(0),
       ambiguous_dom_(false),
-      trim_to_ad_tags_(false) {}
+      trim_to_ad_tags_(false),
+      done_callback_(nullptr),
+      all_done_expected_(false),
+      is_all_done_(false) {}
 
-ThreatDetails::~ThreatDetails() {}
+ThreatDetails::~ThreatDetails() {
+  DCHECK(all_done_expected_ == is_all_done_);
+}
 
 bool ThreatDetails::OnMessageReceived(const IPC::Message& message,
                                       RenderFrameHost* render_frame_host) {
@@ -646,6 +657,8 @@ void ThreatDetails::AddDOMDetails(
 void ThreatDetails::FinishCollection(bool did_proceed, int num_visit) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
+  all_done_expected_ = true;
+
   // Do a second pass over the elements and update iframe elements to have
   // references to their children. Children may have been received from a
   // different renderer than the iframe element.
@@ -739,14 +752,17 @@ void ThreatDetails::OnCacheCollectionReady() {
   std::string serialized;
   if (!report_->SerializeToString(&serialized)) {
     DLOG(ERROR) << "Unable to serialize the threat report.";
+    AllDone();
     return;
   }
 
   // For measuring performance impact of ad sampling reports, we may want to
   // do all the heavy lifting of creating the report but not actually send it.
   if (report_->type() == ClientSafeBrowsingReportRequest::AD_SAMPLE &&
-      base::FeatureList::IsEnabled(kAdSamplerCollectButDontSendFeature))
+      base::FeatureList::IsEnabled(kAdSamplerCollectButDontSendFeature)) {
+    AllDone();
     return;
+  }
 
   BrowserThread::PostTask(
       content::BrowserThread::UI, FROM_HERE,
@@ -754,6 +770,14 @@ void ThreatDetails::OnCacheCollectionReady() {
                  base::Unretained(WebUIInfoSingleton::GetInstance()),
                  base::Passed(&report_)));
   ui_manager_->SendSerializedThreatDetails(serialized);
+
+  AllDone();
 }
 
+void ThreatDetails::AllDone() {
+  is_all_done_ = true;
+  BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(done_callback_, base::Unretained(web_contents())));
+}
 }  // namespace safe_browsing

components/safe_browsing/browser/threat_details.h

@@ -64,6 +64,10 @@ using KeyToFrameTreeIdMap = base::hash_map<std::string, int>;
 // the Element IDs of the top-level HTML Elements in this frame.
 using FrameTreeIdToChildIdsMap = base::hash_map<int, std::unordered_set<int>>;
 
+// Callback used to notify a caller that ThreatDetails has finished creating and
+// sending a report.
+using ThreatDetailsDoneCallback = base::Callback<void(content::WebContents*)>;
+
 class ThreatDetails : public base::RefCountedThreadSafe<
                           ThreatDetails,
                           content::BrowserThread::DeleteOnUIThread>,
@@ -78,7 +82,8 @@ class ThreatDetails : public base::RefCountedThreadSafe<
       const UnsafeResource& resource,
       net::URLRequestContextGetter* request_context_getter,
       history::HistoryService* history_service,
-      bool trim_to_ad_tags);
+      bool trim_to_ad_tags,
+      ThreatDetailsDoneCallback done_callback);
 
   // Makes the passed |factory| the factory used to instantiate
   // SafeBrowsingBlockingPage objects. Useful for tests.
@@ -111,7 +116,9 @@ class ThreatDetails : public base::RefCountedThreadSafe<
                 const UnsafeResource& resource,
                 net::URLRequestContextGetter* request_context_getter,
                 history::HistoryService* history_service,
-                bool trim_to_ad_tags);
+                bool trim_to_ad_tags,
+                ThreatDetailsDoneCallback done_callback);
+
   // Default constructor for testing only.
   ThreatDetails();
 
@@ -179,6 +186,9 @@ class ThreatDetails : public base::RefCountedThreadSafe<
                      const std::vector<AttributeNameValue>& attributes,
                      const ClientSafeBrowsingReportRequest::Resource* resource);
 
+  // Called when the report is complete. Runs |done_callback_|.
+  void AllDone();
+
   scoped_refptr<BaseUIManager> ui_manager_;
 
   const UnsafeResource resource_;
@@ -240,6 +250,16 @@ class ThreatDetails : public base::RefCountedThreadSafe<
   // Used to collect redirect urls from the history service
   scoped_refptr<ThreatDetailsRedirectsCollector> redirects_collector_;
 
+  // Callback to run when the report is finished.
+  ThreatDetailsDoneCallback done_callback_;
+
+  // Whether this ThreatDetails has begun finalizing the report and is expected
+  // to invoke |done_callback_| when it finishes.
+  bool all_done_expected_;
+
+  // Whether the |done_callback_| has been invoked.
+  bool is_all_done_;
+
   FRIEND_TEST_ALL_PREFIXES(ThreatDetailsTest, HistoryServiceUrls);
   FRIEND_TEST_ALL_PREFIXES(ThreatDetailsTest, HttpsResourceSanitization);
   FRIEND_TEST_ALL_PREFIXES(ThreatDetailsTest, HTTPCacheNoEntries);
@@ -263,7 +283,8 @@ class ThreatDetailsFactory {
       const security_interstitials::UnsafeResource& unsafe_resource,
       net::URLRequestContextGetter* request_context_getter,
       history::HistoryService* history_service,
-      bool trim_to_ad_tags) = 0;
+      bool trim_to_ad_tags,
+      ThreatDetailsDoneCallback done_callback) = 0;
 };
 
 }  // namespace safe_browsing

components/safe_browsing/triggers/trigger_manager.cc

@@ -14,6 +14,9 @@
 #include "content/public/browser/browser_thread.h"
 #include "net/url_request/url_request_context_getter.h"
 
+DEFINE_WEB_CONTENTS_USER_DATA_KEY(
+    safe_browsing::TriggerManagerWebContentsHelper);
+
 namespace safe_browsing {
 
 namespace {
@@ -83,7 +86,9 @@ DataCollectorsContainer::DataCollectorsContainer() {}
 DataCollectorsContainer::~DataCollectorsContainer() {}
 
 TriggerManager::TriggerManager(BaseUIManager* ui_manager)
-    : ui_manager_(ui_manager), trigger_throttler_(new TriggerThrottler()) {}
+    : ui_manager_(ui_manager),
+      trigger_throttler_(new TriggerThrottler()),
+      weak_factory_(this) {}
 
 TriggerManager::~TriggerManager() {}
 
@@ -151,19 +156,19 @@ bool TriggerManager::StartCollectingThreatDetails(
   if (!CanStartDataCollection(error_display_options, trigger_type))
     return false;
 
-  // Ensure we're not already collecting data on this tab.
-  // TODO(lpz): this check should be more specific to check that this type of
-  // data collector is not running on this tab (once additional data collectors
-  // exist).
-  if (base::ContainsKey(data_collectors_map_, web_contents))
+  // Ensure we're not already collecting ThreatDetails on this tab. Create an
+  // entry in the map for this |web_contents| if it's not there already.
+  DataCollectorsContainer* collectors = &data_collectors_map_[web_contents];
+  if (collectors->threat_details != nullptr)
     return false;
 
-  DataCollectorsContainer* collectors = &data_collectors_map_[web_contents];
   bool should_trim_threat_details = trigger_type == TriggerType::AD_SAMPLE;
   collectors->threat_details =
       scoped_refptr<ThreatDetails>(ThreatDetails::NewThreatDetails(
           ui_manager_, web_contents, resource, request_context_getter,
-          history_service, should_trim_threat_details));
+          history_service, should_trim_threat_details,
+          base::Bind(&TriggerManager::ThreatDetailsDone,
+                     weak_factory_.GetWeakPtr())));
   return true;
 }
 
@@ -175,39 +180,75 @@ bool TriggerManager::FinishCollectingThreatDetails(
     int num_visits,
     const SBErrorOptions& error_display_options) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  // Make sure there's a data collector running on this tab.
-  // TODO(lpz): this check should be more specific to check that the right type
-  // of data collector is running on this tab (once additional data collectors
-  // exist).
+  // Make sure there's a ThreatDetails collector running on this tab.
   if (!base::ContainsKey(data_collectors_map_, web_contents))
     return false;
+  DataCollectorsContainer* collectors = &data_collectors_map_[web_contents];
+  if (collectors->threat_details == nullptr)
+    return false;
 
   // Determine whether a report should be sent.
   bool should_send_report = CanSendReport(error_display_options, trigger_type);
 
-  DataCollectorsContainer* collectors = &data_collectors_map_[web_contents];
-  // Find the data collector and tell it to finish collecting data, and then
-  // remove it from our map. We release ownership of the data collector here but
-  // it will live until the end of the FinishCollection call because it
-  // implements RefCountedThreadSafe.
   if (should_send_report) {
-    scoped_refptr<ThreatDetails> threat_details = collectors->threat_details;
+    // Find the data collector and tell it to finish collecting data. We expect
+    // it to notify us when it's finished so we can clean up references to it.
+
     content::BrowserThread::PostDelayedTask(
         content::BrowserThread::IO, FROM_HERE,
-        base::BindOnce(&ThreatDetails::FinishCollection, threat_details,
-                       did_proceed, num_visits),
+        base::BindOnce(&ThreatDetails::FinishCollection,
+                       collectors->threat_details, did_proceed, num_visits),
         delay);
 
     // Record that this trigger fired and collected data.
     trigger_throttler_->TriggerFired(trigger_type);
+  } else {
+    // We aren't telling ThreatDetails to finish the report so we should clean
+    // up our map ourselves.
+    ThreatDetailsDone(web_contents);
   }
 
-  // Regardless of whether the report got sent, clean up the data collector on
-  // this tab.
+  return should_send_report;
+}
+
+void TriggerManager::ThreatDetailsDone(content::WebContents* web_contents) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  // Clean up the ThreatDetailsdata collector on the specified tab.
+  if (!base::ContainsKey(data_collectors_map_, web_contents))
+    return;
+
+  DataCollectorsContainer* collectors = &data_collectors_map_[web_contents];
   collectors->threat_details = nullptr;
+}
+
+void TriggerManager::WebContentsDestroyed(content::WebContents* web_contents) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  if (!base::ContainsKey(data_collectors_map_, web_contents))
+    return;
   data_collectors_map_.erase(web_contents);
+}
 
-  return should_send_report;
+TriggerManagerWebContentsHelper::TriggerManagerWebContentsHelper(
+    content::WebContents* web_contents,
+    TriggerManager* trigger_manager)
+    : content::WebContentsObserver(web_contents),
+      trigger_manager_(trigger_manager) {}
+
+TriggerManagerWebContentsHelper::~TriggerManagerWebContentsHelper() {}
+
+void TriggerManagerWebContentsHelper::CreateForWebContents(
+    content::WebContents* web_contents,
+    TriggerManager* trigger_manager) {
+  DCHECK(web_contents);
+  if (!FromWebContents(web_contents)) {
+    web_contents->SetUserData(
+        UserDataKey(), base::WrapUnique(new TriggerManagerWebContentsHelper(
+                           web_contents, trigger_manager)));
+  }
+}
+
+void TriggerManagerWebContentsHelper::WebContentsDestroyed() {
+  trigger_manager_->WebContentsDestroyed(web_contents());
 }
 
 }  // namespace safe_browsing

components/safe_browsing/triggers/trigger_manager.h

@@ -13,6 +13,8 @@
 #include "components/security_interstitials/content/unsafe_resource.h"
 #include "components/security_interstitials/core/base_safe_browsing_error_ui.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/browser/web_contents_user_data.h"
 
 class PrefService;
 
@@ -113,6 +115,14 @@ class TriggerManager {
       int num_visits,
       const SBErrorOptions& error_display_options);
 
+  // Called when a ThreatDetails report finishes for the specified
+  // |web_contents|.
+  void ThreatDetailsDone(content::WebContents* web_contents);
+
+  // Called when the specified |web_contents| is being destroyed. Used to clean
+  // up our map.
+  void WebContentsDestroyed(content::WebContents* web_contents);
+
  private:
   friend class TriggerManagerTest;
 
@@ -123,15 +133,46 @@ class TriggerManager {
   // TODO(lpz): we may only need a the PingManager here.
   BaseUIManager* ui_manager_;
 
-  // Map of the data collectors running on each tabs.
+  // Map of the data collectors running on each tabs. New keys are added the
+  // first time any trigger tries to collect data on a tab and are removed when
+  // the tab is destroyed. The values can be null if a trigger has finished on
+  // a tab but the tab remains open.
   DataCollectorsMap data_collectors_map_;
 
   // Keeps track of how often triggers fire and throttles them when needed.
   std::unique_ptr<TriggerThrottler> trigger_throttler_;
 
+  base::WeakPtrFactory<TriggerManager> weak_factory_;
+  // WeakPtrFactory should be last, don't add any members below it.
   DISALLOW_COPY_AND_ASSIGN(TriggerManager);
 };
 
+// A helper class that listens for events happening on a WebContents and can
+// notify TriggerManager of any that are relevant.
+class TriggerManagerWebContentsHelper
+    : public content::WebContentsObserver,
+      public content::WebContentsUserData<TriggerManagerWebContentsHelper> {
+ public:
+  ~TriggerManagerWebContentsHelper() override;
+
+  // Creates a TriggerManagerWebContentsHelper and scopes its lifetime to the
+  // specified |web_contents|.
+  static void CreateForWebContents(content::WebContents* web_contents,
+                                   TriggerManager* trigger_manager);
+
+  // WebContentsObserver implementation.
+  void WebContentsDestroyed() override;
+
+ private:
+  friend class content::WebContentsUserData<TriggerManagerWebContentsHelper>;
+
+  TriggerManagerWebContentsHelper(content::WebContents* web_contents,
+                                  TriggerManager* trigger_manager);
+
+  // Trigger Manager will be notified of any relevant WebContents events.
+  TriggerManager* trigger_manager_;
+};
+
 }  // namespace safe_browsing
 
 #endif  // COMPONENTS_SAFE_BROWSING_TRIGGERS_TRIGGER_MANAGER_H_

components/safe_browsing/triggers/trigger_manager_unittest.cc

@@ -45,7 +45,8 @@ class MockThreatDetailsFactory : public ThreatDetailsFactory {
       const security_interstitials::UnsafeResource& unsafe_resource,
       net::URLRequestContextGetter* request_context_getter,
       history::HistoryService* history_service,
-      bool trim_to_ad_tags) override {
+      bool trim_to_ad_tags,
+      ThreatDetailsDoneCallback done_callback) override {
     MockThreatDetails* threat_details = new MockThreatDetails();
     return threat_details;
   }
@@ -124,8 +125,14 @@ class TriggerManagerTest : public ::testing::Test {
     }
     SBErrorOptions options =
         TriggerManager::GetSBErrorDisplayOptions(pref_service_, *web_contents);
-    return trigger_manager_.FinishCollectingThreatDetails(
+    bool result = trigger_manager_.FinishCollectingThreatDetails(
         trigger_type, web_contents, base::TimeDelta(), false, 0, options);
+
+    // Invoke the callback if the report was to be sent.
+    if (expect_report_sent)
+      trigger_manager_.ThreatDetailsDone(web_contents);
+
+    return result;
   }
 
   const DataCollectorsMap& data_collectors_map() {
@@ -159,9 +166,10 @@ TEST_F(TriggerManagerTest, StartAndFinishCollectingThreatDetails) {
   EXPECT_TRUE(StartCollectingThreatDetails(TriggerType::SECURITY_INTERSTITIAL,
                                            web_contents1));
   EXPECT_THAT(data_collectors_map(), UnorderedElementsAre(Key(web_contents1)));
+  EXPECT_NE(nullptr, data_collectors_map().at(web_contents1).threat_details);
   EXPECT_TRUE(FinishCollectingThreatDetails(TriggerType::SECURITY_INTERSTITIAL,
                                             web_contents1, true));
-  EXPECT_TRUE(data_collectors_map().empty());
+  EXPECT_EQ(nullptr, data_collectors_map().at(web_contents1).threat_details);
 
   // More complex scenarios can happen, where collection happens on two
   // WebContents at the same time, possibly starting and completing in different
@@ -173,36 +181,40 @@ TEST_F(TriggerManagerTest, StartAndFinishCollectingThreatDetails) {
                                            web_contents2));
   EXPECT_THAT(data_collectors_map(),
               UnorderedElementsAre(Key(web_contents1), Key(web_contents2)));
+  EXPECT_NE(nullptr, data_collectors_map().at(web_contents1).threat_details);
+  EXPECT_NE(nullptr, data_collectors_map().at(web_contents2).threat_details);
   EXPECT_TRUE(FinishCollectingThreatDetails(TriggerType::SECURITY_INTERSTITIAL,
                                             web_contents2, true));
-  EXPECT_THAT(data_collectors_map(), UnorderedElementsAre(Key(web_contents1)));
+  EXPECT_NE(nullptr, data_collectors_map().at(web_contents1).threat_details);
+  EXPECT_EQ(nullptr, data_collectors_map().at(web_contents2).threat_details);
   EXPECT_TRUE(FinishCollectingThreatDetails(TriggerType::SECURITY_INTERSTITIAL,
                                             web_contents1, true));
-  EXPECT_TRUE(data_collectors_map().empty());
+  EXPECT_EQ(nullptr, data_collectors_map().at(web_contents1).threat_details);
+  EXPECT_EQ(nullptr, data_collectors_map().at(web_contents2).threat_details);
 
   // Calling Start twice with the same WebContents is an error, and will return
   // false the second time. But it can still be completed normally.
   EXPECT_TRUE(StartCollectingThreatDetails(TriggerType::SECURITY_INTERSTITIAL,
                                            web_contents1));
-  EXPECT_THAT(data_collectors_map(), UnorderedElementsAre(Key(web_contents1)));
+  EXPECT_NE(nullptr, data_collectors_map().at(web_contents1).threat_details);
   EXPECT_FALSE(StartCollectingThreatDetails(TriggerType::SECURITY_INTERSTITIAL,
                                             web_contents1));
-  EXPECT_THAT(data_collectors_map(), UnorderedElementsAre(Key(web_contents1)));
+  EXPECT_NE(nullptr, data_collectors_map().at(web_contents1).threat_details);
   EXPECT_TRUE(FinishCollectingThreatDetails(TriggerType::SECURITY_INTERSTITIAL,
                                             web_contents1, true));
-  EXPECT_TRUE(data_collectors_map().empty());
+  EXPECT_EQ(nullptr, data_collectors_map().at(web_contents1).threat_details);
 
   // Calling Finish twice with the same WebContents is an error, and will return
   // false the second time. It's basically a no-op.
   EXPECT_TRUE(StartCollectingThreatDetails(TriggerType::SECURITY_INTERSTITIAL,
                                            web_contents1));
-  EXPECT_THAT(data_collectors_map(), UnorderedElementsAre(Key(web_contents1)));
+  EXPECT_NE(nullptr, data_collectors_map().at(web_contents1).threat_details);
   EXPECT_TRUE(FinishCollectingThreatDetails(TriggerType::SECURITY_INTERSTITIAL,
                                             web_contents1, true));
-  EXPECT_TRUE(data_collectors_map().empty());
+  EXPECT_EQ(nullptr, data_collectors_map().at(web_contents1).threat_details);
   EXPECT_FALSE(FinishCollectingThreatDetails(TriggerType::SECURITY_INTERSTITIAL,
                                              web_contents1, false));
-  EXPECT_TRUE(data_collectors_map().empty());
+  EXPECT_EQ(nullptr, data_collectors_map().at(web_contents1).threat_details);
 }
 
 TEST_F(TriggerManagerTest, NoDataCollection_Incognito) {
@@ -252,7 +264,7 @@ TEST_F(TriggerManagerTest, UserOptedOutOfSBER_DataCollected_NoReportSent) {
   EXPECT_THAT(data_collectors_map(), UnorderedElementsAre(Key(web_contents)));
   EXPECT_FALSE(FinishCollectingThreatDetails(TriggerType::SECURITY_INTERSTITIAL,
                                              web_contents, false));
-  EXPECT_TRUE(data_collectors_map().empty());
+  EXPECT_EQ(nullptr, data_collectors_map().at(web_contents).threat_details);
 }
 
 TEST_F(TriggerManagerTest, UserOptsOutOfSBER_DataCollected_NoReportSent) {
@@ -268,7 +280,7 @@ TEST_F(TriggerManagerTest, UserOptsOutOfSBER_DataCollected_NoReportSent) {
 
   EXPECT_FALSE(FinishCollectingThreatDetails(TriggerType::SECURITY_INTERSTITIAL,
                                              web_contents, false));
-  EXPECT_TRUE(data_collectors_map().empty());
+  EXPECT_EQ(nullptr, data_collectors_map().at(web_contents).threat_details);
 }
 
 TEST_F(TriggerManagerTest, UserOptsInToSBER_DataCollected_ReportSent) {
@@ -285,7 +297,7 @@ TEST_F(TriggerManagerTest, UserOptsInToSBER_DataCollected_ReportSent) {
 
   EXPECT_TRUE(FinishCollectingThreatDetails(TriggerType::SECURITY_INTERSTITIAL,
                                             web_contents, true));
-  EXPECT_TRUE(data_collectors_map().empty());
+  EXPECT_EQ(nullptr, data_collectors_map().at(web_contents).threat_details);
 }
 
 TEST_F(TriggerManagerTest,
@@ -302,7 +314,7 @@ TEST_F(TriggerManagerTest,
 
   EXPECT_FALSE(FinishCollectingThreatDetails(TriggerType::SECURITY_INTERSTITIAL,
                                              web_contents, false));
-  EXPECT_TRUE(data_collectors_map().empty());
+  EXPECT_EQ(nullptr, data_collectors_map().at(web_contents).threat_details);
 }
 
 TEST_F(TriggerManagerTest, NoCollectionWhenOutOfQuota) {
@@ -318,14 +330,14 @@ TEST_F(TriggerManagerTest, NoCollectionWhenOutOfQuota) {
   EXPECT_THAT(data_collectors_map(), UnorderedElementsAre(Key(web_contents)));
   EXPECT_TRUE(FinishCollectingThreatDetails(TriggerType::AD_SAMPLE,
                                             web_contents, true));
-  EXPECT_TRUE(data_collectors_map().empty());
+  EXPECT_EQ(nullptr, data_collectors_map().at(web_contents).threat_details);
 
   // Turn off the AD_SAMPLE trigger inside the throttler, the trigger should no
   // longer be able to fire.
   SetTriggerHasQuota(TriggerType::AD_SAMPLE, false);
   EXPECT_FALSE(
       StartCollectingThreatDetails(TriggerType::AD_SAMPLE, web_contents));
-  EXPECT_TRUE(data_collectors_map().empty());
+  EXPECT_EQ(nullptr, data_collectors_map().at(web_contents).threat_details);
 }
 
 TEST_F(TriggerManagerTest, AdSamplerTrigger) {
@@ -340,7 +352,7 @@ TEST_F(TriggerManagerTest, AdSamplerTrigger) {
   EXPECT_THAT(data_collectors_map(), UnorderedElementsAre(Key(web_contents)));
   EXPECT_TRUE(FinishCollectingThreatDetails(TriggerType::AD_SAMPLE,
                                             web_contents, true));
-  EXPECT_TRUE(data_collectors_map().empty());
+  EXPECT_EQ(nullptr, data_collectors_map().at(web_contents).threat_details);
 
   // Disabling SBEROptInAllowed disables this trigger.
   SetPref(prefs::kSafeBrowsingExtendedReportingOptInAllowed, false);