[IndexedDB] Enable multi-threaded scopes with blocking close.

This patch enables multi-threaded LevelDBScopes usage by IndexedDB. It
does this by:
* Changing the mode during scopes initialization,
* Changing the RequestDestruction parameters to accept a WaitableEvent
* Modifies the destructor of IDBOriginState to request this destruction
  & then wait on the event.
* Modifies all unittests to match the new API pattern.

This allows cleanup & revert tasks to now take place off of the main
IndexedDB sequence, ensuring that cleanup & deferred range deletion do
not block IndexedDB operations.

See doc for more reasoning & alternatives:
https://docs.google.com/document/d/1xlSye5iCVU2kRsdzpue3-OE-BIBhmhTfMWss4XYxygI/edit#

Bug: 1058977
Change-Id: I679990a8ef057531a9ad3f710c9411377881eb11
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2090630Reviewed-by: enne <enne@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749233}

components/services/storage/indexed_db/leveldb/leveldb_state.cc

@@ -4,8 +4,10 @@
 
 #include "components/services/storage/indexed_db/leveldb/leveldb_state.h"
 
+#include "base/location.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_functions.h"
+#include "base/synchronization/waitable_event.h"
 #include "third_party/leveldatabase/src/include/leveldb/env.h"
 
 namespace content {
@@ -43,24 +45,20 @@ LevelDBState::LevelDBState(std::unique_ptr<leveldb::Env> optional_in_memory_env,
       name_for_tracing_(std::move(name_for_tracing)),
       destruction_requested_(false) {}
 
-bool LevelDBState::RequestDestruction(
-    base::OnceClosure on_state_destruction,
-    scoped_refptr<base::SequencedTaskRunner> task_runner) {
-  if (destruction_requested_.exchange(true, std::memory_order_relaxed))
-    return false;
-
-  DCHECK(!on_destruction_);
-  DCHECK(!on_destruction_task_runner_);
-  on_destruction_ = std::move(on_state_destruction);
-  on_destruction_task_runner_ = std::move(task_runner);
-  return true;
+void LevelDBState::RequestDestruction(
+    base::WaitableEvent* signal_on_destruction) {
+  DCHECK(signal_on_destruction);
+  bool destruct_already_requested =
+      destruction_requested_.exchange(true, std::memory_order_relaxed);
+  CHECK(!destruct_already_requested)
+      << "RequestDestruction can only be called one time.";
+  DCHECK(!signal_on_destruction_);
+  signal_on_destruction_ = signal_on_destruction;
 }
 
 LevelDBState::~LevelDBState() {
-  if (on_destruction_) {
-    on_destruction_task_runner_->PostTask(FROM_HERE,
-                                          std::move(on_destruction_));
-  }
+  if (signal_on_destruction_)
+    signal_on_destruction_->Signal();
   if (!db_)
     return;
   base::TimeTicks begin_time = base::TimeTicks::Now();

components/services/storage/indexed_db/leveldb/leveldb_state.h

@@ -9,15 +9,17 @@
 #include <memory>
 #include <utility>
 
-#include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/memory/ref_counted.h"
-#include "base/memory/scoped_refptr.h"
 #include "base/sequenced_task_runner.h"
 #include "third_party/leveldatabase/src/include/leveldb/comparator.h"
 #include "third_party/leveldatabase/src/include/leveldb/db.h"
 #include "third_party/leveldatabase/src/include/leveldb/filter_policy.h"
 
+namespace base {
+class WaitableEvent;
+}  // namespace base
+
 namespace content {
 
 // Encapsulates a leveldb database and comparator, allowing them to be used
@@ -35,16 +37,18 @@ class LevelDBState : public base::RefCountedThreadSafe<LevelDBState> {
       std::unique_ptr<leveldb::DB> in_memory_database,
       std::string name_for_tracing);
 
-  // Returns if this call was successfully the first call to request destruction
-  // of this state. Can be called on any thread. The given |task_runner| will be
-  // used to call the |on_destruction| closure, which is called on the
-  // destruction of this state.
-  bool RequestDestruction(base::OnceClosure on_destruction,
-                          scoped_refptr<base::SequencedTaskRunner> task_runner);
+  // Can only be called once. |signal_on_destruction| must outlive this class,
+  // and will be signaled on the destruction of this state (in the destructor).
+  // Can be called on any thread.
+  void RequestDestruction(base::WaitableEvent* signal_on_destruction);
 
   bool destruction_requested() const {
     return destruction_requested_.load(std::memory_order_relaxed);
   }
+  // Only valid if destruction_requested() returns true.
+  base::WaitableEvent* destruction_event() const {
+    return signal_on_destruction_;
+  }
 
   const leveldb::Comparator* comparator() const { return comparator_; }
   leveldb::DB* db() const { return db_.get(); }
@@ -74,11 +78,10 @@ class LevelDBState : public base::RefCountedThreadSafe<LevelDBState> {
   // This member transitions from false to true at most once in the instance's
   // lifetime.
   std::atomic_bool destruction_requested_;
-  // These members are written only once (when |destruction_requested_|
-  // transitions from false to true) and read only once in the destructor, so
-  // they are thread-compatible.
-  base::OnceClosure on_destruction_;
-  scoped_refptr<base::SequencedTaskRunner> on_destruction_task_runner_;
+  // |signal_on_destruction_| is written only once (when
+  // |destruction_requested_| transitions from false to true) and read only once
+  // in the destructor, so it is thread-compatible.
+  base::WaitableEvent* signal_on_destruction_ = nullptr;
 };
 
 }  // namespace content

components/services/storage/indexed_db/scopes/leveldb_scopes_tasks_unittest.cc

@@ -139,8 +139,7 @@ TEST_F(LevelDBScopesTasksTest, CleanupAbortsOnDestructionRequested) {
                         CleanupScopeTask::CleanupMode::kExecuteCleanupTasks,
                         kWriteBatchSizeForTesting);
 
-  leveldb_->RequestDestruction(base::DoNothing(),
-                               base::SequencedTaskRunnerHandle::Get());
+  leveldb_->RequestDestruction(&leveldb_close_event_);
   leveldb::Status s = task.Run();
   ASSERT_TRUE(s.ok()) << s.ToString();
   EXPECT_TRUE(LoadAt(delete_range_start_key_).ok());

components/services/storage/indexed_db/scopes/leveldb_scopes_test_utils.cc

@@ -9,6 +9,7 @@
 #include "base/run_loop.h"
 #include "base/sequenced_task_runner.h"
 #include "base/strings/stringprintf.h"
+#include "base/synchronization/waitable_event_watcher.h"
 #include "base/system/sys_info.h"
 #include "base/test/bind_test_util.h"
 #include "base/threading/sequenced_task_runner_handle.h"
@@ -64,12 +65,26 @@ void LevelDBScopesTestBase::TearDown() {
 void LevelDBScopesTestBase::CloseScopesAndDestroyLevelDBState() {
   if (leveldb_) {
     base::RunLoop loop;
-    if (leveldb_->RequestDestruction(loop.QuitClosure(),
-                                     base::SequencedTaskRunnerHandle::Get())) {
-      leveldb_.reset();
-      loop.Run();
+    base::WaitableEvent* leveldb_close_event_ptr;
+    base::WaitableEventWatcher event_watcher;
+    if (leveldb_->destruction_requested()) {
+      leveldb_close_event_ptr = leveldb_->destruction_event();
+    } else {
+      leveldb_close_event_ptr = &leveldb_close_event_;
+      leveldb_->RequestDestruction(leveldb_close_event_ptr);
     }
+    event_watcher.StartWatching(
+        leveldb_close_event_ptr,
+        base::BindLambdaForTesting([&](base::WaitableEvent*) { loop.Quit(); }),
+        base::SequencedTaskRunnerHandle::Get());
     leveldb_.reset();
+    loop.Run();
+    // There is a possible race in |leveldb_close_event| where the signaling
+    // thread is still in the WaitableEvent::Signal() method. To ensure that
+    // the other thread exits their Signal method, any method on the
+    // WaitableEvent can be called to acquire the internal lock (which will
+    // subsequently wait for the other thread to exit the Signal method).
+    EXPECT_TRUE(leveldb_close_event_ptr->IsSignaled());
   }
 }
 

components/services/storage/indexed_db/scopes/leveldb_scopes_test_utils.h

@@ -103,6 +103,8 @@ class LevelDBScopesTestBase : public testing::Test {
   // may need to run cleanup tasks that close files residing in the former.
   base::ScopedTempDir temp_directory_;
   base::test::TaskEnvironment task_env_;
+  // For use with calling leveldb_->RequestDestruction(...);
+  base::WaitableEvent leveldb_close_event_;
 
   const std::string simple_lock_begin_ = "0000000001";
   const std::string simple_lock_end_ = "0000000010";

content/browser/indexed_db/indexed_db_backing_store_unittest.cc

@@ -23,6 +23,8 @@
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/synchronization/waitable_event_watcher.h"
 #include "base/task/post_task.h"
 #include "base/task/thread_pool.h"
 #include "base/test/bind_test_util.h"
@@ -361,15 +363,29 @@ class IndexedDBBackingStoreTest : public testing::Test {
         base::RunLoop loop;
         IndexedDBOriginState* per_origin_factory =
             factory->GetOriginFactory(origin);
-        per_origin_factory->backing_store()
-            ->db()
-            ->leveldb_state()
-            ->RequestDestruction(loop.QuitClosure(),
-                                 base::SequencedTaskRunnerHandle::Get());
+
+        auto* leveldb_state =
+            per_origin_factory->backing_store()->db()->leveldb_state();
+
+        base::WaitableEvent leveldb_close_event;
+        base::WaitableEventWatcher event_watcher;
+        leveldb_state->RequestDestruction(&leveldb_close_event);
+        event_watcher.StartWatching(
+            &leveldb_close_event,
+            base::BindLambdaForTesting(
+                [&](base::WaitableEvent*) { loop.Quit(); }),
+            base::SequencedTaskRunnerHandle::Get());
+
         idb_context_->ForceCloseSync(
             origin,
             storage::mojom::ForceCloseReason::FORCE_CLOSE_DELETE_ORIGIN);
         loop.Run();
+        // There is a possible race in |leveldb_close_event| where the signaling
+        // thread is still in the WaitableEvent::Signal() method. To ensure that
+        // the other thread exits their Signal method, any method on the
+        // WaitableEvent can be called to acquire the internal lock (which will
+        // subsequently wait for the other thread to exit the Signal method).
+        EXPECT_TRUE(leveldb_close_event.IsSignaled());
       }
       // All leveldb databases are closed, and they can be deleted.
       for (auto origin : idb_context_->GetAllOrigins()) {

content/browser/indexed_db/indexed_db_factory_impl.cc

@@ -769,7 +769,7 @@ IndexedDBFactoryImpl::GetOrOpenOriginFactory(
   // Scopes must be single sequence to keep methods like ForceClose synchronous.
   // See https://crbug.com/980685
   s = backing_store->db()->scopes()->StartRecoveryAndCleanupTasks(
-      LevelDBScopes::TaskRunnerMode::kUseCurrentSequence);
+      LevelDBScopes::TaskRunnerMode::kNewCleanupAndRevertSequences);
 
   if (UNLIKELY(!s.ok())) {
     ReportOpenStatus(indexed_db::INDEXED_DB_BACKING_STORE_OPEN_NO_RECOVERY,

content/browser/indexed_db/indexed_db_factory_unittest.cc

@@ -90,32 +90,18 @@ class IndexedDBFactoryTest : public testing::Test {
       // deletion of the leveldb state. Once the states are no longer around,
       // delete all of the databases on disk.
       auto open_factory_origins = factory->GetOpenOrigins();
-      base::RunLoop loop;
-      auto callback = base::BarrierClosure(
-          open_factory_origins.size(), base::BindLambdaForTesting([&]() {
-            // All leveldb databases are closed, and they can be deleted.
-            for (auto origin : context_->GetAllOrigins()) {
-              bool success = false;
-              storage::mojom::IndexedDBControlAsyncWaiter waiter(
-                  context_.get());
-              waiter.DeleteForOrigin(origin, &success);
-              EXPECT_TRUE(success);
-            }
-            loop.Quit();
-          }));
       for (auto origin : open_factory_origins) {
-        IndexedDBOriginState* per_origin_factory =
-            factory->GetOriginFactory(origin);
-        per_origin_factory->backing_store()
-            ->db()
-            ->leveldb_state()
-            ->RequestDestruction(callback,
-                                 base::SequencedTaskRunnerHandle::Get());
         context_->ForceCloseSync(
             origin,
             storage::mojom::ForceCloseReason::FORCE_CLOSE_DELETE_ORIGIN);
       }
-      loop.Run();
+      // All leveldb databases are closed, and they can be deleted.
+      for (auto origin : context_->GetAllOrigins()) {
+        bool success = false;
+        storage::mojom::IndexedDBControlAsyncWaiter waiter(context_.get());
+        waiter.DeleteForOrigin(origin, &success);
+        EXPECT_TRUE(success);
+      }
     }
     if (temp_dir_.IsValid())
       ASSERT_TRUE(temp_dir_.Delete());

content/browser/indexed_db/indexed_db_origin_state.cc

@@ -13,6 +13,7 @@
 #include "base/feature_list.h"
 #include "base/rand_util.h"
 #include "base/stl_util.h"
+#include "base/synchronization/waitable_event.h"
 #include "components/services/storage/indexed_db/transactional_leveldb/transactional_leveldb_database.h"
 #include "components/services/storage/indexed_db/transactional_leveldb/transactional_leveldb_factory.h"
 #include "components/services/storage/indexed_db/transactional_leveldb/transactional_leveldb_transaction.h"
@@ -109,8 +110,16 @@ IndexedDBOriginState::IndexedDBOriginState(
 
 IndexedDBOriginState::~IndexedDBOriginState() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (backing_store_ && backing_store_->IsBlobCleanupPending())
+  if (!backing_store_)
+    return;
+  if (backing_store_->IsBlobCleanupPending())
     backing_store_->ForceRunBlobCleanup();
+
+  base::WaitableEvent leveldb_destruct_event;
+  backing_store_->db()->leveldb_state()->RequestDestruction(
+      &leveldb_destruct_event);
+  backing_store_.reset();
+  leveldb_destruct_event.Wait();
 }
 
 void IndexedDBOriginState::AbortAllTransactions(bool compact) {

content/browser/indexed_db/indexed_db_origin_state.h

@@ -213,7 +213,7 @@ class CONTENT_EXPORT IndexedDBOriginState {
   ClosingState closing_stage_ = ClosingState::kNotClosing;
   base::OneShotTimer close_timer_;
   const std::unique_ptr<DisjointRangeLockManager> lock_manager_;
-  const std::unique_ptr<IndexedDBBackingStore> backing_store_;
+  std::unique_ptr<IndexedDBBackingStore> backing_store_;
 
   OriginDBMap databases_;
   // This is the refcount for the number of IndexedDBOriginStateHandle's given

content/browser/indexed_db/indexed_db_unittest.cc

@@ -119,33 +119,20 @@ class IndexedDBTest : public testing::Test {
       // deletion of the leveldb state. Once the states are no longer around,
       // delete all of the databases on disk.
       auto open_factory_origins = factory->GetOpenOrigins();
-      base::RunLoop loop;
-      auto callback = base::BarrierClosure(
-          open_factory_origins.size(), base::BindLambdaForTesting([&]() {
-            // All leveldb databases are closed, and they can be deleted.
-            for (auto origin : context_->GetAllOrigins()) {
-              bool success = false;
-              storage::mojom::IndexedDBControlAsyncWaiter waiter(
-                  context_.get());
-              waiter.DeleteForOrigin(origin, &success);
-              EXPECT_TRUE(success);
-            }
-            loop.Quit();
-          }));
       for (auto origin : open_factory_origins) {
-        IndexedDBOriginState* per_origin_factory =
-            factory->GetOriginFactory(origin);
-        per_origin_factory->backing_store()
-            ->db()
-            ->leveldb_state()
-            ->RequestDestruction(callback,
-                                 base::SequencedTaskRunnerHandle::Get());
         context_->ForceCloseSync(
             origin,
             storage::mojom::ForceCloseReason::FORCE_CLOSE_DELETE_ORIGIN);
       }
-      loop.Run();
+      // All leveldb databases are closed, and they can be deleted.
+      for (auto origin : context_->GetAllOrigins()) {
+        bool success = false;
+        storage::mojom::IndexedDBControlAsyncWaiter waiter(context_.get());
+        waiter.DeleteForOrigin(origin, &success);
+        EXPECT_TRUE(success);
+      }
     }
+
     if (temp_dir_.IsValid())
       ASSERT_TRUE(temp_dir_.Delete());
   }