Skip to content

Commit

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

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 eefb8c5
>
> 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}
  • Loading branch information
@natechapin
natechapin authored and Chromium LUCI CQ committed May 18, 2021
1 parent 029cb1a commit a015926
Show file tree
Hide file tree
Showing 21 changed files with 28 additions and 717 deletions.
284 changes: 2 additions & 282 deletions content/browser/prerender/prerender_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,18 @@
#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"
#include "content/browser/prerender/prerender_metrics.h"
#include "content/browser/renderer_host/back_forward_cache_impl.h"
#include "content/browser/renderer_host/frame_tree_node.h"
#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/common/content_navigation_policy.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/navigation_handle.h"
#include "content/public/browser/render_document_host_user_data.h"
#include "content/public/common/content_client.h"
#include "content/public/common/content_features.h"
Expand Down Expand Up @@ -243,13 +238,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 @@ -1788,280 +1782,6 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, LazyLoading) {
EXPECT_EQ(GetRequestCount(kImageUrl), 1);
}

IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest,
SessionStorageAfterBackNavigation_NoProcessReuse) {
// When BackForwardCache feature is enabled, this test doesn't work, because
// this test is checking the behavior of a new renderer process which is
// created for a back forward navigation from a prerendered page.
if (IsBackForwardCacheEnabled())
return;

const GURL kInitialUrl = GetUrl("/prerender/session_storage.html");
const GURL kPrerenderingUrl =
GetUrl("/prerender/session_storage.html?prerendering=");

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

std::unique_ptr<RenderProcessHostWatcher> process_host_watcher =
std::make_unique<RenderProcessHostWatcher>(
current_frame_host()->GetProcess(),
RenderProcessHostWatcher::WATCH_FOR_HOST_DESTRUCTION);

AddPrerender(kPrerenderingUrl);
NavigatePrimaryPage(kPrerenderingUrl);

EXPECT_EQ("initial", EvalJs(current_frame_host(),
"window.sessionKeysInPrerenderingchange")
.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_watcher->Wait();

// Navigate back to the initial page.
content::TestNavigationObserver observer(shell()->web_contents());
shell()->GoBackOrForward(-1);
observer.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");
const GURL kPrerenderingUrl =
GetUrl("/prerender/session_storage.html?prerendering=");

// 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();

AddPrerender(kPrerenderingUrl);
NavigatePrimaryPage(kPrerenderingUrl);

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

// Navigate back to the initial page.
content::TestNavigationObserver observer(shell()->web_contents());
shell()->GoBackOrForward(-1);
observer.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");
const GURL kPrerenderingUrl =
GetUrl("/prerender/session_storage.html?prerendering=");

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

AddPrerender(kPrerenderingUrl);
NavigatePrimaryPage(kPrerenderingUrl);

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

// Navigate back to the initial page.
content::TestNavigationObserver observer(shell()->web_contents());
shell()->GoBackOrForward(-1);
observer.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, {{"enable_same_site", "true"}}},
{kBackForwardCacheNoTimeEviction, {}}},
// 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");
const GURL kPrerenderingUrl =
GetUrl("/prerender/session_storage.html?prerendering=");

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

RenderFrameDeletedObserver deleted_observer(
shell()->web_contents()->GetMainFrame());

AddPrerender(kPrerenderingUrl);
NavigatePrimaryPage(kPrerenderingUrl);

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

// Navigate back to the initial page.
shell()->GoBackOrForward(-1);
WaitForLoadStop(shell()->web_contents());

// Expect the navigation to be served from the back-forward cache to verify
// the test is testing what is intended.
ASSERT_EQ(shell()->web_contents()->GetMainFrame(),
deleted_observer.render_frame_host());

// 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());
}

#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");
const GURL kPrerenderingUrl =
GetUrl("/prerender/session_storage.html?prerendering=");

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

CrashStorageServiceAndWaitForRestart();

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

AddPrerender(kPrerenderingUrl);
NavigatePrimaryPage(kPrerenderingUrl);

EXPECT_EQ("initial", EvalJs(current_frame_host(),
"window.sessionKeysInPrerenderingchange")
.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");
const GURL kPrerenderingUrl =
GetUrl("/prerender/session_storage.html?prerendering=");

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

const int host_id = AddPrerender(kPrerenderingUrl);

CrashStorageServiceAndWaitForRestart();

EXPECT_EQ(
"initial",
EvalJs(current_frame_host(), "getSessionStorageKeys()").ExtractString());
EXPECT_EQ(
"initial, prerendering",
EvalJs(GetPrerenderedMainFrameHost(host_id), "getSessionStorageKeys()")
.ExtractString());

NavigatePrimaryPage(kPrerenderingUrl);

EXPECT_EQ("initial", EvalJs(current_frame_host(),
"window.sessionKeysInPrerenderingchange")
.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
5 changes: 5 additions & 0 deletions content/browser/renderer_host/back_forward_cache_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ namespace {

using blink::scheduler::WebSchedulerTrackedFeature;

// Removes the time limit for cached content. This is used on bots to identify
// accidentally passing tests.
const base::Feature kBackForwardCacheNoTimeEviction{
"BackForwardCacheNoTimeEviction", base::FEATURE_DISABLED_BY_DEFAULT};

// The default number of entries the BackForwardCache can hold per tab.
static constexpr size_t kDefaultBackForwardCacheSize = 1;

Expand Down
5 changes: 0 additions & 5 deletions content/browser/renderer_host/back_forward_cache_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,6 @@ constexpr base::Feature kRecordBackForwardCacheMetricsWithoutEnabling{
"RecordBackForwardCacheMetricsWithoutEnabling",
base::FEATURE_DISABLED_BY_DEFAULT};

// Removes the time limit for cached content. This is used on bots to identify
// accidentally passing tests.
constexpr base::Feature kBackForwardCacheNoTimeEviction{
"BackForwardCacheNoTimeEviction", base::FEATURE_DISABLED_BY_DEFAULT};

// BackForwardCache:
//
// After the user navigates away from a document, the old one goes into the
Expand Down
Loading

0 comments on commit a015926

Please sign in to comment.