Revert "Use the same SessionStorageNamespace for prerendering"

This reverts commit eefb8c561ab863863b0541125df363fef040eabb. Reason for revert: PrerenderBackForwardCacheBrowserTest.SessionStorageAfterBackNavigation flaky on multiple bots. Example: https://ci.chromium.org/ui/p/chromium/builders/ci/linux-ubsan-vptr/4247/test-results Original change's description: > Use the same SessionStorageNamespace for prerendering > > Currently there is an issue that the Session Storage is not carried over > to the prerendering page. This is because a new Session Storage > Namespace is used for the prerendering page. > > To fix this issue, this CL changes PrerenderHost::PageHolder to copy the > Session Storage Namespace from the initiator page to the prerendering > page. > > We don’t want the Session Storage state in the storage service be > updated by the prerendering page. And we want to synchronize the Session > Storage state of the prerendering page with the initiator page when the > prerendering page is activated. So this CL introduces a flag > |is_session_storage_for_prerendering_| in CachedStorageArea, and make > CachedStorageArea not to send the changes of the Session Storage state > to the storage service, and make StorageArea recreate |cached_area_| > when the prerendering page is activated. > > This is the "clone & swap" mechanism for session storage in prerendering > described in whatwg/storage#119. > > This CL still has an issue that when the initial renderer process is > reused after the back navigation from a prerendered page, the Session > Storage state is not correctly propagated to the initial renderer > process. This issue will be fixed in the next CL. > https://crrev.com/c/2849654 > > Bug: 1197383 > Change-Id: Ib43386ccf75f8c867bcddb4b77b333ee0d5b5d79 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2850242 > Reviewed-by: Kinuko Yasuda <[email protected]> > Reviewed-by: Matt Falkenhagen <[email protected]> > Reviewed-by: Marijn Kruisselbrink <[email protected]> > Commit-Queue: Tsuyoshi Horo <[email protected]> > Cr-Commit-Position: refs/heads/master@{#881985} Bug: 1197383 Change-Id: Iff45106b5e3f5d6f9b2c71f9273769cf93ff48c5 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2891974 Bot-Commit: Rubber Stamper <[email protected]> Owners-Override: Owen Min <[email protected]> Commit-Queue: Owen Min <[email protected]> Auto-Submit: Owen Min <[email protected]> Cr-Commit-Position: refs/heads/master@{#882084} NOKEYCHECK=True GitOrigin-RevId: 1fa2fab156f293450721192b2376b5ac8f2a6a2e
mjfroman · May 12, 2021 · a1e870c · a1e870c
1 parent b9a2981
commit a1e870c
Show file tree

Hide file tree

Showing 16 changed files with 17 additions and 375 deletions.
diff --git a/blink/renderer/core/dom/document.cc b/blink/renderer/core/dom/document.cc
@@ -8078,12 +8078,6 @@ const Node* Document::GetFindInPageActiveMatchNode() const {
   return find_in_page_active_match_node_;
 }
 
-void Document::RegisterWillDispatchPrerenderchangeCallback(
-    base::OnceClosure closure) {
-  DCHECK(is_prerendering_);
-  will_dispatch_prerenderchange_callbacks_.push_back(std::move(closure));
-}
-
 void Document::ActivateForPrerendering() {
   DCHECK(RuntimeEnabledFeatures::Prerender2Enabled());
 
@@ -8102,12 +8096,6 @@ void Document::ActivateForPrerendering() {
   if (DocumentLoader* loader = Loader())
     loader->NotifyPrerenderingDocumentActivated();
 
-  Vector<base::OnceClosure> callbacks;
-  callbacks.swap(will_dispatch_prerenderchange_callbacks_);
-  for (auto& callback : callbacks) {
-    std::move(callback).Run();
-  }
-
   DispatchEvent(*Event::Create(event_type_names::kPrerenderingchange));
 
   if (LocalFrame* frame = GetFrame())

diff --git a/blink/renderer/core/dom/document.h b/blink/renderer/core/dom/document.h
@@ -323,8 +323,6 @@ class CORE_EXPORT Document : public ContainerNode,
 
   bool IsPrerendering() const { return is_prerendering_; }
 
-  void RegisterWillDispatchPrerenderchangeCallback(base::OnceClosure);
-
   network::mojom::ReferrerPolicy GetReferrerPolicy() const;
 
   bool DocumentPolicyFeatureObserved(
@@ -1840,10 +1838,6 @@ class CORE_EXPORT Document : public ContainerNode,
   // https://github.com/jeremyroman/alternate-loading-modes/blob/main/prerendering-state.md#documentprerendering
   bool is_prerendering_;
 
-  // Callbacks to execute upon activation of a prerendered page, just before the
-  // prerenderchange event is dispatched.
-  Vector<base::OnceClosure> will_dispatch_prerenderchange_callbacks_;
-
   bool evaluate_media_queries_on_style_recalc_;
 
   // If we do ignore the pending stylesheet count, then we need to add a boolean

diff --git a/blink/renderer/modules/storage/cached_storage_area.cc b/blink/renderer/modules/storage/cached_storage_area.cc
@@ -102,13 +102,10 @@ bool CachedStorageArea::SetItem(const String& key,
   KURL page_url = source->GetPageUrl();
   String source_id = areas_->at(source);
   String source_string = PackSource(page_url, source_id);
-
-  if (!is_session_storage_for_prerendering_) {
-    remote_area_->Put(StringToUint8Vector(key, GetKeyFormat()),
-                      StringToUint8Vector(value, value_format),
-                      optional_old_value, source_string,
-                      MakeSuccessCallback(source));
-  }
+  remote_area_->Put(StringToUint8Vector(key, GetKeyFormat()),
+                    StringToUint8Vector(value, value_format),
+                    optional_old_value, source_string,
+                    MakeSuccessCallback(source));
   if (!IsSessionStorage())
     EnqueuePendingMutation(key, value, old_value, source_string);
   else if (old_value != value)
@@ -130,11 +127,9 @@ void CachedStorageArea::RemoveItem(const String& key, Source* source) {
   KURL page_url = source->GetPageUrl();
   String source_id = areas_->at(source);
   String source_string = PackSource(page_url, source_id);
-  if (!is_session_storage_for_prerendering_) {
-    remote_area_->Delete(StringToUint8Vector(key, GetKeyFormat()),
-                         optional_old_value, source_string,
-                         MakeSuccessCallback(source));
-  }
+  remote_area_->Delete(StringToUint8Vector(key, GetKeyFormat()),
+                       optional_old_value, source_string,
+                       MakeSuccessCallback(source));
   if (!IsSessionStorage())
     EnqueuePendingMutation(key, String(), old_value, source_string);
   else
@@ -169,10 +164,8 @@ void CachedStorageArea::Clear(Source* source) {
   KURL page_url = source->GetPageUrl();
   String source_id = areas_->at(source);
   String source_string = PackSource(page_url, source_id);
-  if (!is_session_storage_for_prerendering_) {
-    remote_area_->DeleteAll(source_string, std::move(new_observer),
-                            MakeSuccessCallback(source));
-  }
+  remote_area_->DeleteAll(source_string, std::move(new_observer),
+                          MakeSuccessCallback(source));
   if (!IsSessionStorage())
     EnqueuePendingMutation(String(), String(), String(), source_string);
   else if (!already_empty)
@@ -189,12 +182,10 @@ CachedStorageArea::CachedStorageArea(
     AreaType type,
     scoped_refptr<const SecurityOrigin> origin,
     scoped_refptr<base::SingleThreadTaskRunner> task_runner,
-    StorageNamespace* storage_namespace,
-    bool is_session_storage_for_prerendering)
+    StorageNamespace* storage_namespace)
     : type_(type),
       origin_(std::move(origin)),
       storage_namespace_(storage_namespace),
-      is_session_storage_for_prerendering_(is_session_storage_for_prerendering),
       task_runner_(std::move(task_runner)),
       areas_(MakeGarbageCollected<HeapHashMap<WeakMember<Source>, String>>()) {
   BindStorageArea();
@@ -227,7 +218,6 @@ void CachedStorageArea::BindStorageArea(
 
 void CachedStorageArea::ResetConnection(
     mojo::PendingRemote<mojom::blink::StorageArea> new_area) {
-  DCHECK(!is_session_storage_for_prerendering_);
   remote_area_.reset();
   BindStorageArea(std::move(new_area));
 

diff --git a/blink/renderer/modules/storage/cached_storage_area.h b/blink/renderer/modules/storage/cached_storage_area.h
@@ -63,8 +63,7 @@ class MODULES_EXPORT CachedStorageArea
   CachedStorageArea(AreaType type,
                     scoped_refptr<const SecurityOrigin> origin,
                     scoped_refptr<base::SingleThreadTaskRunner> ipc_runner,
-                    StorageNamespace* storage_namespace,
-                    bool is_session_storage_for_prerendering);
+                    StorageNamespace* storage_namespace);
 
   // These correspond to blink::Storage.
   unsigned GetLength();
@@ -98,10 +97,6 @@ class MODULES_EXPORT CachedStorageArea
   void ResetConnection(
       mojo::PendingRemote<mojom::blink::StorageArea> new_area = {});
 
-  bool is_session_storage_for_prerendering() const {
-    return is_session_storage_for_prerendering_;
-  }
-
   void SetRemoteAreaForTesting(
       mojo::PendingRemote<mojom::blink::StorageArea> area) {
     remote_area_.Bind(std::move(area));
@@ -182,12 +177,6 @@ class MODULES_EXPORT CachedStorageArea
   const AreaType type_;
   const scoped_refptr<const SecurityOrigin> origin_;
   const WeakPersistent<StorageNamespace> storage_namespace_;
-  // Session storage state for prerendering is initialized by cloning the
-  // primary session storage state. It is used locally by the prerendering
-  // context, and does not get propagated back to the primary state (i.e., via
-  // remote_area_). For more details:
-  // https://docs.google.com/document/d/1I5Hr8I20-C1GBr4tAXdm0U8a1RDUKHt4n7WcH4fxiSE/edit?usp=sharing
-  const bool is_session_storage_for_prerendering_;
   const scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
 
   std::unique_ptr<StorageAreaMap> map_;

diff --git a/blink/renderer/modules/storage/cached_storage_area_test.cc b/blink/renderer/modules/storage/cached_storage_area_test.cc
@@ -38,7 +38,7 @@ class CachedStorageAreaTest : public testing::Test {
                            : CachedStorageArea::AreaType::kLocalStorage;
     cached_area_ = base::MakeRefCounted<CachedStorageArea>(
         area_type, kOrigin, scheduler::GetSingleThreadTaskRunnerForTesting(),
-        nullptr, /*is_session_storage_for_prerendering=*/false);
+        nullptr);
     cached_area_->SetRemoteAreaForTesting(
         mock_storage_area_.GetInterfaceRemote());
     source_area_ = MakeGarbageCollected<FakeAreaSource>(kPageUrl);

diff --git a/blink/renderer/modules/storage/dom_window_storage.cc b/blink/renderer/modules/storage/dom_window_storage.cc
@@ -90,16 +90,10 @@ StorageArea* DOMWindowStorage::sessionStorage(
       StorageNamespace::From(window->GetFrame()->GetPage());
   if (!storage_namespace)
     return nullptr;
-  scoped_refptr<CachedStorageArea> cached_storage_area;
-  if (window->document()->IsPrerendering()) {
-    cached_storage_area = storage_namespace->CreateCachedAreaForPrerender(
-        window->GetSecurityOrigin());
-  } else {
-    cached_storage_area =
-        storage_namespace->GetCachedArea(window->GetSecurityOrigin());
-  }
+  auto storage_area =
+      storage_namespace->GetCachedArea(window->GetSecurityOrigin());
   session_storage_ =
-      StorageArea::Create(window, std::move(cached_storage_area),
+      StorageArea::Create(window, std::move(storage_area),
                           StorageArea::StorageType::kSessionStorage);
 
   if (!session_storage_->CanAccessStorage()) {

diff --git a/blink/renderer/modules/storage/storage_area.cc b/blink/renderer/modules/storage/storage_area.cc
@@ -41,7 +41,6 @@
 #include "third_party/blink/renderer/modules/storage/storage_namespace.h"
 #include "third_party/blink/renderer/platform/bindings/exception_state.h"
 #include "third_party/blink/renderer/platform/weborigin/security_origin.h"
-#include "third_party/blink/renderer/platform/wtf/functional.h"
 #include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
 
 namespace blink {
@@ -74,11 +73,6 @@ StorageArea::StorageArea(LocalDOMWindow* window,
   DCHECK(window);
   DCHECK(cached_area_);
   cached_area_->RegisterSource(this);
-  if (cached_area_->is_session_storage_for_prerendering()) {
-    DomWindow()->document()->RegisterWillDispatchPrerenderchangeCallback(
-        WTF::Bind(&StorageArea::OnDocumentActivatedForPrerendering,
-                  WrapWeakPersistent(this)));
-  }
 }
 
 unsigned StorageArea::length(ExceptionState& exception_state) const {
@@ -252,18 +246,4 @@ blink::WebScopedVirtualTimePauser StorageArea::CreateWebScopedVirtualTimePauser(
       ->CreateWebScopedVirtualTimePauser(name, duration);
 }
 
-void StorageArea::OnDocumentActivatedForPrerendering() {
-  StorageNamespace* storage_namespace =
-      StorageNamespace::From(DomWindow()->GetFrame()->GetPage());
-  if (!storage_namespace)
-    return;
-
-  // Swap out the session storage state used within prerendering, and replace it
-  // with the normal session storage state. For more details:
-  // https://docs.google.com/document/d/1I5Hr8I20-C1GBr4tAXdm0U8a1RDUKHt4n7WcH4fxiSE/edit?usp=sharing
-  cached_area_ =
-      storage_namespace->GetCachedArea(DomWindow()->GetSecurityOrigin());
-  cached_area_->RegisterSource(this);
-}
-
 }  // namespace blink
diff --git a/blink/renderer/modules/storage/storage_area.h b/blink/renderer/modules/storage/storage_area.h
@@ -92,10 +92,7 @@ class StorageArea final : public ScriptWrappable,
 
  private:
   void RecordModificationInMetrics();
-
-  void OnDocumentActivatedForPrerendering();
-
-  scoped_refptr<CachedStorageArea> cached_area_;
+  const scoped_refptr<CachedStorageArea> cached_area_;
   StorageType storage_type_;
   const bool should_enqueue_events_;
 

diff --git a/blink/renderer/modules/storage/storage_namespace.cc b/blink/renderer/modules/storage/storage_namespace.cc
@@ -100,23 +100,11 @@ scoped_refptr<CachedStorageArea> StorageNamespace::GetCachedArea(
   result = base::MakeRefCounted<CachedStorageArea>(
       IsSessionStorage() ? CachedStorageArea::AreaType::kSessionStorage
                          : CachedStorageArea::AreaType::kLocalStorage,
-      origin, controller_->TaskRunner(), this,
-      /*is_session_storage_for_prerendering=*/false);
+      origin, controller_->TaskRunner(), this);
   cached_areas_.insert(std::move(origin), result);
   return result;
 }
 
-scoped_refptr<CachedStorageArea> StorageNamespace::CreateCachedAreaForPrerender(
-    const SecurityOrigin* origin_ptr) {
-  DCHECK((IsSessionStorage()));
-  scoped_refptr<const SecurityOrigin> origin(origin_ptr);
-  return base::MakeRefCounted<CachedStorageArea>(
-      IsSessionStorage() ? CachedStorageArea::AreaType::kSessionStorage
-                         : CachedStorageArea::AreaType::kLocalStorage,
-      origin, controller_->TaskRunner(), this,
-      /*is_session_storage_for_prerendering=*/true);
-}
-
 void StorageNamespace::CloneTo(const String& target) {
   DCHECK(IsSessionStorage()) << "Cannot clone a local storage namespace.";
   EnsureConnected();

diff --git a/blink/renderer/modules/storage/storage_namespace.h b/blink/renderer/modules/storage/storage_namespace.h
@@ -84,9 +84,6 @@ class MODULES_EXPORT StorageNamespace final
 
   scoped_refptr<CachedStorageArea> GetCachedArea(const SecurityOrigin* origin);
 
-  scoped_refptr<CachedStorageArea> CreateCachedAreaForPrerender(
-      const SecurityOrigin* origin);
-
   // Only valid to call this if |this| and |target| are session storage
   // namespaces.
   void CloneTo(const String& target);

diff --git a/..._tests/wpt_internal/prerender/resources/session-storage-carry-over-to-prerender-page.html b/..._tests/wpt_internal/prerender/resources/session-storage-carry-over-to-prerender-page.html
diff --git a/...b_tests/wpt_internal/prerender/resources/session-storage-isolated-while-prerendering.html b/...b_tests/wpt_internal/prerender/resources/session-storage-isolated-while-prerendering.html
diff --git a/...web_tests/wpt_internal/prerender/resources/session-storage-no-leak-to-initiator-page.html b/...web_tests/wpt_internal/prerender/resources/session-storage-no-leak-to-initiator-page.html