[IndexedDB] Moving origin deletion from Context to Control

This patch continues to move methods from IndexedDBContext to the mojo
interface IndexedDBControl. In particular, this is moving the method
DeleteForOrigin.

This patch also removes the now unnecessary leveldb workaround for empty
directories.

See tracking doc here:
https://docs.google.com/document/d/18suNOOzuEJbqgRJF0MB2VqdTyYqS4cvI2PGaCpyPXSw/edit?ts=5e138c46#heading=h.23paiqht163

Bug: 1015214
Change-Id: I36bccba416c7a5b182f1f85e01a02cc3e1e40623
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1988811Reviewed-by: enne <enne@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#730034}

chrome/browser/browsing_data/browsing_data_indexed_db_helper.cc

@@ -22,7 +22,6 @@
 #include "url/origin.h"
 
 using content::BrowserThread;
-using content::IndexedDBContext;
 using content::StorageUsageInfo;
 
 BrowsingDataIndexedDBHelper::BrowsingDataIndexedDBHelper(
@@ -43,13 +42,8 @@ void BrowsingDataIndexedDBHelper::StartFetching(FetchCallback callback) {
 
 void BrowsingDataIndexedDBHelper::DeleteIndexedDB(const GURL& origin) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  storage_partition_->GetIndexedDBContext()->IDBTaskRunner()->PostTask(
-      FROM_HERE,
-      base::BindOnce(
-          &BrowsingDataIndexedDBHelper::DeleteIndexedDBInIndexedDBThread,
-          base::WrapRefCounted(this),
-          base::WrapRefCounted(storage_partition_->GetIndexedDBContext()),
-          origin));
+  storage_partition_->GetIndexedDBControl().DeleteForOrigin(
+      url::Origin::Create(origin), base::DoNothing());
 }
 
 void BrowsingDataIndexedDBHelper::IndexedDBUsageInfoReceived(
@@ -67,13 +61,6 @@ void BrowsingDataIndexedDBHelper::IndexedDBUsageInfoReceived(
   std::move(callback).Run(std::move(result));
 }
 
-void BrowsingDataIndexedDBHelper::DeleteIndexedDBInIndexedDBThread(
-    scoped_refptr<content::IndexedDBContext> context,
-    const GURL& origin) {
-  DCHECK(context->IDBTaskRunner()->RunsTasksInCurrentSequence());
-  context->DeleteForOrigin(url::Origin::Create(origin));
-}
-
 CannedBrowsingDataIndexedDBHelper::CannedBrowsingDataIndexedDBHelper(
     content::StoragePartition* storage_partition)
     : BrowsingDataIndexedDBHelper(storage_partition) {}

chrome/browser/browsing_data/browsing_data_indexed_db_helper.h

@@ -15,7 +15,6 @@
 #include "base/memory/ref_counted.h"
 #include "base/strings/string16.h"
 #include "components/services/storage/public/mojom/indexed_db_control.mojom-forward.h"
-#include "content/public/browser/indexed_db_context.h"
 #include "url/gurl.h"
 #include "url/origin.h"
 
@@ -59,11 +58,6 @@ class BrowsingDataIndexedDBHelper
       FetchCallback callback,
       std::vector<storage::mojom::IndexedDBStorageUsageInfoPtr> origins);
 
-  // Delete a single indexed database in the IndexedDB thread.
-  void DeleteIndexedDBInIndexedDBThread(
-      scoped_refptr<content::IndexedDBContext> context,
-      const GURL& origin);
-
   DISALLOW_COPY_AND_ASSIGN(BrowsingDataIndexedDBHelper);
 };
 

components/services/storage/public/mojom/indexed_db_control.mojom

@@ -30,4 +30,7 @@ interface IndexedDBControl {
   // returned array has one IndexedDBStorageUsageInfo per origin in the
   // partition that has IndexedDB usage data.
   GetUsage() => (array<IndexedDBStorageUsageInfo> info);
+
+  // Deletes all indexed db files for the given origin.
+  DeleteForOrigin(url.mojom.Origin origin) => (bool success);
 };

content/browser/indexed_db/indexed_db_backing_store_unittest.cc

@@ -26,6 +26,7 @@
 #include "components/services/storage/indexed_db/scopes/disjoint_range_lock_manager.h"
 #include "components/services/storage/indexed_db/transactional_leveldb/leveldb_write_batch.h"
 #include "components/services/storage/indexed_db/transactional_leveldb/transactional_leveldb_database.h"
+#include "components/services/storage/public/mojom/indexed_db_control.mojom-test-utils.h"
 #include "content/browser/indexed_db/indexed_db_class_factory.h"
 #include "content/browser/indexed_db/indexed_db_context_impl.h"
 #include "content/browser/indexed_db/indexed_db_factory_impl.h"
@@ -261,7 +262,10 @@ class IndexedDBBackingStoreTest : public testing::Test {
       }
       // All leveldb databases are closed, and they can be deleted.
       for (auto origin : idb_context_->GetAllOrigins()) {
-        idb_context_->DeleteForOrigin(origin);
+        bool success = false;
+        storage::mojom::IndexedDBControlAsyncWaiter waiter(idb_context_.get());
+        waiter.DeleteForOrigin(origin, &success);
+        EXPECT_TRUE(success);
       }
     }
     if (temp_dir_.IsValid())

content/browser/indexed_db/indexed_db_browsertest.cc

@@ -28,6 +28,7 @@
 #include "build/build_config.h"
 #include "components/services/storage/indexed_db/scopes/varint_coding.h"
 #include "components/services/storage/indexed_db/transactional_leveldb/transactional_leveldb_database.h"
+#include "components/services/storage/public/mojom/indexed_db_control.mojom-test-utils.h"
 #include "content/browser/blob_storage/chrome_blob_storage_context.h"
 #include "content/browser/browser_main_loop.h"
 #include "content/browser/indexed_db/indexed_db_class_factory.h"
@@ -181,15 +182,20 @@ class IndexedDBBrowserTest : public ContentBrowserTest,
         storage::GetHardCodedSettings(per_host_quota_kilobytes * KB));
   }
 
-  void DeleteForOrigin(const Origin& origin, Shell* browser = nullptr) {
+  bool DeleteForOrigin(const Origin& origin, Shell* browser = nullptr) {
     base::RunLoop loop;
-    IndexedDBContextImpl* context = GetContext();
-    context->IDBTaskRunner()->PostTask(FROM_HERE,
-                                       base::BindLambdaForTesting([&]() {
-                                         context->DeleteForOrigin(kFileOrigin);
+    IndexedDBContextImpl* context = GetContext(browser);
+    bool result = false;
+    context->IDBTaskRunner()->PostTask(
+        FROM_HERE, base::BindLambdaForTesting([&]() {
+          context->DeleteForOrigin(
+              origin, base::BindLambdaForTesting([&](bool success) {
+                result = success;
                 loop.Quit();
               }));
+        }));
     loop.Run();
+    return result;
   }
 
   int64_t RequestUsage(const Origin& origin, Shell* browser = nullptr) {
@@ -685,14 +691,7 @@ IN_PROC_BROWSER_TEST_F(IndexedDBBrowserTest, DeleteForOriginIncognito) {
 
   EXPECT_GT(RequestUsage(origin, browser), 5 * 1024);
 
-  IndexedDBContextImpl* context = GetContext(browser);
-  base::RunLoop loop;
-  context->IDBTaskRunner()->PostTask(FROM_HERE,
-                                     base::BindLambdaForTesting([&]() {
-                                       context->DeleteForOrigin(origin);
-                                       loop.Quit();
-                                     }));
-  loop.Run();
+  DeleteForOrigin(origin, browser);
 
   EXPECT_EQ(0, RequestUsage(origin, browser));
 }

content/browser/indexed_db/indexed_db_context_impl.cc

@@ -129,6 +129,40 @@ void IndexedDBContextImpl::GetUsage(GetUsageCallback usage_callback) {
   std::move(usage_callback).Run(std::move(result));
 }
 
+// Note - this is being kept async (instead of having a 'sync' version) to allow
+// ForceClose to become asynchronous.  This is required for
+// https://crbug.com/965142.
+void IndexedDBContextImpl::DeleteForOrigin(const Origin& origin,
+                                           DeleteForOriginCallback callback) {
+  DCHECK(IDBTaskRunner()->RunsTasksInCurrentSequence());
+  ForceClose(origin, FORCE_CLOSE_DELETE_ORIGIN);
+  if (!HasOrigin(origin)) {
+    std::move(callback).Run(true);
+    return;
+  }
+
+  if (is_incognito()) {
+    GetOriginSet()->erase(origin);
+    origin_size_map_.erase(origin);
+    std::move(callback).Run(true);
+    return;
+  }
+
+  base::FilePath idb_directory = GetLevelDBPath(origin);
+  EnsureDiskUsageCacheInitialized(origin);
+
+  leveldb::Status s =
+      IndexedDBClassFactory::Get()->leveldb_factory().DestroyLevelDB(
+          idb_directory);
+  if (s.ok()) {
+    base::DeleteFileRecursively(GetBlobStorePath(origin));
+    GetOriginSet()->erase(origin);
+    origin_size_map_.erase(origin);
+  }
+  QueryDiskAndUpdateQuotaUsage(origin);
+  std::move(callback).Run(s.ok());
+}
+
 IndexedDBFactoryImpl* IndexedDBContextImpl::GetIDBFactory() {
   DCHECK(IDBTaskRunner()->RunsTasksInCurrentSequence());
   if (!indexeddb_factory_.get()) {
@@ -325,42 +359,6 @@ base::Time IndexedDBContextImpl::GetOriginLastModified(const Origin& origin) {
   return file_info.last_modified;
 }
 
-void IndexedDBContextImpl::DeleteForOrigin(const Origin& origin) {
-  DCHECK(IDBTaskRunner()->RunsTasksInCurrentSequence());
-  ForceClose(origin, FORCE_CLOSE_DELETE_ORIGIN);
-  if (!HasOrigin(origin))
-    return;
-
-  if (is_incognito()) {
-    GetOriginSet()->erase(origin);
-    origin_size_map_.erase(origin);
-    return;
-  }
-
-  base::FilePath idb_directory = GetLevelDBPath(origin);
-  EnsureDiskUsageCacheInitialized(origin);
-
-  leveldb::Status s =
-      IndexedDBClassFactory::Get()->leveldb_factory().DestroyLevelDB(
-          idb_directory);
-  if (!s.ok()) {
-    LOG(WARNING) << "Failed to delete LevelDB database: "
-                 << idb_directory.AsUTF8Unsafe();
-  } else {
-    // LevelDB does not delete empty directories; work around this.
-    // TODO(jsbell): Remove when upstream bug is fixed.
-    // https://github.com/google/leveldb/issues/215
-    const bool kNonRecursive = false;
-    base::DeleteFile(idb_directory, kNonRecursive);
-  }
-  base::DeleteFileRecursively(GetBlobStorePath(origin));
-  QueryDiskAndUpdateQuotaUsage(origin);
-  if (s.ok()) {
-    GetOriginSet()->erase(origin);
-    origin_size_map_.erase(origin);
-  }
-}
-
 void IndexedDBContextImpl::CopyOriginData(const Origin& origin,
                                           IndexedDBContext* dest_context) {
   DCHECK(IDBTaskRunner()->RunsTasksInCurrentSequence());

content/browser/indexed_db/indexed_db_context_impl.h

@@ -87,6 +87,8 @@ class CONTENT_EXPORT IndexedDBContextImpl
 
   // mojom::IndexedDBControl implementation:
   void GetUsage(GetUsageCallback usage_callback) override;
+  void DeleteForOrigin(const url::Origin& origin,
+                       DeleteForOriginCallback callback) override;
 
   IndexedDBFactoryImpl* GetIDBFactory();
 
@@ -99,7 +101,6 @@ class CONTENT_EXPORT IndexedDBContextImpl
 
   // IndexedDBContext implementation:
   base::SequencedTaskRunner* IDBTaskRunner() override;
-  void DeleteForOrigin(const url::Origin& origin) override;
   void CopyOriginData(const url::Origin& origin,
                       IndexedDBContext* dest_context) override;
   base::FilePath GetFilePathForTesting(const url::Origin& origin) override;
@@ -131,6 +132,8 @@ class CONTENT_EXPORT IndexedDBContextImpl
 
   // ForceClose takes a value rather than a reference since it may release the
   // owning object.
+  // ForceClose needs to move to async to support multi-thread scopes. See
+  // https://crbug.com/965142.
   void ForceClose(const url::Origin origin, ForceCloseReason reason);
   bool ForceSchemaDowngrade(const url::Origin& origin);
   V2SchemaCorruptionStatus HasV2SchemaCorruption(const url::Origin& origin);

content/browser/indexed_db/indexed_db_factory_unittest.cc

@@ -92,7 +92,11 @@ class IndexedDBFactoryTest : public testing::Test {
           open_factory_origins.size(), base::BindLambdaForTesting([&]() {
             // All leveldb databases are closed, and they can be deleted.
             for (auto origin : context_->GetAllOrigins()) {
-              context_->DeleteForOrigin(origin);
+              bool success = false;
+              storage::mojom::IndexedDBControlAsyncWaiter waiter(
+                  context_.get());
+              waiter.DeleteForOrigin(origin, &success);
+              EXPECT_TRUE(success);
             }
             loop.Quit();
           }));

content/browser/indexed_db/indexed_db_quota_client.cc

@@ -11,23 +11,26 @@
 
 #include "base/bind.h"
 #include "base/logging.h"
+#include "base/task/post_task.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "content/browser/indexed_db/indexed_db_context_impl.h"
 #include "net/base/url_util.h"
 #include "storage/browser/database/database_util.h"
 #include "third_party/blink/public/mojom/quota/quota_types.mojom.h"
 
 using blink::mojom::StorageType;
-using storage::QuotaClient;
 using storage::DatabaseUtil;
+using storage::QuotaClient;
 
 namespace content {
 namespace {
 
-blink::mojom::QuotaStatusCode DeleteOriginDataOnIndexedDBThread(
-    IndexedDBContextImpl* context,
-    const url::Origin& origin) {
-  context->DeleteForOrigin(origin);
-  return blink::mojom::QuotaStatusCode::kOk;
+void DidDeleteIDBData(scoped_refptr<base::SequencedTaskRunner> task_runner,
+                      IndexedDBQuotaClient::DeletionCallback callback,
+                      bool) {
+  task_runner->PostTask(
+      FROM_HERE,
+      base::BindOnce(std::move(callback), blink::mojom::QuotaStatusCode::kOk));
 }
 
 int64_t GetOriginUsageOnIndexedDBThread(IndexedDBContextImpl* context,
@@ -72,7 +75,9 @@ IndexedDBQuotaClient::IndexedDBQuotaClient(
 
 IndexedDBQuotaClient::~IndexedDBQuotaClient() {}
 
-QuotaClient::ID IndexedDBQuotaClient::id() const { return kIndexedDatabase; }
+QuotaClient::ID IndexedDBQuotaClient::id() const {
+  return kIndexedDatabase;
+}
 
 void IndexedDBQuotaClient::GetOriginUsage(const url::Origin& origin,
                                           StorageType type,
@@ -144,11 +149,13 @@ void IndexedDBQuotaClient::DeleteOriginData(const url::Origin& origin,
     return;
   }
 
-  base::PostTaskAndReplyWithResult(
-      indexed_db_context_->IDBTaskRunner(), FROM_HERE,
-      base::BindOnce(&DeleteOriginDataOnIndexedDBThread,
-                     base::RetainedRef(indexed_db_context_), origin),
-      std::move(callback));
+  indexed_db_context_->IDBTaskRunner()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&IndexedDBContextImpl::DeleteForOrigin,
+                     base::RetainedRef(indexed_db_context_), origin,
+                     base::BindOnce(DidDeleteIDBData,
+                                    base::SequencedTaskRunnerHandle::Get(),
+                                    std::move(callback))));
 }
 
 bool IndexedDBQuotaClient::DoesSupport(StorageType type) const {

content/browser/indexed_db/indexed_db_unittest.cc

@@ -16,6 +16,7 @@
 #include "base/time/default_clock.h"
 #include "components/services/storage/indexed_db/scopes/scopes_lock_manager.h"
 #include "components/services/storage/indexed_db/transactional_leveldb/transactional_leveldb_database.h"
+#include "components/services/storage/public/mojom/indexed_db_control.mojom-test-utils.h"
 #include "content/browser/indexed_db/indexed_db_connection.h"
 #include "content/browser/indexed_db/indexed_db_context_impl.h"
 #include "content/browser/indexed_db/indexed_db_execution_context_connection_tracker.h"
@@ -125,7 +126,11 @@ class IndexedDBTest : public testing::Test {
           open_factory_origins.size(), base::BindLambdaForTesting([&]() {
             // All leveldb databases are closed, and they can be deleted.
             for (auto origin : context_->GetAllOrigins()) {
-              context_->DeleteForOrigin(origin);
+              bool success = false;
+              storage::mojom::IndexedDBControlAsyncWaiter waiter(
+                  context_.get());
+              waiter.DeleteForOrigin(origin, &success);
+              EXPECT_TRUE(success);
             }
             loop.Quit();
           }));
@@ -287,7 +292,10 @@ TEST_F(IndexedDBTest, ForceCloseOpenDatabasesOnDelete) {
 
   RunPostedTasks();
 
-  context()->DeleteForOrigin(kTestOrigin);
+  bool success = false;
+  storage::mojom::IndexedDBControlAsyncWaiter waiter(context());
+  waiter.DeleteForOrigin(kTestOrigin, &success);
+  EXPECT_TRUE(success);
 
   EXPECT_FALSE(base::DirectoryExists(test_path));
 }
@@ -301,13 +309,16 @@ TEST_F(IndexedDBTest, DeleteFailsIfDirectoryLocked) {
   auto lock = LockForTesting(test_path);
   ASSERT_TRUE(lock);
 
+  bool success = false;
   base::RunLoop loop;
   context()->IDBTaskRunner()->PostTask(
       FROM_HERE, base::BindLambdaForTesting([&]() {
-        context()->DeleteForOrigin(kTestOrigin);
+        storage::mojom::IndexedDBControlAsyncWaiter waiter(context());
+        waiter.DeleteForOrigin(kTestOrigin, &success);
         loop.Quit();
       }));
   loop.Run();
+  EXPECT_FALSE(success);
 
   EXPECT_TRUE(base::DirectoryExists(test_path));
 }

content/public/browser/indexed_db_context.h

@@ -28,15 +28,14 @@ namespace content {
 // Call these methods only via the exposed IDBTaskRunner.
 // Refcounted because this class is used throughout the codebase on different
 // threads.
+// This class is in the process of being removed in lieu of the
+// IndexedDBControl mojo interface.
 class IndexedDBContext
     : public base::RefCountedDeleteOnSequence<IndexedDBContext> {
  public:
   // Only call the below methods by posting to this IDBTaskRunner.
   virtual base::SequencedTaskRunner* IDBTaskRunner() = 0;
 
-  // Deletes all indexed db files for the given origin.
-  virtual void DeleteForOrigin(const url::Origin& origin) = 0;
-
   // Copies the indexed db files from this context to another. The
   // indexed db directory in the destination context needs to be empty.
   virtual void CopyOriginData(const url::Origin& origin,