diff options
| author | davidben@chromium.org <davidben@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-11-19 04:33:42 +0000 |
|---|---|---|
| committer | davidben@chromium.org <davidben@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-11-19 04:33:42 +0000 |
| commit | 4972fc82e6b6df8da332275d541bcead15d5a65e (patch) | |
| tree | adac31a2be698268a6c03599251c4f4601345364 | |
| parent | be12cc446c38f5d149318e8282dd32b726c02640 (diff) | |
| download | chromium_src-4972fc82e6b6df8da332275d541bcead15d5a65e.zip chromium_src-4972fc82e6b6df8da332275d541bcead15d5a65e.tar.gz chromium_src-4972fc82e6b6df8da332275d541bcead15d5a65e.tar.bz2 | |
Preserve should_replace_current_entry across request transfers.
When a request transfer happens, we lose the should_replace_current_entry bit
of the navigation. This results in us getting history wrong when, say, a hosted
Google Calendar app is installed and we client-redirect to it while not logged
in, triggering a server redirect to accounts.google.com and a request transfer.
Push this state down to the renderer into NavigationState so it can be
stapled to the request, bubbled back up to the CrossSiteResourceHandler and
into the transferred navigation.
Also test and fix a case where the state does not round-trip correctly if a
fresh renderer with a browser-initiated navigation decides to fork the
navigation back up to the browser anyway.
BUG=311721
TEST=SitePerProcessBrowserTest.ReplaceEntryCrossProcessThenTransfers
SitePerProcessBrowserTest.ReplaceEntryInProcessThenTransfers
SitePerProcessBrowserTest.ReplaceEntryCrossProcessTwice
Review URL: https://codereview.chromium.org/71993002
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@235918 0039d316-1c4b-4281-b951-d872f2087c98
23 files changed, 334 insertions, 32 deletions
diff --git a/content/browser/frame_host/render_view_host_manager.cc b/content/browser/frame_host/render_view_host_manager.cc index a8473cd..3bad2c8 100644 --- a/content/browser/frame_host/render_view_host_manager.cc +++ b/content/browser/frame_host/render_view_host_manager.cc @@ -33,7 +33,7 @@ namespace content { RenderViewHostManager::PendingNavigationParams::PendingNavigationParams() - : is_transfer(false), frame_id(-1) { + : is_transfer(false), frame_id(-1), should_replace_current_entry(false) { } RenderViewHostManager::PendingNavigationParams::PendingNavigationParams( @@ -42,13 +42,15 @@ RenderViewHostManager::PendingNavigationParams::PendingNavigationParams( const std::vector<GURL>& transfer_url_chain, Referrer referrer, PageTransition page_transition, - int64 frame_id) + int64 frame_id, + bool should_replace_current_entry) : global_request_id(global_request_id), is_transfer(is_transfer), transfer_url_chain(transfer_url_chain), referrer(referrer), page_transition(page_transition), - frame_id(frame_id) { + frame_id(frame_id), + should_replace_current_entry(should_replace_current_entry) { } RenderViewHostManager::PendingNavigationParams::~PendingNavigationParams() {} @@ -268,7 +270,7 @@ void RenderViewHostManager::SwappedOut(RenderViewHost* render_view_host) { CURRENT_TAB, pending_nav_params_->frame_id, pending_nav_params_->global_request_id, - false, + pending_nav_params_->should_replace_current_entry, true); } else if (pending_render_view_host_) { RenderProcessHostImpl* pending_process = @@ -413,7 +415,8 @@ void RenderViewHostManager::OnCrossSiteResponse( const std::vector<GURL>& transfer_url_chain, const Referrer& referrer, PageTransition page_transition, - int64 frame_id) { + int64 frame_id, + bool should_replace_current_entry) { // This should be called either when the pending RVH is ready to commit or // when we realize that the current RVH's request requires a transfer. DCHECK( @@ -427,7 +430,7 @@ void RenderViewHostManager::OnCrossSiteResponse( // If is_transfer is false, we will just run the unload handler and resume. pending_nav_params_.reset(new PendingNavigationParams( global_request_id, is_transfer, transfer_url_chain, referrer, - page_transition, frame_id)); + page_transition, frame_id, should_replace_current_entry)); // Run the unload handler of the current page. SwapOutOldPage(); diff --git a/content/browser/frame_host/render_view_host_manager.h b/content/browser/frame_host/render_view_host_manager.h index 377758f..14afcfe 100644 --- a/content/browser/frame_host/render_view_host_manager.h +++ b/content/browser/frame_host/render_view_host_manager.h @@ -224,7 +224,8 @@ class CONTENT_EXPORT RenderViewHostManager const std::vector<GURL>& transfer_url_chain, const Referrer& referrer, PageTransition page_transition, - int64 frame_id) OVERRIDE; + int64 frame_id, + bool should_replace_current_entry) OVERRIDE; // NotificationObserver implementation. virtual void Observe(int type, @@ -259,7 +260,8 @@ class CONTENT_EXPORT RenderViewHostManager const std::vector<GURL>& transfer_url, Referrer referrer, PageTransition page_transition, - int64 frame_id); + int64 frame_id, + bool should_replace_current_entry); ~PendingNavigationParams(); // The child ID and request ID for the pending navigation. Present whether @@ -286,6 +288,10 @@ class CONTENT_EXPORT RenderViewHostManager // If |is_transfer|, this is the frame ID to use in RequestTransferURL. int64 frame_id; + + // If |is_transfer|, this is whether the navigation should replace the + // current history entry. + bool should_replace_current_entry; }; // Returns whether this tab should transition to a new renderer for diff --git a/content/browser/loader/cross_site_resource_handler.cc b/content/browser/loader/cross_site_resource_handler.cc index f6274bd..c7afeb7 100644 --- a/content/browser/loader/cross_site_resource_handler.cc +++ b/content/browser/loader/cross_site_resource_handler.cc @@ -25,19 +25,47 @@ namespace content { namespace { -void OnCrossSiteResponseHelper(int render_view_id, - const GlobalRequestID& global_request_id, - bool is_transfer, - const std::vector<GURL>& transfer_url_chain, - const Referrer& referrer, - PageTransition page_transition, - int64 frame_id) { +// The parameters to OnCrossSiteResponseHelper exceed the number of arguments +// base::Bind supports. +struct CrossSiteResponseParams { + CrossSiteResponseParams(int render_view_id, + const GlobalRequestID& global_request_id, + bool is_transfer, + const std::vector<GURL>& transfer_url_chain, + const Referrer& referrer, + PageTransition page_transition, + int64 frame_id, + bool should_replace_current_entry) + : render_view_id(render_view_id), + global_request_id(global_request_id), + is_transfer(is_transfer), + transfer_url_chain(transfer_url_chain), + referrer(referrer), + page_transition(page_transition), + frame_id(frame_id), + should_replace_current_entry(should_replace_current_entry) { + } + + int render_view_id; + GlobalRequestID global_request_id; + bool is_transfer; + std::vector<GURL> transfer_url_chain; + Referrer referrer; + PageTransition page_transition; + int64 frame_id; + bool should_replace_current_entry; +}; + +void OnCrossSiteResponseHelper(const CrossSiteResponseParams& params) { RenderViewHostImpl* rvh = - RenderViewHostImpl::FromID(global_request_id.child_id, render_view_id); + RenderViewHostImpl::FromID(params.global_request_id.child_id, + params.render_view_id); if (rvh) { rvh->OnCrossSiteResponse( - global_request_id, is_transfer, transfer_url_chain, referrer, - page_transition, frame_id); + params.global_request_id, params.is_transfer, + params.transfer_url_chain, params.referrer, + params.page_transition, params.frame_id, + params.should_replace_current_entry); } } @@ -244,13 +272,14 @@ void CrossSiteResourceHandler::StartCrossSiteTransition( FROM_HERE, base::Bind( &OnCrossSiteResponseHelper, - info->GetRouteID(), - global_id, - should_transfer, - transfer_url_chain, - referrer, - info->GetPageTransition(), - frame_id)); + CrossSiteResponseParams(info->GetRouteID(), + global_id, + should_transfer, + transfer_url_chain, + referrer, + info->GetPageTransition(), + frame_id, + info->should_replace_current_entry()))); } void CrossSiteResourceHandler::ResumeIfDeferred() { diff --git a/content/browser/loader/resource_dispatcher_host_impl.cc b/content/browser/loader/resource_dispatcher_host_impl.cc index 80356e7..6da6d49 100644 --- a/content/browser/loader/resource_dispatcher_host_impl.cc +++ b/content/browser/loader/resource_dispatcher_host_impl.cc @@ -1083,6 +1083,7 @@ void ResourceDispatcherHostImpl::BeginRequest( request_data.parent_frame_id, request_data.resource_type, request_data.transition_type, + request_data.should_replace_current_entry, false, // is download false, // is stream allow_download, @@ -1230,6 +1231,7 @@ ResourceRequestInfoImpl* ResourceDispatcherHostImpl::CreateRequestInfo( -1, // parent_frame_id ResourceType::SUB_RESOURCE, PAGE_TRANSITION_LINK, + false, // should_replace_current_entry download, // is_download false, // is_stream download, // allow_download diff --git a/content/browser/loader/resource_request_info_impl.cc b/content/browser/loader/resource_request_info_impl.cc index 29a80e6..c7eed3b 100644 --- a/content/browser/loader/resource_request_info_impl.cc +++ b/content/browser/loader/resource_request_info_impl.cc @@ -43,6 +43,7 @@ void ResourceRequestInfo::AllocateForTesting( 0, // parent_frame_id resource_type, // resource_type PAGE_TRANSITION_LINK, // transition_type + false, // should_replace_current_entry false, // is_download false, // is_stream true, // allow_download @@ -95,6 +96,7 @@ ResourceRequestInfoImpl::ResourceRequestInfoImpl( int64 parent_frame_id, ResourceType::Type resource_type, PageTransition transition_type, + bool should_replace_current_entry, bool is_download, bool is_stream, bool allow_download, @@ -113,6 +115,7 @@ ResourceRequestInfoImpl::ResourceRequestInfoImpl( frame_id_(frame_id), parent_is_main_frame_(parent_is_main_frame), parent_frame_id_(parent_frame_id), + should_replace_current_entry_(should_replace_current_entry), is_download_(is_download), is_stream_(is_stream), allow_download_(allow_download), diff --git a/content/browser/loader/resource_request_info_impl.h b/content/browser/loader/resource_request_info_impl.h index 3da351a..b6f8563 100644 --- a/content/browser/loader/resource_request_info_impl.h +++ b/content/browser/loader/resource_request_info_impl.h @@ -49,6 +49,7 @@ class ResourceRequestInfoImpl : public ResourceRequestInfo, int64 parent_frame_id, ResourceType::Type resource_type, PageTransition transition_type, + bool should_replace_current_entry, bool is_download, bool is_stream, bool allow_download, @@ -109,6 +110,13 @@ class ResourceRequestInfoImpl : public ResourceRequestInfo, cross_site_handler_ = h; } + // Whether this request is part of a navigation that should replace the + // current session history entry. This state is shuffled up and down the stack + // for request transfers. + bool should_replace_current_entry() const { + return should_replace_current_entry_; + } + // Identifies the type of process (renderer, plugin, etc.) making the request. int process_type() const { return process_type_; } @@ -145,6 +153,7 @@ class ResourceRequestInfoImpl : public ResourceRequestInfo, int64 frame_id_; bool parent_is_main_frame_; int64 parent_frame_id_; + bool should_replace_current_entry_; bool is_download_; bool is_stream_; bool allow_download_; diff --git a/content/browser/loader/resource_scheduler_unittest.cc b/content/browser/loader/resource_scheduler_unittest.cc index 9f8605b..60722ee 100644 --- a/content/browser/loader/resource_scheduler_unittest.cc +++ b/content/browser/loader/resource_scheduler_unittest.cc @@ -153,6 +153,7 @@ class ResourceSchedulerTest : public testing::Test { 0, // parent_frame_id ResourceType::SUB_RESOURCE, // resource_type PAGE_TRANSITION_LINK, // transition_type + false, // should_replace_current_entry false, // is_download false, // is_stream true, // allow_download diff --git a/content/browser/renderer_host/render_view_host_delegate.h b/content/browser/renderer_host/render_view_host_delegate.h index dfefdec..c02954d 100644 --- a/content/browser/renderer_host/render_view_host_delegate.h +++ b/content/browser/renderer_host/render_view_host_delegate.h @@ -104,7 +104,8 @@ class CONTENT_EXPORT RenderViewHostDelegate { const std::vector<GURL>& transfer_url_chain, const Referrer& referrer, PageTransition page_transition, - int64 frame_id) = 0; + int64 frame_id, + bool should_replace_current_entry) = 0; protected: virtual ~RendererManagement() {} diff --git a/content/browser/renderer_host/render_view_host_impl.cc b/content/browser/renderer_host/render_view_host_impl.cc index 65b3823..2372870 100644 --- a/content/browser/renderer_host/render_view_host_impl.cc +++ b/content/browser/renderer_host/render_view_host_impl.cc @@ -669,13 +669,14 @@ void RenderViewHostImpl::OnCrossSiteResponse( const std::vector<GURL>& transfer_url_chain, const Referrer& referrer, PageTransition page_transition, - int64 frame_id) { + int64 frame_id, + bool should_replace_current_entry) { RenderViewHostDelegate::RendererManagement* manager = delegate_->GetRendererManagementDelegate(); if (manager) { manager->OnCrossSiteResponse(this, global_request_id, is_transfer, transfer_url_chain, referrer, page_transition, - frame_id); + frame_id, should_replace_current_entry); } } diff --git a/content/browser/renderer_host/render_view_host_impl.h b/content/browser/renderer_host/render_view_host_impl.h index ec5fc15..18e3a6f 100644 --- a/content/browser/renderer_host/render_view_host_impl.h +++ b/content/browser/renderer_host/render_view_host_impl.h @@ -301,7 +301,8 @@ class CONTENT_EXPORT RenderViewHostImpl const std::vector<GURL>& transfer_url_chain, const Referrer& referrer, PageTransition page_transition, - int64 frame_id); + int64 frame_id, + bool should_replace_current_entry); // Tells the renderer that this RenderView will soon be swapped out, and thus // not to create any new modal dialogs until it happens. This must be done diff --git a/content/browser/site_per_process_browsertest.cc b/content/browser/site_per_process_browsertest.cc index bfe0dcc..cf92072 100644 --- a/content/browser/site_per_process_browsertest.cc +++ b/content/browser/site_per_process_browsertest.cc @@ -8,16 +8,20 @@ #include "content/browser/frame_host/frame_tree.h" #include "content/browser/renderer_host/render_view_host_impl.h" #include "content/browser/web_contents/web_contents_impl.h" +#include "content/public/browser/navigation_entry.h" #include "content/public/browser/notification_observer.h" #include "content/public/browser/notification_service.h" #include "content/public/browser/notification_types.h" #include "content/public/browser/web_contents_observer.h" #include "content/public/common/content_switches.h" #include "content/public/test/browser_test_utils.h" +#include "content/public/test/test_navigation_observer.h" #include "content/public/test/test_utils.h" #include "content/shell/browser/shell.h" +#include "content/shell/browser/shell_content_browser_client.h" #include "content/test/content_browser_test.h" #include "content/test/content_browser_test_utils.h" +#include "net/base/escape.h" #include "net/dns/mock_host_resolver.h" namespace content { @@ -160,6 +164,20 @@ class SitePerProcessBrowserTest : public ContentBrowserTest { return result; } + void NavigateToURLContentInitiated(Shell* window, + const GURL& url, + bool should_replace_current_entry) { + std::string script; + if (should_replace_current_entry) + script = base::StringPrintf("location.replace('%s')", url.spec().c_str()); + else + script = base::StringPrintf("location.href = '%s'", url.spec().c_str()); + TestNavigationObserver load_observer(shell()->web_contents(), 1); + bool result = ExecuteScript(window->web_contents(), script); + EXPECT_TRUE(result); + load_observer.Wait(); + } + virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE { command_line->AppendSwitch(switches::kSitePerProcess); } @@ -479,4 +497,160 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, EXPECT_EQ(rvh->main_frame_id(), root->frame_id()); } +// Tests that the |should_replace_current_entry| flag persists correctly across +// request transfers that began with a cross-process navigation. +IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, + ReplaceEntryCrossProcessThenTranfers) { + const NavigationController& controller = + shell()->web_contents()->GetController(); + host_resolver()->AddRule("*", "127.0.0.1"); + ASSERT_TRUE(test_server()->Start()); + + // These must all stay in scope with replace_host. + GURL::Replacements replace_host; + std::string a_com("A.com"); + std::string b_com("B.com"); + + // Navigate to a starting URL, so there is a history entry to replace. + GURL url1 = test_server()->GetURL("files/site_isolation/blank.html?1"); + NavigateToURL(shell(), url1); + + // Force all future navigations to transfer. Note that this includes same-site + // navigiations which may cause double process swaps (via OpenURL and then via + // transfer). This test intentionally exercises that case. + ShellContentBrowserClient::SetSwapProcessesForRedirect(true); + + // Navigate to a page on A.com with entry replacement. This navigation is + // cross-site, so the renderer will send it to the browser via OpenURL to give + // to a new process. It will then be transferred into yet another process due + // to the call above. + GURL url2 = test_server()->GetURL("files/site_isolation/blank.html?2"); + replace_host.SetHostStr(a_com); + url2 = url2.ReplaceComponents(replace_host); + NavigateToURLContentInitiated(shell(), url2, true); + + // There should be one history entry. url2 should have replaced url1. + EXPECT_TRUE(controller.GetPendingEntry() == NULL); + EXPECT_EQ(1, controller.GetEntryCount()); + EXPECT_EQ(0, controller.GetCurrentEntryIndex()); + EXPECT_EQ(url2, controller.GetEntryAtIndex(0)->GetURL()); + + // Now navigate as before to a page on B.com, but normally (without + // replacement). This will still perform a double process-swap as above, via + // OpenURL and then transfer. + GURL url3 = test_server()->GetURL("files/site_isolation/blank.html?3"); + replace_host.SetHostStr(b_com); + url3 = url3.ReplaceComponents(replace_host); + NavigateToURLContentInitiated(shell(), url3, false); + + // There should be two history entries. url2 should have replaced url1. url2 + // should not have replaced url3. + EXPECT_TRUE(controller.GetPendingEntry() == NULL); + EXPECT_EQ(2, controller.GetEntryCount()); + EXPECT_EQ(1, controller.GetCurrentEntryIndex()); + EXPECT_EQ(url2, controller.GetEntryAtIndex(0)->GetURL()); + EXPECT_EQ(url3, controller.GetEntryAtIndex(1)->GetURL()); +} + +// Tests that the |should_replace_current_entry| flag persists correctly across +// request transfers that began with a content-initiated in-process +// navigation. This test is the same as the test above, except transfering from +// in-process. +IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, + ReplaceEntryInProcessThenTranfers) { + const NavigationController& controller = + shell()->web_contents()->GetController(); + ASSERT_TRUE(test_server()->Start()); + + // Navigate to a starting URL, so there is a history entry to replace. + GURL url = test_server()->GetURL("files/site_isolation/blank.html?1"); + NavigateToURL(shell(), url); + + // Force all future navigations to transfer. Note that this includes same-site + // navigiations which may cause double process swaps (via OpenURL and then via + // transfer). All navigations in this test are same-site, so it only swaps + // processes via request transfer. + ShellContentBrowserClient::SetSwapProcessesForRedirect(true); + + // Navigate in-process with entry replacement. It will then be transferred + // into a new one due to the call above. + GURL url2 = test_server()->GetURL("files/site_isolation/blank.html?2"); + NavigateToURLContentInitiated(shell(), url2, true); + + // There should be one history entry. url2 should have replaced url1. + EXPECT_TRUE(controller.GetPendingEntry() == NULL); + EXPECT_EQ(1, controller.GetEntryCount()); + EXPECT_EQ(0, controller.GetCurrentEntryIndex()); + EXPECT_EQ(url2, controller.GetEntryAtIndex(0)->GetURL()); + + // Now navigate as before, but without replacement. + GURL url3 = test_server()->GetURL("files/site_isolation/blank.html?3"); + NavigateToURLContentInitiated(shell(), url3, false); + + // There should be two history entries. url2 should have replaced url1. url2 + // should not have replaced url3. + EXPECT_TRUE(controller.GetPendingEntry() == NULL); + EXPECT_EQ(2, controller.GetEntryCount()); + EXPECT_EQ(1, controller.GetCurrentEntryIndex()); + EXPECT_EQ(url2, controller.GetEntryAtIndex(0)->GetURL()); + EXPECT_EQ(url3, controller.GetEntryAtIndex(1)->GetURL()); +} + +// Tests that the |should_replace_current_entry| flag persists correctly across +// request transfers that cross processes twice from renderer policy. +IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, + ReplaceEntryCrossProcessTwice) { + const NavigationController& controller = + shell()->web_contents()->GetController(); + host_resolver()->AddRule("*", "127.0.0.1"); + ASSERT_TRUE(test_server()->Start()); + + // These must all stay in scope with replace_host. + GURL::Replacements replace_host; + std::string a_com("A.com"); + std::string b_com("B.com"); + + // Navigate to a starting URL, so there is a history entry to replace. + GURL url1 = test_server()->GetURL("files/site_isolation/blank.html?1"); + NavigateToURL(shell(), url1); + + // Navigate to a page on A.com which redirects to B.com with entry + // replacement. This will switch processes via OpenURL twice. First to A.com, + // and second in response to the server redirect to B.com. The second swap is + // also renderer-initiated via OpenURL because decidePolicyForNavigation is + // currently applied on redirects. + GURL url2b = test_server()->GetURL("files/site_isolation/blank.html?2"); + replace_host.SetHostStr(b_com); + url2b = url2b.ReplaceComponents(replace_host); + GURL url2a = test_server()->GetURL( + "server-redirect?" + net::EscapeQueryParamValue(url2b.spec(), false)); + replace_host.SetHostStr(a_com); + url2a = url2a.ReplaceComponents(replace_host); + NavigateToURLContentInitiated(shell(), url2a, true); + + // There should be one history entry. url2b should have replaced url1. + EXPECT_TRUE(controller.GetPendingEntry() == NULL); + EXPECT_EQ(1, controller.GetEntryCount()); + EXPECT_EQ(0, controller.GetCurrentEntryIndex()); + EXPECT_EQ(url2b, controller.GetEntryAtIndex(0)->GetURL()); + + // Now repeat without replacement. + GURL url3b = test_server()->GetURL("files/site_isolation/blank.html?3"); + replace_host.SetHostStr(b_com); + url3b = url3b.ReplaceComponents(replace_host); + GURL url3a = test_server()->GetURL( + "server-redirect?" + net::EscapeQueryParamValue(url3b.spec(), false)); + replace_host.SetHostStr(a_com); + url3a = url3a.ReplaceComponents(replace_host); + NavigateToURLContentInitiated(shell(), url3a, false); + + // There should be two history entries. url2b should have replaced url1. url2b + // should not have replaced url3b. + EXPECT_TRUE(controller.GetPendingEntry() == NULL); + EXPECT_EQ(2, controller.GetEntryCount()); + EXPECT_EQ(1, controller.GetCurrentEntryIndex()); + EXPECT_EQ(url2b, controller.GetEntryAtIndex(0)->GetURL()); + EXPECT_EQ(url3b, controller.GetEntryAtIndex(1)->GetURL()); +} + } // namespace content diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc index 31294a7..2d1cb87 100644 --- a/content/browser/web_contents/web_contents_impl.cc +++ b/content/browser/web_contents/web_contents_impl.cc @@ -203,6 +203,7 @@ void MakeNavigateParams(const NavigationEntryImpl& entry, ViewMsg_Navigate_Params* params) { params->page_id = entry.GetPageID(); params->should_clear_history_list = entry.should_clear_history_list(); + params->should_replace_current_entry = entry.should_replace_entry(); if (entry.should_clear_history_list()) { // Set the history list related parameters to the same values a // NavigationController would return before its first navigation. This will diff --git a/content/browser/web_contents/web_contents_impl_unittest.cc b/content/browser/web_contents/web_contents_impl_unittest.cc index 94e0146..2c98ee2 100644 --- a/content/browser/web_contents/web_contents_impl_unittest.cc +++ b/content/browser/web_contents/web_contents_impl_unittest.cc @@ -1065,7 +1065,7 @@ TEST_F(WebContentsImplTest, CrossSiteCantPreemptAfterUnload) { url_chain.push_back(GURL()); contents()->GetRenderManagerForTesting()->OnCrossSiteResponse( pending_rvh, GlobalRequestID(0, 0), false, url_chain, Referrer(), - PAGE_TRANSITION_TYPED, 1); + PAGE_TRANSITION_TYPED, 1, false); // Suppose the original renderer navigates now, while the unload request is in // flight. We should ignore it, wait for the unload ack, and let the pending diff --git a/content/child/request_extra_data.cc b/content/child/request_extra_data.cc index ddd2208..4074079 100644 --- a/content/child/request_extra_data.cc +++ b/content/child/request_extra_data.cc @@ -19,6 +19,7 @@ RequestExtraData::RequestExtraData(WebReferrerPolicy referrer_policy, int64 parent_frame_id, bool allow_download, PageTransition transition_type, + bool should_replace_current_entry, int transferred_request_child_id, int transferred_request_request_id) : webkit_glue::WebURLRequestExtraDataImpl(referrer_policy, @@ -31,6 +32,7 @@ RequestExtraData::RequestExtraData(WebReferrerPolicy referrer_policy, parent_frame_id_(parent_frame_id), allow_download_(allow_download), transition_type_(transition_type), + should_replace_current_entry_(should_replace_current_entry), transferred_request_child_id_(transferred_request_child_id), transferred_request_request_id_(transferred_request_request_id) { } diff --git a/content/child/request_extra_data.h b/content/child/request_extra_data.h index d0c85fb..7ad43fd 100644 --- a/content/child/request_extra_data.h +++ b/content/child/request_extra_data.h @@ -27,6 +27,7 @@ class CONTENT_EXPORT RequestExtraData int64 parent_frame_id, bool allow_download, PageTransition transition_type, + bool should_replace_current_entry, int transferred_request_child_id, int transferred_request_request_id); virtual ~RequestExtraData(); @@ -38,6 +39,9 @@ class CONTENT_EXPORT RequestExtraData int64 parent_frame_id() const { return parent_frame_id_; } bool allow_download() const { return allow_download_; } PageTransition transition_type() const { return transition_type_; } + bool should_replace_current_entry() const { + return should_replace_current_entry_; + } int transferred_request_child_id() const { return transferred_request_child_id_; } @@ -53,6 +57,7 @@ class CONTENT_EXPORT RequestExtraData int64 parent_frame_id_; bool allow_download_; PageTransition transition_type_; + bool should_replace_current_entry_; int transferred_request_child_id_; int transferred_request_request_id_; diff --git a/content/child/resource_dispatcher.cc b/content/child/resource_dispatcher.cc index 37c52b0..657ffe1 100644 --- a/content/child/resource_dispatcher.cc +++ b/content/child/resource_dispatcher.cc @@ -135,6 +135,8 @@ IPCResourceLoaderBridge::IPCResourceLoaderBridge( request_.parent_frame_id = extra_data->parent_frame_id(); request_.allow_download = extra_data->allow_download(); request_.transition_type = extra_data->transition_type(); + request_.should_replace_current_entry = + extra_data->should_replace_current_entry(); request_.transferred_request_child_id = extra_data->transferred_request_child_id(); request_.transferred_request_request_id = @@ -147,6 +149,7 @@ IPCResourceLoaderBridge::IPCResourceLoaderBridge( request_.parent_frame_id = -1; request_.allow_download = true; request_.transition_type = PAGE_TRANSITION_LINK; + request_.should_replace_current_entry = false; request_.transferred_request_child_id = -1; request_.transferred_request_request_id = -1; } diff --git a/content/child/resource_dispatcher_unittest.cc b/content/child/resource_dispatcher_unittest.cc index a5b0880..76627ad 100644 --- a/content/child/resource_dispatcher_unittest.cc +++ b/content/child/resource_dispatcher_unittest.cc @@ -177,7 +177,7 @@ class ResourceDispatcherTest : public testing::Test, public IPC::Sender { blink::WebString(), false, true, 0, GURL(), false, -1, true, - PAGE_TRANSITION_LINK, -1, -1); + PAGE_TRANSITION_LINK, false, -1, -1); request_info.extra_data = &extra_data; return dispatcher_->CreateBridge(request_info); diff --git a/content/common/resource_messages.h b/content/common/resource_messages.h index 111b944..d47052f 100644 --- a/content/common/resource_messages.h +++ b/content/common/resource_messages.h @@ -192,6 +192,10 @@ IPC_STRUCT_BEGIN(ResourceHostMsg_Request) IPC_STRUCT_MEMBER(content::PageTransition, transition_type) + // For navigations, whether this navigation should replace the current session + // history entry on commit. + IPC_STRUCT_MEMBER(bool, should_replace_current_entry) + // The following two members identify a previous request that has been // created before this navigation has been transferred to a new render view. // This serves the purpose of recycling the old request. diff --git a/content/common/view_messages.h b/content/common/view_messages.h index dcf233f..df630d3 100644 --- a/content/common/view_messages.h +++ b/content/common/view_messages.h @@ -661,6 +661,11 @@ IPC_STRUCT_BEGIN(ViewMsg_Navigate_Params) // The type of transition. IPC_STRUCT_MEMBER(content::PageTransition, transition) + // Informs the RenderView the pending navigation should replace the current + // history entry when it commits. This is used for cross-process redirects so + // the transferred navigation can recover the navigation state. + IPC_STRUCT_MEMBER(bool, should_replace_current_entry) + // Opaque history state (received by ViewHostMsg_UpdateState). IPC_STRUCT_MEMBER(content::PageState, page_state) diff --git a/content/public/renderer/navigation_state.cc b/content/public/renderer/navigation_state.cc index 5745a2b..a1fc183 100644 --- a/content/public/renderer/navigation_state.cc +++ b/content/public/renderer/navigation_state.cc @@ -17,6 +17,7 @@ NavigationState::NavigationState(content::PageTransition transition_type, pending_page_id_(pending_page_id), pending_history_list_offset_(pending_history_list_offset), history_list_was_cleared_(history_list_was_cleared), + should_replace_current_entry_(false), was_within_same_page_(false), transferred_request_child_id_(-1), transferred_request_request_id_(-1), diff --git a/content/public/renderer/navigation_state.h b/content/public/renderer/navigation_state.h index 01f92a0..64b8511 100644 --- a/content/public/renderer/navigation_state.h +++ b/content/public/renderer/navigation_state.h @@ -51,6 +51,23 @@ class CONTENT_EXPORT NavigationState { return history_list_was_cleared_; } + // If is_content_initiated() is false, whether this navigation should replace + // the current entry in the back/forward history list. Otherwise, use + // replacesCurrentHistoryItem() on the WebDataSource. + // + // TODO(davidben): It would be good to unify these and have only one source + // for the two cases. We can plumb this through WebFrame::loadRequest to set + // lockBackForwardList on the FrameLoadRequest. However, this breaks process + // swaps because FrameLoader::loadWithNavigationAction treats loads before a + // FrameLoader has committedFirstRealDocumentLoad as a replacement. (Added for + // http://crbug.com/178380). + bool should_replace_current_entry() const { + return should_replace_current_entry_; + } + void set_should_replace_current_entry(bool value) { + should_replace_current_entry_ = value; + } + // Contains the transition type that the browser specified when it // initiated the load. content::PageTransition transition_type() const { return transition_type_; } @@ -111,6 +128,7 @@ class CONTENT_EXPORT NavigationState { int32 pending_page_id_; int pending_history_list_offset_; bool history_list_was_cleared_; + bool should_replace_current_entry_; bool was_within_same_page_; int transferred_request_child_id_; diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc index 03d7927..3165905 100644 --- a/content/renderer/render_frame_impl.cc +++ b/content/renderer/render_frame_impl.cc @@ -658,6 +658,21 @@ void RenderFrameImpl::willSendRequest( } } + // Attach |should_replace_current_entry| state to requests so that, should + // this navigation later require a request transfer, all state is preserved + // when it is re-created in the new process. + bool should_replace_current_entry = false; + if (navigation_state->is_content_initiated()) { + should_replace_current_entry = data_source->replacesCurrentHistoryItem(); + } else { + // If the navigation is browser-initiated, the NavigationState contains the + // correct value instead of the WebDataSource. + // + // TODO(davidben): Avoid this awkward duplication of state. See comment on + // NavigationState::should_replace_current_entry(). + should_replace_current_entry = + navigation_state->should_replace_current_entry(); + } request.setExtraData( new RequestExtraData(referrer_policy, custom_user_agent, @@ -669,6 +684,7 @@ void RenderFrameImpl::willSendRequest( frame->parent() ? frame->parent()->identifier() : -1, navigation_state->allow_download(), transition_type, + should_replace_current_entry, navigation_state->transferred_request_child_id(), navigation_state->transferred_request_request_id())); diff --git a/content/renderer/render_view_impl.cc b/content/renderer/render_view_impl.cc index 9a06f97..b21d9e0 100644 --- a/content/renderer/render_view_impl.cc +++ b/content/renderer/render_view_impl.cc @@ -2234,7 +2234,19 @@ void RenderViewImpl::OpenURL(WebFrame* frame, params.frame_id = frame->identifier(); WebDataSource* ds = frame->provisionalDataSource(); if (ds) { - params.should_replace_current_entry = ds->replacesCurrentHistoryItem(); + DocumentState* document_state = DocumentState::FromDataSource(ds); + NavigationState* navigation_state = document_state->navigation_state(); + if (navigation_state->is_content_initiated()) { + params.should_replace_current_entry = ds->replacesCurrentHistoryItem(); + } else { + // This is necessary to preserve the should_replace_current_entry value on + // cross-process redirects, in the event it was set by a previous process. + // + // TODO(davidben): Avoid this awkward duplication of state. See comment on + // NavigationState::should_replace_current_entry(). + params.should_replace_current_entry = + navigation_state->should_replace_current_entry(); + } } else { params.should_replace_current_entry = false; } @@ -3566,6 +3578,8 @@ NavigationState* RenderViewImpl::CreateNavigationStateFromPending() { params.pending_history_list_offset, params.should_clear_history_list, params.transition); + navigation_state->set_should_replace_current_entry( + params.should_replace_current_entry); navigation_state->set_transferred_request_child_id( params.transferred_request_child_id); navigation_state->set_transferred_request_request_id( @@ -3692,6 +3706,8 @@ void RenderViewImpl::didFailProvisionalLoad(WebFrame* frame, // AUTO_SUBFRAME loads should always be treated as loads that do not advance // the page id. // + // TODO(davidben): This should also take the failed navigation's replacement + // state into account, if a location.replace() failed. bool replace = navigation_state->pending_page_id() != -1 || PageTransitionCoreTypeIs(navigation_state->transition_type(), @@ -3711,6 +3727,7 @@ void RenderViewImpl::didFailProvisionalLoad(WebFrame* frame, navigation_state->transition_type(); pending_navigation_params_->request_time = document_state->request_time(); + pending_navigation_params_->should_replace_current_entry = replace; } // Provide the user with a more helpful error page? |