Formalize the restrictions of BrowsingDataRemover::RemoveWithFilter()

chrome/browser/android/preferences/pref_service_bridge.cc

Index: chrome/browser/android/preferences/pref_service_bridge.cc
diff --git a/chrome/browser/android/preferences/pref_service_bridge.cc b/chrome/browser/android/preferences/pref_service_bridge.cc
index c83f95de18782ed07a15a98be04fc04e8b61e026..426aaeaebd1353deba735d8d1455e4e16264988d 100644
--- a/chrome/browser/android/preferences/pref_service_bridge.cc
+++ b/chrome/browser/android/preferences/pref_service_bridge.cc
@@ -513,19 +513,28 @@ static ScopedJavaLocalRef<jstring> GetLatestVersionWhenClickedUpdateMenuItem(
 
 namespace {
 
-// Redirects a BrowsingDataRemover completion callback back into Java.
+// Merges |task_count| BrowsingDataRemover completion callbacks and redirects
+// them back into Java.
 class ClearBrowsingDataObserver : public BrowsingDataRemover::Observer {
  public:
   // |obj| is expected to be the object passed into ClearBrowsingData(); e.g. a
   // ChromePreference.
   ClearBrowsingDataObserver(JNIEnv* env,
                             jobject obj,
-                            BrowsingDataRemover* browsing_data_remover)
-      : weak_chrome_native_preferences_(env, obj), observer_(this) {
+                            BrowsingDataRemover* browsing_data_remover,
+                            int task_count)
+      : task_count_(task_count),
+        weak_chrome_native_preferences_(env, obj),
+        observer_(this) {
+    DCHECK_GT(task_count, 0);
     observer_.Add(browsing_data_remover);
   }
 
   void OnBrowsingDataRemoverDone() override {
+    DCHECK(task_count_);
+    if (--task_count_)
+      return;
+
     // We delete ourselves when done.
     std::unique_ptr<ClearBrowsingDataObserver> auto_delete(this);
 
@@ -538,6 +547,7 @@ class ClearBrowsingDataObserver : public BrowsingDataRemover::Observer {
   }
 
  private:
+  int task_count_;
   JavaObjectWeakGlobalRef weak_chrome_native_preferences_;
   ScopedObserver<BrowsingDataRemover, BrowsingDataRemover::Observer> observer_;
 };
@@ -650,16 +660,38 @@ static void ClearBrowsingData(
                                                          excluding_domains);
   }
 
+  // Delete the filterable types with a filter, and the rest completely.
+  // TODO(msramek): This is only necessary until the filter is implemented in
+  // all data storage backends.
+  int filterable_mask = remove_mask & BrowsingDataRemover::FILTERABLE_DATATYPES;
+  int nonfilterable_mask =
+      remove_mask & ~BrowsingDataRemover::FILTERABLE_DATATYPES;
+
   // ClearBrowsingDataObserver deletes itself when |browsing_data_remover| is
-  // done.
-  ClearBrowsingDataObserver* observer =
-      new ClearBrowsingDataObserver(env, obj, browsing_data_remover);
-
-  browsing_data_remover->RemoveWithFilterAndReply(
-      BrowsingDataRemover::Period(
-          static_cast<browsing_data::TimePeriod>(time_period)),
-      remove_mask, BrowsingDataHelper::UNPROTECTED_WEB,
-      std::move(filter_builder), observer);
+  // done with both removal tasks.
+  ClearBrowsingDataObserver* observer = new ClearBrowsingDataObserver(
+      env, obj, browsing_data_remover, 2 /* tasks_count */);

[Bernhard Bauer, 2016/08/11 15:16:16]

I was actually thinking to increase the count for each of the masks that is
non-zero, but this works too (and it avoids the edge case where both are zero).

[msramek, 2016/08/11 15:36:31]

I see. Yes, I was exactly trying to avoid that edge case so that it's very
obvious that |observer| won't leak.

The current solution allows you to quickly convince yourself about it - we
initialize it to 2, and on the next 20 lines it's clear that all execution paths
lead to 2 callbacks.

+
+  if (filterable_mask) {
+    browsing_data_remover->RemoveWithFilterAndReply(
+        BrowsingDataRemover::Period(
+            static_cast<browsing_data::TimePeriod>(time_period)),
+        filterable_mask, BrowsingDataHelper::UNPROTECTED_WEB,
+        std::move(filter_builder), observer);
+  } else {
+    // Make sure |observer| doesn't wait for the filtered task.
+    observer->OnBrowsingDataRemoverDone();
+  }
+
+  if (nonfilterable_mask) {
+    browsing_data_remover->RemoveAndReply(
+        BrowsingDataRemover::Period(
+            static_cast<browsing_data::TimePeriod>(time_period)),
+        nonfilterable_mask, BrowsingDataHelper::UNPROTECTED_WEB, observer);
+  } else {
+    // Make sure |observer| doesn't wait for the non-filtered task.
+    observer->OnBrowsingDataRemoverDone();
+  }
 }
 
 static jboolean CanDeleteBrowsingHistory(JNIEnv* env,