0

Allow content embedders to suppress failed navigations.

Android WebView and Instant NTP both attempt to suppress failed
navigations. Currently, this is done on the renderer side, but this
makes it harder for the browser to reason about the state of a pending
commit, since the renderer may choose to ignore it.

For Android WebView, expose a new helper on NavigationHandle to allow
WebView embedders to optionally suppress errors if desired.

For Chrome NTP, it turns out this is unnecessary. The renderer-side
suppression was added to prevent an error page from flashing before
falling back to the local NTP. However, the fallback is handled by
NewTabPageNavigationThrottle, which initiates the fallback navigation
in its `WillFailRequest()` handler. Initiating this fallback navigation
will cancel the failing navigation, so the renderer-side suppression
will never be used anyway.

Bug: 1117282
Change-Id: I125d46436fb6ebca57b64f308cade5249ea03658
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2401323
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#810970}
This commit is contained in:
Daniel Cheng
2020-09-26 09:55:22 +00:00
committed by Commit Bot
parent e5a133462c
commit f776864c39
20 changed files with 88 additions and 192 deletions

@ -116,14 +116,7 @@ void AwRenderViewHostExt::SetBackgroundColor(SkColor c) {
}
void AwRenderViewHostExt::SetWillSuppressErrorPage(bool suppress) {
// We need to store state on the browser-side, as state might need to be
// synchronized again later (see AwRenderViewHostExt::RenderFrameCreated)
if (will_suppress_error_page_ == suppress)
return;
will_suppress_error_page_ = suppress;
web_contents()->SendToAllFrames(new AwViewMsg_WillSuppressErrorPage(
MSG_ROUTING_NONE, will_suppress_error_page_));
}
void AwRenderViewHostExt::SetJsOnlineProperty(bool network_up) {
@ -159,14 +152,12 @@ void AwRenderViewHostExt::RenderFrameCreated(
frame_host->Send(new AwViewMsg_SetBackgroundColor(
frame_host->GetRoutingID(), background_color_));
}
}
// Synchronizing error page suppression state down to the renderer cannot be
// done when RenderViewHostChanged is fired (similar to how other settings do
// it) because for cross-origin navigations in multi-process mode, the
// navigation will already have started then. Also, newly created subframes
// need to inherit the state.
frame_host->Send(new AwViewMsg_WillSuppressErrorPage(
frame_host->GetRoutingID(), will_suppress_error_page_));
void AwRenderViewHostExt::DidStartNavigation(
content::NavigationHandle* navigation_handle) {
if (will_suppress_error_page_)
navigation_handle->SetSilentlyIgnoreErrors();
}
void AwRenderViewHostExt::DidFinishNavigation(

@ -84,6 +84,8 @@ class AwRenderViewHostExt : public content::WebContentsObserver {
void RenderViewHostChanged(content::RenderViewHost* old_host,
content::RenderViewHost* new_host) override;
void RenderFrameCreated(content::RenderFrameHost* frame_host) override;
void DidStartNavigation(
content::NavigationHandle* navigation_handle) override;
void DidFinishNavigation(
content::NavigationHandle* navigation_handle) override;
void OnPageScaleFactorChanged(float page_scale_factor) override;

@ -87,11 +87,6 @@ IPC_MESSAGE_ROUTED3(AwViewMsg_SmoothScroll,
int /* target_y */,
base::TimeDelta /* duration */)
// Sent to inform renderers whether the internal error page should be shown or
// not.
IPC_MESSAGE_ROUTED1(AwViewMsg_WillSuppressErrorPage,
bool /* will_suppress_error_page */)
//-----------------------------------------------------------------------------
// RenderView messages
// These are messages sent from the renderer to the browser process.

@ -181,20 +181,6 @@ bool AwContentRendererClient::HasErrorPage(int http_status_code) {
return http_status_code >= 400;
}
bool AwContentRendererClient::ShouldSuppressErrorPage(
content::RenderFrame* render_frame,
const GURL& url,
int error_code) {
DCHECK(render_frame != nullptr);
AwRenderFrameExt* render_frame_ext =
AwRenderFrameExt::FromRenderFrame(render_frame);
if (render_frame_ext == nullptr)
return false;
return render_frame_ext->GetWillSuppressErrorPage();
}
void AwContentRendererClient::PrepareErrorPage(
content::RenderFrame* render_frame,
const blink::WebURLError& error,

@ -38,9 +38,6 @@ class AwContentRendererClient : public content::ContentRendererClient,
void RenderFrameCreated(content::RenderFrame* render_frame) override;
void RenderViewCreated(content::RenderView* render_view) override;
bool HasErrorPage(int http_status_code) override;
bool ShouldSuppressErrorPage(content::RenderFrame* render_frame,
const GURL& url,
int error_code) override;
void PrepareErrorPage(content::RenderFrame* render_frame,
const blink::WebURLError& error,
const std::string& http_method,

@ -229,8 +229,6 @@ bool AwRenderFrameExt::OnMessageReceived(const IPC::Message& message) {
OnResetScrollAndScaleState)
IPC_MESSAGE_HANDLER(AwViewMsg_SetInitialPageScale, OnSetInitialPageScale)
IPC_MESSAGE_HANDLER(AwViewMsg_SetBackgroundColor, OnSetBackgroundColor)
IPC_MESSAGE_HANDLER(AwViewMsg_WillSuppressErrorPage,
OnSetWillSuppressErrorPage)
IPC_MESSAGE_HANDLER(AwViewMsg_SmoothScroll, OnSmoothScroll)
IPC_MESSAGE_UNHANDLED(handled = false)
IPC_END_MESSAGE_MAP()
@ -349,14 +347,6 @@ void AwRenderFrameExt::OnSmoothScroll(int target_x,
webview->SmoothScroll(target_x, target_y, duration);
}
void AwRenderFrameExt::OnSetWillSuppressErrorPage(bool suppress) {
this->will_suppress_error_page_ = suppress;
}
bool AwRenderFrameExt::GetWillSuppressErrorPage() {
return this->will_suppress_error_page_;
}
blink::WebView* AwRenderFrameExt::GetWebView() {
if (!render_frame() || !render_frame()->GetRenderView() ||
!render_frame()->GetRenderView()->GetWebView())

@ -31,8 +31,6 @@ class AwRenderFrameExt : public content::RenderFrameObserver {
static AwRenderFrameExt* FromRenderFrame(content::RenderFrame* render_frame);
bool GetWillSuppressErrorPage();
private:
~AwRenderFrameExt() override;
@ -60,8 +58,6 @@ class AwRenderFrameExt : public content::RenderFrameObserver {
void OnSmoothScroll(int target_x, int target_y, base::TimeDelta duration);
void OnSetWillSuppressErrorPage(bool suppress);
blink::WebView* GetWebView();
blink::WebFrameWidget* GetWebFrameWidget();
@ -69,9 +65,6 @@ class AwRenderFrameExt : public content::RenderFrameObserver {
blink::AssociatedInterfaceRegistry registry_;
// Some WebView users might want to show their own error pages / logic
bool will_suppress_error_page_ = false;
DISALLOW_COPY_AND_ASSIGN(AwRenderFrameExt);
};

@ -14,6 +14,7 @@
#include "chrome/test/base/ui_test_utils.h"
#include "components/search_engines/template_url_service.h"
#include "content/public/browser/navigation_entry.h"
#include "content/public/browser/navigation_handle.h"
#include "content/public/browser/web_contents.h"
#include "content/public/test/browser_test.h"
#include "net/test/embedded_test_server/embedded_test_server.h"
@ -45,9 +46,11 @@ class NewTabPageNavigationThrottleTest : public InProcessBrowserTest {
// navigated to.
GURL NavigateToNewTabPage() {
ui_test_utils::NavigateToURL(browser(), GURL(chrome::kChromeUINewTabURL));
content::WebContents* contents =
browser()->tab_strip_model()->GetWebContentsAt(0);
return contents->GetController().GetLastCommittedEntry()->GetURL();
return web_contents()->GetController().GetLastCommittedEntry()->GetURL();
}
content::WebContents* web_contents() {
return browser()->tab_strip_model()->GetWebContentsAt(0);
}
net::EmbeddedTestServer* https_test_server() { return &https_test_server_; }
@ -67,10 +70,43 @@ IN_PROC_BROWSER_TEST_F(NewTabPageNavigationThrottleTest, NoThrottle) {
IN_PROC_BROWSER_TEST_F(NewTabPageNavigationThrottleTest,
FailedRequestThrottle) {
ASSERT_TRUE(https_test_server()->Start());
SetNewTabPage(https_test_server()->GetURL("/instant_extended.html").spec());
const GURL instant_ntp_url =
https_test_server()->GetURL("/instant_extended.html");
SetNewTabPage(instant_ntp_url.spec());
ASSERT_TRUE(https_test_server()->ShutdownAndWaitUntilComplete());
// Helper to assert that the failed request to `instant_ntp_url` never commits
// an error page. This doesn't simply use `TestNavigationManager` since that
// automatically pauses navigations, which is not needed or useful here.
class FailedRequestObserver : public content::WebContentsObserver {
public:
explicit FailedRequestObserver(content::WebContents* contents,
const GURL& instant_ntp_url)
: WebContentsObserver(contents), instant_ntp_url_(instant_ntp_url) {}
// WebContentsObserver overrides:
void DidFinishNavigation(content::NavigationHandle* handle) override {
if (handle->GetURL() != instant_ntp_url_)
return;
did_finish_ = true;
did_commit_ = handle->HasCommitted();
}
bool did_finish() const { return did_finish_; }
bool did_commit() const { return did_commit_; }
private:
const GURL instant_ntp_url_;
bool did_finish_ = false;
bool did_commit_ = false;
};
FailedRequestObserver observer(web_contents(), instant_ntp_url);
// Failed navigation makes a redirect to the local NTP.
EXPECT_EQ(chrome::kChromeSearchLocalNtpUrl, NavigateToNewTabPage());
EXPECT_TRUE(observer.did_finish());
EXPECT_FALSE(observer.did_commit());
}
IN_PROC_BROWSER_TEST_F(NewTabPageNavigationThrottleTest, LocalNewTabPage) {

@ -1204,18 +1204,6 @@ bool ChromeContentRendererClient::HasErrorPage(int http_status_code) {
error_page::Error::kHttpErrorDomain, http_status_code);
}
bool ChromeContentRendererClient::ShouldSuppressErrorPage(
content::RenderFrame* render_frame,
const GURL& url,
int error_code) {
// Do not flash an error page if the Instant new tab page fails to load.
bool is_instant_ntp = false;
#if !defined(OS_ANDROID)
is_instant_ntp = SearchBouncer::GetInstance()->IsNewTabPage(url);
#endif
return is_instant_ntp;
}
bool ChromeContentRendererClient::ShouldTrackUseCounter(const GURL& url) {
bool is_instant_ntp = false;
#if !defined(OS_ANDROID)

@ -104,9 +104,6 @@ class ChromeContentRendererClient
content::RenderFrame* render_frame,
const base::FilePath& plugin_path) override;
bool HasErrorPage(int http_status_code) override;
bool ShouldSuppressErrorPage(content::RenderFrame* render_frame,
const GURL& url,
int error_code) override;
bool ShouldTrackUseCounter(const GURL& url) override;
void PrepareErrorPage(content::RenderFrame* render_frame,
const blink::WebURLError& error,

@ -232,16 +232,6 @@ TEST_F(ChromeContentRendererClientTest, NaClRestriction) {
// SearchBouncer doesn't exist on Android.
#if !defined(OS_ANDROID)
TEST_F(ChromeContentRendererClientTest, ShouldSuppressErrorPage) {
ChromeContentRendererClient client;
SearchBouncer::GetInstance()->SetNewTabPageURL(GURL("http://example.com/n"));
EXPECT_FALSE(client.ShouldSuppressErrorPage(
nullptr, GURL("http://example.com"), net::OK));
EXPECT_TRUE(client.ShouldSuppressErrorPage(
nullptr, GURL("http://example.com/n"), net::OK));
SearchBouncer::GetInstance()->SetNewTabPageURL(GURL::EmptyGURL());
}
TEST_F(ChromeContentRendererClientTest, ShouldTrackUseCounter) {
ChromeContentRendererClient client;
SearchBouncer::GetInstance()->SetNewTabPageURL(GURL("http://example.com/n"));

@ -4810,6 +4810,10 @@ bool NavigationRequest::GetIsOverridingUserAgent() {
return entry_overrides_ua_;
}
void NavigationRequest::SetSilentlyIgnoreErrors() {
silently_ignore_errors_ = true;
}
void NavigationRequest::ForceCSPForResponse(const std::string& csp) {
commit_params_->forced_content_security_policies.push_back(csp);
}
@ -5091,6 +5095,8 @@ bool NavigationRequest::MaybeCancelFailedNavigation() {
// If the request was canceled by the user, do not show an error page.
if (net::ERR_ABORTED == net_error_ ||
// Some embedders suppress error pages to allow custom error handling.
silently_ignore_errors_ ||
// <webview> guests suppress net::ERR_BLOCKED_BY_CLIENT.
(net::ERR_BLOCKED_BY_CLIENT == net_error_ &&
silently_ignore_blocked_by_client_)) {
frame_tree_node_->ResetNavigationRequest(false);

@ -320,6 +320,7 @@ class CONTENT_EXPORT NavigationRequest
bool IsServedFromBackForwardCache() override;
void SetIsOverridingUserAgent(bool override_ua) override;
bool GetIsOverridingUserAgent() override;
void SetSilentlyIgnoreErrors() override;
// Called on the UI thread by the Navigator to start the navigation.
// The NavigationRequest can be deleted while BeginNavigation() is called.
@ -1460,6 +1461,13 @@ class CONTENT_EXPORT NavigationRequest
// navigation.
bool did_same_site_proactive_browsing_instance_swap_ = false;
// Controls whether or not an error page is displayed on error. If set to
// true, an error will be treated as if the user simply cancelled the
// navigation.
bool silently_ignore_errors_ = false;
// Similar but only suppresses the error page when the error code is
// net::ERR_BLOCKED_BY_CLIENT.
bool silently_ignore_blocked_by_client_ = false;
// Observers listening to cookie access notifications for the network requests

@ -409,6 +409,10 @@ class CONTENT_EXPORT NavigationHandle {
virtual void SetIsOverridingUserAgent(bool override_ua) = 0;
virtual bool GetIsOverridingUserAgent() = 0;
// Suppress any errors during a navigation and behave as if the user cancelled
// the navigation: no error page will commit.
virtual void SetSilentlyIgnoreErrors() = 0;
// Testing methods ----------------------------------------------------------
//
// The following methods should be used exclusively for writing unit tests.

@ -53,12 +53,6 @@ bool ContentRendererClient::HasErrorPage(int http_status_code) {
return false;
}
bool ContentRendererClient::ShouldSuppressErrorPage(RenderFrame* render_frame,
const GURL& url,
int error_code) {
return false;
}
bool ContentRendererClient::ShouldTrackUseCounter(const GURL& url) {
return true;
}

@ -131,12 +131,6 @@ class CONTENT_EXPORT ContentRendererClient {
// status code.
virtual bool HasErrorPage(int http_status_code);
// Returns true if the embedder prefers not to show an error page for a failed
// navigation to |url| with |error_code| in |render_frame|.
virtual bool ShouldSuppressErrorPage(RenderFrame* render_frame,
const GURL& url,
int error_code);
// Returns false for new tab page activities, which should be filtered out in
// UseCounter; returns true otherwise.
virtual bool ShouldTrackUseCounter(const GURL& url);

@ -124,18 +124,22 @@ class MockNavigationHandle : public NavigationHandle {
const base::Optional<url::Origin>& GetInitiatorOrigin() override {
return initiator_origin_;
}
MOCK_METHOD1(RegisterThrottleForTesting,
void(std::unique_ptr<NavigationThrottle>));
MOCK_METHOD0(IsDeferredForTesting, bool());
MOCK_METHOD1(RegisterSubresourceOverride,
void(blink::mojom::TransferrableURLLoaderPtr));
MOCK_METHOD0(FromDownloadCrossOriginRedirect, bool());
MOCK_METHOD0(IsSameProcess, bool());
MOCK_METHOD0(GetNavigationEntryOffset, int());
MOCK_METHOD1(ForceEnableOriginTrials,
void(const std::vector<std::string>& trials));
MOCK_METHOD1(SetIsOverridingUserAgent, void(bool));
MOCK_METHOD0(GetIsOverridingUserAgent, bool());
MOCK_METHOD(void,
RegisterThrottleForTesting,
(std::unique_ptr<NavigationThrottle>));
MOCK_METHOD(bool, IsDeferredForTesting, ());
MOCK_METHOD(void,
RegisterSubresourceOverride,
(blink::mojom::TransferrableURLLoaderPtr));
MOCK_METHOD(bool, FromDownloadCrossOriginRedirect, ());
MOCK_METHOD(bool, IsSameProcess, ());
MOCK_METHOD(int, GetNavigationEntryOffset, ());
MOCK_METHOD(void,
ForceEnableOriginTrials,
(const std::vector<std::string>& trials));
MOCK_METHOD(void, SetIsOverridingUserAgent, (bool));
MOCK_METHOD(bool, GetIsOverridingUserAgent, ());
MOCK_METHOD(void, SetSilentlyIgnoreErrors, ());
void set_url(const GURL& url) { url_ = url; }
void set_previous_url(const GURL& previous_url) {

@ -3474,25 +3474,10 @@ void RenderFrameImpl::CommitFailedNavigation(
navigation_params->http_method = WebString::FromASCII(common_params->method);
navigation_params->error_code = error_code;
if (!ShouldDisplayErrorPageForFailedLoad(error_code, common_params->url)) {
// The browser expects this frame to be loading an error page. Inform it
// that the load stopped.
AbortCommitNavigation();
GetFrameHost()->DidStopLoading();
browser_side_navigation_pending_ = false;
browser_side_navigation_pending_url_ = GURL();
// Disarm AssertNavigationCommits and pretend that the commit actually
// happened. Signalling that the load stopped /should/ signal the browser to
// tear down the speculative RFH associated with |this|...
// TODO(dcheng): This is potentially buggy. This means that the browser
// asked the renderer to commit a failed navigation, but it declined. In the
// future, when speculative RFH management is refactored, this will likely
// have to self-discard if |this| is provisional.
navigation_commit_state_ = NavigationCommitState::kDidCommit;
return;
}
// This is already checked in `NavigationRequest::OnRequestFailedInternal` and
// `NavigationRequest::OnFailureChecksCompleted` on the browser side, so the
// renderer should never see this.
CHECK_NE(net::ERR_ABORTED, error_code);
// On load failure, a frame can ask its owner to render fallback content.
// When that happens, don't load an error page.
@ -6357,24 +6342,6 @@ void RenderFrameImpl::SendUpdateState() {
SingleHistoryItemToPageState(current_history_item_));
}
bool RenderFrameImpl::ShouldDisplayErrorPageForFailedLoad(
int error_code,
const GURL& unreachable_url) {
// This is already checked in `NavigationRequest::OnRequestFailedInternal` and
// `NavigationRequest::OnFailureChecksCompleted` on the browser side, so the
// renderer should never see this.
if (error_code == net::ERR_ABORTED)
CHECK(false);
// Allow the embedder to suppress an error page.
if (GetContentClient()->renderer()->ShouldSuppressErrorPage(
this, unreachable_url, error_code)) {
return false;
}
return true;
}
GURL RenderFrameImpl::GetLoadingUrl() const {
WebDocumentLoader* document_loader = frame_->GetDocumentLoader();

@ -1064,9 +1064,6 @@ class CONTENT_EXPORT RenderFrameImpl
std::string* data,
GURL* base_url);
bool ShouldDisplayErrorPageForFailedLoad(int error_code,
const GURL& unreachable_url);
// |transition_type| corresponds to the document which triggered this request.
void WillSendRequestInternal(blink::WebURLRequest& request,
bool for_main_frame,

@ -2580,12 +2580,6 @@ class RendererErrorPageTest : public RenderViewImplTest {
private:
class TestContentRendererClient : public ContentRendererClient {
public:
bool ShouldSuppressErrorPage(RenderFrame* render_frame,
const GURL& url,
int error_code) override {
return url == "http://example.com/suppress";
}
void PrepareErrorPage(content::RenderFrame* render_frame,
const blink::WebURLError& error,
const std::string& http_method,
@ -2607,40 +2601,10 @@ class RendererErrorPageTest : public RenderViewImplTest {
};
};
#if defined(OS_ANDROID)
// Crashing on Android: http://crbug.com/311341
#define MAYBE_Suppresses DISABLED_Suppresses
#else
#define MAYBE_Suppresses Suppresses
#endif
TEST_F(RendererErrorPageTest, MAYBE_Suppresses) {
TEST_F(RendererErrorPageTest, RegularError) {
auto common_params = CreateCommonNavigationParams();
common_params->navigation_type = mojom::NavigationType::DIFFERENT_DOCUMENT;
common_params->url = GURL("http://example.com/suppress");
TestRenderFrame* main_frame = static_cast<TestRenderFrame*>(frame());
main_frame->NavigateWithError(
std::move(common_params), CreateCommitNavigationParams(),
net::ERR_FILE_NOT_FOUND, net::ResolveErrorInfo(net::OK),
"A suffusion of yellow.");
const int kMaxOutputCharacters = 22;
EXPECT_EQ("", WebFrameContentDumper::DumpWebViewAsText(view()->GetWebView(),
kMaxOutputCharacters)
.Ascii());
}
#if defined(OS_ANDROID)
// Crashing on Android: http://crbug.com/311341
#define MAYBE_DoesNotSuppress DISABLED_DoesNotSuppress
#else
#define MAYBE_DoesNotSuppress DoesNotSuppress
#endif
TEST_F(RendererErrorPageTest, MAYBE_DoesNotSuppress) {
auto common_params = CreateCommonNavigationParams();
common_params->navigation_type = mojom::NavigationType::DIFFERENT_DOCUMENT;
common_params->url = GURL("http://example.com/dont-suppress");
common_params->url = GURL("http://example.com/error-page");
TestRenderFrame* main_frame = static_cast<TestRenderFrame*>(frame());
main_frame->NavigateWithError(
std::move(common_params), CreateCommitNavigationParams(),
@ -2656,14 +2620,7 @@ TEST_F(RendererErrorPageTest, MAYBE_DoesNotSuppress) {
.Ascii());
}
#if defined(OS_ANDROID)
// Crashing on Android: http://crbug.com/311341
#define MAYBE_HttpStatusCodeErrorWithEmptyBody \
DISABLED_HttpStatusCodeErrorWithEmptyBody
#else
#define MAYBE_HttpStatusCodeErrorWithEmptyBody HttpStatusCodeErrorWithEmptyBody
#endif
TEST_F(RendererErrorPageTest, MAYBE_HttpStatusCodeErrorWithEmptyBody) {
TEST_F(RendererErrorPageTest, HttpStatusCodeErrorWithEmptyBody) {
// Start a load that will reach provisional state synchronously,
// but won't complete synchronously.
auto common_params = CreateCommonNavigationParams();