Make BrowsingDataRemover (mostly) use OnceCallbacks instead of Callbacks

Also clean up the code a little in the process, getting rid of
differentiated trackers for each subtask.

Bug: 714018
Change-Id: Ibe2f20ef7748a82a10f284cb346b1614c80f394a
Reviewed-on: https://chromium-review.googlesource.com/833247Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525193}

chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc

@@ -392,7 +392,7 @@ void ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData(
     int remove_mask,
     const BrowsingDataFilterBuilder& filter_builder,
     int origin_type_mask,
-    const base::Closure& callback) {
+    base::OnceClosure callback) {
   DCHECK(((remove_mask & ~FILTERABLE_DATA_TYPES) == 0) ||
          filter_builder.IsEmptyBlacklist());
 
@@ -433,7 +433,7 @@ void ChromeBrowsingDataRemoverDelegate::RemoveEmbedderData(
   //////////////////////////////////////////////////////////////////////////////
   // INITIALIZATION
   synchronous_clear_operations_.Start();
-  callback_ = callback;
+  callback_ = std::move(callback);
 
   delete_begin_ = delete_begin;
   delete_end_ = delete_end;
@@ -1130,7 +1130,7 @@ void ChromeBrowsingDataRemoverDelegate::NotifyIfDone() {
     return;
 
   DCHECK(!callback_.is_null());
-  callback_.Run();
+  std::move(callback_).Run();
 }
 
 bool ChromeBrowsingDataRemoverDelegate::AllDone() {

chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.h

@@ -188,7 +188,7 @@ class ChromeBrowsingDataRemoverDelegate
       int remove_mask,
       const content::BrowsingDataFilterBuilder& filter_builder,
       int origin_type_mask,
-      const base::Closure& callback) override;
+      base::OnceClosure callback) override;
 
 #if defined(OS_ANDROID)
   void OverrideWebappRegistryForTesting(
@@ -246,7 +246,7 @@ class ChromeBrowsingDataRemoverDelegate
   base::Time delete_end_;
 
   // Completion callback to call when all data are deleted.
-  base::Closure callback_;
+  base::OnceClosure callback_;
 
   // A callback to NotifyIfDone() used by SubTasks instances.
   const base::Closure sub_task_forward_callback_;

content/browser/browsing_data/browsing_data_remover_impl.cc

@@ -105,7 +105,7 @@ void ClearHttpAuthCacheOnIOThread(
 }
 
 void OnClearedChannelIDsOnIOThread(net::URLRequestContextGetter* rq_context,
-                                   const base::Closure& callback) {
+                                   base::OnceClosure callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   // Need to close open SSL connections which may be using the channel ids we
@@ -115,69 +115,33 @@ void OnClearedChannelIDsOnIOThread(net::URLRequestContextGetter* rq_context,
   rq_context->GetURLRequestContext()
       ->ssl_config_service()
       ->NotifySSLConfigChange();
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, callback);
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, std::move(callback));
 }
 
 void ClearChannelIDsOnIOThread(
     const base::Callback<bool(const std::string&)>& domain_predicate,
     base::Time delete_begin,
     base::Time delete_end,
-    scoped_refptr<net::URLRequestContextGetter> rq_context,
-    const base::Closure& callback) {
+    scoped_refptr<net::URLRequestContextGetter> request_context,
+    base::OnceClosure callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   net::ChannelIDService* channel_id_service =
-      rq_context->GetURLRequestContext()->channel_id_service();
+      request_context->GetURLRequestContext()->channel_id_service();
   channel_id_service->GetChannelIDStore()->DeleteForDomainsCreatedBetween(
       domain_predicate, delete_begin, delete_end,
       base::Bind(&OnClearedChannelIDsOnIOThread,
-                 base::RetainedRef(std::move(rq_context)), callback));
+                 base::RetainedRef(std::move(request_context)),
+                 base::Passed(std::move(callback))));
 }
 
 }  // namespace
 
-BrowsingDataRemoverImpl::SubTask::SubTask(const base::Closure& forward_callback)
-    : is_pending_(false),
-      forward_callback_(forward_callback),
-      weak_ptr_factory_(this) {
-  DCHECK(!forward_callback_.is_null());
-}
-
-BrowsingDataRemoverImpl::SubTask::~SubTask() {}
-
-void BrowsingDataRemoverImpl::SubTask::Start() {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK(!is_pending_);
-  is_pending_ = true;
-}
-
-base::Closure BrowsingDataRemoverImpl::SubTask::GetCompletionCallback() {
-  return base::Bind(&BrowsingDataRemoverImpl::SubTask::CompletionCallback,
-                    weak_ptr_factory_.GetWeakPtr());
-}
-
-void BrowsingDataRemoverImpl::SubTask::CompletionCallback() {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK(is_pending_);
-  is_pending_ = false;
-  forward_callback_.Run();
-}
-
 BrowsingDataRemoverImpl::BrowsingDataRemoverImpl(
     BrowserContext* browser_context)
     : browser_context_(browser_context),
       remove_mask_(-1),
       origin_type_mask_(-1),
       is_removing_(false),
-      sub_task_forward_callback_(
-          base::Bind(&BrowsingDataRemoverImpl::NotifyIfDone,
-                     base::Unretained(this))),
-      synchronous_clear_operations_(sub_task_forward_callback_),
-      clear_embedder_data_(sub_task_forward_callback_),
-      clear_cache_(sub_task_forward_callback_),
-      clear_networking_history_(sub_task_forward_callback_),
-      clear_channel_ids_(sub_task_forward_callback_),
-      clear_http_auth_cache_(sub_task_forward_callback_),
-      clear_storage_partition_data_(sub_task_forward_callback_),
       storage_partition_for_testing_(nullptr),
       weak_ptr_factory_(this) {
   DCHECK(browser_context_);
@@ -324,7 +288,8 @@ void BrowsingDataRemoverImpl::RemoveImpl(
   // 3. Do not support partial deletion, i.e. only delete your data if
   //    |filter_builder.IsEmptyBlacklist()|. Add a comment explaining why this
   //    is acceptable.
-  synchronous_clear_operations_.Start();
+  base::OnceClosure synchronous_clear_operations(
+      CreatePendingTaskCompletionClosure());
 
   // crbug.com/140910: Many places were calling this with base::Time() as
   // delete_end, even though they should've used base::Time::Max().
@@ -373,16 +338,15 @@ void BrowsingDataRemoverImpl::RemoveImpl(
       origin_type_mask_ & ORIGIN_TYPE_UNPROTECTED_WEB) {
     base::RecordAction(UserMetricsAction("ClearBrowsingData_ChannelIDs"));
     // Since we are running on the UI thread don't call GetURLRequestContext().
-    scoped_refptr<net::URLRequestContextGetter> rq_context =
+    scoped_refptr<net::URLRequestContextGetter> request_context =
         BrowserContext::GetDefaultStoragePartition(browser_context_)
             ->GetURLRequestContext();
-    clear_channel_ids_.Start();
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE,
         base::BindOnce(&ClearChannelIDsOnIOThread,
                        filter_builder.BuildChannelIDFilter(), delete_begin_,
-                       delete_end_, std::move(rq_context),
-                       clear_channel_ids_.GetCompletionCallback()));
+                       delete_end_, std::move(request_context),
+                       CreatePendingTaskCompletionClosure()));
   }
 
   //////////////////////////////////////////////////////////////////////////////
@@ -443,8 +407,6 @@ void BrowsingDataRemoverImpl::RemoveImpl(
   }
 
   if (storage_partition_remove_mask) {
-    clear_storage_partition_data_.Start();
-
     uint32_t quota_storage_remove_mask =
         ~StoragePartition::QUOTA_MANAGED_STORAGE_MASK_PERSISTENT;
 
@@ -471,10 +433,10 @@ void BrowsingDataRemoverImpl::RemoveImpl(
 
     storage_partition->ClearData(
         storage_partition_remove_mask, quota_storage_remove_mask,
-        base::Bind(&DoesOriginMatchMaskAndURLs, origin_type_mask_, filter,
-                   embedder_matcher),
+        base::BindRepeating(&DoesOriginMatchMaskAndURLs, origin_type_mask_,
+                            filter, embedder_matcher),
         cookie_matcher, delete_begin_, delete_end_,
-        clear_storage_partition_data_.GetCompletionCallback());
+        CreatePendingTaskCompletionClosure());
   }
 
   //////////////////////////////////////////////////////////////////////////////
@@ -484,18 +446,16 @@ void BrowsingDataRemoverImpl::RemoveImpl(
 
     // TODO(msramek): Clear the cache of all renderers.
 
-    clear_cache_.Start();
     storage_partition->ClearHttpAndMediaCaches(
         delete_begin, delete_end,
         filter_builder.IsEmptyBlacklist() ? base::Callback<bool(const GURL&)>()
                                           : filter,
-        clear_cache_.GetCompletionCallback());
+        CreatePendingTaskCompletionClosure());
 
     // When clearing cache, wipe accumulated network related data
     // (TransportSecurityState and HttpServerPropertiesManager data).
-    clear_networking_history_.Start();
     storage_partition->GetNetworkContext()->ClearNetworkingHistorySince(
-        delete_begin, clear_networking_history_.GetCompletionCallback());
+        delete_begin, CreatePendingTaskCompletionClosure());
 
     // Tell the shader disk cache to clear.
     base::RecordAction(UserMetricsAction("ClearBrowsingData_ShaderCache"));
@@ -509,25 +469,23 @@ void BrowsingDataRemoverImpl::RemoveImpl(
     scoped_refptr<net::URLRequestContextGetter> request_context =
         BrowserContext::GetDefaultStoragePartition(browser_context_)
             ->GetURLRequestContext();
-    clear_http_auth_cache_.Start();
     BrowserThread::PostTaskAndReply(
         BrowserThread::IO, FROM_HERE,
         base::BindOnce(&ClearHttpAuthCacheOnIOThread,
                        std::move(request_context), delete_begin_),
-        clear_http_auth_cache_.GetCompletionCallback());
+        CreatePendingTaskCompletionClosure());
   }
 
   //////////////////////////////////////////////////////////////////////////////
   // Embedder data.
   if (embedder_delegate_) {
-    clear_embedder_data_.Start();
     embedder_delegate_->RemoveEmbedderData(
         delete_begin_, delete_end_, remove_mask, filter_builder,
-        origin_type_mask, clear_embedder_data_.GetCompletionCallback());
+        origin_type_mask, CreatePendingTaskCompletionClosure());
   }
 
   // Notify in case all actions taken were synchronous.
-  synchronous_clear_operations_.GetCompletionCallback().Run();
+  std::move(synchronous_clear_operations).Run();
 }
 
 void BrowsingDataRemoverImpl::AddObserver(Observer* observer) {
@@ -584,15 +542,6 @@ BrowsingDataRemoverImpl::RemovalTask::RemovalTask(
 
 BrowsingDataRemoverImpl::RemovalTask::~RemovalTask() {}
 
-bool BrowsingDataRemoverImpl::AllDone() {
-  return !synchronous_clear_operations_.is_pending() &&
-         !clear_embedder_data_.is_pending() && !clear_cache_.is_pending() &&
-         !clear_networking_history_.is_pending() &&
-         !clear_channel_ids_.is_pending() &&
-         !clear_http_auth_cache_.is_pending() &&
-         !clear_storage_partition_data_.is_pending();
-}
-
 void BrowsingDataRemoverImpl::Notify() {
   // Some tests call |RemoveImpl| directly, without using the task scheduler.
   // TODO(msramek): Improve those tests so we don't have to do this. Tests
@@ -629,12 +578,15 @@ void BrowsingDataRemoverImpl::Notify() {
       base::BindOnce(&BrowsingDataRemoverImpl::RunNextTask, GetWeakPtr()));
 }
 
-void BrowsingDataRemoverImpl::NotifyIfDone() {
+void BrowsingDataRemoverImpl::OnTaskComplete() {
   // TODO(brettw) http://crbug.com/305259: This should also observe session
   // clearing (what about other things such as passwords, etc.?) and wait for
   // them to complete before continuing.
 
-  if (!AllDone())
+  DCHECK_GT(num_pending_tasks_, 0);
+  num_pending_tasks_--;
+
+  if (num_pending_tasks_ > 0)
     return;
 
   if (!would_complete_callback_.is_null()) {
@@ -646,6 +598,13 @@ void BrowsingDataRemoverImpl::NotifyIfDone() {
   Notify();
 }
 
+base::OnceClosure
+BrowsingDataRemoverImpl::CreatePendingTaskCompletionClosure() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  num_pending_tasks_++;
+  return base::Bind(&BrowsingDataRemoverImpl::OnTaskComplete, GetWeakPtr());
+}
+
 base::WeakPtr<BrowsingDataRemoverImpl> BrowsingDataRemoverImpl::GetWeakPtr() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   base::WeakPtr<BrowsingDataRemoverImpl> weak_ptr =

content/browser/browsing_data/browsing_data_remover_impl.h

@@ -33,32 +33,6 @@ class CONTENT_EXPORT BrowsingDataRemoverImpl
     : public BrowsingDataRemover,
       public base::SupportsUserData::Data {
  public:
-  // Used to track the deletion of a single data storage backend.
-  class SubTask {
-   public:
-    // Creates a SubTask that calls |forward_callback| when completed.
-    // |forward_callback| is only kept as a reference and must outlive SubTask.
-    explicit SubTask(const base::Closure& forward_callback);
-    ~SubTask();
-
-    // Indicate that the task is in progress and we're waiting.
-    void Start();
-
-    // Returns a callback that should be called to indicate that the task
-    // has been finished.
-    base::Closure GetCompletionCallback();
-
-    // Whether the task is still in progress.
-    bool is_pending() const { return is_pending_; }
-
-   private:
-    void CompletionCallback();
-
-    bool is_pending_;
-    const base::Closure& forward_callback_;
-    base::WeakPtrFactory<SubTask> weak_ptr_factory_;
-  };
-
   explicit BrowsingDataRemoverImpl(BrowserContext* browser_context);
   ~BrowsingDataRemoverImpl() override;
 
@@ -170,11 +144,14 @@ class CONTENT_EXPORT BrowsingDataRemoverImpl
   // Notifies observers and transitions to the idle state.
   void Notify();
 
-  // Checks if we are all done, and if so, calls Notify().
-  void NotifyIfDone();
+  // Called by the closures returned by CreatePendingTaskCompletionClosure().
+  // Checks if all tasks have completed, and if so, calls Notify().
+  void OnTaskComplete();
 
-  // Returns true if we're all done.
-  bool AllDone();
+  // Increments the number of pending tasks by one, and returns a OnceClosure
+  // that calls OnTaskComplete(). The Remover is complete once all the closures
+  // created by this method have been invoked.
+  base::OnceClosure CreatePendingTaskCompletionClosure();
 
   // Like GetWeakPtr(), but returns a weak pointer to BrowsingDataRemoverImpl
   // for internal purposes.
@@ -210,18 +187,7 @@ class CONTENT_EXPORT BrowsingDataRemoverImpl
   base::Callback<void(const base::Closure& continue_to_completion)>
       would_complete_callback_;
 
-  // A callback to NotifyIfDone() used by SubTasks instances.
-  const base::Closure sub_task_forward_callback_;
-
-  // Keeping track of various subtasks to be completed.
-  // These may only be accessed from UI thread in order to avoid races!
-  SubTask synchronous_clear_operations_;
-  SubTask clear_embedder_data_;
-  SubTask clear_cache_;
-  SubTask clear_networking_history_;
-  SubTask clear_channel_ids_;
-  SubTask clear_http_auth_cache_;
-  SubTask clear_storage_partition_data_;
+  int num_pending_tasks_ = 0;
 
   // Observers of the global state and individual tasks.
   base::ObserverList<Observer, true> observer_list_;

content/public/browser/browsing_data_remover_delegate.h

@@ -49,7 +49,7 @@ class BrowsingDataRemoverDelegate {
       int remove_mask,
       const BrowsingDataFilterBuilder& filter_builder,
       int origin_type_mask,
-      const base::Closure& callback) = 0;
+      base::OnceClosure callback) = 0;
 };
 
 }  // namespace content

content/public/test/mock_browsing_data_remover_delegate.cc

@@ -31,11 +31,11 @@ void MockBrowsingDataRemoverDelegate::RemoveEmbedderData(
     int remove_mask,
     const BrowsingDataFilterBuilder& filter_builder,
     int origin_type_mask,
-    const base::Closure& callback) {
+    base::OnceClosure callback) {
   actual_calls_.emplace_back(delete_begin, delete_end, remove_mask,
                              origin_type_mask, filter_builder.Copy(),
                              true /* should_compare_filter */);
-  callback.Run();
+  std::move(callback).Run();
 }
 
 void MockBrowsingDataRemoverDelegate::ExpectCall(

content/public/test/mock_browsing_data_remover_delegate.h

@@ -29,7 +29,7 @@ class MockBrowsingDataRemoverDelegate : public BrowsingDataRemoverDelegate {
                           int remove_mask,
                           const BrowsingDataFilterBuilder& filter_builder,
                           int origin_type_mask,
-                          const base::Closure& callback) override;
+                          base::OnceClosure callback) override;
 
   // Add an expected call for testing.
   void ExpectCall(const base::Time& delete_begin,