[SessionStorageS13N] Fixed possible double-open of namespace in renderer.

Bug: 716490
Change-Id: Ia7b81f19e365b8fc2385ee1bbf843fa5c63a051b
Reviewed-on: https://chromium-review.googlesource.com/1011560
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550829}

content/renderer/dom_storage/local_storage_cached_areas.cc

@@ -6,6 +6,7 @@
 
 #include "base/metrics/histogram_macros.h"
 #include "base/sys_info.h"
+#include "content/common/dom_storage/dom_storage_namespace_ids.h"
 #include "content/common/dom_storage/dom_storage_types.h"
 #include "content/public/common/content_features.h"
 #include "content/renderer/dom_storage/local_storage_cached_area.h"
@@ -40,7 +41,7 @@ scoped_refptr<LocalStorageCachedArea> LocalStorageCachedAreas::GetCachedArea(
 scoped_refptr<LocalStorageCachedArea>
 LocalStorageCachedAreas::GetSessionStorageArea(const std::string& namespace_id,
                                                const url::Origin& origin) {
-  DCHECK_NE(namespace_id, kLocalStorageNamespaceId);
+  DCHECK_NE(kLocalStorageNamespaceId, namespace_id);
   return GetCachedArea(namespace_id, origin, main_thread_scheduler_);
 }
 
@@ -48,19 +49,21 @@ void LocalStorageCachedAreas::CloneNamespace(
     const std::string& source_namespace,
     const std::string& destination_namespace) {
   DCHECK(base::FeatureList::IsEnabled(features::kMojoSessionStorage));
+  DCHECK_EQ(kSessionStorageNamespaceIdLength, source_namespace.size());
+  DCHECK_EQ(kSessionStorageNamespaceIdLength, destination_namespace.size());
+
   auto namespace_it = cached_namespaces_.find(source_namespace);
-  if (namespace_it != cached_namespaces_.end()) {
-    namespace_it->second.session_storage_namespace->Clone(
-        destination_namespace);
-    return;
+  if (namespace_it == cached_namespaces_.end()) {
+    namespace_it =
+        cached_namespaces_
+            .emplace(std::make_pair(source_namespace, DOMStorageNamespace()))
+            .first;
+    storage_partition_service_->OpenSessionStorage(
+        source_namespace,
+        mojo::MakeRequest(&namespace_it->second.session_storage_namespace));
   }
-  // The clone call still has to be sent, as we can have a case where the
-  // storage is never opened but there is still data there (from a restore or
-  // an earlier clone).
-  mojom::SessionStorageNamespacePtr session_storage_namespace;
-  storage_partition_service_->OpenSessionStorage(
-      source_namespace, mojo::MakeRequest(&session_storage_namespace));
-  session_storage_namespace->Clone(destination_namespace);
+  DCHECK(namespace_it->second.session_storage_namespace);
+  namespace_it->second.session_storage_namespace->Clone(destination_namespace);
 }
 
 size_t LocalStorageCachedAreas::TotalCacheSize() const {
@@ -126,9 +129,11 @@ scoped_refptr<LocalStorageCachedArea> LocalStorageCachedAreas::GetCachedArea(
           origin, storage_partition_service_, this, scheduler);
     } else {
       DCHECK(base::FeatureList::IsEnabled(features::kMojoSessionStorage));
-      storage_partition_service_->OpenSessionStorage(
-          namespace_id,
-          mojo::MakeRequest(&dom_namespace->session_storage_namespace));
+      if (!dom_namespace->session_storage_namespace) {
+        storage_partition_service_->OpenSessionStorage(
+            namespace_id,
+            mojo::MakeRequest(&dom_namespace->session_storage_namespace));
+      }
       result = base::MakeRefCounted<LocalStorageCachedArea>(
           namespace_id, origin, dom_namespace->session_storage_namespace.get(),
           this, scheduler);

content/renderer/dom_storage/local_storage_cached_areas_unittest.cc

@@ -4,8 +4,11 @@
 
 #include "content/renderer/dom_storage/local_storage_cached_areas.h"
 
+#include "base/guid.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/test/scoped_task_environment.h"
+#include "content/public/common/content_features.h"
 #include "content/renderer/dom_storage/local_storage_cached_area.h"
 #include "content/renderer/dom_storage/mock_leveldb_wrapper.h"
 #include "third_party/blink/public/platform/scheduler/test/fake_renderer_scheduler.h"
@@ -56,4 +59,26 @@ TEST(LocalStorageCachedAreasTest, CacheLimit) {
   EXPECT_EQ(cached_area2->memory_used(), cached_areas.TotalCacheSize());
 }
 
+TEST(LocalStorageCachedAreasTest, CloneBeforeGetArea) {
+  base::test::ScopedTaskEnvironment scoped_task_environment;
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(features::kMojoSessionStorage);
+  const std::string kNamespace1 = base::GenerateGUID();
+  const std::string kNamespace2 = base::GenerateGUID();
+  const url::Origin kOrigin = url::Origin::Create(GURL("http://dom_storage1/"));
+
+  blink::scheduler::FakeRendererScheduler renderer_scheduler;
+
+  MockLevelDBWrapper mock_leveldb_wrapper;
+  LocalStorageCachedAreas cached_areas(&mock_leveldb_wrapper,
+                                       &renderer_scheduler);
+
+  cached_areas.CloneNamespace(kNamespace1, kNamespace2);
+
+  scoped_refptr<LocalStorageCachedArea> cached_area1 =
+      cached_areas.GetSessionStorageArea(kNamespace1, kOrigin);
+  EXPECT_TRUE(cached_area1);
+  EXPECT_EQ(1ul, mock_leveldb_wrapper.NumNamespaceBindings());
+}
+
 }  // namespace content

content/renderer/dom_storage/mock_leveldb_wrapper.h

@@ -56,6 +56,8 @@ class MockLevelDBWrapper : public mojom::StoragePartitionService,
   // Methods and members for use by test fixtures.
   bool HasBindings() { return !bindings_.empty(); }
 
+  size_t NumNamespaceBindings() { return namespace_bindings_.size(); }
+
   void ResetObservations() {
     observed_get_all_ = false;
     observed_put_ = false;