[SessionStorage] Holding reference to forked map during fork operation.

Maps that are being forked from are not being held open until the fork
operation is complete, which is probably causing the GetAll hanges in
the attached bug.

This idea came from mek@ out of the discussion here:
https://docs.google.com/document/d/1Pw8W74-7NCHpxrtglVHqFZpLV_p2JFwdMNqo03kx2l8/edit#heading=h.lwd5sacdhs8q

R=mek@chromium.org

Bug: 897581
Change-Id: I700bbc580b0f4360354b87b4b9a8cfb3cb4bd6ee
Reviewed-on: https://chromium-review.googlesource.com/c/1489897
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635779}

content/browser/dom_storage/session_storage_area_impl.cc

@@ -139,7 +139,7 @@ void SessionStorageAreaImpl::CreateNewMap(
       shared_data_map_ = SessionStorageDataMap::CreateClone(
           shared_data_map_->listener(),
           register_new_map_callback_.Run(namespace_entry_, origin_),
-          shared_data_map_->storage_area());
+          shared_data_map_);
       break;
     case NewMapType::EMPTY_FROM_DELETE_ALL: {
       // The code optimizes the 'delete all' for shared maps by just creating

content/browser/dom_storage/session_storage_data_map.cc

@@ -24,9 +24,9 @@ scoped_refptr<SessionStorageDataMap> SessionStorageDataMap::Create(
 scoped_refptr<SessionStorageDataMap> SessionStorageDataMap::CreateClone(
     Listener* listener,
     scoped_refptr<SessionStorageMetadata::MapData> map_data,
-    StorageAreaImpl* clone_from) {
-  return base::WrapRefCounted(
-      new SessionStorageDataMap(listener, std::move(map_data), clone_from));
+    scoped_refptr<SessionStorageDataMap> clone_from) {
+  return base::WrapRefCounted(new SessionStorageDataMap(
+      listener, std::move(map_data), std::move(clone_from)));
 }
 
 std::vector<leveldb::mojom::BatchedOperationPtr>
@@ -58,12 +58,14 @@ SessionStorageDataMap::SessionStorageDataMap(
 SessionStorageDataMap::SessionStorageDataMap(
     Listener* listener,
     scoped_refptr<SessionStorageMetadata::MapData> map_data,
-    StorageAreaImpl* forking_from)
+    scoped_refptr<SessionStorageDataMap> forking_from)
     : listener_(listener),
+      clone_from_data_map_(std::move(forking_from)),
       map_data_(std::move(map_data)),
-      storage_area_impl_(forking_from->ForkToNewPrefix(map_data_->KeyPrefix(),
-                                                       this,
-                                                       GetOptions())),
+      storage_area_impl_(clone_from_data_map_->storage_area()->ForkToNewPrefix(
+          map_data_->KeyPrefix(),
+          this,
+          GetOptions())),
       storage_area_ptr_(storage_area_impl_.get()) {
   DCHECK(listener_);
   DCHECK(map_data_);
@@ -85,6 +87,10 @@ void SessionStorageDataMap::RemoveBindingReference() {
   storage_area()->ScheduleImmediateCommit();
 }
 
+void SessionStorageDataMap::OnMapLoaded(leveldb::mojom::DatabaseError) {
+  clone_from_data_map_.reset();
+}
+
 // static
 StorageAreaImpl::Options SessionStorageDataMap::GetOptions() {
   // Delay for a moment after a value is set in anticipation

content/browser/dom_storage/session_storage_data_map.h

@@ -49,7 +49,7 @@ class CONTENT_EXPORT SessionStorageDataMap final
   static scoped_refptr<SessionStorageDataMap> CreateClone(
       Listener* listener,
       scoped_refptr<SessionStorageMetadata::MapData> map_data,
-      StorageAreaImpl* clone_from);
+      scoped_refptr<SessionStorageDataMap> clone_from);
 
   Listener* listener() const { return listener_; }
 
@@ -82,13 +82,21 @@ class CONTENT_EXPORT SessionStorageDataMap final
   SessionStorageDataMap(
       Listener* listener,
       scoped_refptr<SessionStorageMetadata::MapData> map_entry,
-      StorageAreaImpl* forking_from);
+      scoped_refptr<SessionStorageDataMap> forking_from);
   ~SessionStorageDataMap() override;
 
+  void OnMapLoaded(leveldb::mojom::DatabaseError error) override;
+
   static StorageAreaImpl::Options GetOptions();
 
   Listener* listener_;
   int binding_count_ = 0;
+
+  // If we're cloning from another map, we need to keep it alive while it forks.
+  // Note, this has to be above the storage_area_impl_ in case the Fork call
+  // completes synchronously.
+  scoped_refptr<SessionStorageDataMap> clone_from_data_map_;
+
   scoped_refptr<SessionStorageMetadata::MapData> map_data_;
   std::unique_ptr<StorageAreaImpl> storage_area_impl_;
   // Holds the same value as |storage_area_impl_|. The reason for this is that

content/browser/dom_storage/session_storage_data_map_unittest.cc

@@ -153,7 +153,7 @@ TEST_F(SessionStorageDataMapTest, Clone) {
           &listener_,
           base::MakeRefCounted<SessionStorageMetadata::MapData>(2,
                                                                 test_origin_),
-          map1->storage_area());
+          map1);
 
   bool success;
   std::vector<blink::mojom::KeyValuePtr> data;

content/browser/dom_storage/storage_area_impl.cc

@@ -641,6 +641,7 @@ void StorageAreaImpl::OnGotMigrationData(std::unique_ptr<ValueMap> data) {
   keys_values_map_ = data ? std::move(*data) : ValueMap();
   map_state_ = MapState::LOADED_KEYS_AND_VALUES;
   CalculateStorageAndMemoryUsed();
+  delegate_->OnMapLoaded(leveldb::mojom::DatabaseError::OK);
 
   if (database_ && !empty()) {
     CreateCommitBatchIfNeeded();
@@ -649,7 +650,6 @@ void StorageAreaImpl::OnGotMigrationData(std::unique_ptr<ValueMap> data) {
       commit_batch_->changed_keys.insert(it.first);
     CommitChanges();
   }
-
   OnLoadComplete();
 }
 

content/browser/dom_storage/storage_area_impl_unittest.cc

@@ -240,6 +240,9 @@ class StorageAreaImplTest : public testing::Test,
   const std::vector<Observation>& observations() { return observations_; }
 
   MockDelegate* delegate() { return &delegate_; }
+  leveldb::mojom::LevelDBDatabase* database() const {
+    return level_db_database_ptr_.get();
+  }
 
   void should_record_send_old_value_observations(bool value) {
     should_record_send_old_value_observations_ = value;
@@ -327,6 +330,19 @@ TEST_F(StorageAreaImplTest, GetLoadedFromMap) {
   EXPECT_FALSE(GetSync(ToBytes("x"), &result));
 }
 
+TEST_F(StorageAreaImplTest, NoDataCallsOnMapLoaded) {
+  StorageAreaImpl::Options options =
+      GetDefaultTestingOptions(CacheMode::KEYS_ONLY_WHEN_POSSIBLE);
+  // Load an area that has no data inside, so the result will be empty and the
+  // migration code is triggered.
+  auto area = std::make_unique<StorageAreaImpl>(database(), test_copy_prefix2_,
+                                                delegate(), options);
+  std::vector<blink::mojom::KeyValuePtr> data;
+  EXPECT_TRUE(test::GetAllSyncOnDedicatedPipe(area.get(), &data));
+  EXPECT_TRUE(data.empty());
+  EXPECT_EQ(1, delegate()->map_load_count());
+}
+
 TEST_F(StorageAreaImplTest, GetFromPutOverwrite) {
   storage_area_impl()->SetCacheModeForTesting(CacheMode::KEYS_AND_VALUES);
   std::vector<uint8_t> key = test_key2_bytes_;