[bfcache] Restore pages from the cache immediately before committing.
With this CL, we now call BackForwardCache::RestoreDocument at the end
of the navigation just before committing, instead of at the start of
the navigation.
This shrinks the window of time where an eviction can occur after
the document has been removed from the cache, but before it
has been committed.
Before this patch, if an eviction arrived after the navigation started,
the behavior was undefined. With this patch, that case is now handled by
canceling the navigation. In a follow-up CL, it will be handled by
restarting the same session history navigation.
This CL also has the (pleasant) side effect of fixing an issue where
throttling wasn't handled correctly in the case of a BackForwardCache
restore, (test added in this CL).
Change-Id: I65a1600cef04f2e10f0d28a5df750b26c85e917a
Bug: 976697,995316
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1760220
Reviewed-by: Nasko Oskov <[email protected]>
Reviewed-by: Arthur Sonzogni <[email protected]>
Commit-Queue: Lowell Manners <[email protected]>
Cr-Commit-Position: refs/heads/master@{#689507}
diff --git a/content/browser/back_forward_cache_browsertest.cc b/content/browser/back_forward_cache_browsertest.cc
index f51b2c1b..293f41b 100644
--- a/content/browser/back_forward_cache_browsertest.cc
+++ b/content/browser/back_forward_cache_browsertest.cc
@@ -3,6 +3,7 @@
// found in the LICENSE file.
#include "base/command_line.h"
+#include "base/test/bind_test_util.h"
#include "base/test/scoped_feature_list.h"
#include "content/browser/frame_host/back_forward_cache.h"
#include "content/browser/frame_host/frame_tree_node.h"
@@ -14,6 +15,8 @@
#include "content/public/test/content_browser_test.h"
#include "content/public/test/content_browser_test_utils.h"
#include "content/public/test/navigation_handle_observer.h"
+#include "content/public/test/test_navigation_throttle.h"
+#include "content/public/test/test_navigation_throttle_inserter.h"
#include "content/public/test/test_utils.h"
#include "content/public/test/url_loader_interceptor.h"
#include "content/shell/browser/shell.h"
@@ -1158,4 +1161,78 @@
EvalJs(shell(), "window.testObservedEvents"));
}
+IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
+ NavigationCancelledAtWillStartRequest) {
+ ASSERT_TRUE(embedded_test_server()->Start());
+ const GURL url_a(embedded_test_server()->GetURL("a.com", "/title1.html"));
+ const GURL url_b(embedded_test_server()->GetURL("b.com", "/title1.html"));
+
+ // 1) Navigate to A.
+ EXPECT_TRUE(NavigateToURL(shell(), url_a));
+ RenderFrameHostImpl* rfh_a = current_frame_host();
+ RenderFrameDeletedObserver delete_observer_rfh_a(rfh_a);
+
+ // 2) Navigate to B.
+ EXPECT_TRUE(NavigateToURL(shell(), url_b));
+ RenderFrameHostImpl* rfh_b = current_frame_host();
+ EXPECT_FALSE(delete_observer_rfh_a.deleted());
+ EXPECT_TRUE(rfh_a->is_in_back_forward_cache());
+
+ // Cancel all navigation attempts.
+ content::TestNavigationThrottleInserter throttle_inserter(
+ shell()->web_contents(),
+ base::BindLambdaForTesting(
+ [&](NavigationHandle* handle) -> std::unique_ptr<NavigationThrottle> {
+ auto throttle = std::make_unique<TestNavigationThrottle>(handle);
+ throttle->SetResponse(TestNavigationThrottle::WILL_START_REQUEST,
+ TestNavigationThrottle::SYNCHRONOUS,
+ NavigationThrottle::CANCEL_AND_IGNORE);
+ return throttle;
+ }));
+
+ // 3) Go back to A, which will be cancelled by the NavigationThrottle.
+ web_contents()->GetController().GoBack();
+ EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+
+ // We should still be showing page B.
+ EXPECT_EQ(rfh_b, current_frame_host());
+}
+
+IN_PROC_BROWSER_TEST_F(BackForwardCacheBrowserTest,
+ NavigationCancelledAtWillProcessResponse) {
+ ASSERT_TRUE(embedded_test_server()->Start());
+ const GURL url_a(embedded_test_server()->GetURL("a.com", "/title1.html"));
+ const GURL url_b(embedded_test_server()->GetURL("b.com", "/title1.html"));
+
+ // 1) Navigate to A.
+ EXPECT_TRUE(NavigateToURL(shell(), url_a));
+ RenderFrameHostImpl* rfh_a = current_frame_host();
+ RenderFrameDeletedObserver delete_observer_rfh_a(rfh_a);
+
+ // 2) Navigate to B.
+ EXPECT_TRUE(NavigateToURL(shell(), url_b));
+ RenderFrameHostImpl* rfh_b = current_frame_host();
+ EXPECT_FALSE(delete_observer_rfh_a.deleted());
+ EXPECT_TRUE(rfh_a->is_in_back_forward_cache());
+
+ // Cancel all navigation attempts.
+ content::TestNavigationThrottleInserter throttle_inserter(
+ shell()->web_contents(),
+ base::BindLambdaForTesting(
+ [&](NavigationHandle* handle) -> std::unique_ptr<NavigationThrottle> {
+ auto throttle = std::make_unique<TestNavigationThrottle>(handle);
+ throttle->SetResponse(TestNavigationThrottle::WILL_PROCESS_RESPONSE,
+ TestNavigationThrottle::SYNCHRONOUS,
+ NavigationThrottle::CANCEL_AND_IGNORE);
+ return throttle;
+ }));
+
+ // 3) Go back to A, which will be cancelled by the NavigationThrottle.
+ web_contents()->GetController().GoBack();
+ EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+
+ // We should still be showing page B.
+ EXPECT_EQ(rfh_b, current_frame_host());
+}
+
} // namespace content
diff --git a/content/browser/frame_host/back_forward_cache.cc b/content/browser/frame_host/back_forward_cache.cc
index 6f5d50e..aece1d4 100644
--- a/content/browser/frame_host/back_forward_cache.cc
+++ b/content/browser/frame_host/back_forward_cache.cc
@@ -225,14 +225,17 @@
DCHECK(render_frame_hosts_.empty());
}
-bool BackForwardCache::ContainsDocument(int navigation_entry_id) {
+RenderFrameHostImpl* BackForwardCache::GetDocument(int navigation_entry_id) {
auto matching_rfh = std::find_if(
render_frame_hosts_.begin(), render_frame_hosts_.end(),
[navigation_entry_id](std::unique_ptr<RenderFrameHostImpl>& rfh) {
return rfh->nav_entry_id() == navigation_entry_id;
});
- return matching_rfh != render_frame_hosts_.end();
+ if (matching_rfh == render_frame_hosts_.end())
+ return nullptr;
+
+ return (*matching_rfh).get();
}
} // namespace content
diff --git a/content/browser/frame_host/back_forward_cache.h b/content/browser/frame_host/back_forward_cache.h
index 03ba10a7..ea572d6 100644
--- a/content/browser/frame_host/back_forward_cache.h
+++ b/content/browser/frame_host/back_forward_cache.h
@@ -42,9 +42,15 @@
static void Freeze(RenderFrameHostImpl* main_rfh);
static void Resume(RenderFrameHostImpl* main_rfh);
- // Returns true if the BackForwardCache contains a document with the supplied
- // |navigation_entry_id|.
- bool ContainsDocument(int navigation_entry_id);
+ // Returns a pointer to a cached RenderFrameHost matching
+ // |navigation_entry_id| if it exists in the BackForwardCache. Returns nullptr
+ // if no matching document is found.
+ //
+ // Note: The returned pointer should be used temporarily only within the
+ // execution of a single task on the event loop. Beyond that, there is no
+ // guarantee the pointer will be valid, because the document may be
+ // removed/evicted from the cache.
+ RenderFrameHostImpl* GetDocument(int navigation_entry_id);
// Remove a document from the BackForwardCache.
void EvictDocument(RenderFrameHostImpl* render_frame_host);
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index 4bc8bd4..d151840 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -2511,7 +2511,7 @@
// BackForwardCache:
// Navigate immediately if the document is in the BackForwardCache.
- if (back_forward_cache_.ContainsDocument(nav_entry_id)) {
+ if (back_forward_cache_.GetDocument(nav_entry_id)) {
DCHECK_EQ(reload_type, ReloadType::NONE);
auto navigation_request = CreateNavigationRequestFromEntry(
root, pending_entry_, pending_entry_->GetFrameEntry(root),
diff --git a/content/browser/frame_host/navigation_request.cc b/content/browser/frame_host/navigation_request.cc
index 5ba36900..b249cbdf 100644
--- a/content/browser/frame_host/navigation_request.cc
+++ b/content/browser/frame_host/navigation_request.cc
@@ -544,15 +544,13 @@
navigation_params->skip_service_worker = true;
}
- std::unique_ptr<RenderFrameHostImpl> rfh_restored_from_back_forward_cache;
+ bool is_served_from_back_forward_cache = false;
if (entry) {
NavigationControllerImpl* controller =
static_cast<NavigationControllerImpl*>(
frame_tree_node->navigator()->GetController());
- // This will be nullptr if there is no matching document in the
- // BackForwardCache.
- rfh_restored_from_back_forward_cache =
- controller->back_forward_cache().RestoreDocument(entry->GetUniqueID());
+ is_served_from_back_forward_cache =
+ controller->back_forward_cache().GetDocument(entry->GetUniqueID());
}
std::unique_ptr<NavigationRequest> navigation_request(new NavigationRequest(
@@ -560,7 +558,7 @@
std::move(commit_params), browser_initiated,
false /* from_begin_navigation */, false /* is_for_commit */, frame_entry,
entry, std::move(navigation_ui_data), nullptr, nullptr,
- std::move(rfh_restored_from_back_forward_cache)));
+ is_served_from_back_forward_cache));
if (frame_entry) {
navigation_request->blob_url_loader_factory_ =
@@ -649,7 +647,7 @@
nullptr, entry,
nullptr, // navigation_ui_data
std::move(navigation_client), std::move(navigation_initiator),
- nullptr // rfh_restored_from_back_forward_cache
+ false // is_served_from_back_forward_cache
));
navigation_request->blob_url_loader_factory_ =
std::move(blob_url_loader_factory);
@@ -721,7 +719,7 @@
nullptr /* navigation_ui_data */,
mojom::NavigationClientAssociatedPtrInfo(),
blink::mojom::NavigationInitiatorPtr(),
- nullptr /* rfh_restores_from_bfcache */));
+ false /* is_served_from_back_forward_cache */));
// Update the state of the NavigationRequest to match the fact that the
// navigation just committed.
@@ -745,7 +743,7 @@
std::unique_ptr<NavigationUIData> navigation_ui_data,
mojom::NavigationClientAssociatedPtrInfo navigation_client,
blink::mojom::NavigationInitiatorPtr navigation_initiator,
- std::unique_ptr<RenderFrameHostImpl> rfh_restored_from_back_forward_cache)
+ bool is_served_from_back_forward_cache)
: frame_tree_node_(frame_tree_node),
common_params_(std::move(common_params)),
begin_params_(std::move(begin_params)),
@@ -765,10 +763,7 @@
devtools_navigation_token_(base::UnguessableToken::Create()),
request_navigation_client_(nullptr),
commit_navigation_client_(nullptr),
- rfh_restored_from_back_forward_cache_(
- std::move(rfh_restored_from_back_forward_cache)),
- is_served_from_back_forward_cache_(
- rfh_restored_from_back_forward_cache_ != nullptr) {
+ is_served_from_back_forward_cache_(is_served_from_back_forward_cache) {
DCHECK(browser_initiated || common_params_->initiator_origin.has_value());
DCHECK(!IsRendererDebugURL(common_params_->url));
DCHECK(common_params_->method == "POST" || !common_params_->post_data);
@@ -1017,6 +1012,10 @@
}
if (!NeedsUrlLoader()) {
+ // The types of pages that don't need a URL Loader should never get served
+ // from the BackForwardCache.
+ DCHECK(!is_served_from_back_forward_cache());
+
// There is no need to make a network request for this navigation, so commit
// it immediately.
TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationRequest", this,
@@ -1024,7 +1023,8 @@
state_ = RESPONSE_STARTED;
// Select an appropriate RenderFrameHost.
- render_frame_host_ = GetFrameHostForNavigation();
+ render_frame_host_ =
+ frame_tree_node_->render_manager()->GetFrameHostForNavigation(*this);
NavigatorImpl::CheckWebUIRendererDoesNotDisplayNormalURL(
render_frame_host_, common_params_->url);
@@ -1484,8 +1484,29 @@
}
// Select an appropriate renderer to commit the navigation.
- if (response_should_be_rendered_) {
- render_frame_host_ = GetFrameHostForNavigation();
+ if (is_served_from_back_forward_cache_) {
+ NavigationControllerImpl* controller =
+ static_cast<NavigationControllerImpl*>(
+ frame_tree_node_->navigator()->GetController());
+ // TODO(https://crbug.com/995316): The render_frame_host_ referenced here
+ // can be evicted while waiting on the NavigationThrottle execution. If the
+ // RenderFrameHost is evicted, it must restart this session history
+ // navigation.
+ render_frame_host_ =
+ controller->back_forward_cache().GetDocument(nav_entry_id_);
+
+ // If render_frame_host_ is nullptr, that means the document was evicted
+ // from the cache since this navigation started. For now, we handle this
+ // by cancelling the navigation.
+ // TODO(https://crbug.com/995316): Reissue the navigation instead of
+ // cancelling it.
+ if (render_frame_host_ == nullptr) {
+ frame_tree_node_->ResetNavigationRequest(false, true);
+ return;
+ }
+ } else if (response_should_be_rendered_) {
+ render_frame_host_ =
+ frame_tree_node_->render_manager()->GetFrameHostForNavigation(*this);
NavigatorImpl::CheckWebUIRendererDoesNotDisplayNormalURL(
render_frame_host_, common_params_->url);
} else {
@@ -1698,12 +1719,14 @@
// account for clearing the expected process if it clears the speculative
// RenderFrameHost. See https://crbug.com/793127.
ResetExpectedProcess();
- render_frame_host = GetFrameHostForNavigation();
+ render_frame_host =
+ frame_tree_node_->render_manager()->GetFrameHostForNavigation(*this);
} else {
if (ShouldKeepErrorPageInCurrentProcess(status.error_code)) {
render_frame_host = frame_tree_node_->current_frame_host();
} else {
- render_frame_host = GetFrameHostForNavigation();
+ render_frame_host =
+ frame_tree_node_->render_manager()->GetFrameHostForNavigation(*this);
}
}
@@ -2164,13 +2187,33 @@
DCHECK(!common_params_->url.SchemeIs(url::kJavaScriptScheme));
DCHECK(!IsRendererDebugURL(common_params_->url));
- frame_tree_node_->TransferNavigationRequestOwnership(render_frame_host_);
-
if (is_served_from_back_forward_cache()) {
- frame_tree_node()->render_manager()->RestoreFromBackForwardCache(
- std::move(rfh_restored_from_back_forward_cache_));
+ NavigationControllerImpl* controller =
+ static_cast<NavigationControllerImpl*>(
+ frame_tree_node_->navigator()->GetController());
- // Commit the restored frame.
+ std::unique_ptr<RenderFrameHostImpl> restored_rfh =
+ controller->back_forward_cache().RestoreDocument(nav_entry_id_);
+
+ // restored_rfh will be nullptr if the document was evicted from the
+ // BackForwardCache during the execution of this navigation.
+ if (!restored_rfh) {
+ // TODO(https://crbug.com/995316): Handle this case by reissuing the
+ // navigation instead of cancelling it.
+ frame_tree_node_->ResetNavigationRequest(false, true);
+ return;
+ }
+
+ // Transfer ownership of this NavigationRequest to the restored
+ // RenderFrameHost.
+ frame_tree_node_->TransferNavigationRequestOwnership(render_frame_host());
+
+ // Move the restored RenderFrameHost into RenderFrameHostManager, in
+ // preparation for committing.
+ frame_tree_node_->render_manager()->RestoreFromBackForwardCache(
+ std::move(restored_rfh));
+
+ // Commit the restored RenderFrameHost.
// Note that this will delete the NavigationRequest.
render_frame_host()->DidCommitBackForwardCacheNavigation(
this, MakeDidCommitProvisionalLoadParamsForBFCache());
@@ -2183,6 +2226,8 @@
render_frame_host_ ==
frame_tree_node_->render_manager()->speculative_frame_host());
+ frame_tree_node_->TransferNavigationRequestOwnership(render_frame_host_);
+
if (IsPerNavigationMojoInterfaceEnabled() && request_navigation_client_ &&
request_navigation_client_.is_bound()) {
if (associated_site_instance_id_ ==
@@ -3241,15 +3286,6 @@
return params;
}
-RenderFrameHostImpl* NavigationRequest::GetFrameHostForNavigation() {
- if (is_served_from_back_forward_cache()) {
- DCHECK(rfh_restored_from_back_forward_cache_);
- return rfh_restored_from_back_forward_cache_.get();
- }
-
- return frame_tree_node_->render_manager()->GetFrameHostForNavigation(*this);
-}
-
bool NavigationRequest::IsExternalProtocol() {
return !GetContentClient()->browser()->IsHandledURL(common_params_->url);
}
diff --git a/content/browser/frame_host/navigation_request.h b/content/browser/frame_host/navigation_request.h
index 1eb78cf38..c4b7a79 100644
--- a/content/browser/frame_host/navigation_request.h
+++ b/content/browser/frame_host/navigation_request.h
@@ -532,8 +532,7 @@
std::unique_ptr<NavigationUIData> navigation_ui_data,
mojom::NavigationClientAssociatedPtrInfo navigation_client,
blink::mojom::NavigationInitiatorPtr navigation_initiator,
- std::unique_ptr<RenderFrameHostImpl>
- rfh_restored_from_back_forward_cache);
+ bool is_served_from_back_forward_cache);
// NavigationURLLoaderDelegate implementation.
void OnRequestRedirected(
@@ -615,8 +614,6 @@
bool url_upgraded_after_redirect,
bool is_response_check);
- RenderFrameHostImpl* GetFrameHostForNavigation();
-
// Builds the parameters used to commit a navigation to a page that was
// restored from the back-forward cache.
std::unique_ptr<FrameHostMsg_DidCommitProvisionalLoad_Params>
@@ -1070,13 +1067,6 @@
// TODO(clamy): Clean this up once the architecture of unit tests is better.
scoped_refptr<net::HttpResponseHeaders> response_headers_for_testing_;
- // The RenderFrameHost that was restored from the back-forward cache. This
- // will be null except for navigations that are restoring a page from the
- // back-forward cache. In this case, it represents the main RenderFrameHost to
- // be restored. It will be moved to become the current document in
- // CommitNavigation(), after which this variable becomes null.
- std::unique_ptr<RenderFrameHostImpl> rfh_restored_from_back_forward_cache_;
-
// Whether this navigation is being served from the back-forward cache.
bool is_served_from_back_forward_cache_;
