Skip to content

Commit

Permalink
Fix tests that 404s or navigates to an empty page with non-OK status
Browse files Browse the repository at this point in the history
These tests do not intend to have failed navigations, but they actually
failed silently, mostly due to 404s because of bad URLs. Soon (after
crrev.com/c/2487024 lands) these tests will actually fail and result in
error pages.

See navigation-dev thread for more context:
https://groups.google.com/a/chromium.org/g/navigation-dev/c/WbNkf2alpPU/m/2tQQ-cXWBgAJ

Bug:1133115

Change-Id: Ib75a04aa90635b9eb778ce2da56fcee83965b053
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2483748
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: David Schinazi <dschinazi@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#819688}
  • Loading branch information
rakina authored and Commit Bot committed Oct 22, 2020
1 parent fc9e551 commit 015f45f
Show file tree
Hide file tree
Showing 11 changed files with 39 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ public void testRendererHistograms() throws Throwable {

// Load a page to ensure the renderer process is created.
mRule.loadUrlSync(mAwContents, mContentsClient.getOnPageFinishedHelper(),
embeddedTestServer.getURL("/simple_page.html"));
embeddedTestServer.getURL("/android_webview/test/data/hello_world.html"));
helper.waitForCallback(finalMetricsCollectedCount, 1);

// At this point we know one of two things must be true:
Expand All @@ -353,7 +353,7 @@ public void testRendererHistograms() throws Throwable {
// have been copied into the browser process.

mRule.loadUrlSync(mAwContents, mContentsClient.getOnPageFinishedHelper(),
embeddedTestServer.getURL("/simple_page.html"));
embeddedTestServer.getURL("/android_webview/test/data/hello_world.html"));
helper.waitForCallback(finalMetricsCollectedCount, 2);

Assert.assertEquals(1,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ public void testRequestedWithHeaderShouldInterceptRequest() throws Throwable {
mTestServer = EmbeddedTestServer.createAndStartServer(
InstrumentationRegistry.getInstrumentation().getContext());
try {
final String url = mTestServer.getURL("/any-http-url-will-suffice.html");
final String url = mTestServer.getURL("/android_webview/test/data/hello_world.html");
mActivityTestRule.loadUrlSync(
mAwContents, mContentsClient.getOnPageFinishedHelper(), url);
AwWebResourceRequest request =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ public void testOnPageFinishedCalledOnDomModificationAfterNavigation() throws Th
+ "}</script>");

mActivityTestRule.triggerPopup(mParentContents, mParentContentsClient, mWebServer,
parentPageHtml, null /* 204 response */, popupPath, "tryOpenWindow()");
parentPageHtml, "<html></html>", popupPath, "tryOpenWindow()");
PopupInfo popupInfo = mActivityTestRule.createPopupContents(mParentContents);
TestCallbackHelperContainer.OnPageFinishedHelper onPageFinishedHelper =
popupInfo.popupContentsClient.getOnPageFinishedHelper();
Expand Down
6 changes: 3 additions & 3 deletions chrome/browser/ssl/security_state_tab_helper_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2262,8 +2262,8 @@ IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperTest, FormSecurityLevelHistogram) {
ASSERT_TRUE(broken_https_server.Start());

// Make the form target the expired certificate server.
net::HostPortPair host_port_pair =
net::HostPortPair::FromURL(broken_https_server.GetURL("/google.html"));
net::HostPortPair host_port_pair = net::HostPortPair::FromURL(
broken_https_server.GetURL("/ssl/google.html"));
std::string replacement_path = GetFilePathWithHostAndPortReplacement(
"/ssl/page_with_form_targeting_insecure_url.html", host_port_pair);
ui_test_utils::NavigateToURL(browser(),
Expand Down Expand Up @@ -2301,7 +2301,7 @@ IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperTest, SafetyTipFormHistogram) {
// Use a different host for targeting the form so that a Safety Tip doesn't
// trigger on the form submission navigation.
net::HostPortPair host_port_pair = net::HostPortPair::FromURL(
form_server.GetURL("example.test", "/google.html"));
form_server.GetURL("example.test", "/ssl/google.html"));
std::string replacement_path = GetFilePathWithHostAndPortReplacement(
"/ssl/page_with_form_targeting_http_url.html", host_port_pair);
ui_test_utils::NavigateToURL(browser(), server.GetURL(replacement_path));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,7 @@ IN_PROC_BROWSER_TEST_F(SupervisedUserIframeFilterTest,
BlockHost(kExampleHost);

GURL blocked_url = embedded_test_server()->GetURL(
kExampleHost, "/supervised_user/with_frames.html");
kExampleHost, "/supervised_user/with_iframes.html");
ui_test_utils::NavigateToURL(browser(), blocked_url);
EXPECT_TRUE(IsInterstitialBeingShownInMainFrame(browser()));

Expand All @@ -623,7 +623,7 @@ IN_PROC_BROWSER_TEST_F(SupervisedUserIframeFilterTest,

// Navigate to another allowed url.
GURL allowed_url = embedded_test_server()->GetURL(
kExampleHost2, "/supervised_user/with_frames.html");
kExampleHost2, "/supervised_user/with_iframes.html");
ui_test_utils::NavigateToURL(browser(), allowed_url);
EXPECT_FALSE(IsInterstitialBeingShownInMainFrame(browser()));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,8 @@ IN_PROC_BROWSER_TEST_F(PopupBlockerBrowserTest, NoPopupsLaunchWhenTabIsClosed) {
embedded_test_server()->GetURL("/popup_blocker/popup-on-unload.html"));
ui_test_utils::NavigateToURL(browser(), url);

GURL url2(embedded_test_server()->GetURL("/popup_blocker/"));
GURL url2(
embedded_test_server()->GetURL("/popup_blocker/popup-success.html"));
ui_test_utils::NavigateToURL(browser(), url2);

// Expect no popup.
Expand Down Expand Up @@ -520,8 +521,9 @@ IN_PROC_BROWSER_TEST_F(PopupBlockerSpecialPolicyBrowserTest,
DisableProactiveBrowsingInstanceSwapFor(
browser()->tab_strip_model()->GetActiveWebContents()->GetMainFrame());

NavigateAndCheckPopupShown(embedded_test_server()->GetURL("/popup_blocker/"),
kExpectPopup);
NavigateAndCheckPopupShown(
embedded_test_server()->GetURL("/popup_blocker/popup-success.html"),
kExpectPopup);
}

// Verify that when you unblock popup, the popup shows in history and omnibox.
Expand Down
14 changes: 12 additions & 2 deletions chrome/browser/ui/signin_reauth_view_controller_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,14 @@ std::unique_ptr<net::test_server::BasicHttpResponse> CreateEmptyResponse(
return http_response;
}

std::unique_ptr<net::test_server::BasicHttpResponse> CreateNonEmptyResponse(
net::HttpStatusCode code) {
auto http_response = std::make_unique<net::test_server::BasicHttpResponse>();
http_response->set_code(code);
http_response->set_content("<html>");
return http_response;
}

std::unique_ptr<net::test_server::HttpResponse> HandleReauthURL(
const GURL& base_url,
const net::test_server::HttpRequest& request) {
Expand All @@ -101,8 +109,10 @@ std::unique_ptr<net::test_server::HttpResponse> HandleReauthURL(
}

if (parameter == "unexpected") {
// Returns a response that isn't expected by Chrome.
return CreateEmptyResponse(net::HTTP_NOT_IMPLEMENTED);
// Returns a response that isn't expected by Chrome. Note that we shouldn't
// return an empty response here because that will result in an error page
// being committed for the navigation.
return CreateNonEmptyResponse(net::HTTP_NOT_IMPLEMENTED);
}

NOTREACHED();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ class ClearBrowsingDataHandlerBrowserTest
};

IN_PROC_BROWSER_TEST_F(ClearBrowsingDataHandlerBrowserTest, GetInstalledApps) {
GURL url(https_server()->GetURL("/"));
GURL url(https_server()->GetURL("/title1.html"));
InstallAndLaunchApp(url);
base::ListValue args;
args.AppendString(kWebUiFunctionName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,18 @@ std::unique_ptr<net::test_server::HttpResponse> HandleServerRedirect(
}

// Run |handler| on requests that exactly match the |relative_url|.
std::unique_ptr<net::test_server::HttpResponse> HandleMatchingRequest(
std::unique_ptr<net::test_server::HttpResponse>
HandleMatchingRequestOrReturnEmptyPage(
const std::string& relative_url,
const net::EmbeddedTestServer::HandleRequestCallback& handler,
const net::test_server::HttpRequest& request) {
GURL match = request.GetURL().Resolve(relative_url);
if (request.GetURL() == match)
return handler.Run(request);
return nullptr;

auto http_response = std::make_unique<net::test_server::BasicHttpResponse>();
http_response->set_code(net::HTTP_OK);
return http_response;
}

class WebAppUrlLoaderTest : public InProcessBrowserTest {
Expand Down Expand Up @@ -100,9 +104,9 @@ class WebAppUrlLoaderTest : public InProcessBrowserTest {
// Set up a server redirect from |src_relative| (a relative URL) to |dest|.
// Must be called before the server is started.
void SetupRedirect(const std::string& src_relative, const std::string& dest) {
embedded_test_server()->RegisterRequestHandler(
base::BindRepeating(&HandleMatchingRequest, src_relative,
base::BindRepeating(&HandleServerRedirect, dest)));
embedded_test_server()->RegisterRequestHandler(base::BindRepeating(
&HandleMatchingRequestOrReturnEmptyPage, src_relative,
base::BindRepeating(&HandleServerRedirect, dest)));
}

private:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<head><title>Page with form that targets an http URL</title>
</head>
<body>
<form action='http://REPLACE_WITH_HOST_AND_PORT/google.html' id='theform' method='post'>
<form action='http://REPLACE_WITH_HOST_AND_PORT/ssl/google.html' id='theform' method='post'>
<input type='text' id='textfield' name='fieldname' value='fieldvalue'>
<input type='submit' id='submit' value='submit'>
</form>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -416,13 +416,13 @@ private void outputResponse(HTTPRequest request, WebServerPrintStream stream)
copyHeadersToResponse = false;
}
} else if (response.mIsNoContent) {
stream.println("HTTP/1.0 200 OK");
stream.println("HTTP/1.0 204 No Content");
copyHeadersToResponse = false;
} else if (response.mIsRedirect) {
stream.println("HTTP/1.0 302 Found");
textBody.append(String.format(bodyTemplate, "Found", "Found"));
} else if (response.mIsEmptyResponse) {
stream.println("HTTP/1.0 403 Forbidden");
stream.println("HTTP/1.0 200 OK");
copyHeadersToResponse = false;
} else {
if (response.mResponseAction != null) response.mResponseAction.run();
Expand Down

0 comments on commit 015f45f

Please sign in to comment.