From a1e870c95033fadc5ef76ccf889ce13429a9f005 Mon Sep 17 00:00:00 2001 From: Owen Min Date: Wed, 12 May 2021 15:55:41 +0000 Subject: [PATCH] Revert "Use the same SessionStorageNamespace for prerendering" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 https://github.com/whatwg/storage/issues/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 > Reviewed-by: Matt Falkenhagen > Reviewed-by: Marijn Kruisselbrink > Commit-Queue: Tsuyoshi Horo > 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 Owners-Override: Owen Min Commit-Queue: Owen Min Auto-Submit: Owen Min Cr-Commit-Position: refs/heads/master@{#882084} NOKEYCHECK=True GitOrigin-RevId: 1fa2fab156f293450721192b2376b5ac8f2a6a2e --- blink/renderer/core/dom/document.cc | 12 --- blink/renderer/core/dom/document.h | 6 -- .../modules/storage/cached_storage_area.cc | 30 +++----- .../modules/storage/cached_storage_area.h | 13 +--- .../storage/cached_storage_area_test.cc | 2 +- .../modules/storage/dom_window_storage.cc | 12 +-- .../renderer/modules/storage/storage_area.cc | 20 ----- blink/renderer/modules/storage/storage_area.h | 5 +- .../modules/storage/storage_namespace.cc | 14 +--- .../modules/storage/storage_namespace.h | 3 - ...-storage-carry-over-to-prerender-page.html | 20 ----- ...n-storage-isolated-while-prerendering.html | 41 ---------- ...ion-storage-no-leak-to-initiator-page.html | 35 --------- .../session-storage-swap-after-activate.html | 76 ------------------- .../resources/session-storage-utils.js | 76 ------------------- .../prerender/session-storage.html | 27 ------- 16 files changed, 17 insertions(+), 375 deletions(-) delete mode 100644 blink/web_tests/wpt_internal/prerender/resources/session-storage-carry-over-to-prerender-page.html delete mode 100644 blink/web_tests/wpt_internal/prerender/resources/session-storage-isolated-while-prerendering.html delete mode 100644 blink/web_tests/wpt_internal/prerender/resources/session-storage-no-leak-to-initiator-page.html delete mode 100644 blink/web_tests/wpt_internal/prerender/resources/session-storage-swap-after-activate.html delete mode 100644 blink/web_tests/wpt_internal/prerender/resources/session-storage-utils.js delete mode 100644 blink/web_tests/wpt_internal/prerender/session-storage.html diff --git a/blink/renderer/core/dom/document.cc b/blink/renderer/core/dom/document.cc index 5d2129f301b5..286cf59c6a50 100644 --- 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 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 index b5c9d8804504..848a5f228fb6 100644 --- 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 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 index 72f4d3bc55c5..9a3840e55198 100644 --- 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 origin, scoped_refptr 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, String>>()) { BindStorageArea(); @@ -227,7 +218,6 @@ void CachedStorageArea::BindStorageArea( void CachedStorageArea::ResetConnection( mojo::PendingRemote 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 index b37e86a4d6d5..28c9c3d0c9aa 100644 --- 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 origin, scoped_refptr 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 new_area = {}); - bool is_session_storage_for_prerendering() const { - return is_session_storage_for_prerendering_; - } - void SetRemoteAreaForTesting( mojo::PendingRemote area) { remote_area_.Bind(std::move(area)); @@ -182,12 +177,6 @@ class MODULES_EXPORT CachedStorageArea const AreaType type_; const scoped_refptr origin_; const WeakPersistent 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 task_runner_; std::unique_ptr map_; diff --git a/blink/renderer/modules/storage/cached_storage_area_test.cc b/blink/renderer/modules/storage/cached_storage_area_test.cc index 06cb445b04f9..e40dc9ff2a74 100644 --- 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( area_type, kOrigin, scheduler::GetSingleThreadTaskRunnerForTesting(), - nullptr, /*is_session_storage_for_prerendering=*/false); + nullptr); cached_area_->SetRemoteAreaForTesting( mock_storage_area_.GetInterfaceRemote()); source_area_ = MakeGarbageCollected(kPageUrl); diff --git a/blink/renderer/modules/storage/dom_window_storage.cc b/blink/renderer/modules/storage/dom_window_storage.cc index 9888455e9075..672329566061 100644 --- 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 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 index 9a0f2c1dafd4..32373c87d730 100644 --- 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 index 698c270cf873..941e2bdf63d4 100644 --- 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 cached_area_; + const scoped_refptr 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 index 3827a9afddbc..b10a0f2d9edc 100644 --- a/blink/renderer/modules/storage/storage_namespace.cc +++ b/blink/renderer/modules/storage/storage_namespace.cc @@ -100,23 +100,11 @@ scoped_refptr StorageNamespace::GetCachedArea( result = base::MakeRefCounted( 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 StorageNamespace::CreateCachedAreaForPrerender( - const SecurityOrigin* origin_ptr) { - DCHECK((IsSessionStorage())); - scoped_refptr origin(origin_ptr); - return base::MakeRefCounted( - 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 index ec3658217d66..bb62188a8d2d 100644 --- 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 GetCachedArea(const SecurityOrigin* origin); - scoped_refptr 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/blink/web_tests/wpt_internal/prerender/resources/session-storage-carry-over-to-prerender-page.html b/blink/web_tests/wpt_internal/prerender/resources/session-storage-carry-over-to-prerender-page.html deleted file mode 100644 index c3a6fe072145..000000000000 --- a/blink/web_tests/wpt_internal/prerender/resources/session-storage-carry-over-to-prerender-page.html +++ /dev/null @@ -1,20 +0,0 @@ - - - - - - diff --git a/blink/web_tests/wpt_internal/prerender/resources/session-storage-isolated-while-prerendering.html b/blink/web_tests/wpt_internal/prerender/resources/session-storage-isolated-while-prerendering.html deleted file mode 100644 index 4fcd5ac1f9f1..000000000000 --- a/blink/web_tests/wpt_internal/prerender/resources/session-storage-isolated-while-prerendering.html +++ /dev/null @@ -1,41 +0,0 @@ - - - - - - diff --git a/blink/web_tests/wpt_internal/prerender/resources/session-storage-no-leak-to-initiator-page.html b/blink/web_tests/wpt_internal/prerender/resources/session-storage-no-leak-to-initiator-page.html deleted file mode 100644 index 0348439363d0..000000000000 --- a/blink/web_tests/wpt_internal/prerender/resources/session-storage-no-leak-to-initiator-page.html +++ /dev/null @@ -1,35 +0,0 @@ - - - - - - diff --git a/blink/web_tests/wpt_internal/prerender/resources/session-storage-swap-after-activate.html b/blink/web_tests/wpt_internal/prerender/resources/session-storage-swap-after-activate.html deleted file mode 100644 index f81841b1a44f..000000000000 --- a/blink/web_tests/wpt_internal/prerender/resources/session-storage-swap-after-activate.html +++ /dev/null @@ -1,76 +0,0 @@ - - - - - - diff --git a/blink/web_tests/wpt_internal/prerender/resources/session-storage-utils.js b/blink/web_tests/wpt_internal/prerender/resources/session-storage-utils.js deleted file mode 100644 index d76886077944..000000000000 --- a/blink/web_tests/wpt_internal/prerender/resources/session-storage-utils.js +++ /dev/null @@ -1,76 +0,0 @@ -// Copyright 2021 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -function getSessionStorageKeys() { - let keys = []; - let txt = ''; - for (let i = 0; i < sessionStorage.length; ++i) { - keys.push(sessionStorage.key(i)); - } - keys.sort(); - keys.forEach((key) => { - if (txt.length) { - txt += ', '; - } - txt += key; - }); - return txt; -} - -function getNextMessage(channel) { - return new Promise(resolve => { - channel.addEventListener('message', e => { - resolve(e.data); - }, {once: true}); - }); -} - -// session_storage_test() is a utility function for running session storage -// related tests that open a initiator page using window.open(). -function session_storage_test(testPath) { - promise_test(async t => { - const testChannel = new BroadcastChannel('test-channel'); - t.add_cleanup(() => { - testChannel.close(); - }); - const gotMessage = getNextMessage(testChannel); - const url = 'resources/' + testPath; - window.open(url, '_blank', 'noopener'); - assert_equals(await gotMessage, 'Done'); - }, testPath); -} - -// RunSessionStorageTest() is a utility function for running session storage -// related tests that requires coordinated code execution on both the initiator -// page and the prerendering page. The passed |func| function will be called -// with the following arguments: -// - isPrerendering: Whether the |func| is called in the prerendering page. -// - url: The URL of the prerendering page. |func| should call -// startPrerendering(url) when |isPrerendering| is false to start the -// prerendering. -// - channel: A Broadcast Channel which can be used to coordinate the code -// execution on the initiator page and the prerendering page. -// - done: A function that should be called when the test completes -// successfully. -async function RunSessionStorageTest(func) { - const url = new URL(document.URL); - url.searchParams.set('prerendering', ''); - const params = new URLSearchParams(location.search); - // The main test page loads the initiator page, then the initiator page will - // prerender itself with the `prerendering` parameter. - const isPrerendering = params.has('prerendering'); - const prerenderChannel = new BroadcastChannel('prerender-channel'); - const testChannel = new BroadcastChannel('test-channel'); - window.addEventListener('unload', () => { - prerenderChannel.close(); - testChannel.close(); - }); - try { - await func(isPrerendering, url.toString(), prerenderChannel, () => { - testChannel.postMessage('Done'); - }) - } catch (e) { - testChannel.postMessage(e.toString()); - } -} diff --git a/blink/web_tests/wpt_internal/prerender/session-storage.html b/blink/web_tests/wpt_internal/prerender/session-storage.html deleted file mode 100644 index 8f6bfc109294..000000000000 --- a/blink/web_tests/wpt_internal/prerender/session-storage.html +++ /dev/null @@ -1,27 +0,0 @@ - - -Same-origin prerendering can access sessionStorage - - - - - - -