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

This reverts commit a0159268472a7ae35a8573518aacad4e4f758b12. Reason for revert: Fixed test failure by checking RenderProcessHost::run_renderer_in_process() Original change's description: > 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} Bug: 1197383 Change-Id: Icb2d0b9362904d404f4bca886e99731e75df4a99 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2905112 Reviewed-by: Kinuko Yasuda <[email protected]> Reviewed-by: Matt Falkenhagen <[email protected]> Commit-Queue: Tsuyoshi Horo <[email protected]> Cr-Commit-Position: refs/heads/master@{#884325} NOKEYCHECK=True GitOrigin-RevId: f33e440034f2ff39062fd6e834acf2babc6871a5
mjfroman · May 19, 2021 · 9d1e09c · 9d1e09c
1 parent 0876526
commit 9d1e09c
Show file tree

Hide file tree

Showing 16 changed files with 375 additions and 17 deletions.
diff --git a/blink/renderer/core/dom/document.cc b/blink/renderer/core/dom/document.cc
@@ -8185,6 +8185,12 @@ 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));
@@ -8197,6 +8203,12 @@ 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,6 +1689,8 @@ class CORE_EXPORT Document : public ContainerNode,
 
   void ActivateForPrerendering();
 
+  void AddWillDispatchPrerenderingchangeCallback(base::OnceClosure);
+
   void AddPostPrerenderingActivationStep(base::OnceClosure callback);
 
   class CORE_EXPORT PaintPreviewScope {
@@ -1863,6 +1865,10 @@ 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,10 +102,13 @@ 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);
-  remote_area_->Put(StringToUint8Vector(key, GetKeyFormat()),
-                    StringToUint8Vector(value, value_format),
-                    optional_old_value, source_string,
-                    MakeSuccessCallback(source));
+
+  if (!is_session_storage_for_prerendering_) {
+    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)
@@ -127,9 +130,11 @@ 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);
-  remote_area_->Delete(StringToUint8Vector(key, GetKeyFormat()),
-                       optional_old_value, source_string,
-                       MakeSuccessCallback(source));
+  if (!is_session_storage_for_prerendering_) {
+    remote_area_->Delete(StringToUint8Vector(key, GetKeyFormat()),
+                         optional_old_value, source_string,
+                         MakeSuccessCallback(source));
+  }
   if (!IsSessionStorage())
     EnqueuePendingMutation(key, String(), old_value, source_string);
   else
@@ -164,8 +169,10 @@ void CachedStorageArea::Clear(Source* source) {
   KURL page_url = source->GetPageUrl();
   String source_id = areas_->at(source);
   String source_string = PackSource(page_url, source_id);
-  remote_area_->DeleteAll(source_string, std::move(new_observer),
-                          MakeSuccessCallback(source));
+  if (!is_session_storage_for_prerendering_) {
+    remote_area_->DeleteAll(source_string, std::move(new_observer),
+                            MakeSuccessCallback(source));
+  }
   if (!IsSessionStorage())
     EnqueuePendingMutation(String(), String(), String(), source_string);
   else if (!already_empty)
@@ -182,10 +189,12 @@ CachedStorageArea::CachedStorageArea(
     AreaType type,
     scoped_refptr<const SecurityOrigin> origin,
     scoped_refptr<base::SingleThreadTaskRunner> task_runner,
-    StorageNamespace* storage_namespace)
+    StorageNamespace* storage_namespace,
+    bool is_session_storage_for_prerendering)
     : 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();
@@ -218,6 +227,7 @@ 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,7 +63,8 @@ class MODULES_EXPORT CachedStorageArea
   CachedStorageArea(AreaType type,
                     scoped_refptr<const SecurityOrigin> origin,
                     scoped_refptr<base::SingleThreadTaskRunner> ipc_runner,
-                    StorageNamespace* storage_namespace);
+                    StorageNamespace* storage_namespace,
+                    bool is_session_storage_for_prerendering);
 
   // These correspond to blink::Storage.
   unsigned GetLength();
@@ -97,6 +98,10 @@ 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));
@@ -177,6 +182,12 @@ 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);
+        nullptr, /*is_session_storage_for_prerendering=*/false);
     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,10 +90,16 @@ StorageArea* DOMWindowStorage::sessionStorage(
       StorageNamespace::From(window->GetFrame()->GetPage());
   if (!storage_namespace)
     return nullptr;
-  auto storage_area =
-      storage_namespace->GetCachedArea(window->GetSecurityOrigin());
+  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());
+  }
   session_storage_ =
-      StorageArea::Create(window, std::move(storage_area),
+      StorageArea::Create(window, std::move(cached_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,6 +41,7 @@
 #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 {
@@ -73,6 +74,11 @@ 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 {
@@ -246,4 +252,18 @@ 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,7 +92,10 @@ class StorageArea final : public ScriptWrappable,
 
  private:
   void RecordModificationInMetrics();
-  const scoped_refptr<CachedStorageArea> cached_area_;
+
+  void OnDocumentActivatedForPrerendering();
+
+  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,11 +100,23 @@ scoped_refptr<CachedStorageArea> StorageNamespace::GetCachedArea(
   result = base::MakeRefCounted<CachedStorageArea>(
       IsSessionStorage() ? CachedStorageArea::AreaType::kSessionStorage
                          : CachedStorageArea::AreaType::kLocalStorage,
-      origin, controller_->TaskRunner(), this);
+      origin, controller_->TaskRunner(), this,
+      /*is_session_storage_for_prerendering=*/false);
   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,6 +84,9 @@ 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
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="utils.js"></script>
+<script src="session-storage-utils.js"></script>
+<script>
+RunSessionStorageTest(async (isPrerendering, url, prerenderChannel, done) => {
+  if (!isPrerendering) {
+    sessionStorage.setItem('set by initiator page', '1');
+    startPrerendering(url);
+  } else {
+    assert_equals(
+        getSessionStorageKeys(),
+        'set by initiator page',
+        'The session storage item set by the initiator page must be carried' +
+        ' over to the prerendering page.');
+    done();
+  }
+});
+</script>
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
@@ -0,0 +1,41 @@
+<!DOCTYPE html>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="utils.js"></script>
+<script src="session-storage-utils.js"></script>
+<script>
+RunSessionStorageTest(async (isPrerendering, url, prerenderChannel, done) => {
+  if (!isPrerendering) {
+    startPrerendering(url);
+    // Wait for the message from the prerendering page.
+    assert_equals(
+        await getNextMessage(prerenderChannel),
+        'From prerendering page 1')
+
+    // Add an item to the session storage.
+    sessionStorage.setItem('set by initiator page', '1');
+
+    // Send the message to the prerendering page.
+    prerenderChannel.postMessage('From initiator page');
+
+  } else {
+    sessionStorage.setItem('set by prerendering page', '1');
+
+    // Send the message to the initiator page.
+    prerenderChannel.postMessage('From prerendering page 1');
+
+    // Wait for the message from the initiator page.
+    assert_equals(
+        await getNextMessage(prerenderChannel),
+        'From initiator page');
+
+    assert_equals(
+        getSessionStorageKeys(),
+        'set by prerendering page',
+        'The session storage item added by the initiator page after the ' +
+        'prerendering page accessed the session storage must not be visible ' +
+        'in the prerendering page.');
+    done();
+  }
+});
+</script>
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
@@ -0,0 +1,35 @@
+<!DOCTYPE html>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="utils.js"></script>
+<script src="session-storage-utils.js"></script>
+<script>
+RunSessionStorageTest(async (isPrerendering, url, prerenderChannel, done) => {
+  if (!isPrerendering) {
+    startPrerendering(url);
+
+    // Wait for the message from the prerendering page.
+    assert_equals(
+        await getNextMessage(prerenderChannel),
+        'From prerendering page')
+
+    assert_equals(
+        getSessionStorageKeys(),
+        '',
+        'The session storage item set by the prerendering page must not be ' +
+        'visible in the initiator page.');
+
+    done();
+  } else {
+    sessionStorage.setItem('set by prerendering page', '1');
+
+    assert_equals(
+        getSessionStorageKeys(),
+        'set by prerendering page',
+        'The session storage item must have been added by the prerendering' +
+        ' page.');
+    // Send the message to the initiator page.
+    prerenderChannel.postMessage('From prerendering page');
+  }
+});
+</script>