Formalize the restrictions of BrowsingDataRemover::RemoveWithFilter()

chrome/browser/android/preferences/pref_service_bridge.cc

 // Copyright 2014 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/android/preferences/pref_service_bridge.h"
 
 #include <jni.h>
 #include <stddef.h>
 
 #include <memory>
@@ skipping 495 matching lines @@
 static ScopedJavaLocalRef<jstring> GetLatestVersionWhenClickedUpdateMenuItem(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj) {
   return ConvertUTF8ToJavaString(
       env, GetPrefService()->GetString(
           prefs::kLatestVersionWhenClickedUpdateMenuItem));
 }
 
 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);
 
     JNIEnv* env = AttachCurrentThread();
     if (weak_chrome_native_preferences_.get(env).is_null())
       return;
 
     Java_PrefServiceBridge_browsingDataCleared(
         env, weak_chrome_native_preferences_.get(env).obj());
   }
 
  private:
+  int task_count_;
   JavaObjectWeakGlobalRef weak_chrome_native_preferences_;
   ScopedObserver<BrowsingDataRemover, BrowsingDataRemover::Observer> observer_;
 };
 
 }  // namespace
 
 static jboolean GetBrowsingDataDeletionPreference(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
     jint data_type) {
@@ skipping 92 matching lines @@
       new RegistrableDomainFilterBuilder(BrowsingDataFilterBuilder::BLACKLIST));
   for (const std::string& domain : excluding_domains) {
     filter_builder->AddRegisterableDomain(domain);
   }
 
   if (!excluding_domains.empty()) {
     ImportantSitesUtil::RecordMetricsForBlacklistedSites(GetOriginalProfile(),
                                                          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);
+  // 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.

 
-  browsing_data_remover->RemoveWithFilterAndReply(
-      BrowsingDataRemover::Period(
-          static_cast<browsing_data::TimePeriod>(time_period)),
-      remove_mask, BrowsingDataHelper::UNPROTECTED_WEB,
-      std::move(filter_builder), observer);
+  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,
                                          const JavaParamRef<jobject>& obj) {
   return GetPrefService()->GetBoolean(prefs::kAllowDeletingBrowserHistory);
 }
 
 static void FetchImportantSites(JNIEnv* env,
                                 const JavaParamRef<jclass>& clazz,
                                 const JavaParamRef<jobject>& java_callback) {
@@ skipping 494 matching lines @@
 
   return ConvertJavaStringToUTF8(android_permission);
 }
 
 static void SetSupervisedUserId(JNIEnv* env,
                                 const JavaParamRef<jobject>& obj,
                                 const JavaParamRef<jstring>& pref) {
   GetPrefService()->SetString(prefs::kSupervisedUserId,
                               ConvertJavaStringToUTF8(env, pref));
 }