Indexed DB: Avoid race in shutdown in clearing Session Only origins

Origins set to "clear on exit" need to get wiped when the storage
partition is shutting down. This was wired into the context's
destructor, but needed to be triggered earlier. Introduce an explicit
Shutdown() call invoked by the storage partition, similar to other
storage backends.

Bug: 750452, 824533
Change-Id: I353c183f842da0016fa35f7d48c462c5e85b67ab
Reviewed-on: https://chromium-review.googlesource.com/c/1289435Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607455}

chrome/browser/browsing_data/browsing_data_remover_browsertest.cc

@@ -1116,14 +1116,12 @@ IN_PROC_BROWSER_TEST_F(BrowsingDataRemoverBrowserTest, StorageRemovedFromDisk) {
   EXPECT_EQ(0, found) << "A non-whitelisted file contains the hostname.";
 }
 
-// TODO(crbug.com/840080, crbug.com/824533): Filesystem, IndexedDb and
+// TODO(crbug.com/840080, crbug.com/824533): Filesystem and
 // CacheStorage can't be deleted on exit correctly at the moment.
 const std::vector<std::string> kSessionOnlyStorageTestTypes{
     "Cookie", "LocalStorage",
     // "FileSystem",
-    "SessionStorage",
-    // "IndexedDb",
-    "WebSql", "ServiceWorker",
+    "SessionStorage", "IndexedDb", "WebSql", "ServiceWorker",
     // "CacheStorage",
 };
 

content/browser/indexed_db/indexed_db_context_impl.cc

@@ -84,28 +84,6 @@ void GetAllOriginsAndPaths(const base::FilePath& indexeddb_path,
 
 }  // namespace
 
-// This will be called after the IndexedDBContext is destroyed.
-void IndexedDBContextImpl::ClearSessionOnlyOrigins(
-    const base::FilePath& indexeddb_path,
-    scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy) {
-  // TODO(jsbell): DCHECK that this is running on an IndexedDB sequence,
-  // if a global handle to it is ever available.
-  std::vector<Origin> origins;
-  std::vector<base::FilePath> file_paths;
-  GetAllOriginsAndPaths(indexeddb_path, &origins, &file_paths);
-  DCHECK_EQ(origins.size(), file_paths.size());
-  auto file_path = file_paths.cbegin();
-  auto origin = origins.cbegin();
-  for (; origin != origins.cend(); ++origin, ++file_path) {
-    const GURL origin_url = GURL(origin->Serialize());
-    if (!special_storage_policy->IsStorageSessionOnly(origin_url))
-      continue;
-    if (special_storage_policy->IsStorageProtected(origin_url))
-      continue;
-    base::DeleteFile(*file_path, true);
-  }
-}
-
 IndexedDBContextImpl::IndexedDBContextImpl(
     const base::FilePath& data_path,
     scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy,
@@ -116,7 +94,8 @@ IndexedDBContextImpl::IndexedDBContextImpl(
       task_runner_(base::CreateSequencedTaskRunnerWithTraits(
           {base::MayBlock(), base::WithBaseSyncPrimitives(),
            base::TaskPriority::USER_VISIBLE,
-           base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})) {
+           // BLOCK_SHUTDOWN to support clearing session-only storage.
+           base::TaskShutdownBehavior::BLOCK_SHUTDOWN})) {
   IDB_TRACE("init");
   if (!data_path.empty())
     data_path_ = data_path.Append(kIndexedDBDirectory);
@@ -542,24 +521,44 @@ IndexedDBContextImpl::~IndexedDBContextImpl() {
                            base::BindOnce(&IndexedDBFactory::ContextDestroyed,
                                           std::move(factory_)));
   }
+}
 
+void IndexedDBContextImpl::Shutdown() {
   if (is_incognito())
     return;
 
   if (force_keep_session_state_)
     return;
 
-  bool has_session_only_databases =
-      special_storage_policy_.get() &&
-      special_storage_policy_->HasSessionOnlyOrigins();
-
-  // Clearing only session-only databases, and there are none.
-  if (!has_session_only_databases)
-    return;
-
-  TaskRunner()->PostTask(FROM_HERE,
-                         base::BindOnce(&ClearSessionOnlyOrigins, data_path_,
-                                        special_storage_policy_));
+  // Clear session-only databases.
+  if (special_storage_policy_ &&
+      special_storage_policy_->HasSessionOnlyOrigins()) {
+    TaskRunner()->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            [](const base::FilePath& indexeddb_path,
+               scoped_refptr<IndexedDBFactory> factory,
+               scoped_refptr<storage::SpecialStoragePolicy>
+                   special_storage_policy) {
+              std::vector<Origin> origins;
+              std::vector<base::FilePath> file_paths;
+              GetAllOriginsAndPaths(indexeddb_path, &origins, &file_paths);
+              DCHECK_EQ(origins.size(), file_paths.size());
+              auto file_path = file_paths.cbegin();
+              auto origin = origins.cbegin();
+              for (; origin != origins.cend(); ++origin, ++file_path) {
+                const GURL origin_url = GURL(origin->Serialize());
+                if (!special_storage_policy->IsStorageSessionOnly(origin_url))
+                  continue;
+                if (special_storage_policy->IsStorageProtected(origin_url))
+                  continue;
+                if (factory.get())
+                  factory->ForceClose(*origin);
+                base::DeleteFile(*file_path, true);
+              }
+            },
+            data_path_, factory_, special_storage_policy_));
+  }
 }
 
 // static

content/browser/indexed_db/indexed_db_context_impl.h

@@ -76,6 +76,9 @@ class CONTENT_EXPORT IndexedDBContextImpl : public IndexedDBContext {
 
   IndexedDBFactory* GetIDBFactory();
 
+  // Called by StoragePartitionImpl to clear session-only data.
+  void Shutdown();
+
   // Disables the exit-time deletion of session-only data.
   void SetForceKeepSessionState() { force_keep_session_state_ = true; }
 

content/browser/indexed_db/indexed_db_unittest.cc

@@ -67,20 +67,20 @@ TEST_F(IndexedDBTest, ClearSessionOnlyDatabases) {
   base::FilePath normal_path;
   base::FilePath session_only_path;
 
-  // Create the scope which will ensure we run the destructor of the context
-  // which should trigger the clean up.
-  {
-    auto idb_context = base::MakeRefCounted<IndexedDBContextImpl>(
-        temp_dir.GetPath(), special_storage_policy_.get(),
-        quota_manager_proxy_.get());
-
-    normal_path = idb_context->GetFilePathForTesting(kNormalOrigin);
-    session_only_path = idb_context->GetFilePathForTesting(kSessionOnlyOrigin);
-    ASSERT_TRUE(base::CreateDirectory(normal_path));
-    ASSERT_TRUE(base::CreateDirectory(session_only_path));
-    RunAllTasksUntilIdle();
-    quota_manager_proxy_->SimulateQuotaManagerDestroyed();
-  }
+  auto idb_context = base::MakeRefCounted<IndexedDBContextImpl>(
+      temp_dir.GetPath(), special_storage_policy_.get(),
+      quota_manager_proxy_.get());
+
+  normal_path = idb_context->GetFilePathForTesting(kNormalOrigin);
+  session_only_path = idb_context->GetFilePathForTesting(kSessionOnlyOrigin);
+  ASSERT_TRUE(base::CreateDirectory(normal_path));
+  ASSERT_TRUE(base::CreateDirectory(session_only_path));
+  RunAllTasksUntilIdle();
+  quota_manager_proxy_->SimulateQuotaManagerDestroyed();
+
+  RunAllTasksUntilIdle();
+
+  idb_context->Shutdown();
 
   RunAllTasksUntilIdle();
 
@@ -95,25 +95,23 @@ TEST_F(IndexedDBTest, SetForceKeepSessionState) {
   base::FilePath normal_path;
   base::FilePath session_only_path;
 
-  // Create the scope which will ensure we run the destructor of the context.
-  {
-    // Create some indexedDB paths.
-    // With the levelDB backend, these are directories.
-    auto idb_context = base::MakeRefCounted<IndexedDBContextImpl>(
-        temp_dir.GetPath(), special_storage_policy_.get(),
-        quota_manager_proxy_.get());
-
-    // Save session state. This should bypass the destruction-time deletion.
-    idb_context->SetForceKeepSessionState();
-
-    normal_path = idb_context->GetFilePathForTesting(kNormalOrigin);
-    session_only_path = idb_context->GetFilePathForTesting(kSessionOnlyOrigin);
-    ASSERT_TRUE(base::CreateDirectory(normal_path));
-    ASSERT_TRUE(base::CreateDirectory(session_only_path));
-    base::RunLoop().RunUntilIdle();
-  }
+  // Create some indexedDB paths.
+  // With the levelDB backend, these are directories.
+  auto idb_context = base::MakeRefCounted<IndexedDBContextImpl>(
+      temp_dir.GetPath(), special_storage_policy_.get(),
+      quota_manager_proxy_.get());
+
+  // Save session state. This should bypass the destruction-time deletion.
+  idb_context->SetForceKeepSessionState();
+
+  normal_path = idb_context->GetFilePathForTesting(kNormalOrigin);
+  session_only_path = idb_context->GetFilePathForTesting(kSessionOnlyOrigin);
+  ASSERT_TRUE(base::CreateDirectory(normal_path));
+  ASSERT_TRUE(base::CreateDirectory(session_only_path));
+  base::RunLoop().RunUntilIdle();
+
+  idb_context->Shutdown();
 
-  // Make sure we wait until the destructor has run.
   base::RunLoop().RunUntilIdle();
 
   // No data was cleared because of SetForceKeepSessionState.

content/browser/storage_partition_impl.cc

@@ -551,6 +551,9 @@ StoragePartitionImpl::~StoragePartitionImpl() {
   if (GetServiceWorkerContext())
     GetServiceWorkerContext()->Shutdown();
 
+  if (GetIndexedDBContext())
+    GetIndexedDBContext()->Shutdown();
+
   if (GetCacheStorageContext())
     GetCacheStorageContext()->Shutdown();