Commit 6ee81d2e authored by Clark DuVall's avatar Clark DuVall Committed by Commit Bot

Revert "Reland "Squash top level error code returned from host resolver.""

This reverts commit c4f3b72d.

Reason for revert: Breaking network_service_out_of_process_chrome_public_test_apk, see http://crbug.com/1041822

Original change's description:
> Reland "Squash top level error code returned from host resolver."
> 
> This is a reland of 834f2092
> 
> Now not squashing ERR_NOT_IMPLEMENTED to avoid disrupting existing
> InProcessBrowserTests.
> 
> Original change's description:
> > Squash top level error code returned from host resolver.
> >
> > If the host resolution process fails, the top level error code should
> > now be only ERR_NAME_NOT_RESOLVED. A more specific error code (e.g.
> > ERR_CONNECTION_REFUSED if a secure mode DoH server was offline) may be
> > found in ResolveErrorInfo.
> >
> > Bug: 1016325
> > Change-Id: I9f5f857fffd71be6485d05280239cd0b649d7a8f
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1974678
> > Commit-Queue: Katharine Daly <dalyk@google.com>
> > Reviewed-by: Matt Menke <mmenke@chromium.org>
> > Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> > Reviewed-by: Eric Orth <ericorth@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#728725}
> 
> Bug: 1016325
> Change-Id: If2d029ea94f1a85f95d641cf02e4283973316a98
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1992191
> Reviewed-by: Eric Orth <ericorth@chromium.org>
> Reviewed-by: Matt Menke <mmenke@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Commit-Queue: Katharine Daly <dalyk@google.com>
> Cr-Commit-Position: refs/heads/master@{#730951}

TBR=jam@chromium.org,mmenke@chromium.org,ericorth@chromium.org,dalyk@google.com

Change-Id: I1d4d1c9b76c73a3762cfe739e5c99993579d1546
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1016325, 1041822
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2001439Reviewed-by: default avatarClark DuVall <cduvall@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#731695}
parent d259d8fc
...@@ -21,7 +21,6 @@ ...@@ -21,7 +21,6 @@
#include "content/public/common/content_switches.h" #include "content/public/common/content_switches.h"
#include "net/base/filename_util.h" #include "net/base/filename_util.h"
#include "net/base/net_errors.h" #include "net/base/net_errors.h"
#include "net/dns/public/resolve_error_info.h"
#include "testing/gtest/include/gtest/gtest.h" #include "testing/gtest/include/gtest/gtest.h"
namespace { namespace {
...@@ -45,8 +44,7 @@ class LoadFailObserver : public content::WebContentsObserver { ...@@ -45,8 +44,7 @@ class LoadFailObserver : public content::WebContentsObserver {
explicit LoadFailObserver(content::WebContents* contents) explicit LoadFailObserver(content::WebContents* contents)
: content::WebContentsObserver(contents), : content::WebContentsObserver(contents),
failed_load_(false), failed_load_(false),
error_code_(net::OK), error_code_(net::OK) { }
resolve_error_info_(net::ResolveErrorInfo(net::OK)) {}
void DidFinishNavigation( void DidFinishNavigation(
content::NavigationHandle* navigation_handle) override { content::NavigationHandle* navigation_handle) override {
...@@ -55,21 +53,16 @@ class LoadFailObserver : public content::WebContentsObserver { ...@@ -55,21 +53,16 @@ class LoadFailObserver : public content::WebContentsObserver {
failed_load_ = true; failed_load_ = true;
error_code_ = navigation_handle->GetNetErrorCode(); error_code_ = navigation_handle->GetNetErrorCode();
resolve_error_info_ = navigation_handle->GetResolveErrorInfo();
validated_url_ = navigation_handle->GetURL(); validated_url_ = navigation_handle->GetURL();
} }
bool failed_load() const { return failed_load_; } bool failed_load() const { return failed_load_; }
net::Error error_code() const { return error_code_; } net::Error error_code() const { return error_code_; }
net::ResolveErrorInfo resolve_error_info() const {
return resolve_error_info_;
}
const GURL& validated_url() const { return validated_url_; } const GURL& validated_url() const { return validated_url_; }
private: private:
bool failed_load_; bool failed_load_;
net::Error error_code_; net::Error error_code_;
net::ResolveErrorInfo resolve_error_info_;
GURL validated_url_; GURL validated_url_;
DISALLOW_COPY_AND_ASSIGN(LoadFailObserver); DISALLOW_COPY_AND_ASSIGN(LoadFailObserver);
...@@ -92,7 +85,6 @@ IN_PROC_BROWSER_TEST_F(InProcessBrowserTest, ExternalConnectionFail) { ...@@ -92,7 +85,6 @@ IN_PROC_BROWSER_TEST_F(InProcessBrowserTest, ExternalConnectionFail) {
ui_test_utils::NavigateToURL(browser(), url); ui_test_utils::NavigateToURL(browser(), url);
EXPECT_TRUE(observer.failed_load()); EXPECT_TRUE(observer.failed_load());
EXPECT_EQ(net::ERR_NOT_IMPLEMENTED, observer.error_code()); EXPECT_EQ(net::ERR_NOT_IMPLEMENTED, observer.error_code());
EXPECT_EQ(net::ERR_NOT_IMPLEMENTED, observer.resolve_error_info().error);
EXPECT_EQ(url, observer.validated_url()); EXPECT_EQ(url, observer.validated_url());
} }
} }
......
...@@ -1313,7 +1313,7 @@ void CheckTestHostNameUsedWithCorrectNetworkIsolationKey(Browser* browser) { ...@@ -1313,7 +1313,7 @@ void CheckTestHostNameUsedWithCorrectNetworkIsolationKey(Browser* browser) {
network::DnsLookupResult result2 = network::DnsLookupResult result2 =
network::BlockingDnsLookup(network_context, kHostPortPair, network::BlockingDnsLookup(network_context, kHostPortPair,
std::move(params), net::NetworkIsolationKey()); std::move(params), net::NetworkIsolationKey());
EXPECT_EQ(net::ERR_NAME_NOT_RESOLVED, result2.error); EXPECT_EQ(net::ERR_DNS_CACHE_MISS, result2.error);
} }
// HostResolver and HostResolverPrivate tests. The PPAPI code used by these // HostResolver and HostResolverPrivate tests. The PPAPI code used by these
......
...@@ -216,10 +216,8 @@ int StaleHostResolver::RequestImpl::Start( ...@@ -216,10 +216,8 @@ int StaleHostResolver::RequestImpl::Start(
cache_parameters.source = net::HostResolverSource::LOCAL_ONLY; cache_parameters.source = net::HostResolverSource::LOCAL_ONLY;
cache_request_ = resolver_->inner_resolver_->CreateRequest( cache_request_ = resolver_->inner_resolver_->CreateRequest(
host_, network_isolation_key_, net_log_, cache_parameters); host_, network_isolation_key_, net_log_, cache_parameters);
int error = cache_error_ =
cache_request_->Start(base::BindOnce([](int error) { NOTREACHED(); })); cache_request_->Start(base::BindOnce([](int error) { NOTREACHED(); }));
DCHECK_NE(net::ERR_IO_PENDING, error);
cache_error_ = cache_request_->GetResolveErrorInfo().error;
DCHECK_NE(net::ERR_IO_PENDING, cache_error_); DCHECK_NE(net::ERR_IO_PENDING, cache_error_);
// If it's a fresh cache hit (or literal), return it synchronously. // If it's a fresh cache hit (or literal), return it synchronously.
if (cache_error_ != net::ERR_DNS_CACHE_MISS && if (cache_error_ != net::ERR_DNS_CACHE_MISS &&
......
...@@ -1643,7 +1643,7 @@ IN_PROC_BROWSER_TEST_F(NavigationRequestHostResolutionFailureTest, ...@@ -1643,7 +1643,7 @@ IN_PROC_BROWSER_TEST_F(NavigationRequestHostResolutionFailureTest,
EXPECT_TRUE(observer.has_committed()); EXPECT_TRUE(observer.has_committed());
EXPECT_TRUE(observer.is_error()); EXPECT_TRUE(observer.is_error());
EXPECT_EQ(net::ERR_NAME_NOT_RESOLVED, observer.net_error_code()); EXPECT_EQ(net::ERR_DNS_TIMED_OUT, observer.net_error_code());
EXPECT_EQ(net::ERR_DNS_TIMED_OUT, observer.resolve_error_info().error); EXPECT_EQ(net::ERR_DNS_TIMED_OUT, observer.resolve_error_info().error);
} }
......
...@@ -45,8 +45,6 @@ class LocalHostResolverProc : public net::HostResolverProc { ...@@ -45,8 +45,6 @@ class LocalHostResolverProc : public net::HostResolverProc {
// queries, rather than perform them. // queries, rather than perform them.
// If you really need to make an external DNS query, use // If you really need to make an external DNS query, use
// net::RuleBasedHostResolverProc and its AllowDirectLookup method. // net::RuleBasedHostResolverProc and its AllowDirectLookup method.
// TODO(crbug.com/1040686): Simulate failure using ERR_NAME_NOT_RESOLVED
// rather than ERR_NOT_IMPLEMENTED.
if (!local) { if (!local) {
DVLOG(1) << "To avoid external dependencies, simulating failure for " DVLOG(1) << "To avoid external dependencies, simulating failure for "
"external DNS lookup of " "external DNS lookup of "
......
...@@ -63,8 +63,8 @@ bool IsClientCertificateError(int error) { ...@@ -63,8 +63,8 @@ bool IsClientCertificateError(int error) {
} }
bool IsDnsError(int error) { bool IsDnsError(int error) {
DCHECK_NE(ERR_NAME_RESOLUTION_FAILED, error); return (error == ERR_NAME_NOT_RESOLVED ||
return error == ERR_NAME_NOT_RESOLVED; error == ERR_NAME_RESOLUTION_FAILED);
} }
Error FileErrorToNetError(base::File::Error file_error) { Error FileErrorToNetError(base::File::Error file_error) {
......
...@@ -242,10 +242,7 @@ HostResolverFlags HostResolver::ParametersToHostResolverFlags( ...@@ -242,10 +242,7 @@ HostResolverFlags HostResolver::ParametersToHostResolverFlags(
// static // static
int HostResolver::SquashErrorCode(int error) { int HostResolver::SquashErrorCode(int error) {
// TODO(crbug.com/1040686): Once InProcessBrowserTests do not use if (error == OK || error == ERR_IO_PENDING ||
// ERR_NOT_IMPLEMENTED to simulate DNS failures, it should be ok to squash
// ERR_NOT_IMPLEMENTED.
if (error == OK || error == ERR_IO_PENDING || error == ERR_NOT_IMPLEMENTED ||
error == ERR_NAME_NOT_RESOLVED) { error == ERR_NAME_NOT_RESOLVED) {
return error; return error;
} else { } else {
......
...@@ -66,9 +66,12 @@ class NET_EXPORT HostResolver { ...@@ -66,9 +66,12 @@ class NET_EXPORT HostResolver {
// On any other returned value, the request was handled synchronously and // On any other returned value, the request was handled synchronously and
// |callback| will not be invoked. // |callback| will not be invoked.
// //
// Results in ERR_NAME_NOT_RESOLVED if the hostname is not resolved. More // Results in ERR_NAME_NOT_RESOLVED if the hostname is invalid, or if it is
// detail about the underlying error can be retrieved using // an incompatible IP literal (e.g. IPv6 is disabled and it is an IPv6
// GetResolveErrorInfo(). // literal).
//
// Results in ERR_DNS_CACHE_MISS if only fast local sources are to be
// queried and a cache lookup attempt fails.
// //
// The parent HostResolver must still be alive when Start() is called, but // The parent HostResolver must still be alive when Start() is called, but
// if it is destroyed before an asynchronous result completes, the request // if it is destroyed before an asynchronous result completes, the request
......
...@@ -650,7 +650,7 @@ class HostResolverManager::RequestImpl ...@@ -650,7 +650,7 @@ class HostResolverManager::RequestImpl
LogFinishRequest(error); LogFinishRequest(error);
DCHECK(callback_); DCHECK(callback_);
std::move(callback_).Run(HostResolver::SquashErrorCode(error)); std::move(callback_).Run(error);
} }
Job* job() const { return job_; } Job* job() const { return job_; }
...@@ -3015,7 +3015,7 @@ int HostResolverManager::Resolve(RequestImpl* request) { ...@@ -3015,7 +3015,7 @@ int HostResolverManager::Resolve(RequestImpl* request) {
effective_secure_dns_mode, base::TimeDelta()); effective_secure_dns_mode, base::TimeDelta());
request->set_error_info(results.error(), request->set_error_info(results.error(),
false /* is_secure_network_error */); false /* is_secure_network_error */);
return HostResolver::SquashErrorCode(results.error()); return results.error();
} }
CreateAndStartJob(effective_query_type, effective_host_resolver_flags, CreateAndStartJob(effective_query_type, effective_host_resolver_flags,
......
...@@ -313,7 +313,7 @@ TEST_F(HttpWithDnsOverHttpsTest, EndToEndFail) { ...@@ -313,7 +313,7 @@ TEST_F(HttpWithDnsOverHttpsTest, EndToEndFail) {
EXPECT_EQ(test_https_requests_served_, 0u); EXPECT_EQ(test_https_requests_served_, 0u);
EXPECT_TRUE(d.response_completed()); EXPECT_TRUE(d.response_completed());
EXPECT_EQ(d.request_status(), net::ERR_NAME_NOT_RESOLVED); EXPECT_EQ(d.request_status(), net::ERR_DNS_MALFORMED_RESPONSE);
const auto& resolve_error_info = req->response_info().resolve_error_info; const auto& resolve_error_info = req->response_info().resolve_error_info;
EXPECT_TRUE(resolve_error_info.is_secure_network_error); EXPECT_TRUE(resolve_error_info.is_secure_network_error);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment