[SessionStorageS13N] Fixed missing namespaces and infinite callbacks

A namespace could be purged between creation and being bound to, so
this removes purging of namespaces to prevent that.

When in the FETCHING_METADATA state, RunWhenConnected would run run
callbacks in an infinite loop. This removes that state.

R=mek@chromium.org

Bug: 854102, 848651, 854359
Change-Id: I0fe09caa251ed29bed279c66578cfbaec08da13d
Reviewed-on: https://chromium-review.googlesource.com/1106806
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568692}

content/browser/dom_storage/session_storage_context_mojo.cc

@@ -128,7 +128,10 @@ void SessionStorageContextMojo::OpenSessionStorage(
     return;
   }
   auto found = namespaces_.find(namespace_id);
-  DCHECK(found != namespaces_.end()) << namespace_id;
+  if (found == namespaces_.end()) {
+    mojo::ReportBadMessage("Namespace not found: " + namespace_id);
+    return;
+  }
 
   if (!found->second->IsPopulated() &&
       !found->second->waiting_on_clone_population()) {
@@ -137,8 +140,8 @@ void SessionStorageContextMojo::OpenSessionStorage(
         data_maps_);
   }
 
-  found->second->Bind(std::move(request), process_id);
   PurgeUnusedAreasIfNeeded();
+  found->second->Bind(std::move(request), process_id);
 
   size_t total_cache_size, unused_area_count;
   GetStatistics(&total_cache_size, &unused_area_count);
@@ -302,13 +305,8 @@ void SessionStorageContextMojo::PurgeMemory() {
   GetStatistics(&total_cache_size, &unused_area_count);
 
   // Purge all areas that don't have bindings.
-  for (auto it = namespaces_.begin(); it != namespaces_.end();) {
-    if (!it->second->IsBound()) {
-      it = namespaces_.erase(it);
-      continue;
-    }
-    it->second->PurgeUnboundAreas();
-    ++it;
+  for (const auto& namespace_pair : namespaces_) {
+    namespace_pair.second->PurgeUnboundAreas();
   }
   // Purge memory from bound maps.
   for (const auto& data_map_pair : data_maps_) {
@@ -345,14 +343,9 @@ void SessionStorageContextMojo::PurgeUnusedAreasIfNeeded() {
   if (purge_reason == SessionStorageCachePurgeReason::kNotNeeded)
     return;
 
-  // Purge all namespaces and areas that don't have bindings.
-  for (auto it = namespaces_.begin(); it != namespaces_.end();) {
-    if (!it->second->IsBound()) {
-      it = namespaces_.erase(it);
-      continue;
-    }
-    it->second->PurgeUnboundAreas();
-    ++it;
+  // Purge all areas that don't have bindings.
+  for (const auto& namespace_pair : namespaces_) {
+    namespace_pair.second->PurgeUnboundAreas();
   }
 
   size_t final_total_cache_size;
@@ -584,21 +577,24 @@ void SessionStorageContextMojo::DoDatabaseDelete(
 }
 
 void SessionStorageContextMojo::RunWhenConnected(base::OnceClosure callback) {
-  DCHECK_NE(connection_state_, CONNECTION_SHUTDOWN);
-
-  // If we don't have a filesystem_connection_, we'll need to establish one.
-  if (connection_state_ == NO_CONNECTION) {
-    connection_state_ = CONNECTION_IN_PROGRESS;
-    InitiateConnection();
-  }
-
-  if (connection_state_ == CONNECTION_IN_PROGRESS) {
-    // Queue this OpenSessionStorage call for when we have a level db pointer.
-    on_database_opened_callbacks_.push_back(std::move(callback));
-    return;
+  switch (connection_state_) {
+    case NO_CONNECTION:
+      // If we don't have a filesystem_connection_, we'll need to establish one.
+      connection_state_ = CONNECTION_IN_PROGRESS;
+      InitiateConnection();
+      FALLTHROUGH;
+    case CONNECTION_IN_PROGRESS:
+      // Queue this OpenSessionStorage call for when we have a level db pointer.
+      on_database_opened_callbacks_.push_back(std::move(callback));
+      return;
+    case CONNECTION_SHUTDOWN:
+      NOTREACHED();
+      return;
+    case CONNECTION_FINISHED:
+      std::move(callback).Run();
+      return;
   }
-
-  std::move(callback).Run();
+  NOTREACHED();
 }
 
 void SessionStorageContextMojo::InitiateConnection(bool in_memory_only) {
@@ -726,7 +722,6 @@ void SessionStorageContextMojo::OnGotDatabaseVersion(
         "SessionStorageContext.OpenResultAfterReadVersionError");
     return;
   }
-  connection_state_ = FETCHING_METADATA;
 
   base::RepeatingClosure barrier = base::BarrierClosure(
       2, base::BindOnce(&SessionStorageContextMojo::OnConnectionFinished,
@@ -753,7 +748,7 @@ void SessionStorageContextMojo::OnGotNamespaces(
     std::vector<leveldb::mojom::BatchedOperationPtr> migration_operations,
     leveldb::mojom::DatabaseError status,
     std::vector<leveldb::mojom::KeyValuePtr> values) {
-  DCHECK(connection_state_ == FETCHING_METADATA);
+  DCHECK_EQ(connection_state_, CONNECTION_IN_PROGRESS);
   bool parsing_failure =
       status == leveldb::mojom::DatabaseError::OK &&
       !metadata_.ParseNamespaces(std::move(values), &migration_operations);
@@ -780,7 +775,7 @@ void SessionStorageContextMojo::OnGotNextMapId(
     base::OnceClosure done,
     leveldb::mojom::DatabaseError status,
     const std::vector<uint8_t>& map_id) {
-  DCHECK(connection_state_ == FETCHING_METADATA);
+  DCHECK_EQ(connection_state_, CONNECTION_IN_PROGRESS);
   if (status == leveldb::mojom::DatabaseError::NOT_FOUND) {
     std::move(done).Run();
     return;
@@ -801,7 +796,7 @@ void SessionStorageContextMojo::OnGotNextMapId(
 }
 
 void SessionStorageContextMojo::OnConnectionFinished() {
-  DCHECK(!database_ || connection_state_ == FETCHING_METADATA);
+  DCHECK(!database_ || connection_state_ == CONNECTION_IN_PROGRESS);
   if (!database_) {
     partition_directory_.reset();
     file_system_.reset();

content/browser/dom_storage/session_storage_context_mojo.h

@@ -13,6 +13,7 @@
 
 #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/weak_ptr.h"
 #include "base/optional.h"
@@ -126,6 +127,8 @@ class CONTENT_EXPORT SessionStorageContextMojo
 
  private:
   friend class DOMStorageBrowserTest;
+  FRIEND_TEST_ALL_PREFIXES(SessionStorageContextMojoTest,
+                           PurgeMemoryDoesNotCrashOrHang);
 
   // Object deletion is done through |ShutdownAndDelete()|.
   ~SessionStorageContextMojo() override;
@@ -200,7 +203,6 @@ class CONTENT_EXPORT SessionStorageContextMojo
 
   enum ConnectionState {
     NO_CONNECTION,
-    FETCHING_METADATA,
     CONNECTION_IN_PROGRESS,
     CONNECTION_FINISHED,
     CONNECTION_SHUTDOWN

content/browser/dom_storage/session_storage_context_mojo_unittest.cc

@@ -439,6 +439,8 @@ TEST_F(SessionStorageContextMojoTest, Scavenging) {
   // This scavenge call should NOT delete the namespace, as we just created it.
   {
     base::RunLoop loop;
+    // Cause the connection to start loading, so we start scavenging mid-load.
+    context()->Flush();
     context()->ScavengeUnusedNamespaces(loop.QuitClosure());
     loop.Run();
   }
@@ -938,52 +940,6 @@ TEST_F(SessionStorageContextMojoTest, DeleteStorage) {
   EXPECT_EQ(0ul, data.size());
 }
 
-TEST_F(SessionStorageContextMojoTest, PurgeMemoryDoesNotCrashOrHang) {
-  std::string namespace_id1 = base::GenerateGUID();
-  std::string namespace_id2 = base::GenerateGUID();
-  url::Origin origin1 = url::Origin::Create(GURL("http://foobar.com"));
-
-  context()->CreateSessionNamespace(namespace_id1);
-  blink::mojom::SessionStorageNamespacePtr ss_namespace1;
-  context()->OpenSessionStorage(kTestProcessId, namespace_id1,
-                                mojo::MakeRequest(&ss_namespace1));
-  blink::mojom::StorageAreaAssociatedPtr leveldb_n1_o1;
-  ss_namespace1->OpenArea(origin1, mojo::MakeRequest(&leveldb_n1_o1));
-
-  context()->CreateSessionNamespace(namespace_id2);
-  blink::mojom::SessionStorageNamespacePtr ss_namespace2;
-  context()->OpenSessionStorage(kTestProcessId, namespace_id2,
-                                mojo::MakeRequest(&ss_namespace2));
-  blink::mojom::StorageAreaAssociatedPtr leveldb_n2_o1;
-  ss_namespace2->OpenArea(origin1, mojo::MakeRequest(&leveldb_n2_o1));
-
-  // Put some data in both.
-  EXPECT_TRUE(test::PutSync(
-      leveldb_n1_o1.get(), leveldb::StringPieceToUint8Vector("key1"),
-      leveldb::StringPieceToUint8Vector("value1"), base::nullopt, "source1"));
-  EXPECT_TRUE(test::PutSync(
-      leveldb_n2_o1.get(), leveldb::StringPieceToUint8Vector("key1"),
-      leveldb::StringPieceToUint8Vector("value2"), base::nullopt, "source1"));
-
-  leveldb_n2_o1.reset();
-  ss_namespace2.reset();
-
-  base::RunLoop().RunUntilIdle();
-
-  // Verify this doesn't crash or hang.
-  context()->PurgeMemory();
-
-  // Test the values is still there.
-  std::vector<blink::mojom::KeyValuePtr> data;
-  EXPECT_TRUE(test::GetAllSync(leveldb_n1_o1.get(), &data));
-  EXPECT_EQ(1ul, data.size());
-
-  base::Optional<std::vector<uint8_t>> opt_value2 =
-      DoTestGet(namespace_id2, origin1, "key1");
-  ASSERT_TRUE(opt_value2);
-  EXPECT_EQ(leveldb::StringPieceToUint8Vector("value2"), opt_value2.value());
-}
-
 TEST_F(SessionStorageContextMojoTest, PurgeInactiveWrappers) {
   std::string namespace_id1 = base::GenerateGUID();
   std::string namespace_id2 = base::GenerateGUID();
@@ -1039,4 +995,61 @@ TEST_F(SessionStorageContextMojoTest, PurgeInactiveWrappers) {
 }
 
 }  // namespace
+
+TEST_F(SessionStorageContextMojoTest, PurgeMemoryDoesNotCrashOrHang) {
+  std::string namespace_id1 = base::GenerateGUID();
+  std::string namespace_id2 = base::GenerateGUID();
+  url::Origin origin1 = url::Origin::Create(GURL("http://foobar.com"));
+
+  context()->CreateSessionNamespace(namespace_id1);
+  blink::mojom::SessionStorageNamespacePtr ss_namespace1;
+  context()->OpenSessionStorage(kTestProcessId, namespace_id1,
+                                mojo::MakeRequest(&ss_namespace1));
+  blink::mojom::StorageAreaAssociatedPtr leveldb_n1_o1;
+  ss_namespace1->OpenArea(origin1, mojo::MakeRequest(&leveldb_n1_o1));
+
+  context()->CreateSessionNamespace(namespace_id2);
+  blink::mojom::SessionStorageNamespacePtr ss_namespace2;
+  context()->OpenSessionStorage(kTestProcessId, namespace_id2,
+                                mojo::MakeRequest(&ss_namespace2));
+  blink::mojom::StorageAreaAssociatedPtr leveldb_n2_o1;
+  ss_namespace2->OpenArea(origin1, mojo::MakeRequest(&leveldb_n2_o1));
+
+  // Put some data in both.
+  EXPECT_TRUE(test::PutSync(
+      leveldb_n1_o1.get(), leveldb::StringPieceToUint8Vector("key1"),
+      leveldb::StringPieceToUint8Vector("value1"), base::nullopt, "source1"));
+  EXPECT_TRUE(test::PutSync(
+      leveldb_n2_o1.get(), leveldb::StringPieceToUint8Vector("key1"),
+      leveldb::StringPieceToUint8Vector("value2"), base::nullopt, "source1"));
+
+  context()->FlushAreaForTesting(namespace_id1, origin1);
+
+  leveldb_n2_o1.reset();
+  ss_namespace2.reset();
+
+  base::RunLoop().RunUntilIdle();
+
+  // Verify this doesn't crash or hang.
+  context()->PurgeMemory();
+
+  size_t memory_used = context()
+                           ->namespaces_[namespace_id1]
+                           ->origin_areas_[origin1]
+                           ->data_map()
+                           ->storage_area()
+                           ->memory_used();
+  EXPECT_EQ(0ul, memory_used);
+
+  // Test the values is still there.
+  std::vector<blink::mojom::KeyValuePtr> data;
+  EXPECT_TRUE(test::GetAllSync(leveldb_n1_o1.get(), &data));
+  EXPECT_EQ(1ul, data.size());
+
+  base::Optional<std::vector<uint8_t>> opt_value2 =
+      DoTestGet(namespace_id2, origin1, "key1");
+  ASSERT_TRUE(opt_value2);
+  EXPECT_EQ(leveldb::StringPieceToUint8Vector("value2"), opt_value2.value());
+}
+
 }  // namespace content

content/browser/dom_storage/session_storage_namespace_impl_mojo.h

@@ -8,6 +8,7 @@
 
 #include "base/callback.h"
 #include "base/containers/flat_map.h"
+#include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
 #include "content/browser/dom_storage/session_storage_area_impl.h"
 #include "content/browser/dom_storage/session_storage_data_map.h"
@@ -134,6 +135,9 @@ class CONTENT_EXPORT SessionStorageNamespaceImplMojo final
   void FlushOriginForTesting(const url::Origin& origin);
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(SessionStorageContextMojoTest,
+                           PurgeMemoryDoesNotCrashOrHang);
+
   const std::string namespace_id_;
   SessionStorageMetadata::NamespaceEntry namespace_entry_;
   leveldb::mojom::LevelDBDatabase* database_;

content/browser/dom_storage/storage_area_impl.cc

@@ -202,6 +202,7 @@ void StorageAreaImpl::PurgeMemory() {
   }
 
   map_state_ = MapState::UNLOADED;
+  memory_used_ = 0;
   keys_only_map_.clear();
   keys_values_map_.clear();
 }