Skip to content

Commit

Permalink
Update content/ navigation tests to expect same-site RFH swap
Browse files Browse the repository at this point in the history 
With RenderDocument, cross-document navigations will use new
RenderFrameHosts (and RenderViewHosts, RenderWidgetHosts, etc for
main frame navigations). This CL updates some navigation-related
tests in content/ that didn't expect those changes.

Bug: 936696
Change-Id: I41dbf58b3990e05cd0599d930d549273e36dd377
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4807454
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1188659}
  • Loading branch information
@rakina
rakina authored and Chromium LUCI CQ committed Aug 26, 2023
1 parent 6923368 commit dfcab2e
Show file tree
Hide file tree
Showing 8 changed files with 155 additions and 134 deletions.
24 changes: 14 additions & 10 deletions content/browser/loader/resource_cache_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,8 @@ IN_PROC_BROWSER_TEST_F(ResourceCacheTest,
}

// Tests that same-origin-same-process navigation doesn't change resource cache
// hosting renderer.
// hosting RenderFrameHost, unless the navigation causes a RenderFrameHost
// change.
// TODO(https://crbug.com/1446495): Flaky on Android. Enable this if we run
// an experiment on Android. No plan to run an experiment on Android for now.
#if BUILDFLAG(IS_ANDROID)
Expand All @@ -208,7 +209,8 @@ IN_PROC_BROWSER_TEST_F(ResourceCacheTest,
const GURL kUrl = embedded_test_server()->GetURL("/simple_page.html");
const GURL kScriptUrl = embedded_test_server()->GetURL("/cacheable.js");

// Disable BFCache so that RenderFrameHost swap won't happen.
// Disable BFCache so that RenderFrameHost swap won't happen if possible (note
// that if RenderDocument is enabled, the swap will still happen).
DisableBackForwardCacheForTesting(shell()->web_contents(),
BackForwardCache::DisableForTestingReason::
TEST_ASSUMES_NO_RENDER_FRAME_CHANGE);
Expand All @@ -217,10 +219,10 @@ IN_PROC_BROWSER_TEST_F(ResourceCacheTest,

// Navigate to a test page and fetch a script.
ASSERT_TRUE(NavigateToURL(shell(), kUrl));
RenderFrameHostImpl* frame = static_cast<RenderFrameHostImpl*>(
shell()->web_contents()->GetPrimaryMainFrame());
ASSERT_TRUE(frame);
ASSERT_TRUE(FetchScript(frame, kScriptUrl));
RenderFrameHostImplWrapper frame(static_cast<RenderFrameHostImpl*>(
shell()->web_contents()->GetPrimaryMainFrame()));
ASSERT_TRUE(frame.get());
ASSERT_TRUE(FetchScript(frame.get(), kScriptUrl));

// Create a new tab, navigate to the test page in the tab. This triggers
// ResourceCache creation in the first tab.
Expand All @@ -230,15 +232,17 @@ IN_PROC_BROWSER_TEST_F(ResourceCacheTest,
second_shell->web_contents()->GetPrimaryMainFrame());
ASSERT_TRUE(second_frame);

ASSERT_TRUE(IsRenderFrameHostingRemoteCache(frame));
ASSERT_TRUE(IsRenderFrameHostingRemoteCache(frame.get()));
ASSERT_FALSE(IsRenderFrameHostingRemoteCache(second_frame));

// Trigger same-origin-same-process navigation. This shouldn't change the
// resource cache host.
// Trigger same-origin-same-process navigation. The resource cache now lives
// in the newly committed document's RenderFrameHost (which might be a
// different RenderFrameHost than the previous document's).
const GURL kUrl2 = embedded_test_server()->GetURL("/hello.html");
ASSERT_TRUE(NavigateToURL(shell(), kUrl2));

ASSERT_TRUE(IsRenderFrameHostingRemoteCache(frame));
ASSERT_TRUE(IsRenderFrameHostingRemoteCache(static_cast<RenderFrameHostImpl*>(
shell()->web_contents()->GetPrimaryMainFrame())));
ASSERT_FALSE(IsRenderFrameHostingRemoteCache(second_frame));

ASSERT_TRUE(FetchScript(second_frame, kScriptUrl));
Expand Down
2 changes: 1 addition & 1 deletion content/browser/renderer_host/navigation_controller_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1882,7 +1882,7 @@ TEST_F(NavigationControllerTest,
// Before it can commit, start loading a different page...
auto navigation2 =
NavigationSimulator::CreateBrowserInitiated(url2, contents());
navigation2->ReadyToCommit();
navigation2->Start();
int entry_id2 = controller.GetPendingEntry()->GetUniqueID();
EXPECT_NE(entry_id1, entry_id2);

Expand Down
56 changes: 35 additions & 21 deletions content/browser/renderer_host/navigator_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,15 +280,14 @@ TEST_F(NavigatorTest, RendererAbortedAboutBlankNavigation) {
contents()->NavigateAndCommit(kUrl0);
EXPECT_TRUE(main_test_rfh()->IsRenderFrameLive());

// The test expects cross-site navigations to change SiteInstances, but not
// same-site navigations. Return if SiteInstance change is not possible, and
// disable same-site back/forward cache to ensure SiteInstance won't change
// for same-site navigations.
// The test expects cross-site navigations to change RenderFrameHosts, but not
// same-site navigations. Return if that can't be satisfied.
DisableBackForwardCacheForTesting(
contents(), BackForwardCache::TEST_ASSUMES_NO_RENDER_FRAME_CHANGE);
if (!ExpectSiteInstanceChangeWithoutBackForwardCache(
main_test_rfh()->GetSiteInstance())) {
return;
main_test_rfh()->GetSiteInstance()) ||
ShouldCreateNewHostForAllFrames()) {
GTEST_SKIP();
}

// Start a renderer-initiated navigation to about:blank.
Expand Down Expand Up @@ -340,15 +339,14 @@ TEST_F(NavigatorTest,
contents()->NavigateAndCommit(kUrl0);
EXPECT_TRUE(main_test_rfh()->IsRenderFrameLive());

// The test expects cross-site navigations to change SiteInstances, but not
// same-site navigations. Return if SiteInstance change is not possible, and
// disable same-site back/forward cache to ensure SiteInstance won't change
// for same-site navigations.
// The test expects cross-site navigations to change RenderFrameHosts, but not
// same-site navigations. Return if that can't be satisfied.
DisableBackForwardCacheForTesting(
contents(), BackForwardCache::TEST_ASSUMES_NO_RENDER_FRAME_CHANGE);
if (!ExpectSiteInstanceChangeWithoutBackForwardCache(
main_test_rfh()->GetSiteInstance())) {
return;
main_test_rfh()->GetSiteInstance()) ||
ShouldCreateNewHostForAllFrames()) {
GTEST_SKIP();
}

// Start a renderer-initiated navigation to about:blank.
Expand Down Expand Up @@ -660,14 +658,22 @@ TEST_F(NavigatorTest, RedirectCrossSite) {
navigation->Start();
NavigationRequest* main_request = node->navigation_request();
ASSERT_TRUE(main_request);
EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
if (ShouldCreateNewHostForAllFrames()) {
EXPECT_TRUE(GetSpeculativeRenderFrameHost(node));
} else {
EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
}

// It then redirects to another site.
navigation->Redirect(kUrl2);

// The redirect should have been followed.
EXPECT_EQ(1, GetLoaderForNavigationRequest(main_request)->redirect_count());
EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
if (ShouldCreateNewHostForAllFrames()) {
EXPECT_TRUE(GetSpeculativeRenderFrameHost(node));
} else {
EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
}

navigation->ReadyToCommit();
TestRenderFrameHost* final_speculative_rfh =
Expand Down Expand Up @@ -1000,7 +1006,7 @@ TEST_F(NavigatorTest,

// PlzNavigate: Test that a reload navigation is properly signaled to the
// RenderFrame when the navigation can commit. A speculative RenderFrameHost
// should not be created at any step.
// should not be created at any step, unless RenderDocument is enabled.
TEST_F(NavigatorTest, Reload) {
const GURL kUrl("http://www.google.com/");
contents()->NavigateAndCommit(kUrl);
Expand All @@ -1015,7 +1021,11 @@ TEST_F(NavigatorTest, Reload) {
EXPECT_EQ(blink::mojom::NavigationType::RELOAD,
main_request->common_params().navigation_type);
reload1->ReadyToCommit();
EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
if (ShouldCreateNewHostForAllFrames()) {
EXPECT_TRUE(GetSpeculativeRenderFrameHost(node));
} else {
EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
}

reload1->Commit();
EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
Expand All @@ -1030,7 +1040,11 @@ TEST_F(NavigatorTest, Reload) {
EXPECT_EQ(blink::mojom::NavigationType::RELOAD_BYPASSING_CACHE,
main_request->common_params().navigation_type);
reload2->ReadyToCommit();
EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
if (ShouldCreateNewHostForAllFrames()) {
EXPECT_TRUE(GetSpeculativeRenderFrameHost(node));
} else {
EXPECT_FALSE(GetSpeculativeRenderFrameHost(node));
}
}

// PlzNavigate: Confirm that a speculative RenderFrameHost is used when
Expand Down Expand Up @@ -1450,15 +1464,15 @@ TEST_F(NavigatorTest, TwoNavigationsRacingCommit) {
first_navigation->ReadyToCommit();
EXPECT_EQ(1u, contents()->GetPrimaryMainFrame()->navigation_requests_.size());

// A second navigation starts and reaches ReadyToCommit.
// A second navigation starts.
auto second_navigation =
NavigationSimulator::CreateBrowserInitiated(kUrl1, contents());
second_navigation->ReadyToCommit();
EXPECT_EQ(2u, contents()->GetPrimaryMainFrame()->navigation_requests_.size());
second_navigation->Start();
EXPECT_EQ(1u, contents()->GetPrimaryMainFrame()->navigation_requests_.size());

// The first navigation commits.
first_navigation->Commit();
EXPECT_EQ(1u, contents()->GetPrimaryMainFrame()->navigation_requests_.size());
EXPECT_EQ(0u, contents()->GetPrimaryMainFrame()->navigation_requests_.size());

// The second navigation commits.
second_navigation->Commit();
Expand Down
10 changes: 7 additions & 3 deletions content/browser/renderer_host/render_frame_host_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "content/browser/renderer_host/input/timeout_monitor.h"
#include "content/browser/renderer_host/navigation_controller_impl.h"
#include "content/browser/renderer_host/render_frame_host_impl.h"
#include "content/common/content_navigation_policy.h"
#include "content/public/browser/cors_origin_pattern_setter.h"
#include "content/public/browser/shared_cors_origin_access_list.h"
#include "content/public/browser/web_contents_delegate.h"
Expand Down Expand Up @@ -114,10 +115,10 @@ TEST_F(RenderFrameHostImplTest, ExpectedMainWorldOrigin) {
// This test is for a bug that only happens when there is no RFH swap on
// same-site navigations, so we should disable same-site proactive
// BrowsingInstance for |initial_rfh| before continiung.
// Note: this will not disable RenderDocument.
// TODO(crbug.com/936696): Skip this test when main-frame RenderDocument is
// enabled.
DisableProactiveBrowsingInstanceSwapFor(initial_rfh);
if (ShouldCreateNewHostForAllFrames()) {
GTEST_SKIP();
}
// Verify expected main world origin in a steady state - after a commit it
// should be the same as the last committed origin.
EXPECT_EQ(url::Origin::Create(initial_url),
Expand Down Expand Up @@ -259,6 +260,9 @@ TEST_F(RenderFrameHostImplTest, IsolationInfoDuringCommit) {
// This test is targetted at the case an RFH is reused between navigations.
RenderFrameHost* initial_rfh = main_rfh();
DisableProactiveBrowsingInstanceSwapFor(main_rfh());
if (ShouldCreateNewHostForAllFrames()) {
GTEST_SKIP();
}

// Check values for the initial commit.
EXPECT_EQ(expected_initial_origin, main_rfh()->GetLastCommittedOrigin());
Expand Down
71 changes: 71 additions & 0 deletions content/browser/renderer_host/render_process_host_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2352,6 +2352,77 @@ IN_PROC_BROWSER_TEST_P(RenderProcessHostTest, ClearResourceCache) {
ASSERT_EQ(num_script_requests_from_renderer, 2u);
}

// Tests that RenderProcessHost reuse works correctly even if the site URL of a
// URL changes.
IN_PROC_BROWSER_TEST_P(RenderProcessHostTest, ReuseSiteURLChanges) {
ASSERT_TRUE(embedded_test_server()->Start());
const GURL kUrl = embedded_test_server()->GetURL("/title1.html");
const GURL kModifiedSiteUrl("custom-scheme://custom");

// At first, trying to get a RenderProcessHost with the
// REUSE_PENDING_OR_COMMITTED_SITE policy should return a new process.
BrowserContext* context = shell()->web_contents()->GetBrowserContext();
scoped_refptr<SiteInstanceImpl> site_instance =
SiteInstanceImpl::CreateReusableInstanceForTesting(context, kUrl);
FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
->GetPrimaryFrameTree()
.root();
EXPECT_NE(root->current_frame_host()->GetProcess(),
site_instance->GetProcess());

// Have the main frame navigate to the first url. Getting a RenderProcessHost
// with the REUSE_PENDING_OR_COMMITTED_SITE policy should now return the
// process of the main RFH.
EXPECT_TRUE(NavigateToURL(shell(), kUrl));
site_instance =
SiteInstanceImpl::CreateReusableInstanceForTesting(context, kUrl);
EXPECT_EQ(root->current_frame_host()->GetProcess(),
site_instance->GetProcess());

// Install the custom ContentBrowserClient. Site URLs are now modified.
// Getting a RenderProcessHost with the REUSE_PENDING_OR_COMMITTED_SITE policy
// should no longer return the process of the main RFH, as the RFH is
// registered with the normal site URL.
{
EffectiveURLContentBrowserTestContentBrowserClient modified_client(
kUrl, kModifiedSiteUrl,
/* requires_dedicated_process */ false);
site_instance =
SiteInstanceImpl::CreateReusableInstanceForTesting(context, kUrl);
EXPECT_NE(root->current_frame_host()->GetProcess(),
site_instance->GetProcess());

// Reload. Getting a RenderProcessHost with the
// REUSE_PENDING_OR_COMMITTED_SITE policy should now return the process of
// the main RFH, as it is now registered with the modified site URL.
shell()->web_contents()->GetController().Reload(ReloadType::NORMAL, false);
EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
site_instance =
SiteInstanceImpl::CreateReusableInstanceForTesting(context, kUrl);
EXPECT_EQ(root->current_frame_host()->GetProcess(),
site_instance->GetProcess());
}

// Remove the custom ContentBrowserClient. Site URLs are back to normal.
// Getting a RenderProcessHost with the REUSE_PENDING_OR_COMMITTED_SITE policy
// should no longer return the process of the main RFH, as it is registered
// with the modified site URL.
site_instance =
SiteInstanceImpl::CreateReusableInstanceForTesting(context, kUrl);
EXPECT_NE(root->current_frame_host()->GetProcess(),
site_instance->GetProcess());

// Reload. Getting a RenderProcessHost with the
// REUSE_PENDING_OR_COMMITTED_SITE policy should now return the process of the
// main RFH, as it is now registered with the regular site URL.
shell()->web_contents()->GetController().Reload(ReloadType::NORMAL, false);
EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
site_instance =
SiteInstanceImpl::CreateReusableInstanceForTesting(context, kUrl);
EXPECT_EQ(root->current_frame_host()->GetProcess(),
site_instance->GetProcess());
}

#if BUILDFLAG(ALLOW_OOP_VIDEO_DECODER)
class FakeStableVideoDecoderFactoryService
: public media::stable::mojom::StableVideoDecoderFactory {
Expand Down
72 changes: 0 additions & 72 deletions content/browser/renderer_host/render_process_host_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -946,78 +946,6 @@ TEST_F(RenderProcessHostUnitTest,
EXPECT_EQ(speculative_process_host_id, site_instance->GetProcess()->GetID());
}

// Tests that RenderProcessHost reuse works correctly even if the site URL of a
// URL changes.
TEST_F(RenderProcessHostUnitTest, ReuseSiteURLChanges) {
const GURL kUrl("http://foo.com");
const GURL kModifiedSiteUrl("custom-scheme://custom");

// At first, trying to get a RenderProcessHost with the
// REUSE_PENDING_OR_COMMITTED_SITE policy should return a new process.
scoped_refptr<SiteInstanceImpl> site_instance =
SiteInstanceImpl::CreateReusableInstanceForTesting(browser_context(),
kUrl);
EXPECT_NE(main_test_rfh()->GetProcess(), site_instance->GetProcess());

// Have the main frame navigate to the first url. Getting a RenderProcessHost
// with the REUSE_PENDING_OR_COMMITTED_SITE policy should now return the
// process of the main RFH.
NavigateAndCommit(kUrl);
site_instance = SiteInstanceImpl::CreateReusableInstanceForTesting(
browser_context(), kUrl);
EXPECT_EQ(main_test_rfh()->GetProcess(), site_instance->GetProcess());

// Install the custom ContentBrowserClient. Site URLs are now modified.
// Getting a RenderProcessHost with the REUSE_PENDING_OR_COMMITTED_SITE policy
// should no longer return the process of the main RFH, as the RFH is
// registered with the normal site URL.
EffectiveURLContentBrowserClient modified_client(
kUrl, kModifiedSiteUrl,
/* requires_dedicated_process */ false);
ContentBrowserClient* regular_client =
SetBrowserClientForTesting(&modified_client);
site_instance = SiteInstanceImpl::CreateReusableInstanceForTesting(
browser_context(), kUrl);
EXPECT_NE(main_test_rfh()->GetProcess(), site_instance->GetProcess());

// Reload. Getting a RenderProcessHost with the
// REUSE_PENDING_OR_COMMITTED_SITE policy should now return the process of the
// main RFH, as it is now registered with the modified site URL.
contents()->GetController().Reload(ReloadType::NORMAL, false);
TestRenderFrameHost* rfh = main_test_rfh();
// In --site-per-process, the reload will use the pending/speculative RFH
// instead of the current one.
if (contents()->GetSpeculativePrimaryMainFrame())
rfh = contents()->GetSpeculativePrimaryMainFrame();
rfh->PrepareForCommit();
rfh->SendNavigate(0, true, kUrl);
site_instance = SiteInstanceImpl::CreateReusableInstanceForTesting(
browser_context(), kUrl);
EXPECT_EQ(rfh->GetProcess(), site_instance->GetProcess());

// Remove the custom ContentBrowserClient. Site URLs are back to normal.
// Getting a RenderProcessHost with the REUSE_PENDING_OR_COMMITTED_SITE policy
// should no longer return the process of the main RFH, as it is registered
// with the modified site URL.
SetBrowserClientForTesting(regular_client);
site_instance = SiteInstanceImpl::CreateReusableInstanceForTesting(
browser_context(), kUrl);
EXPECT_NE(rfh->GetProcess(), site_instance->GetProcess());

// Reload. Getting a RenderProcessHost with the
// REUSE_PENDING_OR_COMMITTED_SITE policy should now return the process of the
// main RFH, as it is now registered with the regular site URL.
contents()->GetController().Reload(ReloadType::NORMAL, false);
rfh = contents()->GetSpeculativePrimaryMainFrame()
? contents()->GetSpeculativePrimaryMainFrame()
: main_test_rfh();
rfh->PrepareForCommit();
rfh->SendNavigate(0, true, kUrl);
site_instance = SiteInstanceImpl::CreateReusableInstanceForTesting(
browser_context(), kUrl);
EXPECT_EQ(rfh->GetProcess(), site_instance->GetProcess());
}

// Tests that RenderProcessHost reuse works correctly even if the site URL of a
// URL we're navigating to changes.
TEST_F(RenderProcessHostUnitTest, ReuseExpectedSiteURLChanges) {
Expand Down

0 comments on commit dfcab2e

Please sign in to comment.