Implement a task scheduler for BrowsingDataRemover

chrome/browser/browsing_data/browsing_data_remover.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/browsing_data/browsing_data_remover.h"
 
 #include <map>
 #include <set>
 #include <string>
 
@@ skipping 234 matching lines @@
       domain_predicate, delete_begin, delete_end,
       base::Bind(&OnClearedChannelIDsOnIOThread,
                  base::RetainedRef(std::move(rq_context)), callback));
 }
 
 }  // namespace
 
 BrowsingDataRemover::CompletionInhibitor*
     BrowsingDataRemover::completion_inhibitor_ = nullptr;
 
+BrowsingDataRemover::RemovalTask::RemovalTask(
+    const TimeRange& time_range,
+    int remove_mask,
+    int origin_type_mask,
+    std::unique_ptr<BrowsingDataFilterBuilder> filter_builder,
+    Observer* observer)
+    : time_range(time_range),
+      remove_mask(remove_mask),
+      origin_type_mask(origin_type_mask),
+      filter_builder(std::move(filter_builder)),
+      observer(observer) {}
+
+BrowsingDataRemover::RemovalTask::~RemovalTask() {}
+
+BrowsingDataRemover::RemovalTask::RemovalTask(
+    RemovalTask&& removalTask) = default;
+BrowsingDataRemover::RemovalTask& BrowsingDataRemover::RemovalTask::operator=(
+    RemovalTask&& removalTask) = default;
+
 bool BrowsingDataRemover::TimeRange::operator==(
     const BrowsingDataRemover::TimeRange& other) const {
   return begin == other.begin && end == other.end;
 }
 
 // static
 BrowsingDataRemover::TimeRange BrowsingDataRemover::Unbounded() {
   return TimeRange(base::Time(), base::Time::Max());
 }
 
@@ skipping 25 matching lines @@
     : profile_(Profile::FromBrowserContext(browser_context)),
       is_removing_(false),
 #if BUILDFLAG(ANDROID_JAVA_UI)
       webapp_registry_(new WebappRegistry()),
 #endif
       weak_ptr_factory_(this) {
   DCHECK(browser_context);
 }
 
 BrowsingDataRemover::~BrowsingDataRemover() {
-  // If we are still removing data, notify observers so they can remove
-  // themselves from the observer list.
+  if (!task_queue_.empty()) {
+    VLOG(1) << "BrowsingDataRemover shuts down with " << task_queue_.size()
+            << " pending tasks";
+  }
+
+  // If we are still removing data, notify observers that their task has been
+  // (albeit unsucessfuly) processed, so they can unregister themselves.
   // TODO(bauerb): If it becomes a problem that browsing data might not actually
   // be fully cleared when an observer is notified, add a success flag.
-  if (is_removing_)
-    Notify();
+  while (!task_queue_.empty()) {
+    if (observer_list_.HasObserver(task_queue_.front().observer))
+      task_queue_.front().observer->OnBrowsingDataRemoverDone();
+    task_queue_.pop();
+  }
 }
 
 void BrowsingDataRemover::Shutdown() {
   history_task_tracker_.TryCancelAll();
   template_url_sub_.reset();
 }
 
 void BrowsingDataRemover::SetRemoving(bool is_removing) {
   DCHECK_NE(is_removing_, is_removing);
   is_removing_ = is_removing;
   FOR_EACH_OBSERVER(Observer, observer_list_,
                     OnBrowsingDataRemoving(is_removing));
 }
 
 void BrowsingDataRemover::Remove(const TimeRange& time_range,
                                  int remove_mask,
                                  int origin_type_mask) {
+  RemoveAndReply(time_range, remove_mask, origin_type_mask, nullptr);
+}
+
+void BrowsingDataRemover::RemoveAndReply(
+    const TimeRange& time_range,
+    int remove_mask,
+    int origin_type_mask,
+    Observer* observer) {
   // Any instance of BrowsingDataFilterBuilder that |IsEmptyBlacklist()|
   // is OK to pass here.
-  RegistrableDomainFilterBuilder builder(
-      RegistrableDomainFilterBuilder::BLACKLIST);
-  DCHECK(builder.IsEmptyBlacklist());
-  RemoveImpl(time_range, remove_mask, builder, origin_type_mask);
+  std::unique_ptr<RegistrableDomainFilterBuilder> filter_builder =
+      base::WrapUnique(new RegistrableDomainFilterBuilder(
+          RegistrableDomainFilterBuilder::BLACKLIST));
+  DCHECK(filter_builder->IsEmptyBlacklist());
+  RemoveWithFilterAndReply(time_range, remove_mask, origin_type_mask,
+                           std::move(filter_builder), observer);
 }
 
 void BrowsingDataRemover::RemoveWithFilter(
     const TimeRange& time_range,
     int remove_mask,
     int origin_type_mask,
-    const BrowsingDataFilterBuilder& filter_builder) {
-  RemoveImpl(time_range, remove_mask, filter_builder, origin_type_mask);
+    std::unique_ptr<BrowsingDataFilterBuilder> filter_builder) {
+  RemoveWithFilterAndReply(time_range, remove_mask, origin_type_mask,
+                           std::move(filter_builder), nullptr);
+}
+
+void BrowsingDataRemover::RemoveWithFilterAndReply(
+    const TimeRange& time_range,
+    int remove_mask,
+    int origin_type_mask,
+    std::unique_ptr<BrowsingDataFilterBuilder> filter_builder,
+    Observer* observer) {
+  DCHECK(!observer || observer_list_.HasObserver(observer))

[Bernhard Bauer, 2016/07/26 14:51:38]

If you allow the |observer| to be null, do you even still need the
Remove(WithFilter) methods without reply?

[msramek, 2016/07/28 09:57:42]

All methods can be reduced to the last one, as demonstrated in the test.

Since the usecase with no filter and no observer should be the most common one,
I think it makes sense to make these simpler versions of the method. It's a few
lines of code here and it lowers the footprint of this CL everywhere else, so it
seems preferable to me. But I'm happy to be convinced otherwise :)

[Bernhard Bauer, 2016/08/03 16:42:46]

Hm, could we make one method that takes no observer and one method that takes a
non-null observer (and have both forward to an internal method)? That way
existing callers for the no-observer method don't need to be changed, but we
would catch cases where someone accidentally passes in a null observer.

[msramek, 2016/08/03 21:16:44]

Yep, that makes more sense actually. Done.

+      << "Every observer must register itself (by calling AddObserver()) "
+      << "before observing a removal task.";
+
+  task_queue_.push(RemovalTask(

[Bernhard Bauer, 2016/07/26 14:51:38]

You could use emplace() for that.

[msramek, 2016/07/28 09:57:42]

Done. Nice :)

+      time_range,
+      remove_mask,
+      origin_type_mask,
+      std::move(filter_builder),
+      observer));
+
+  // If this is the only scheduled task, execute it immediately. Otherwise,
+  // it will be automatically executed when all tasks scheduled before it
+  // finish.
+  if (task_queue_.size() == 1) {
+    SetRemoving(true);
+    RunNextTask();
+  }
+}
+
+void BrowsingDataRemover::RunNextTask() {
+  DCHECK(!task_queue_.empty());
+  const RemovalTask& removal_task = task_queue_.front();
+
+  RemoveImpl(removal_task.time_range,
+             removal_task.remove_mask,
+             *removal_task.filter_builder,
+             removal_task.origin_type_mask);
 }
 
 void BrowsingDataRemover::RemoveImpl(
     const TimeRange& time_range,
     int remove_mask,
     const BrowsingDataFilterBuilder& filter_builder,
     int origin_type_mask) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   waiting_for_synchronous_clear_operations_ = true;
 
   // crbug.com/140910: Many places were calling this with base::Time() as
   // delete_end, even though they should've used base::Time::Max().
   DCHECK_NE(base::Time(), time_range.end);
 
-  SetRemoving(true);
   delete_begin_ = time_range.begin;
   delete_end_ = time_range.end;
   remove_mask_ = remove_mask;
   origin_type_mask_ = origin_type_mask;
 
   base::Callback<bool(const GURL& url)> filter =
       filter_builder.BuildGeneralFilter();
   base::Callback<bool(const ContentSettingsPattern& url)> same_pattern_filter =
       filter_builder.BuildWebsiteSettingsPatternMatchesFilter();
 
@@ skipping 737 matching lines @@
   // else notifies observers and deletes this BrowsingDataRemover.
   TemplateURLService* model =
       TemplateURLServiceFactory::GetForProfile(profile_);
   model->RemoveAutoGeneratedBetween(delete_begin_, delete_end_);
   waiting_for_clear_keyword_data_ = false;
   template_url_sub_.reset();
   NotifyIfDone();
 }
 
 void BrowsingDataRemover::Notify() {
-  SetRemoving(false);
-  FOR_EACH_OBSERVER(Observer, observer_list_, OnBrowsingDataRemoverDone());
+  // Some tests call |RemoveImpl| directly, without using the task scheduler.
+  // TODO(msramek): Improve those tests so we don't have to do this. Tests
+  // relying on |RemoveImpl| do so because they need to pass in
+  // BrowsingDataFilterBuilder while still keeping ownership of it. Making
+  // BrowsingDataFilterBuilder copyable would solve this.
+  if (!is_removing_) {
+    DCHECK(task_queue_.empty());
+    return;
+  }
+
+  // Inform the observer of the current task unless it has unregistered
+  // itself in the meantime.
+  DCHECK(!task_queue_.empty());
+
+  if (task_queue_.front().observer != nullptr &&
+      observer_list_.HasObserver(task_queue_.front().observer)) {
+    task_queue_.front().observer->OnBrowsingDataRemoverDone();
+  }
+
+  task_queue_.pop();
+
+  if (task_queue_.empty()) {
+    // All removal tasks have finished. Inform the observers that we're idle.
+    SetRemoving(false);
+    return;
+  }
+
+  // Yield to the UI thread before executing the next removal task.
+  // TODO(msramek): Consider also adding a backoff if too many tasks
+  // are scheduled.
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&BrowsingDataRemover::RunNextTask,
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
 void BrowsingDataRemover::NotifyIfDone() {
   // 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())
     return;
 
@@ skipping 168 matching lines @@
   waiting_for_clear_offline_page_data_ = false;
   NotifyIfDone();
 }
 #endif
 
 void BrowsingDataRemover::OnClearedDomainReliabilityMonitor() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   waiting_for_clear_domain_reliability_monitor_ = false;
   NotifyIfDone();
 }