Revert "Reland "Use the same SessionStorageNamespace for prerendering""

This reverts commit c226ef4e5aaa66edbf29db3239e91bd49bcac2d2. Reason for revert: PrerenderSingleProcessBrowserTest.SessionStorageAfterBackNavigation reliably failing on linux-chromeos-chrome since first run with this CL: https://ci.chromium.org/p/chrome/builders/ci/linux-chromeos-chrome/14428 Original change's description: > Reland "Use the same SessionStorageNamespace for prerendering" > > This is a reland of eefb8c561ab863863b0541125df363fef040eabb > > The original CL was reverted because the > PrerenderBackForwardCacheBrowserTest was flaky. The test didn’t check > the behavior of BackForwardCache. It was just running the same tests of > PrerenderBrowserTest.SessionStorageAfterBackNavigation_NoProcessReuse or > SessionStorageAfterBackNavigation_KeepInitialProcess. If the initial > process have been killed before the back navigation, the test failed. > > To make the BackForwardCache logic work this CL changed the browser test > as followings: > - Added enable_same_site flag. > - Stopped using BroadcastChannel which prevent BFCache. > > PS1 is the same as the original CL. > > > 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: If83c11d44e37b598111ab1c5ce4a78dfd3757176 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2891279 > Reviewed-by: Hiroki Nakagawa <[email protected]> > Reviewed-by: Kinuko Yasuda <[email protected]> > Reviewed-by: Fergal Daly <[email protected]> > Reviewed-by: Matt Falkenhagen <[email protected]> > Commit-Queue: Tsuyoshi Horo <[email protected]> > Cr-Commit-Position: refs/heads/master@{#883819} Bug: 1197383 Change-Id: I8e506a142374fa10c3f9d475bf24d7ba78af93fc No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2903294 Reviewed-by: Gayane Petrosyan <[email protected]> Commit-Queue: Gayane Petrosyan <[email protected]> Auto-Submit: Nate Chapin <[email protected]> Owners-Override: Nate Chapin <[email protected]> Cr-Commit-Position: refs/heads/master@{#884008} NOKEYCHECK=True GitOrigin-RevId: a0159268472a7ae35a8573518aacad4e4f758b12
mjfroman · May 18, 2021 · 55b914a · 55b914a
1 parent c1683b1
commit 55b914a
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
@@ -8185,12 +8185,6 @@ void Document::ActivateForPrerendering() {
   if (DocumentLoader* loader = Loader())
     loader->NotifyPrerenderingDocumentActivated();
 
-  Vector<base::OnceClosure> callbacks;
-  callbacks.swap(will_dispatch_prerenderingchange_callbacks_);
-  for (auto& callback : callbacks) {
-    std::move(callback).Run();
-  }
-
   // https://jeremyroman.github.io/alternate-loading-modes/#prerendering-browsing-context-activate
   // Step 8.3.4 "Fire an event named prerenderingchange at doc."
   DispatchEvent(*Event::Create(event_type_names::kPrerenderingchange));
@@ -8203,12 +8197,6 @@ void Document::ActivateForPrerendering() {
     frame->DidActivateForPrerendering();
 }
 
-void Document::AddWillDispatchPrerenderingchangeCallback(
-    base::OnceClosure closure) {
-  DCHECK(is_prerendering_);
-  will_dispatch_prerenderingchange_callbacks_.push_back(std::move(closure));
-}
-
 void Document::AddPostPrerenderingActivationStep(base::OnceClosure callback) {
   DCHECK(is_prerendering_);
   post_prerendering_activation_callbacks_.push_back(std::move(callback));

diff --git a/blink/renderer/core/dom/document.h b/blink/renderer/core/dom/document.h
@@ -1689,8 +1689,6 @@ class CORE_EXPORT Document : public ContainerNode,
 
   void ActivateForPrerendering();
 
-  void AddWillDispatchPrerenderingchangeCallback(base::OnceClosure);
-
   void AddPostPrerenderingActivationStep(base::OnceClosure callback);
 
   class CORE_EXPORT PaintPreviewScope {
@@ -1865,10 +1863,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
-  // prerenderingchange event is dispatched.
-  Vector<base::OnceClosure> will_dispatch_prerenderingchange_callbacks_;
-
   // The callback list for post-prerendering activation step.
   // https://jeremyroman.github.io/alternate-loading-modes/#document-post-prerendering-activation-steps-list
   Vector<base::OnceClosure> post_prerendering_activation_callbacks_;

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()->AddWillDispatchPrerenderingchangeCallback(
-        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