SessionStorage: Pause mojo receiver while connecting to database.

Creation and deletion of namespace should never be re-ordered. Creation
doesn't depend on having the database open (which often allows delaying
opening the database until after chrome startup has completed), however
deletion does sometimes require the database. Previously part of the
delete operation could be reordered to happen after later create
operations, resulting in situations where state could get out of sync.

This CL fixes all that by completely pausing processing incoming mojo
calls while the database is being opened. This will ensure that no new
namespaces get created while we're waiting for the database to be opened
while in the process of deleting a namespace.

Bug: 1128318
Change-Id: Iea28b92172da88d3719853a42dbb323da2522eb7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2419124
Auto-Submit: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809187}

components/services/storage/dom_storage/session_storage_impl.cc

@@ -188,6 +188,7 @@ void SessionStorageImpl::BindStorageArea(
 }
 
 void SessionStorageImpl::CreateNamespace(const std::string& namespace_id) {
+  DCHECK_NE(connection_state_, CONNECTION_IN_PROGRESS);
   if (namespaces_.find(namespace_id) != namespaces_.end())
     return;
 
@@ -199,6 +200,7 @@ void SessionStorageImpl::CloneNamespace(
     const std::string& clone_from_namespace_id,
     const std::string& clone_to_namespace_id,
     mojom::SessionStorageCloneType clone_type) {
+  DCHECK_NE(connection_state_, CONNECTION_IN_PROGRESS);
   if (namespaces_.find(clone_to_namespace_id) != namespaces_.end()) {
     // Non-immediate clones expect to be paired with a |Clone| from the mojo
     // namespace object. If that clone has already happened, then we don't need
@@ -264,6 +266,7 @@ void SessionStorageImpl::CloneNamespace(
 
 void SessionStorageImpl::DeleteNamespace(const std::string& namespace_id,
                                          bool should_persist) {
+  DCHECK_NE(connection_state_, CONNECTION_IN_PROGRESS);
   auto namespace_it = namespaces_.find(namespace_id);
   // If the namespace has pending clones, do the clone now before destroying it.
   if (namespace_it != namespaces_.end()) {
@@ -710,6 +713,7 @@ void SessionStorageImpl::RunWhenConnected(base::OnceClosure callback) {
     case NO_CONNECTION:
       // If we don't have a filesystem_connection_, we'll need to establish one.
       connection_state_ = CONNECTION_IN_PROGRESS;
+      receiver_.Pause();
       on_database_opened_callbacks_.push_back(std::move(callback));
       InitiateConnection();
       return;
@@ -973,6 +977,7 @@ void SessionStorageImpl::OnConnectionFinished() {
   // |database_| should be known to either be valid or invalid by now. Run our
   // delayed bindings.
   connection_state_ = CONNECTION_FINISHED;
+  receiver_.Resume();
   std::vector<base::OnceClosure> callbacks;
   std::swap(callbacks, on_database_opened_callbacks_);
   for (size_t i = 0; i < callbacks.size(); ++i)
@@ -993,6 +998,7 @@ void SessionStorageImpl::DeleteAndRecreateDatabase(const char* histogram_name) {
   // Reset state to be in process of connecting. This will cause requests for
   // StorageAreas to be queued until the connection is complete.
   connection_state_ = CONNECTION_IN_PROGRESS;
+  receiver_.Pause();
   commit_error_count_ = 0;
   database_.reset();
   open_result_histogram_ = histogram_name;

components/services/storage/dom_storage/session_storage_impl.h

@@ -119,14 +119,23 @@ class SessionStorageImpl : public base::trace_event::MemoryDumpProvider,
     return database_->database();
   }
 
+  const SessionStorageMetadata& GetMetadataForTesting() const {
+    return metadata_;
+  }
+
+  SessionStorageNamespaceImpl* GetNamespaceForTesting(const std::string& id) {
+    auto it = namespaces_.find(id);
+    if (it == namespaces_.end())
+      return nullptr;
+    return it->second.get();
+  }
+
   // Wait for the database to be opened, or for opening to fail. If the database
   // is already opened, |callback| is invoked immediately.
   void SetDatabaseOpenCallbackForTesting(base::OnceClosure callback);
 
  private:
   friend class DOMStorageBrowserTest;
-  FRIEND_TEST_ALL_PREFIXES(SessionStorageImplTest,
-                           PurgeMemoryDoesNotCrashOrHang);
 
   // These values are written to logs.  New enum values can be added, but
   // existing enums must never be renumbered or deleted and reused.

components/services/storage/dom_storage/session_storage_impl_unittest.cc

@@ -82,7 +82,7 @@ class SessionStorageImplTest : public testing::Test {
     backing_mode_ = backing_mode;
   }
 
-  SessionStorageImpl* session_storage() {
+  SessionStorageImpl* session_storage_impl() {
     if (!session_storage_) {
       remote_session_storage_.reset();
       session_storage_ = new SessionStorageImpl(
@@ -94,7 +94,13 @@ class SessionStorageImplTest : public testing::Test {
     return session_storage_;
   }
 
+  mojom::SessionStorageControl* session_storage() {
+    session_storage_impl();
+    return remote_session_storage_.get();
+  }
+
   void ShutDownSessionStorage() {
+    remote_session_storage_.FlushForTesting();
     session_storage_->ShutdownAndDelete();
     session_storage_ = nullptr;
     RunUntilIdle();
@@ -143,6 +149,7 @@ class SessionStorageImplTest : public testing::Test {
  protected:
   const base::FilePath& temp_path() const { return temp_dir_.GetPath(); }
   void RunUntilIdle() { task_environment_.RunUntilIdle(); }
+  void FlushMojo() { remote_session_storage_.FlushForTesting(); }
 
   bool bad_message_called_ = false;
 
@@ -408,6 +415,7 @@ TEST_F(SessionStorageImplTest, ImmediateCloning) {
 
   // Delete that namespace, copy again after a put.
   session_storage()->DeleteNamespace(namespace_id2, false);
+  FlushMojo();
 
   // Put some data.
   EXPECT_TRUE(test::PutSync(area_n1.get(), StringPieceToUint8Vector("key1"),
@@ -429,6 +437,7 @@ TEST_F(SessionStorageImplTest, ImmediateCloning) {
   }
 
   session_storage()->DeleteNamespace(namespace_id2, false);
+  FlushMojo();
 
   // Verify that cloning from the namespace object will result in a bad message.
   session_storage()->CloneNamespace(namespace_id1, namespace_id2,
@@ -612,7 +621,7 @@ TEST_F(SessionStorageImplTest, RecreateOnCommitFailure) {
   base::Optional<base::RunLoop> open_loop;
   size_t num_database_open_requests = 0;
   size_t num_databases_destroyed = 0;
-  session_storage()->SetDatabaseOpenCallbackForTesting(
+  session_storage_impl()->SetDatabaseOpenCallbackForTesting(
       base::BindLambdaForTesting([&] {
         ++num_database_open_requests;
         open_loop->Quit();
@@ -641,7 +650,7 @@ TEST_F(SessionStorageImplTest, RecreateOnCommitFailure) {
   open_loop->Run();
 
   // Ensure that the first opened database always fails to write data.
-  session_storage()->GetDatabaseForTesting().PostTaskWithThisObject(
+  session_storage_impl()->GetDatabaseForTesting().PostTaskWithThisObject(
       FROM_HERE, base::BindLambdaForTesting([&](DomStorageDatabase* db) {
         db->MakeAllCommitsFailForTesting();
         db->SetDestructionCallbackForTesting(
@@ -658,8 +667,7 @@ TEST_F(SessionStorageImplTest, RecreateOnCommitFailure) {
 
   // Also prepare for another database connection, next time providing a
   // functioning database.
-  open_loop.emplace();
-  session_storage()->SetDatabaseOpenCallbackForTesting(
+  session_storage_impl()->SetDatabaseOpenCallbackForTesting(
       base::BindLambdaForTesting([&] {
         ++num_database_open_requests;
         open_loop->Quit();
@@ -690,7 +698,7 @@ TEST_F(SessionStorageImplTest, RecreateOnCommitFailure) {
     RunUntilIdle();
     // And we need to flush after every change. Otherwise changes get batched up
     // and only one commit is done some time later.
-    session_storage()->FlushAreaForTesting(namespace_id, origin1);
+    session_storage_impl()->FlushAreaForTesting(namespace_id, origin1);
   }
   area_o1.reset();
 
@@ -745,7 +753,7 @@ TEST_F(SessionStorageImplTest, DontRecreateOnRepeatedCommitFailure) {
   base::Optional<base::RunLoop> open_loop;
   size_t num_database_open_requests = 0;
   size_t num_databases_destroyed = 0;
-  session_storage()->SetDatabaseOpenCallbackForTesting(
+  session_storage_impl()->SetDatabaseOpenCallbackForTesting(
       base::BindLambdaForTesting([&] {
         ++num_database_open_requests;
         open_loop->Quit();
@@ -761,7 +769,7 @@ TEST_F(SessionStorageImplTest, DontRecreateOnRepeatedCommitFailure) {
   open_loop->Run();
 
   // Ensure that this database always fails to write data.
-  session_storage()->GetDatabaseForTesting().PostTaskWithThisObject(
+  session_storage_impl()->GetDatabaseForTesting().PostTaskWithThisObject(
       FROM_HERE, base::BindLambdaForTesting([&](DomStorageDatabase* db) {
         db->MakeAllCommitsFailForTesting();
         db->SetDestructionCallbackForTesting(
@@ -775,13 +783,13 @@ TEST_F(SessionStorageImplTest, DontRecreateOnRepeatedCommitFailure) {
   // reconnect to the database, which should happen after several commit
   // errors.
   open_loop.emplace();
-  session_storage()->SetDatabaseOpenCallbackForTesting(
+  session_storage_impl()->SetDatabaseOpenCallbackForTesting(
       base::BindLambdaForTesting([&] {
         ++num_database_open_requests;
         open_loop->Quit();
 
         // Ensure that this database also always fails to write data.
-        session_storage()->GetDatabaseForTesting().Post(
+        session_storage_impl()->GetDatabaseForTesting().Post(
             FROM_HERE, &DomStorageDatabase::MakeAllCommitsFailForTesting);
       }));
 
@@ -798,7 +806,7 @@ TEST_F(SessionStorageImplTest, DontRecreateOnRepeatedCommitFailure) {
     RunUntilIdle();
     // And we need to flush after every change. Otherwise changes get batched up
     // and only one commit is done some time later.
-    session_storage()->FlushAreaForTesting(namespace_id, origin1);
+    session_storage_impl()->FlushAreaForTesting(namespace_id, origin1);
 
     old_value = value;
     value[0]++;
@@ -831,7 +839,7 @@ TEST_F(SessionStorageImplTest, DontRecreateOnRepeatedCommitFailure) {
     RunUntilIdle();
     // And we need to flush after every change. Otherwise changes get batched up
     // and only one commit is done some time later.
-    session_storage()->FlushAreaForTesting(namespace_id, origin1);
+    session_storage_impl()->FlushAreaForTesting(namespace_id, origin1);
 
     old_value = value;
     value[0]++;
@@ -930,13 +938,13 @@ TEST_F(SessionStorageImplTest, PurgeInactiveWrappers) {
   EXPECT_TRUE(test::PutSync(area.get(), StringPieceToUint8Vector("key1"),
                             StringPieceToUint8Vector("value1"), base::nullopt,
                             "source1"));
-  session_storage()->FlushAreaForTesting(namespace_id1, origin1);
+  session_storage_impl()->FlushAreaForTesting(namespace_id1, origin1);
 
   area.reset();
 
   // Clear all the data from the backing database.
   base::RunLoop loop;
-  session_storage()->DatabaseForTesting()->DeletePrefixed(
+  session_storage_impl()->DatabaseForTesting()->DeletePrefixed(
       StringPieceToUint8Vector("map"),
       base::BindLambdaForTesting([&](leveldb::Status status) {
         loop.Quit();
@@ -1146,17 +1154,17 @@ TEST_F(SessionStorageImplTest, PurgeMemoryDoesNotCrashOrHang) {
                             StringPieceToUint8Vector("value2"), base::nullopt,
                             "source1"));
 
-  session_storage()->FlushAreaForTesting(namespace_id1, origin1);
+  session_storage_impl()->FlushAreaForTesting(namespace_id1, origin1);
 
   area_n2.reset();
 
   RunUntilIdle();
 
   // Verify this doesn't crash or hang.
-  session_storage()->PurgeMemory();
+  session_storage_impl()->PurgeMemory();
 
-  size_t memory_used = session_storage()
-                           ->namespaces_[namespace_id1]
+  size_t memory_used = session_storage_impl()
+                           ->GetNamespaceForTesting(namespace_id1)
                            ->origin_areas_[origin1]
                            ->data_map()
                            ->storage_area()
@@ -1280,4 +1288,41 @@ TEST_F(SessionStorageImplTest, DeleteAfterCloneWithoutMojoClone) {
   EXPECT_EQ(1ul, data.size());
 }
 
+// Regression test for https://crbug.com/1128318
+TEST_F(SessionStorageImplTest, Bug1128318) {
+  std::string namespace_id1 = base::GenerateGUID();
+  std::string namespace_id2 = base::GenerateGUID();
+  std::string namespace_id3 = base::GenerateGUID();
+
+  // Create two namespaces by cloning.
+  session_storage()->CloneNamespace(
+      namespace_id1, namespace_id2,
+      mojom::SessionStorageCloneType::kWaitForCloneOnNamespace);
+  session_storage()->CloneNamespace(
+      namespace_id2, namespace_id3,
+      mojom::SessionStorageCloneType::kWaitForCloneOnNamespace);
+  // And delete both namespaces again. Since namespace_id2 has child namespaces
+  // that are waiting for a clone (namespace_id3), the delete will not complete
+  // until the database has been initialized.
+  session_storage()->DeleteNamespace(namespace_id2, false);
+  session_storage()->DeleteNamespace(namespace_id3, false);
+  // Now recreate one of the namespaces. The previous delete should have fully
+  // completed before the namespace is recreated to prevent any dangling
+  // references.
+  session_storage()->CloneNamespace(
+      namespace_id2, namespace_id3,
+      mojom::SessionStorageCloneType::kWaitForCloneOnNamespace);
+  RunUntilIdle();
+
+  // At this point `namespace_id3` should be alive. It should not exist in meta
+  // data yet, as that would only be populated once the actual clone happens.
+  // As such, the namespace_entry for the namespace should be null.
+  auto* ns = session_storage_impl()->GetNamespaceForTesting(namespace_id3);
+  EXPECT_TRUE(ns);
+  EXPECT_FALSE(base::Contains(
+      session_storage_impl()->GetMetadataForTesting().namespace_origin_map(),
+      namespace_id3));
+  EXPECT_EQ(ns->namespace_entry(), SessionStorageMetadata::NamespaceEntry());
+}
+
 }  // namespace storage