Skip to content

Commit

Permalink
Revert "Use the same SessionStorageNamespace for prerendering"
Browse files Browse the repository at this point in the history 
This reverts commit eefb8c5.

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}
  • Loading branch information
Owen Min authored and Chromium LUCI CQ committed May 12, 2021
1 parent 508c4a8 commit 1fa2fab
Show file tree
Hide file tree
Showing 19 changed files with 23 additions and 743 deletions.
288 changes: 2 additions & 286 deletions content/browser/prerender/prerender_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@
#include "base/test/scoped_feature_list.h"
#include "base/thread_annotations.h"
#include "build/build_config.h"
#include "components/services/storage/public/mojom/storage_service.mojom.h"
#include "components/services/storage/public/mojom/test_api.test-mojom.h"
#include "content/browser/file_system_access/file_system_chooser_test_helpers.h"
#include "content/browser/prerender/prerender_host.h"
#include "content/browser/prerender/prerender_host_registry.h"
Expand All @@ -29,7 +27,6 @@
#include "content/browser/renderer_host/input/synthetic_tap_gesture.h"
#include "content/browser/renderer_host/render_frame_host_impl.h"
#include "content/browser/renderer_host/render_view_host_impl.h"
#include "content/browser/storage_partition_impl.h"
#include "content/browser/web_contents/web_contents_impl.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/browser_thread.h"
Expand Down Expand Up @@ -223,13 +220,12 @@ class PrerenderBrowserTest : public ContentBrowserTest {
return prerender_helper_.get();
}

void SetUpCommandLine(base::CommandLine* command_line) override {
private:
void SetUpCommandLine(base::CommandLine* command_line) final {
// Useful for testing CSP:prefetch-src
base::CommandLine::ForCurrentProcess()->AppendSwitch(
switches::kEnableExperimentalWebPlatformFeatures);
}

private:
net::test_server::EmbeddedTestServer ssl_server_{
net::test_server::EmbeddedTestServer::TYPE_HTTPS};

Expand Down Expand Up @@ -1740,286 +1736,6 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, LazyLoading) {
EXPECT_EQ(GetRequestCount(kImageUrl), 1);
}

class RenderProcessHostDestroyedWaiter : public RenderProcessHostObserver {
public:
explicit RenderProcessHostDestroyedWaiter(RenderProcessHost* host) {
observation_.Observe(host);
quit_closure_ = run_loop_.QuitClosure();
}

~RenderProcessHostDestroyedWaiter() override = default;

void Wait() { run_loop_.Run(); }

// RenderProcessHostObserver implements:
void RenderProcessHostDestroyed(RenderProcessHost* host) override {
observation_.Reset();
std::move(quit_closure_).Run();
}

private:
base::ScopedObservation<RenderProcessHost, RenderProcessHostObserver>
observation_{this};
base::RunLoop run_loop_;
base::OnceClosure quit_closure_;
};

IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
SessionStorageAfterBackNavigation_NoProcessReuse) {
const GURL kInitialUrl = GetUrl("/prerender/session_storage.html");

// Navigate to an initial page.
ASSERT_TRUE(NavigateToURL(shell(), kInitialUrl));

std::unique_ptr<RenderProcessHostDestroyedWaiter>
process_host_destroyed_waiter =
std::make_unique<RenderProcessHostDestroyedWaiter>(
current_frame_host()->GetProcess());

EXPECT_TRUE(ExecJs(current_frame_host(), "startPrerender();"));

content::TestNavigationObserver observer1(shell()->web_contents());
EXPECT_TRUE(ExecJs(current_frame_host(), "activatePrerenderedPage();"));
observer1.Wait();
EXPECT_EQ("initial",
EvalJs(current_frame_host(), "window.sessionKeysInPrerenderchange")
.ExtractString());
EXPECT_EQ(
"activated, initial",
EvalJs(current_frame_host(), "getSessionStorageKeys()").ExtractString());

// Make sure that the initial renderer process is destroyed. So that the
// initial renderer process will not be reused after the back forward
// navigation below.
process_host_destroyed_waiter->Wait();

// Navigate back to the initial page.
content::TestNavigationObserver observer2(shell()->web_contents());
shell()->GoBackOrForward(-1);
observer2.Wait();
EXPECT_EQ(shell()->web_contents()->GetLastCommittedURL(), kInitialUrl);

EXPECT_EQ(
"activated, initial",
EvalJs(current_frame_host(), "getSessionStorageKeys()").ExtractString());
}

IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
SessionStorageAfterBackNavigation_KeepInitialProcess) {
const GURL kInitialUrl = GetUrl("/prerender/session_storage.html");

// Navigate to an initial page.
ASSERT_TRUE(NavigateToURL(shell(), kInitialUrl));

RenderProcessHostImpl* initial_process_host =
static_cast<RenderProcessHostImpl*>(current_frame_host()->GetProcess());
// Increment the keep alive ref count of the renderer process to keep it alive
// so it is reused on the back navigation below. The test checks that the
// session storage state changed in the activated page is correctly propagated
// after a back navigation that uses an existing renderer process. (Note: This
// is not working correctly now.)
initial_process_host->IncrementKeepAliveRefCount();

EXPECT_TRUE(ExecJs(current_frame_host(), "startPrerender();"));

content::TestNavigationObserver observer1(shell()->web_contents());
EXPECT_TRUE(ExecJs(current_frame_host(), "activatePrerenderedPage();"));
observer1.Wait();
EXPECT_EQ("initial",
EvalJs(current_frame_host(), "window.sessionKeysInPrerenderchange")
.ExtractString());
EXPECT_EQ(
"activated, initial",
EvalJs(current_frame_host(), "getSessionStorageKeys()").ExtractString());

// Navigate back to the initial page.
content::TestNavigationObserver observer2(shell()->web_contents());
shell()->GoBackOrForward(-1);
observer2.Wait();
EXPECT_EQ(shell()->web_contents()->GetLastCommittedURL(), kInitialUrl);

// There is a known issue that when the initial renderer process is reused
// after the back navigation, the session storage state changed in the
// activated is not correctly propagated to the initial renderer process.
// TODO(crbug.com/1197383): Fix this issue.
EXPECT_EQ(
// This should be "activated, initial".
"initial",
EvalJs(current_frame_host(), "getSessionStorageKeys()").ExtractString());
}

class PrerenderSingleProcessBrowserTest : public PrerenderBrowserTest {
public:
PrerenderSingleProcessBrowserTest() = default;

void SetUpCommandLine(base::CommandLine* cmd_line) override {
PrerenderBrowserTest::SetUpCommandLine(cmd_line);
cmd_line->AppendSwitch("single-process");
}
};

IN_PROC_BROWSER_TEST_F(PrerenderSingleProcessBrowserTest,
SessionStorageAfterBackNavigation) {
const GURL kInitialUrl = GetUrl("/prerender/session_storage.html");

// Navigate to an initial page.
ASSERT_TRUE(NavigateToURL(shell(), kInitialUrl));
EXPECT_TRUE(ExecJs(current_frame_host(), "startPrerender();"));

content::TestNavigationObserver observer1(shell()->web_contents());
EXPECT_TRUE(ExecJs(current_frame_host(), "activatePrerenderedPage();"));
observer1.Wait();
EXPECT_EQ("initial",
EvalJs(current_frame_host(), "window.sessionKeysInPrerenderchange")
.ExtractString());
EXPECT_EQ(
"activated, initial",
EvalJs(current_frame_host(), "getSessionStorageKeys()").ExtractString());

// Navigate back to the initial page.
content::TestNavigationObserver observer2(shell()->web_contents());
shell()->GoBackOrForward(-1);
observer2.Wait();
EXPECT_EQ(shell()->web_contents()->GetLastCommittedURL(), kInitialUrl);

EXPECT_EQ(
"activated, initial",
EvalJs(current_frame_host(), "getSessionStorageKeys()").ExtractString());
}

class PrerenderBackForwardCacheBrowserTest : public PrerenderBrowserTest {
public:
PrerenderBackForwardCacheBrowserTest() {
feature_list_.InitWithFeaturesAndParameters(
{{features::kBackForwardCache,
// Set a very long TTL before expiration (longer than the test
// timeout) so tests that are expecting deletion don't pass when
// they shouldn't.
{{"TimeToLiveInBackForwardCacheInSeconds", "3600"}}}},
// Allow BackForwardCache for all devices regardless of their memory.
{features::kBackForwardCacheMemoryControls});
}

private:
base::test::ScopedFeatureList feature_list_;
};

IN_PROC_BROWSER_TEST_F(PrerenderBackForwardCacheBrowserTest,
SessionStorageAfterBackNavigation) {
const GURL kInitialUrl = GetUrl("/prerender/session_storage.html");

// Navigate to an initial page.
ASSERT_TRUE(NavigateToURL(shell(), kInitialUrl));
EXPECT_TRUE(ExecJs(current_frame_host(), "startPrerender();"));

content::TestNavigationObserver observer1(shell()->web_contents());
EXPECT_TRUE(ExecJs(current_frame_host(), "activatePrerenderedPage();"));
observer1.Wait();
EXPECT_EQ("initial",
EvalJs(current_frame_host(), "window.sessionKeysInPrerenderchange")
.ExtractString());
EXPECT_EQ(
"activated, initial",
EvalJs(current_frame_host(), "getSessionStorageKeys()").ExtractString());

#if DCHECK_IS_ON()
// There is a DCHECK failure issue while navigating back from a prerendered
// page (crbug.com/1201914). To avoid this issue, returns here.
// TODO(crbug.com/1201914): Remove this when the DCHECK issue is fixed.
return;
#else // DCHECK_IS_ON()
// Navigate back to the initial page.
content::TestNavigationObserver observer2(shell()->web_contents());
shell()->GoBackOrForward(-1);
observer2.Wait();
EXPECT_EQ(shell()->web_contents()->GetLastCommittedURL(), kInitialUrl);

// There is a known issue that when the initial renderer process is reused
// after the back navigation, the session storage state changed in the
// activated is not correctly propagated to the initial renderer process.
// TODO(crbug.com/1197383): Fix this issue.
EXPECT_EQ(
// This should be "activated, initial".
"initial",
EvalJs(current_frame_host(), "getSessionStorageKeys()").ExtractString());
#endif // DCHECK_IS_ON()
}

#if !defined(OS_ANDROID)
// StorageServiceOutOfProcess is not implemented on Android.

class PrerenderRestartStorageServiceBrowserTest : public PrerenderBrowserTest {
public:
PrerenderRestartStorageServiceBrowserTest() {
// These tests only make sense when the service is running
// out-of-process.
feature_list_.InitAndEnableFeature(features::kStorageServiceOutOfProcess);
}

protected:
void CrashStorageServiceAndWaitForRestart() {
mojo::Remote<storage::mojom::StorageService>& service =
StoragePartitionImpl::GetStorageServiceForTesting();
base::RunLoop loop;
service.set_disconnect_handler(base::BindLambdaForTesting([&] {
loop.Quit();
service.reset();
}));
mojo::Remote<storage::mojom::TestApi> test_api;
StoragePartitionImpl::GetStorageServiceForTesting()->BindTestApi(
test_api.BindNewPipeAndPassReceiver().PassPipe());
test_api->CrashNow();
loop.Run();
}

private:
base::test::ScopedFeatureList feature_list_;
};

IN_PROC_BROWSER_TEST_F(PrerenderRestartStorageServiceBrowserTest,
RestartStorageServiceBeforePrerendering) {
const GURL kInitialUrl = GetUrl("/prerender/session_storage.html");

// Navigate to an initial page.
ASSERT_TRUE(NavigateToURL(shell(), kInitialUrl));

CrashStorageServiceAndWaitForRestart();

EXPECT_TRUE(ExecJs(current_frame_host(), "startPrerender();"));

content::TestNavigationObserver observer(shell()->web_contents());
EXPECT_TRUE(ExecJs(current_frame_host(), "activatePrerenderedPage();"));
observer.Wait();
EXPECT_EQ("initial",
EvalJs(current_frame_host(), "window.sessionKeysInPrerenderchange")
.ExtractString());
EXPECT_EQ(
"activated, initial",
EvalJs(current_frame_host(), "getSessionStorageKeys()").ExtractString());
}

IN_PROC_BROWSER_TEST_F(PrerenderRestartStorageServiceBrowserTest,
RestartStorageServiceWhilePrerendering) {
const GURL kInitialUrl = GetUrl("/prerender/session_storage.html");

// Navigate to an initial page.
ASSERT_TRUE(NavigateToURL(shell(), kInitialUrl));

EXPECT_TRUE(ExecJs(current_frame_host(), "startPrerender();"));
CrashStorageServiceAndWaitForRestart();

content::TestNavigationObserver observer(shell()->web_contents());
EXPECT_TRUE(ExecJs(current_frame_host(), "activatePrerenderedPage();"));
observer.Wait();
EXPECT_EQ("initial",
EvalJs(current_frame_host(), "window.sessionKeysInPrerenderchange")
.ExtractString());
EXPECT_EQ(
"activated, initial",
EvalJs(current_frame_host(), "getSessionStorageKeys()").ExtractString());
}
#endif

class PrerenderWithProactiveBrowsingInstanceSwap : public PrerenderBrowserTest {
public:
PrerenderWithProactiveBrowsingInstanceSwap() {
Expand Down
19 changes: 4 additions & 15 deletions content/browser/prerender/prerender_host.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#include "content/browser/renderer_host/render_frame_host_impl.h"
#include "content/browser/renderer_host/render_frame_proxy_host.h"
#include "content/browser/renderer_host/render_view_host_impl.h"
#include "content/browser/site_instance_impl.h"
#include "content/browser/web_contents/web_contents_impl.h"
#include "content/public/browser/back_forward_cache.h"
#include "content/public/browser/navigation_controller.h"
Expand Down Expand Up @@ -78,20 +77,10 @@ class PrerenderHost::PageHolder : public FrameTree::Delegate,
&web_contents,
&web_contents,
FrameTree::Type::kPrerender)) {
scoped_refptr<SiteInstance> site_instance =
SiteInstance::Create(web_contents.GetBrowserContext());
frame_tree_->Init(site_instance.get(),
/*renderer_initiated_creation=*/false,
/*main_frame_name=*/"");

const auto& site_info =
static_cast<SiteInstanceImpl*>(site_instance.get())->GetSiteInfo();
// Use the same SessionStorageNamespace as the primary page for the
// prerendering page.
frame_tree_->controller().SetSessionStorageNamespace(
site_info.GetStoragePartitionId(site_instance->GetBrowserContext()),
web_contents_.GetFrameTree()->controller().GetSessionStorageNamespace(
site_info));
frame_tree_->Init(
SiteInstance::Create(web_contents.GetBrowserContext()).get(),
/*renderer_initiated_creation=*/false,
/*main_frame_name=*/"");

// TODO(https://crbug.com/1199679): This should be moved to FrameTree::Init
web_contents_.NotifySwappedFromRenderManager(
Expand Down
Loading

0 comments on commit 1fa2fab

Please sign in to comment.