Let StoragePartition wait for localstorage deletions

This CL changes StoragePartition to wait for localstorage deletions
to finish before calling its callback.

Some extension tests can leak memory because they don't wait until
StorageParitition deletions are finished. This CL adds a method to
StoragePartition to check if it is done and changes extension tests
to always wait for StoragePartition.

Bug: 796151
Change-Id: Ica6f6ef47457eb34f82923ec6ae144f6b5b67fda
Reviewed-on: https://chromium-review.googlesource.com/852234
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533707}

chrome/browser/extensions/extension_gcm_app_handler_unittest.cc

@@ -49,6 +49,7 @@
 #include "components/signin/core/browser/signin_manager.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/storage_partition.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_utils.h"
 #include "extensions/browser/extension_system.h"
@@ -272,6 +273,10 @@ class ExtensionGCMAppHandlerTest : public testing::Test {
 
     waiter_.PumpUILoop();
     gcm_app_handler_->Shutdown();
+    auto* partition =
+        content::BrowserContext::GetDefaultStoragePartition(profile());
+    if (partition)
+      partition->WaitForDeletionTasksForTesting();
   }
 
   // Returns a barebones test extension.

chrome/browser/extensions/extension_service_test_base.cc

@@ -31,6 +31,7 @@
 #include "components/sync_preferences/pref_service_mock_factory.h"
 #include "components/sync_preferences/pref_service_syncable.h"
 #include "content/public/browser/browser_context.h"
+#include "content/public/browser/storage_partition.h"
 #include "content/public/common/service_manager_connection.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
@@ -283,6 +284,15 @@ void ExtensionServiceTestBase::SetUp() {
   LoadErrorReporter::GetInstance()->ClearErrors();
 }
 
+void ExtensionServiceTestBase::TearDown() {
+  if (profile_) {
+    auto* partition =
+        content::BrowserContext::GetDefaultStoragePartition(profile_.get());
+    if (partition)
+      partition->WaitForDeletionTasksForTesting();
+  }
+}
+
 void ExtensionServiceTestBase::SetUpTestCase() {
   // Safe to call multiple times.
   LoadErrorReporter::Init(false);  // no noisy errors.

chrome/browser/extensions/extension_service_test_base.h

@@ -77,6 +77,7 @@ class ExtensionServiceTestBase : public testing::Test {
 
   // testing::Test implementation.
   void SetUp() override;
+  void TearDown() override;
 
   // Create a set of InitParams to install an ExtensionService into |temp_dir_|.
   ExtensionServiceInitParams CreateDefaultInitParams();

content/browser/storage_partition_impl.cc

@@ -10,6 +10,7 @@
 #include <set>
 #include <vector>
 
+#include "base/barrier_closure.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/location.h"
@@ -169,21 +170,21 @@ void OnLocalStorageUsageInfo(
     const std::vector<LocalStorageUsageInfo>& infos) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
+  base::RepeatingClosure barrier = base::BarrierClosure(infos.size(), callback);
   for (size_t i = 0; i < infos.size(); ++i) {
     if (!origin_matcher.is_null() &&
         !origin_matcher.Run(infos[i].origin, special_storage_policy.get())) {
+      barrier.Run();
       continue;
     }
 
     if (infos[i].last_modified >= delete_begin &&
         infos[i].last_modified <= delete_end) {
-      // TODO(dullweber): |callback| should be passed to DeleteLocalStorage()
-      // but then ASAN complains about a few tests that need to be fixed.
-      dom_storage_context->DeleteLocalStorage(infos[i].origin,
-                                              base::BindOnce(&base::DoNothing));
+      dom_storage_context->DeleteLocalStorage(infos[i].origin, barrier);
+    } else {
+      barrier.Run();
     }
   }
-  callback.Run();
 }
 
 void OnSessionStorageUsageInfo(
@@ -220,13 +221,11 @@ void ClearLocalStorageOnUIThread(
                       origin_matcher.Run(storage_origin,
                                          special_storage_policy.get());
     if (can_delete) {
-      // TODO(dullweber): |callback| should be passed to
-      // DeleteLocalStorageForPhysicalOrigin() but then ASAN complains about a
-      // few tests that need to be fixed.
-      dom_storage_context->DeleteLocalStorageForPhysicalOrigin(
-          storage_origin, base::BindOnce(&base::DoNothing));
+      dom_storage_context->DeleteLocalStorageForPhysicalOrigin(storage_origin,
+                                                               callback);
+    } else {
+      callback.Run();
     }
-    callback.Run();
     return;
   }
 
@@ -399,6 +398,8 @@ struct StoragePartitionImpl::DataDeletionHelper {
         callback(std::move(callback)),
         task_count(0) {}
 
+  ~DataDeletionHelper() {}
+
   void IncrementTaskCountOnUI();
   void DecrementTaskCount();  // Callable on any thread.
 
@@ -459,6 +460,7 @@ StoragePartitionImpl::StoragePartitionImpl(
     : partition_path_(partition_path),
       special_storage_policy_(special_storage_policy),
       browser_context_(browser_context),
+      deletion_helpers_running_(0),
       weak_factory_(this) {}
 
 StoragePartitionImpl::~StoragePartitionImpl() {
@@ -799,15 +801,27 @@ void StoragePartitionImpl::ClearDataImpl(
     base::OnceClosure callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   DataDeletionHelper* helper = new DataDeletionHelper(
-      remove_mask, quota_storage_remove_mask, std::move(callback));
+      remove_mask, quota_storage_remove_mask,
+      base::BindOnce(&StoragePartitionImpl::DeletionHelperDone,
+                     weak_factory_.GetWeakPtr(), std::move(callback)));
   // |helper| deletes itself when done in
   // DataDeletionHelper::DecrementTaskCount().
+  deletion_helpers_running_++;
   helper->ClearDataOnUIThread(
       storage_origin, origin_matcher, cookie_matcher, GetPath(),
       GetURLRequestContext(), dom_storage_context_.get(), quota_manager_.get(),
       special_storage_policy_.get(), filesystem_context_.get(), begin, end);
 }
 
+void StoragePartitionImpl::DeletionHelperDone(base::OnceClosure callback) {
+  std::move(callback).Run();
+  deletion_helpers_running_--;
+  if (on_deletion_helpers_done_callback_ && deletion_helpers_running_ == 0) {
+    // Notify tests that storage partition is done with all deletion tasks.
+    std::move(on_deletion_helpers_done_callback_).Run();
+  }
+}
+
 void StoragePartitionImpl::
     QuotaManagedDataDeletionHelper::IncrementTaskCountOnIO() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
@@ -1094,6 +1108,14 @@ void StoragePartitionImpl::FlushNetworkInterfaceForTesting() {
     url_loader_factory_for_browser_process_.FlushForTesting();
 }
 
+void StoragePartitionImpl::WaitForDeletionTasksForTesting() {
+  if (deletion_helpers_running_) {
+    base::RunLoop loop;
+    on_deletion_helpers_done_callback_ = loop.QuitClosure();
+    loop.Run();
+  }
+}
+
 BrowserContext* StoragePartitionImpl::browser_context() const {
   return browser_context_;
 }

content/browser/storage_partition_impl.h

@@ -123,6 +123,7 @@ class CONTENT_EXPORT StoragePartitionImpl
   void Flush() override;
   void ClearBluetoothAllowedDevicesMapForTesting() override;
   void FlushNetworkInterfaceForTesting() override;
+  void WaitForDeletionTasksForTesting() override;
 
   BackgroundFetchContext* GetBackgroundFetchContext();
   BackgroundSyncContext* GetBackgroundSyncContext();
@@ -238,6 +239,8 @@ class CONTENT_EXPORT StoragePartitionImpl
                      const base::Time end,
                      base::OnceClosure callback);
 
+  void DeletionHelperDone(base::OnceClosure callback);
+
   // Used by StoragePartitionImplMap.
   //
   // TODO(ajwong): These should be taken in the constructor and in Create() but
@@ -323,6 +326,12 @@ class CONTENT_EXPORT StoragePartitionImpl
   // See comments for site_for_service_worker().
   GURL site_for_service_worker_;
 
+  // Track number of running deletion. For test use only.
+  int deletion_helpers_running_;
+
+  // Called when all deletions are done. For test use only.
+  base::OnceClosure on_deletion_helpers_done_callback_;
+
   base::WeakPtrFactory<StoragePartitionImpl> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(StoragePartitionImpl);

content/public/browser/storage_partition.h

@@ -204,6 +204,9 @@ class CONTENT_EXPORT StoragePartition {
   // use only.
   virtual void FlushNetworkInterfaceForTesting() = 0;
 
+  // Wait until all deletions tasks are finished. For test use only.
+  virtual void WaitForDeletionTasksForTesting() = 0;
+
  protected:
   virtual ~StoragePartition() {}
 };

content/public/test/test_storage_partition.cc

@@ -126,4 +126,6 @@ void TestStoragePartition::ClearBluetoothAllowedDevicesMapForTesting() {}
 
 void TestStoragePartition::FlushNetworkInterfaceForTesting() {}
 
+void TestStoragePartition::WaitForDeletionTasksForTesting() {}
+
 }  // namespace content

content/public/test/test_storage_partition.h

@@ -167,6 +167,7 @@ class TestStoragePartition : public StoragePartition {
 
   void ClearBluetoothAllowedDevicesMapForTesting() override;
   void FlushNetworkInterfaceForTesting() override;
+  void WaitForDeletionTasksForTesting() override;
 
  private:
   base::FilePath file_path_;