Revert "Constrain DOM Storage backend lifetime"

This reverts commit f219c238.

Reason for revert: LocalStorageImplTest.CorruptionOnDisk became flaky.
Three builds are failing in a row.
Example build:
https://ci.chromium.org/p/chromium/builders/ci/Win10%20Tests%20x64%20%28dbg%29/18708

Original change's description:
> Constrain DOM Storage backend lifetime
>
> LocalStorageImpl and SessionStorageImpl are self-owned objects with
> lifetime loosely bound to their corresponding Mojo interface receivers.
> This means it's possible for them to outlive the service itself (i.e.
> the StorageServiceImpl instance) and continue scheduling work during
> the service process's shutdown. They also use shutdown-blocking
> TaskRunners for some of their tasks, which is problematic given the
> potentially too-long lifetime: if a non-shutdown-blocking task races
> with shutdown completion and then attempts to schedule a shutdown-
> blocking task, we have a bad time.
>
> This CL fixes the issue by changing LocalStorageImpl and
> SessionStorageImpl to have stronger ownership within PartitionImpl
> and initiating an explicit shutdown step when the partition is
> torn down. This step effectively inhibits the scheduling of any
> subsequent non-shutdown-blocking tasks for these objects, ensuring
> that any remaining work comes in the form of shutdown-blocking tasks
> scheduled prior to or during the explicit shutdown step, or from
> other shutdown-blocking tasks posted by those tasks. In other words,
> shutdown remains continuously blocked until these objects are done
> scheduling work.
>
> This achieves the same shutdown-blocking behavior without the risk
> of a race against shutdown completion.
>
> Fixed: 1135957
> Change-Id: Ie2fcc25df7fafe390c736d2c56d139433c69bf8c
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2533934
> Reviewed-by: Victor Costan <pwnall@chromium.org>
> Commit-Queue: Ken Rockot <rockot@google.com>
> Cr-Commit-Position: refs/heads/master@{#829483}

TBR=rockot@google.com,pwnall@chromium.org

Change-Id: Ibad7482357ba3122fd88afad31ed4a85b6651c83
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2551866Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#829577}

components/services/storage/dom_storage/local_storage_impl.cc

@@ -479,8 +479,11 @@ LocalStorageImpl::LocalStorageImpl(
       ->RegisterDumpProviderWithSequencedTaskRunner(
           this, "LocalStorage", task_runner, MemoryDumpProvider::Options());
 
-  if (receiver)
+  if (receiver) {
     control_receiver_.Bind(std::move(receiver));
+    control_receiver_.set_disconnect_handler(base::BindOnce(
+        &LocalStorageImpl::ShutdownAndDelete, base::Unretained(this)));
+  }
 }
 
 void LocalStorageImpl::BindStorageArea(
@@ -582,17 +585,13 @@ void LocalStorageImpl::FlushOriginForTesting(const url::Origin& origin) {
   it->second->storage_area()->ScheduleImmediateCommit();
 }
 
-void LocalStorageImpl::ShutDown(base::OnceClosure callback) {
+void LocalStorageImpl::ShutdownAndDelete() {
   DCHECK_NE(connection_state_, CONNECTION_SHUTDOWN);
-  DCHECK(callback);
-
-  control_receiver_.reset();
-  shutdown_complete_callback_ = std::move(callback);
 
   // Nothing to do if no connection to the database was ever finished.
   if (connection_state_ != CONNECTION_FINISHED) {
     connection_state_ = CONNECTION_SHUTDOWN;
-    OnShutdownComplete();
+    OnShutdownComplete(leveldb::Status::OK());
     return;
   }
 
@@ -601,8 +600,9 @@ void LocalStorageImpl::ShutDown(base::OnceClosure callback) {
   // Flush any uncommitted data.
   for (const auto& it : areas_) {
     auto* area = it.second->storage_area();
-    LOCAL_HISTOGRAM_BOOLEAN("LocalStorageContext.ShutDown.MaybeDroppedChanges",
-                            area->has_pending_load_tasks());
+    LOCAL_HISTOGRAM_BOOLEAN(
+        "LocalStorageContext.ShutdownAndDelete.MaybeDroppedChanges",
+        area->has_pending_load_tasks());
     area->ScheduleImmediateCommit();
     // TODO(dmurph): Monitor the above histogram, and if dropping changes is
     // common then handle that here.
@@ -612,7 +612,7 @@ void LocalStorageImpl::ShutDown(base::OnceClosure callback) {
   // Respect the content policy settings about what to
   // keep and what to discard.
   if (force_keep_session_state_) {
-    OnShutdownComplete();
+    OnShutdownComplete(leveldb::Status::OK());
     return;  // Keep everything.
   }
 
@@ -621,7 +621,7 @@ void LocalStorageImpl::ShutDown(base::OnceClosure callback) {
         base::BindOnce(&LocalStorageImpl::OnGotStorageUsageForShutdown,
                        base::Unretained(this)));
   } else {
-    OnShutdownComplete();
+    OnShutdownComplete(leveldb::Status::OK());
   }
 }
 
@@ -797,9 +797,6 @@ void LocalStorageImpl::RunWhenConnected(base::OnceClosure callback) {
 }
 
 void LocalStorageImpl::InitiateConnection(bool in_memory_only) {
-  if (connection_state_ == CONNECTION_SHUTDOWN)
-    return;
-
   DCHECK_EQ(connection_state_, CONNECTION_IN_PROGRESS);
 
   if (!directory_.empty() && directory_.IsAbsolute() && !in_memory_only) {
@@ -896,9 +893,6 @@ void LocalStorageImpl::OnGotDatabaseVersion(leveldb::Status status,
 }
 
 void LocalStorageImpl::OnConnectionFinished() {
-  if (connection_state_ == CONNECTION_SHUTDOWN)
-    return;
-
   DCHECK_EQ(connection_state_, CONNECTION_IN_PROGRESS);
   // If connection was opened successfully, reset tried_to_recreate_during_open_
   // to enable recreating the database on future errors.
@@ -917,9 +911,6 @@ void LocalStorageImpl::OnConnectionFinished() {
 }
 
 void LocalStorageImpl::DeleteAndRecreateDatabase(const char* histogram_name) {
-  if (connection_state_ == CONNECTION_SHUTDOWN)
-    return;
-
   // We're about to set database_ to null, so delete the StorageAreaImpls
   // that might still be using the old database.
   for (const auto& it : areas_)
@@ -1084,22 +1075,15 @@ void LocalStorageImpl::OnGotStorageUsageForShutdown(
 
   if (!origins_to_delete.empty()) {
     DeleteOrigins(database_.get(), std::move(origins_to_delete),
-                  base::BindOnce(&LocalStorageImpl::OnOriginsDeleted,
+                  base::BindOnce(&LocalStorageImpl::OnShutdownComplete,
                                  base::Unretained(this)));
   } else {
-    OnShutdownComplete();
+    OnShutdownComplete(leveldb::Status::OK());
   }
 }
 
-void LocalStorageImpl::OnOriginsDeleted(leveldb::Status status) {
-  OnShutdownComplete();
-}
-
-void LocalStorageImpl::OnShutdownComplete() {
-  DCHECK(shutdown_complete_callback_);
-  // Flush any final tasks on DB task runner before invoking the callback.
-  leveldb_task_runner_->PostTaskAndReply(
-      FROM_HERE, base::DoNothing(), std::move(shutdown_complete_callback_));
+void LocalStorageImpl::OnShutdownComplete(leveldb::Status status) {
+  delete this;
 }
 
 void LocalStorageImpl::GetStatistics(size_t* total_cache_size,

components/services/storage/dom_storage/local_storage_impl.h

@@ -51,7 +51,6 @@ class LocalStorageImpl : public base::trace_event::MemoryDumpProvider,
                    scoped_refptr<base::SequencedTaskRunner> task_runner,
                    scoped_refptr<base::SequencedTaskRunner> legacy_task_runner,
                    mojo::PendingReceiver<mojom::LocalStorageControl> receiver);
-  ~LocalStorageImpl() override;
 
   void FlushOriginForTesting(const url::Origin& origin);
 
@@ -62,11 +61,10 @@ class LocalStorageImpl : public base::trace_event::MemoryDumpProvider,
   void SetForceKeepSessionState() { force_keep_session_state_ = true; }
 
   // Called when the owning BrowserContext is ending.
-  // Schedules the commit of any unsaved changes and will delete or keep data on
-  // disk per the content settings and special storage policies.  `callback` is
-  // invoked when shutdown is complete, which may happen even before ShutDown
-  // returns.
-  void ShutDown(base::OnceClosure callback);
+  // Schedules the commit of any unsaved changes and will delete
+  // and keep data on disk per the content settings and special storage
+  // policies.
+  void ShutdownAndDelete();
 
   // Clears unused storage areas, when thresholds are reached.
   void PurgeUnusedAreasIfNeeded();
@@ -107,6 +105,8 @@ class LocalStorageImpl : public base::trace_event::MemoryDumpProvider,
 
   class StorageAreaHolder;
 
+  ~LocalStorageImpl() override;
+
   // Runs |callback| immediately if already connected to a database, otherwise
   // delays running |callback| untill after a connection has been established.
   // Initiates connecting to the database if no connection is in progres yet.
@@ -131,8 +131,7 @@ class LocalStorageImpl : public base::trace_event::MemoryDumpProvider,
 
   void OnGotStorageUsageForShutdown(
       std::vector<mojom::LocalStorageUsageInfoPtr> usage);
-  void OnOriginsDeleted(leveldb::Status status);
-  void OnShutdownComplete();
+  void OnShutdownComplete(leveldb::Status status);
 
   void GetStatistics(size_t* total_cache_size, size_t* unused_area_count);
   void OnCommitResult(leveldb::Status status);
@@ -192,8 +191,6 @@ class LocalStorageImpl : public base::trace_event::MemoryDumpProvider,
 
   mojo::Receiver<mojom::LocalStorageControl> control_receiver_{this};
 
-  base::OnceClosure shutdown_complete_callback_;
-
   base::WeakPtrFactory<LocalStorageImpl> weak_ptr_factory_{this};
 };
 

components/services/storage/dom_storage/session_storage_impl.cc

@@ -115,6 +115,8 @@ SessionStorageImpl::SessionStorageImpl(
       ->RegisterDumpProviderWithSequencedTaskRunner(
           this, "SessionStorage", std::move(memory_dump_task_runner),
           base::trace_event::MemoryDumpProvider::Options());
+  receiver_.set_disconnect_handler(base::BindOnce(
+      &SessionStorageImpl::ShutdownAndDelete, base::Unretained(this)));
 }
 
 SessionStorageImpl::~SessionStorageImpl() {
@@ -379,12 +381,8 @@ void SessionStorageImpl::CleanUpStorage(CleanUpStorageCallback callback) {
   }
 }
 
-void SessionStorageImpl::ShutDown(base::OnceClosure callback) {
+void SessionStorageImpl::ShutdownAndDelete() {
   DCHECK_NE(connection_state_, CONNECTION_SHUTDOWN);
-  DCHECK(callback);
-
-  receiver_.reset();
-  shutdown_complete_callback_ = std::move(callback);
 
   // The namespaces will DCHECK if they are destructed with pending clones. It
   // is valid to drop these on shutdown.
@@ -395,7 +393,7 @@ void SessionStorageImpl::ShutDown(base::OnceClosure callback) {
   // Nothing to do if no connection to the database was ever finished.
   if (connection_state_ != CONNECTION_FINISHED) {
     connection_state_ = CONNECTION_SHUTDOWN;
-    OnShutdownComplete();
+    OnShutdownComplete(leveldb::Status::OK());
     return;
   }
   connection_state_ = CONNECTION_SHUTDOWN;
@@ -404,7 +402,7 @@ void SessionStorageImpl::ShutDown(base::OnceClosure callback) {
   for (const auto& it : data_maps_) {
     auto* area = it.second->storage_area();
     LOCAL_HISTOGRAM_BOOLEAN(
-        "SessionStorageContext.ShutDown.MaybeDroppedChanges",
+        "SessionStorageContext.ShutdownAndDelete.MaybeDroppedChanges",
         area->has_pending_load_tasks());
     area->ScheduleImmediateCommit();
     // TODO(dmurph): Monitor the above histogram, and if dropping changes is
@@ -412,7 +410,7 @@ void SessionStorageImpl::ShutDown(base::OnceClosure callback) {
     area->CancelAllPendingRequests();
   }
 
-  OnShutdownComplete();
+  OnShutdownComplete(leveldb::Status::OK());
 }
 
 void SessionStorageImpl::PurgeMemory() {
@@ -605,9 +603,6 @@ void SessionStorageImpl::OnDataMapDestruction(
 }
 
 void SessionStorageImpl::OnCommitResult(leveldb::Status status) {
-  if (connection_state_ == CONNECTION_SHUTDOWN)
-    return;
-
   DCHECK_EQ(connection_state_, CONNECTION_FINISHED);
   UMA_HISTOGRAM_ENUMERATION("SessionStorageContext.CommitResult",
                             leveldb_env::GetLevelDBStatusUMAValue(status),
@@ -843,9 +838,6 @@ void SessionStorageImpl::OnGotDatabaseMetadata(
     ValueAndStatus version,
     KeyValuePairsAndStatus namespaces,
     ValueAndStatus next_map_id) {
-  if (connection_state_ == CONNECTION_SHUTDOWN)
-    return;
-
   std::vector<AsyncDomStorageDatabase::BatchDatabaseTask> migration_tasks;
 
   MetadataParseResult version_parse =
@@ -972,9 +964,6 @@ SessionStorageImpl::MetadataParseResult SessionStorageImpl::ParseNextMapId(
 }
 
 void SessionStorageImpl::OnConnectionFinished() {
-  if (connection_state_ == CONNECTION_SHUTDOWN)
-    return;
-
   DCHECK(!database_ || connection_state_ == CONNECTION_IN_PROGRESS);
 
   // If connection was opened successfully, reset tried_to_recreate_during_open_
@@ -996,9 +985,6 @@ void SessionStorageImpl::OnConnectionFinished() {
 }
 
 void SessionStorageImpl::DeleteAndRecreateDatabase(const char* histogram_name) {
-  if (connection_state_ == CONNECTION_SHUTDOWN)
-    return;
-
   // We're about to set database_ to null, so delete the StorageAreas
   // that might still be using the old database.
   for (const auto& it : data_maps_)
@@ -1056,11 +1042,8 @@ void SessionStorageImpl::OnDBDestroyed(bool recreate_in_memory,
   InitiateConnection(recreate_in_memory);
 }
 
-void SessionStorageImpl::OnShutdownComplete() {
-  DCHECK(shutdown_complete_callback_);
-  // Flush any final tasks on DB task runner before invoking the callback.
-  leveldb_task_runner_->PostTaskAndReply(
-      FROM_HERE, base::DoNothing(), std::move(shutdown_complete_callback_));
+void SessionStorageImpl::OnShutdownComplete(leveldb::Status status) {
+  delete this;
 }
 
 void SessionStorageImpl::GetStatistics(size_t* total_cache_size,

components/services/storage/dom_storage/session_storage_impl.h

@@ -15,7 +15,6 @@
 #include "base/callback_forward.h"
 #include "base/files/file_path.h"
 #include "base/gtest_prod_util.h"
-#include "base/memory/ref_counted.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/threading/sequence_bound.h"
@@ -68,8 +67,6 @@ class SessionStorageImpl : public base::trace_event::MemoryDumpProvider,
       std::string leveldb_name,
       mojo::PendingReceiver<mojom::SessionStorageControl> receiver);
 
-  ~SessionStorageImpl() override;
-
   // mojom::SessionStorageControl implementation:
   void BindNamespace(
       const std::string& namespace_id,
@@ -97,11 +94,10 @@ class SessionStorageImpl : public base::trace_event::MemoryDumpProvider,
                        bool should_persist) override;
 
   // Called when the client (i.e. the corresponding browser storage partition)
-  // disconnects. Schedules the commit of any unsaved changes. All data on disk
-  // (where there was no call to DeleteNamespace will stay on disk for later
-  // restoring. `callback` is invoked when shutdown is complete, which may
-  // happen even before ShutDown returns.
-  void ShutDown(base::OnceClosure callback);
+  // disconnects. Schedules the commit of any unsaved changes then deletes this
+  // object. All data on disk (where there was no call to
+  // |DeleteNamespace| will stay on disk for later restoring.
+  void ShutdownAndDelete();
 
   // Clears unused storage areas, when thresholds are reached.
   void PurgeUnusedAreasIfNeeded();
@@ -153,6 +149,9 @@ class SessionStorageImpl : public base::trace_event::MemoryDumpProvider,
     kMaxValue = kSuccess
   };
 
+  // Object deletion is done through |ShutdownAndDelete()|.
+  ~SessionStorageImpl() override;
+
   scoped_refptr<SessionStorageMetadata::MapData> RegisterNewAreaMap(
       SessionStorageMetadata::NamespaceEntry namespace_entry,
       const url::Origin& origin);
@@ -224,7 +223,7 @@ class SessionStorageImpl : public base::trace_event::MemoryDumpProvider,
   void DeleteAndRecreateDatabase(const char* histogram_name);
   void OnDBDestroyed(bool recreate_in_memory, leveldb::Status status);
 
-  void OnShutdownComplete();
+  void OnShutdownComplete(leveldb::Status status);
 
   void GetStatistics(size_t* total_cache_size, size_t* unused_areas_count);
 
@@ -283,8 +282,6 @@ class SessionStorageImpl : public base::trace_event::MemoryDumpProvider,
   // Name of an extra histogram to log open results to, if not null.
   const char* open_result_histogram_ = nullptr;
 
-  base::OnceClosure shutdown_complete_callback_;
-
   base::WeakPtrFactory<SessionStorageImpl> weak_ptr_factory_{this};
 };
 

components/services/storage/dom_storage/session_storage_impl_unittest.cc

@@ -6,7 +6,6 @@
 
 #include <stdint.h>
 
-#include <memory>
 #include <vector>
 
 #include "base/bind.h"
@@ -58,6 +57,10 @@ class SessionStorageImplTest : public testing::Test {
   SessionStorageImplTest() { CHECK(temp_dir_.CreateUniqueTempDir()); }
 
   ~SessionStorageImplTest() override {
+    // There may be pending tasks to clean up files in the temp dir. Make sure
+    // they run so temp dir deletion can succeed.
+    RunUntilIdle();
+
     EXPECT_TRUE(temp_dir_.Delete());
   }
 
@@ -82,13 +85,13 @@ class SessionStorageImplTest : public testing::Test {
   SessionStorageImpl* session_storage_impl() {
     if (!session_storage_) {
       remote_session_storage_.reset();
-      session_storage_ = std::make_unique<SessionStorageImpl>(
+      session_storage_ = new SessionStorageImpl(
           temp_path(), blocking_task_runner_,
           base::SequencedTaskRunnerHandle::Get(), backing_mode_,
           kSessionStorageDirectory,
           remote_session_storage_.BindNewPipeAndPassReceiver());
     }
-    return session_storage_.get();
+    return session_storage_;
   }
 
   mojom::SessionStorageControl* session_storage() {
@@ -98,11 +101,9 @@ class SessionStorageImplTest : public testing::Test {
 
   void ShutDownSessionStorage() {
     remote_session_storage_.FlushForTesting();
-
-    base::RunLoop loop;
-    session_storage_->ShutDown(loop.QuitClosure());
-    loop.Run();
-    session_storage_.reset();
+    session_storage_->ShutdownAndDelete();
+    session_storage_ = nullptr;
+    RunUntilIdle();
   }
 
   void DoTestPut(const std::string& namespace_id,
@@ -160,7 +161,7 @@ class SessionStorageImplTest : public testing::Test {
   scoped_refptr<base::SequencedTaskRunner> blocking_task_runner_{
       base::ThreadPool::CreateSequencedTaskRunner(
           {base::MayBlock(), base::TaskShutdownBehavior::BLOCK_SHUTDOWN})};
-  std::unique_ptr<SessionStorageImpl> session_storage_;
+  SessionStorageImpl* session_storage_ = nullptr;
   mojo::Remote<mojom::SessionStorageControl> remote_session_storage_;
 
   DISALLOW_COPY_AND_ASSIGN(SessionStorageImplTest);

components/services/storage/partition_impl.cc

@@ -4,12 +4,9 @@
 
 #include "components/services/storage/partition_impl.h"
 
-#include <memory>
 #include <utility>
 
 #include "base/bind.h"
-#include "base/sequenced_task_runner.h"
-#include "base/synchronization/waitable_event.h"
 #include "base/task/post_task.h"
 #include "base/task/thread_pool.h"
 #include "base/threading/sequenced_task_runner_handle.h"
@@ -24,26 +21,6 @@ namespace {
 
 const char kSessionStorageDirectory[] = "Session Storage";
 
-template <typename T>
-base::OnceClosure MakeDeferredDeleter(std::unique_ptr<T> object) {
-  return base::BindOnce(
-      [](scoped_refptr<base::SequencedTaskRunner> task_runner, T* object) {
-        task_runner->DeleteSoon(FROM_HERE, object);
-      },
-      base::SequencedTaskRunnerHandle::Get(),
-      // NOTE: We release `object` immediately. In the case
-      // where this task never runs, we prefer to leak the
-      // object rather than potentilaly destroying it on the
-      // wrong sequence.
-      object.release());
-}
-
-template <typename T>
-void ShutDown(std::unique_ptr<T> object) {
-  if (T* ptr = object.get())
-    ptr->ShutDown(MakeDeferredDeleter(std::move(object)));
-}
-
 }  // namespace
 
 PartitionImpl::PartitionImpl(StorageServiceImpl* service,
@@ -53,10 +30,7 @@ PartitionImpl::PartitionImpl(StorageServiceImpl* service,
       &PartitionImpl::OnDisconnect, base::Unretained(this)));
 }
 
-PartitionImpl::~PartitionImpl() {
-  ShutDown(std::move(local_storage_));
-  ShutDown(std::move(session_storage_));
-}
+PartitionImpl::~PartitionImpl() = default;
 
 void PartitionImpl::BindReceiver(
     mojo::PendingReceiver<mojom::Partition> receiver) {
@@ -81,7 +55,8 @@ void PartitionImpl::BindOriginContext(
 
 void PartitionImpl::BindSessionStorageControl(
     mojo::PendingReceiver<mojom::SessionStorageControl> receiver) {
-  session_storage_ = std::make_unique<SessionStorageImpl>(
+  // This object deletes itself on disconnection.
+  session_storage_ = new SessionStorageImpl(
       path_.value_or(base::FilePath()),
       base::ThreadPool::CreateSequencedTaskRunner(
           {base::MayBlock(), base::WithBaseSyncPrimitives(),
@@ -101,7 +76,8 @@ void PartitionImpl::BindSessionStorageControl(
 
 void PartitionImpl::BindLocalStorageControl(
     mojo::PendingReceiver<mojom::LocalStorageControl> receiver) {
-  local_storage_ = std::make_unique<LocalStorageImpl>(
+  // This object deletes itself on disconnection.
+  local_storage_ = new LocalStorageImpl(
       path_.value_or(base::FilePath()), base::SequencedTaskRunnerHandle::Get(),
       base::ThreadPool::CreateSequencedTaskRunner(
           {base::MayBlock(), base::WithBaseSyncPrimitives(),

components/services/storage/partition_impl.h

@@ -63,8 +63,9 @@ class PartitionImpl : public mojom::Partition {
   mojo::ReceiverSet<mojom::Partition> receivers_;
   std::map<url::Origin, std::unique_ptr<OriginContextImpl>> origin_contexts_;
 
-  std::unique_ptr<SessionStorageImpl> session_storage_;
-  std::unique_ptr<LocalStorageImpl> local_storage_;
+  // These objects own themselves.
+  SessionStorageImpl* session_storage_;
+  LocalStorageImpl* local_storage_;
 
   DISALLOW_COPY_AND_ASSIGN(PartitionImpl);
 };