Add histograms to monitor ChromeBrowsingDataLifetimeManager deletions

Create BrowsingDataRemoverBrowserTestBase

Bug: 1026442
Change-Id: I05b0ebff060e3114a536146e82716a875485c676
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2524724
Commit-Queue: Yann Dago <ydago@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826826}

chrome/browser/browsing_data/chrome_browsing_data_lifetime_manager.cc

@@ -4,12 +4,18 @@
 
 #include "chrome/browser/browsing_data/chrome_browsing_data_lifetime_manager.h"
 
+#include <algorithm>
+#include <limits>
 #include <string>
+#include <utility>
 
 #include "base/containers/flat_set.h"
+#include "base/location.h"
+#include "base/metrics/histogram_functions.h"
+#include "base/scoped_observation.h"
+#include "base/single_thread_task_runner.h"
 #include "base/task/task_traits.h"
 #include "base/values.h"
-#include "build/build_config.h"
 #include "chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
@@ -29,9 +35,74 @@
 
 namespace {
 
+constexpr int kInitialCleanupDelayInMinutes = 2;
+
 using ScheduledRemovalSettings =
     ChromeBrowsingDataLifetimeManager::ScheduledRemovalSettings;
 
+// An observer of all the browsing data removal tasks that are started by the
+// ChromeBrowsingDataLifetimeManager that records the the tasks starts and
+// completed states as well as their durations.
+class BrowsingDataLifetimeManagerRemoverObserver
+    : public content::BrowsingDataRemover::Observer {
+ public:
+  ~BrowsingDataLifetimeManagerRemoverObserver() override = default;
+
+  // Creates an instance of BrowsingDataLifetimeManagerRemoverObserver that
+  // manages its own lifetime. The instance will be deleted after
+  // |OnBrowsingDataRemoverDone| is called.
+  static content::BrowsingDataRemover::Observer* Create(
+      content::BrowsingDataRemover* remover,
+      bool filterable_deletion) {
+    return new BrowsingDataLifetimeManagerRemoverObserver(remover,
+                                                          filterable_deletion);
+  }
+
+  // content::BrowsingDataRemover::Observer:
+  void OnBrowsingDataRemoverDone(uint64_t failed_data_types) override {
+    base::UmaHistogramMediumTimes(duration_histogram(),
+                                  base::TimeTicks::Now() - start_time_);
+    base::UmaHistogramBoolean(state_histogram(),
+                              /*BooleanStartedCompleted.Completed*/ true);
+    delete this;
+  }
+
+ private:
+  BrowsingDataLifetimeManagerRemoverObserver(
+      content::BrowsingDataRemover* remover,
+      bool filterable_deletion)
+      : start_time_(base::TimeTicks::Now()),
+        filterable_deletion_(filterable_deletion) {
+    browsing_data_remover_observer_.Observe(remover);
+    base::UmaHistogramBoolean(state_histogram(),
+                              /*BooleanStartedCompleted.Started*/ false);
+  }
+
+  const char* duration_histogram() const {
+    static constexpr char kDurationScheduledFilterableDeletion[] =
+        "History.BrowsingDataLifetime.Duration.ScheduledFilterableDeletion";
+    static constexpr char kDurationScheduledUnfilterableDeletion[] =
+        "History.BrowsingDataLifetime.Duration.ScheduledUnfilterableDeletion";
+    return filterable_deletion_ ? kDurationScheduledFilterableDeletion
+                                : kDurationScheduledUnfilterableDeletion;
+  }
+
+  const char* state_histogram() const {
+    static constexpr char kStateScheduledFilterableDeletion[] =
+        "History.BrowsingDataLifetime.State.ScheduledFilterableDeletion";
+    static constexpr char kStateScheduledUnfilterableDeletion[] =
+        "History.BrowsingDataLifetime.State.ScheduledUnfilterableDeletion";
+    return filterable_deletion_ ? kStateScheduledFilterableDeletion
+                                : kStateScheduledUnfilterableDeletion;
+  }
+
+  base::ScopedObservation<content::BrowsingDataRemover,
+                          content::BrowsingDataRemover::Observer>
+      browsing_data_remover_observer_{this};
+  const base::TimeTicks start_time_;
+  const bool filterable_deletion_;
+};
+
 uint64_t GetOriginTypeMask(const base::Value& data_types) {
   uint64_t result = 0;
   for (const auto& data_type : data_types.GetList()) {
@@ -108,8 +179,6 @@ base::flat_set<GURL> GetOpenedUrls(Profile* profile) {
   return result;
 }
 
-int kInitialCleanupDelayInMinutes = 2;
-
 }  // namespace
 
 namespace browsing_data {
@@ -137,15 +206,14 @@ const char kDataTypes[] = "data_types";
 
 ChromeBrowsingDataLifetimeManager::ChromeBrowsingDataLifetimeManager(
     content::BrowserContext* browser_context)
-    : profile_(Profile::FromBrowserContext(browser_context)),
-      browsing_data_remover_observer_(this) {
+    : profile_(Profile::FromBrowserContext(browser_context)) {
   DCHECK(!profile_->IsGuestSession() || profile_->IsOffTheRecord());
   pref_change_registrar_.Init(profile_->GetPrefs());
   pref_change_registrar_.Add(
       browsing_data::prefs::kBrowsingDataLifetime,
       base::BindRepeating(
           &ChromeBrowsingDataLifetimeManager::UpdateScheduledRemovalSettings,
-          weak_ptr_factory_.GetWeakPtr()));
+          base::Unretained(this)));
 
   // When the service is instantiated, wait a few minutes after Chrome startup
   // to start deleting data.
@@ -166,29 +234,18 @@ ChromeBrowsingDataLifetimeManager::~ChromeBrowsingDataLifetimeManager() =
     default;
 
 void ChromeBrowsingDataLifetimeManager::Shutdown() {
-  browsing_data_remover_observer_.RemoveAll();
   pref_change_registrar_.RemoveAll();
   weak_ptr_factory_.InvalidateWeakPtrs();
 }
 
-void ChromeBrowsingDataLifetimeManager::OnBrowsingDataRemoverDone(
-    uint64_t failed_data_types) {
-  // TODO (crbug/2324203): Add histograms to see how many times data deletion
-  // are ran and how long they take.
-}
-
 void ChromeBrowsingDataLifetimeManager::UpdateScheduledRemovalSettings() {
   weak_ptr_factory_.InvalidateWeakPtrs();
   scheduled_removals_settings_ =
       ConvertToScheduledRemovalSettings(profile_->GetPrefs()->GetList(
           browsing_data::prefs::kBrowsingDataLifetime));
-  browsing_data_remover_observer_.RemoveAll();
 
-  if (!scheduled_removals_settings_.empty()) {
-    browsing_data_remover_observer_.Add(
-        content::BrowserContext::GetBrowsingDataRemover(profile_));
+  if (!scheduled_removals_settings_.empty())
     StartScheduledBrowsingDataRemoval();
-  }
 }
 
 void ChromeBrowsingDataLifetimeManager::StartScheduledBrowsingDataRemoval() {
@@ -217,8 +274,10 @@ void ChromeBrowsingDataLifetimeManager::StartScheduledBrowsingDataRemoval() {
       remover->RemoveWithFilterAndReply(
           base::Time::Min(), deletion_end_time, filterable_remove_mask,
           removal_settings.origin_type_mask, std::move(filter_builder),
-          testing_data_remover_observer_ ? testing_data_remover_observer_
-                                         : this);
+          testing_data_remover_observer_
+              ? testing_data_remover_observer_
+              : BrowsingDataLifetimeManagerRemoverObserver::Create(
+                    remover, /*filterable_deletion=*/true));
     }
 
     auto unfilterable_remove_mask =
@@ -228,8 +287,10 @@ void ChromeBrowsingDataLifetimeManager::StartScheduledBrowsingDataRemoval() {
       remover->RemoveAndReply(
           base::Time::Min(), deletion_end_time, unfilterable_remove_mask,
           removal_settings.origin_type_mask,
-          testing_data_remover_observer_ ? testing_data_remover_observer_
-                                         : this);
+          testing_data_remover_observer_
+              ? testing_data_remover_observer_
+              : BrowsingDataLifetimeManagerRemoverObserver::Create(
+                    remover, /*filterable_deletion=*/false));
     }
 
     smallest_time_to_live =

chrome/browser/browsing_data/chrome_browsing_data_lifetime_manager.h

@@ -5,12 +5,12 @@
 #ifndef CHROME_BROWSER_BROWSING_DATA_CHROME_BROWSING_DATA_LIFETIME_MANAGER_H_
 #define CHROME_BROWSER_BROWSING_DATA_CHROME_BROWSING_DATA_LIFETIME_MANAGER_H_
 
+#include <stdint.h>
+#include <utility>
 #include <vector>
 
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
-#include "base/scoped_observer.h"
-#include "base/single_thread_task_runner.h"
 #include "base/time/time.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/prefs/pref_change_registrar.h"
@@ -18,7 +18,6 @@
 
 namespace content {
 class BrowserContext;
-class BrowsingDataRemover;
 }  // namespace content
 
 class Profile;
@@ -46,9 +45,7 @@ extern const char kDataTypes[];
 }  // namespace browsing_data
 
 // Controls the lifetime of some browsing data.
-class ChromeBrowsingDataLifetimeManager
-    : public KeyedService,
-      public content::BrowsingDataRemover::Observer {
+class ChromeBrowsingDataLifetimeManager : public KeyedService {
  public:
   struct ScheduledRemovalSettings {
     uint64_t remove_mask;
@@ -58,20 +55,15 @@ class ChromeBrowsingDataLifetimeManager
 
   explicit ChromeBrowsingDataLifetimeManager(
       content::BrowserContext* browser_context);
-  ~ChromeBrowsingDataLifetimeManager() override;
-
   ChromeBrowsingDataLifetimeManager(const ChromeBrowsingDataLifetimeManager&) =
       delete;
-
   ChromeBrowsingDataLifetimeManager& operator=(
       const ChromeBrowsingDataLifetimeManager&) = delete;
+  ~ChromeBrowsingDataLifetimeManager() override;
 
   // KeyedService:
   void Shutdown() override;
 
-  // content::BrowsingDataRemover::Observer:
-  void OnBrowsingDataRemoverDone(uint64_t failed_data_types) override;
-
   // Sets the end time of the period for which data must be deleted, for all
   // configurations. If this is |end_time_for_testing| has no value, use the
   // computed end time from each configuration.
@@ -94,9 +86,6 @@ class ChromeBrowsingDataLifetimeManager
   Profile* profile_;
   content::BrowsingDataRemover::Observer* testing_data_remover_observer_ =
       nullptr;
-  ScopedObserver<content::BrowsingDataRemover,
-                 content::BrowsingDataRemover::Observer>
-      browsing_data_remover_observer_;
   base::Optional<base::Time> end_time_for_testing_;
   base::WeakPtrFactory<ChromeBrowsingDataLifetimeManager> weak_ptr_factory_{
       this};

chrome/browser/browsing_data/chrome_browsing_data_lifetime_manager_browsertest.cc

@@ -96,6 +96,33 @@ class ChromeBrowsingDataLifetimeManagerTest
   }
 };
 
+IN_PROC_BROWSER_TEST_P(ChromeBrowsingDataLifetimeManagerTest, PrefChange) {
+  static constexpr char kCookiesPref[] =
+      R"([{"time_to_live_in_hours": 1, "data_types":
+      ["cookies_and_other_site_data"]}])";
+  static constexpr char kDownloadHistoryPref[] =
+      R"([{"time_to_live_in_hours": 1, "data_types":["download_history"]}])";
+
+  GURL url = embedded_test_server()->GetURL("/browsing_data/site_data.html");
+  ui_test_utils::NavigateToURL(GetBrowser(), url);
+
+  // Add cookie.
+  SetDataForType("Cookie");
+  EXPECT_TRUE(HasDataForType("Cookie"));
+
+  // Expect that cookies are deleted.
+  ApplyBrowsingDataLifetimeDeletion(kCookiesPref);
+  EXPECT_FALSE(HasDataForType("Cookie"));
+
+  // Download an item.
+  DownloadAnItem();
+  VerifyDownloadCount(1u);
+
+  // Change the pref and verify that download history is deleted.
+  ApplyBrowsingDataLifetimeDeletion(kDownloadHistoryPref);
+  VerifyDownloadCount(0u);
+}
+
 IN_PROC_BROWSER_TEST_P(ChromeBrowsingDataLifetimeManagerTest,
                        ScheduledRemovalDownload) {
   static constexpr char kPref[] =

tools/metrics/histograms/histograms_xml/history/histograms.xml

@@ -37,6 +37,37 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
   </summary>
 </histogram>
 
+<histogram
+    name="History.BrowsingDataLifetime.Duration.Scheduled{DeletionType}Deletion"
+    units="ms" expires_after="2021-11-01">
+  <owner>ydago@chromium.org</owner>
+  <owner>dullweber@chromium.org</owner>
+  <summary>
+    The time that passed while performing a scheduled data deletion using the
+    BrowsingDataSettings policy. The deletion will be for a specific hourly time
+    range, the data types being deleted are {DeletionType}.
+  </summary>
+  <token key="DeletionType">
+    <variant name="Filterable" summary="filterable"/>
+    <variant name="Unfilterable" summary="unfilterable"/>
+  </token>
+</histogram>
+
+<histogram
+    name="History.BrowsingDataLifetime.State.Scheduled{DeletionType}Deletion"
+    enum="BooleanStartedCompleted" expires_after="2021-11-01">
+  <owner>ydago@chromium.org</owner>
+  <owner>dullweber@chromium.org</owner>
+  <summary>
+    The states in which a scheduled deletion of {DeletionType} browsing data
+    went through.
+  </summary>
+  <token key="DeletionType">
+    <variant name="Filterable" summary="filterable"/>
+    <variant name="Unfilterable" summary="unfilterable"/>
+  </token>
+</histogram>
+
 <histogram name="History.ClearBrowsingData.Duration.FilteredDeletion"
     units="ms" expires_after="never">
 <!-- expires-never: tracked as an important privacy metric. -->