From 13b22cfd649aa96c7e62ec94c940d57f0074298e Mon Sep 17 00:00:00 2001 From: Mattias Nissler Date: Thu, 9 Jul 2015 14:47:04 +0200 Subject: [PATCH] Revert of Change the mojo ProxyResolver to use ProxyResolverV8Tracing. (patchset #3 id:220001 of https://codereview.chromium.org/1166523003/) Reason for revert: This change introduced a use-after free that lit up the memory waterfall. Links to a couple of affected bots: http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Tests/builds/7754/steps/net_unittests/logs/stdio http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20ChromeOS%20MSan%20Tests/builds/2715/steps/net_unittests/logs/stdio http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28valgrind%29%281%29/builds/42456/steps/memory%20test%3A%20net/logs/stdio http://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Unit%20%28DrMemory%20full%29%20%281%29/builds/1635/steps/memory%20test%3A%20net/logs/stdio Report: MojoProxyResolverV8TracingBindingsTest.Basic (run #1): [ RUN ] MojoProxyResolverV8TracingBindingsTest.Basic ==31178==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x7fbd2d6e8eaf in NotifyEvent net/test/event_waiter.h:27:34 #1 0x7fbd2d6e8eaf in net::MojoProxyResolverV8TracingBindingsTest::Alert(mojo::String const&) net/proxy/mojo_proxy_resolver_v8_tracing_bindings_unittest.cc:33:0 #2 0x7fbd2d6e6ecb in AlertOnTaskRunner net/proxy/mojo_proxy_resolver_v8_tracing_bindings.h:67:7 #3 0x7fbd2d6e6ecb in net::MojoProxyResolverV8TracingBindings\u003Cnet::MojoProxyResolverV8TracingBindingsTest>::Alert(std::__1::basic_string\u003Cunsigned short, base::string16_char_traits, std::__1::allocator\u003Cunsigned short> > const&) net/proxy/mojo_proxy_resolver_v8_tracing_bindings.h:42:0 #4 0x7fbd2d6e0c7e in SendAlertAndError net/proxy/mojo_proxy_resolver_v8_tracing_bindings_unittest.cc:45:5 #5 0x7fbd2d6e0c7e in net::MojoProxyResolverV8TracingBindingsTest_Basic_Test::TestBody() net/proxy/mojo_proxy_resolver_v8_tracing_bindings_unittest.cc:62:0 #6 0x7fbd308384d7 in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2420:12 #7 0x7fbd308384d7 in testing::Test::Run() testing/gtest/src/gtest.cc:2436:0 #8 0x7fbd3083b965 in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2612:5 #9 0x7fbd3083d322 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2730:5 #10 0x7fbd3085b73a in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4604:11 #11 0x7fbd3085a717 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2420:12 #12 0x7fbd3085a717 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4222:0 #13 0x7fbd323e8959 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2326:10 #14 0x7fbd323e8959 in base::TestSuite::Run() base/test/test_suite.cc:229:0 #15 0x7fbd323cded9 in Run base/callback.h:396:12 #16 0x7fbd323cded9 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback\u003Cint ()> const&, int, bool, base::Callback\u003Cvoid ()> const&) base/test/launcher/unit_test_launcher.cc:184:0 #17 0x7fbd323cd7a0 in base::LaunchUnitTests(int, char**, base::Callback\u003Cint ()> const&) base/test/launcher/unit_test_launcher.cc:423:10 #18 0x7fbd2fab8304 in main net/test/run_all_unittests.cc:64:10 #19 0x7fbd2745776c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226:0 #20 0x7fbd2b7d61a8 in _start ??:0:0 Uninitialized value was created by a heap allocation #0 0x7fbd2b82d402 in operator new(unsigned long) ??:0:0 #1 0x7fbd2d6e64e5 in testing::internal::TestFactoryImpl\u003Cnet::MojoProxyResolverV8TracingBindingsTest_Basic_Test>::CreateTest() testing/gtest/include/gtest/internal/gtest-internal.h:486:39 #2 0x7fbd3083b8ef in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::TestFactoryBase, testing::Test *> testing/gtest/src/gtest.cc:2420:12 #3 0x7fbd3083b8ef in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2603:0 #4 0x7fbd3083d322 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2730:5 #5 0x7fbd3085b73a in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4604:11 #6 0x7fbd3085a717 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2420:12 #7 0x7fbd3085a717 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4222:0 #8 0x7fbd323e8959 in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2326:10 #9 0x7fbd323e8959 in base::TestSuite::Run() base/test/test_suite.cc:229:0 #10 0x7fbd323cded9 in Run base/callback.h:396:12 #11 0x7fbd323cded9 in base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback\u003Cint ()> const&, int, bool, base::Callback\u003Cvoid ()> const&) base/test/launcher/unit_test_launcher.cc:184:0 #12 0x7fbd323cd7a0 in base::LaunchUnitTests(int, char**, base::Callback\u003Cint ()> const&) base/test/launcher/unit_test_launcher.cc:423:10 #13 0x7fbd2fab8304 in main net/test/run_all_unittests.cc:64:10 #14 0x7fbd2745776c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226:0 SUMMARY: MemorySanitizer: use-of-uninitialized-value (/tmp/run_tha_testEIzO42/out/Release/net_unittests+0x2416eaf) Exiting Original issue's description: > Change the mojo ProxyResolver to use ProxyResolverV8Tracing. > > This allows a much simpler implementation of GetLoadState and forwarding > errors to a ProxyResolverErrorObserver as well as enabling a > straightforward implementation of NetLog event reporting. > > BUG=467832 > > Committed: https://crrev.com/9f5bf59b57e6b0662d9b9e0f6715ef7b5af092c0 > Cr-Commit-Position: refs/heads/master@{#337993} TBR=eroman@chromium.org,amistry@chromium.org,sammc@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=467832 Review URL: https://codereview.chromium.org/1224083003 . Cr-Commit-Position: refs/heads/master@{#338019} --- ...ity_process_mojo_proxy_resolver_factory.cc | 6 +- ...lity_process_mojo_proxy_resolver_factory.h | 2 + net/BUILD.gn | 5 +- net/dns/host_resolver_mojo.cc | 19 +- net/dns/host_resolver_mojo.h | 27 +- net/dns/host_resolver_mojo_unittest.cc | 53 +++- net/dns/mojo_host_resolver_impl.cc | 14 +- net/dns/mojo_host_resolver_impl.h | 29 +- net/dns/mojo_host_resolver_impl_unittest.cc | 43 ++- net/interfaces/host_resolver_service.mojom | 9 + net/interfaces/proxy_resolver_service.mojom | 18 +- net/net.gyp | 5 +- net/net.gypi | 2 +- .../in_process_mojo_proxy_resolver_factory.cc | 5 +- .../in_process_mojo_proxy_resolver_factory.h | 2 + net/proxy/mojo_proxy_resolver_factory.h | 2 + net/proxy/mojo_proxy_resolver_factory_impl.cc | 88 ++++-- net/proxy/mojo_proxy_resolver_factory_impl.h | 13 +- ...jo_proxy_resolver_factory_impl_unittest.cc | 253 +++++++++++++----- net/proxy/mojo_proxy_resolver_impl.cc | 19 +- net/proxy/mojo_proxy_resolver_impl.h | 5 +- .../mojo_proxy_resolver_impl_unittest.cc | 158 ++++++----- .../mojo_proxy_resolver_v8_tracing_bindings.h | 92 ------- ...y_resolver_v8_tracing_bindings_unittest.cc | 91 ------- .../proxy_resolver_error_observer_mojo.cc | 46 ++++ .../proxy_resolver_error_observer_mojo.h | 43 +++ ...y_resolver_error_observer_mojo_unittest.cc | 105 ++++++++ net/proxy/proxy_resolver_factory_mojo.cc | 191 +++++-------- net/proxy/proxy_resolver_factory_mojo.h | 7 +- .../proxy_resolver_factory_mojo_unittest.cc | 192 ++----------- net/proxy/proxy_service_mojo.cc | 3 +- net/proxy/proxy_service_mojo_unittest.cc | 122 +-------- 32 files changed, 805 insertions(+), 864 deletions(-) delete mode 100644 net/proxy/mojo_proxy_resolver_v8_tracing_bindings.h delete mode 100644 net/proxy/mojo_proxy_resolver_v8_tracing_bindings_unittest.cc create mode 100644 net/proxy/proxy_resolver_error_observer_mojo.cc create mode 100644 net/proxy/proxy_resolver_error_observer_mojo.h create mode 100644 net/proxy/proxy_resolver_error_observer_mojo_unittest.cc diff --git a/chrome/browser/net/utility_process_mojo_proxy_resolver_factory.cc b/chrome/browser/net/utility_process_mojo_proxy_resolver_factory.cc index 4c4e888ebaa0..dac0191eab6f 100644 --- a/chrome/browser/net/utility_process_mojo_proxy_resolver_factory.cc +++ b/chrome/browser/net/utility_process_mojo_proxy_resolver_factory.cc @@ -66,6 +66,8 @@ scoped_ptr UtilityProcessMojoProxyResolverFactory::CreateResolver( const mojo::String& pac_script, mojo::InterfaceRequest req, + net::interfaces::HostResolverPtr host_resolver, + net::interfaces::ProxyResolverErrorObserverPtr error_observer, net::interfaces::ProxyResolverFactoryRequestClientPtr client) { DCHECK(thread_checker_.CalledOnValidThread()); if (!resolver_factory_) @@ -79,7 +81,9 @@ UtilityProcessMojoProxyResolverFactory::CreateResolver( } idle_timer_.Stop(); num_proxy_resolvers_++; - resolver_factory_->CreateResolver(pac_script, req.Pass(), client.Pass()); + resolver_factory_->CreateResolver(pac_script, req.Pass(), + host_resolver.Pass(), error_observer.Pass(), + client.Pass()); return make_scoped_ptr(new base::ScopedClosureRunner( base::Bind(&UtilityProcessMojoProxyResolverFactory::OnResolverDestroyed, base::Unretained(this)))); diff --git a/chrome/browser/net/utility_process_mojo_proxy_resolver_factory.h b/chrome/browser/net/utility_process_mojo_proxy_resolver_factory.h index d4223aa4e14d..02aaeb6193f3 100644 --- a/chrome/browser/net/utility_process_mojo_proxy_resolver_factory.h +++ b/chrome/browser/net/utility_process_mojo_proxy_resolver_factory.h @@ -31,6 +31,8 @@ class UtilityProcessMojoProxyResolverFactory scoped_ptr CreateResolver( const mojo::String& pac_script, mojo::InterfaceRequest req, + net::interfaces::HostResolverPtr host_resolver, + net::interfaces::ProxyResolverErrorObserverPtr error_observer, net::interfaces::ProxyResolverFactoryRequestClientPtr client) override; private: diff --git a/net/BUILD.gn b/net/BUILD.gn index 45931187b29a..8454d7a0cd52 100644 --- a/net/BUILD.gn +++ b/net/BUILD.gn @@ -856,7 +856,8 @@ if (use_v8_in_net && !is_android) { "proxy/mojo_proxy_resolver_factory_impl.h", "proxy/mojo_proxy_resolver_impl.cc", "proxy/mojo_proxy_resolver_impl.h", - "proxy/mojo_proxy_resolver_v8_tracing_bindings.h", + "proxy/proxy_resolver_error_observer_mojo.cc", + "proxy/proxy_resolver_error_observer_mojo.h", ] deps = [ @@ -1509,7 +1510,7 @@ if (!is_android && !is_mac) { "dns/mojo_host_resolver_impl_unittest.cc", "proxy/mojo_proxy_resolver_factory_impl_unittest.cc", "proxy/mojo_proxy_resolver_impl_unittest.cc", - "proxy/mojo_proxy_resolver_v8_tracing_bindings_unittest.cc", + "proxy/proxy_resolver_error_observer_mojo_unittest.cc", "proxy/proxy_resolver_factory_mojo_unittest.cc", "proxy/proxy_service_mojo_unittest.cc", ] diff --git a/net/dns/host_resolver_mojo.cc b/net/dns/host_resolver_mojo.cc index 161f586f61a4..56404055d819 100644 --- a/net/dns/host_resolver_mojo.cc +++ b/net/dns/host_resolver_mojo.cc @@ -49,10 +49,16 @@ class HostResolverMojo::Job : public interfaces::HostResolverRequestClient { base::WeakPtr host_cache_; }; -HostResolverMojo::HostResolverMojo(Impl* impl) - : impl_(impl), +HostResolverMojo::HostResolverMojo(interfaces::HostResolverPtr resolver, + const base::Closure& disconnect_callback) + : resolver_(resolver.Pass()), + disconnect_callback_(disconnect_callback), host_cache_(HostCache::CreateDefaultCache()), host_cache_weak_factory_(host_cache_.get()) { + if (!disconnect_callback_.is_null()) { + resolver_.set_connection_error_handler(base::Bind( + &HostResolverMojo::OnConnectionError, base::Unretained(this))); + } } HostResolverMojo::~HostResolverMojo() = default; @@ -77,8 +83,8 @@ int HostResolverMojo::Resolve(const RequestInfo& info, interfaces::HostResolverRequestClientPtr handle; *request_handle = new Job(key, addresses, callback, mojo::GetProxy(&handle), host_cache_weak_factory_.GetWeakPtr()); - impl_->ResolveDns(interfaces::HostResolverRequestInfo::From(info), - handle.Pass()); + resolver_->Resolve(interfaces::HostResolverRequestInfo::From(info), + handle.Pass()); return ERR_IO_PENDING; } @@ -101,6 +107,11 @@ HostCache* HostResolverMojo::GetHostCache() { return host_cache_.get(); } +void HostResolverMojo::OnConnectionError() { + DCHECK(!disconnect_callback_.is_null()); + disconnect_callback_.Run(); +} + int HostResolverMojo::ResolveFromCacheInternal(const RequestInfo& info, const HostCache::Key& key, AddressList* addresses) { diff --git a/net/dns/host_resolver_mojo.h b/net/dns/host_resolver_mojo.h index 23fb0ff0c8bc..373cb9e8b4cf 100644 --- a/net/dns/host_resolver_mojo.h +++ b/net/dns/host_resolver_mojo.h @@ -5,6 +5,7 @@ #ifndef NET_DNS_HOST_RESOLVER_MOJO_H_ #define NET_DNS_HOST_RESOLVER_MOJO_H_ +#include "base/callback.h" #include "base/memory/weak_ptr.h" #include "base/threading/thread_checker.h" #include "net/dns/host_cache.h" @@ -15,19 +16,12 @@ namespace net { class AddressList; class BoundNetLog; -// A HostResolver implementation that converts requests to mojo types and -// forwards them to a mojo Impl interface. +// A HostResolver implementation that delegates to an interfaces::HostResolver +// mojo interface. class HostResolverMojo : public HostResolver { public: - class Impl { - public: - virtual ~Impl() = default; - virtual void ResolveDns(interfaces::HostResolverRequestInfoPtr, - interfaces::HostResolverRequestClientPtr) = 0; - }; - - // |impl| must outlive |this|. - explicit HostResolverMojo(Impl* impl); + HostResolverMojo(interfaces::HostResolverPtr resolver, + const base::Closure& disconnect_callback); ~HostResolverMojo() override; // HostResolver overrides. @@ -43,14 +37,23 @@ class HostResolverMojo : public HostResolver { void CancelRequest(RequestHandle req) override; HostCache* GetHostCache() override; + void set_disconnect_callback(const base::Closure& disconnect_callback) { + disconnect_callback_ = disconnect_callback; + } + private: class Job; + // Mojo error handler. + void OnConnectionError(); + int ResolveFromCacheInternal(const RequestInfo& info, const HostCache::Key& key, AddressList* addresses); - Impl* const impl_; + interfaces::HostResolverPtr resolver_; + + base::Closure disconnect_callback_; scoped_ptr host_cache_; base::WeakPtrFactory host_cache_weak_factory_; diff --git a/net/dns/host_resolver_mojo_unittest.cc b/net/dns/host_resolver_mojo_unittest.cc index 209bda88a812..774c9451dea3 100644 --- a/net/dns/host_resolver_mojo_unittest.cc +++ b/net/dns/host_resolver_mojo_unittest.cc @@ -83,10 +83,11 @@ struct HostResolverAction { Error error = OK; }; -class MockMojoHostResolver : public HostResolverMojo::Impl { +class MockMojoHostResolver : public interfaces::HostResolver { public: - explicit MockMojoHostResolver( - const base::Closure& request_connection_error_callback); + MockMojoHostResolver(mojo::InterfaceRequest request, + const base::Closure& resolver_connection_error_callback, + const base::Closure& request_connection_error_callback); ~MockMojoHostResolver() override; void AddAction(scoped_ptr action); @@ -95,31 +96,46 @@ class MockMojoHostResolver : public HostResolverMojo::Impl { return requests_received_; } - void ResolveDns(interfaces::HostResolverRequestInfoPtr request_info, - interfaces::HostResolverRequestClientPtr client) override; - private: + // interfaces::HostResolver override. + void Resolve(interfaces::HostResolverRequestInfoPtr request_info, + interfaces::HostResolverRequestClientPtr client) override; + + void OnConnectionError(); + + mojo::Binding binding_; ScopedVector actions_; size_t results_returned_ = 0; mojo::Array requests_received_; + const base::Closure resolver_connection_error_callback_; const base::Closure request_connection_error_callback_; ScopedVector requests_; }; MockMojoHostResolver::MockMojoHostResolver( + mojo::InterfaceRequest request, + const base::Closure& resolver_connection_error_callback, const base::Closure& request_connection_error_callback) - : request_connection_error_callback_(request_connection_error_callback) { + : binding_(this, request.Pass()), + resolver_connection_error_callback_(resolver_connection_error_callback), + request_connection_error_callback_(request_connection_error_callback) { + binding_.set_connection_error_handler(base::Bind( + &MockMojoHostResolver::OnConnectionError, base::Unretained(this))); } MockMojoHostResolver::~MockMojoHostResolver() { EXPECT_EQ(results_returned_, actions_.size()); } +void MockMojoHostResolver::OnConnectionError() { + resolver_connection_error_callback_.Run(); +} + void MockMojoHostResolver::AddAction(scoped_ptr action) { actions_.push_back(action.Pass()); } -void MockMojoHostResolver::ResolveDns( +void MockMojoHostResolver::Resolve( interfaces::HostResolverRequestInfoPtr request_info, interfaces::HostResolverRequestClientPtr client) { requests_received_.push_back(request_info.Pass()); @@ -145,15 +161,24 @@ void MockMojoHostResolver::ResolveDns( class HostResolverMojoTest : public testing::Test { protected: enum class ConnectionErrorSource { + RESOLVER, REQUEST, + CLIENT, }; using Waiter = EventWaiter; void SetUp() override { + interfaces::HostResolverPtr resolver_ptr; mock_resolver_.reset(new MockMojoHostResolver( + mojo::GetProxy(&resolver_ptr), + base::Bind(&Waiter::NotifyEvent, base::Unretained(&waiter_), + ConnectionErrorSource::RESOLVER), base::Bind(&Waiter::NotifyEvent, base::Unretained(&waiter_), ConnectionErrorSource::REQUEST))); - resolver_.reset(new HostResolverMojo(mock_resolver_.get())); + resolver_.reset(new HostResolverMojo( + resolver_ptr.Pass(), + base::Bind(&Waiter::NotifyEvent, base::Unretained(&waiter_), + ConnectionErrorSource::CLIENT))); } int Resolve(const HostResolver::RequestInfo& request_info, @@ -348,6 +373,16 @@ TEST_F(HostResolverMojoTest, ImplDropsClientConnection) { EXPECT_FALSE(request.is_my_ip_address); } +TEST_F(HostResolverMojoTest, DestroyImpl) { + mock_resolver_.reset(); + waiter_.WaitForEvent(ConnectionErrorSource::CLIENT); +} + +TEST_F(HostResolverMojoTest, DestroyClient) { + resolver_.reset(); + waiter_.WaitForEvent(ConnectionErrorSource::RESOLVER); +} + TEST_F(HostResolverMojoTest, ResolveFromCache_Miss) { HostResolver::RequestInfo request_info( HostPortPair::FromString("example.com:8080")); diff --git a/net/dns/mojo_host_resolver_impl.cc b/net/dns/mojo_host_resolver_impl.cc index c39079c94e97..5acba89fc952 100644 --- a/net/dns/mojo_host_resolver_impl.cc +++ b/net/dns/mojo_host_resolver_impl.cc @@ -21,7 +21,6 @@ class MojoHostResolverImpl::Job { Job(MojoHostResolverImpl* resolver_service, net::HostResolver* resolver, const net::HostResolver::RequestInfo& request_info, - const BoundNetLog& net_log, interfaces::HostResolverRequestClientPtr client); ~Job(); @@ -37,16 +36,14 @@ class MojoHostResolverImpl::Job { MojoHostResolverImpl* resolver_service_; net::HostResolver* resolver_; net::HostResolver::RequestInfo request_info_; - const BoundNetLog net_log_; interfaces::HostResolverRequestClientPtr client_; net::HostResolver::RequestHandle handle_; AddressList result_; base::ThreadChecker thread_checker_; }; -MojoHostResolverImpl::MojoHostResolverImpl(net::HostResolver* resolver, - const BoundNetLog& net_log) - : resolver_(resolver), net_log_(net_log) { +MojoHostResolverImpl::MojoHostResolverImpl(net::HostResolver* resolver) + : resolver_(resolver) { } MojoHostResolverImpl::~MojoHostResolverImpl() { @@ -60,7 +57,7 @@ void MojoHostResolverImpl::Resolve( DCHECK(thread_checker_.CalledOnValidThread()); Job* job = new Job(this, resolver_, request_info->To(), - net_log_, client.Pass()); + client.Pass()); pending_jobs_.insert(job); job->Start(); } @@ -76,12 +73,10 @@ MojoHostResolverImpl::Job::Job( MojoHostResolverImpl* resolver_service, net::HostResolver* resolver, const net::HostResolver::RequestInfo& request_info, - const BoundNetLog& net_log, interfaces::HostResolverRequestClientPtr client) : resolver_service_(resolver_service), resolver_(resolver), request_info_(request_info), - net_log_(net_log), client_(client.Pass()), handle_(nullptr) { client_.set_connection_error_handler(base::Bind( @@ -90,11 +85,12 @@ MojoHostResolverImpl::Job::Job( void MojoHostResolverImpl::Job::Start() { DVLOG(1) << "Resolve " << request_info_.host_port_pair().ToString(); + BoundNetLog net_log; int result = resolver_->Resolve(request_info_, DEFAULT_PRIORITY, &result_, base::Bind(&MojoHostResolverImpl::Job::OnResolveDone, base::Unretained(this)), - &handle_, net_log_); + &handle_, net_log); if (result != ERR_IO_PENDING) OnResolveDone(result); diff --git a/net/dns/mojo_host_resolver_impl.h b/net/dns/mojo_host_resolver_impl.h index b0b11abb50ba..aa359ef3e04b 100644 --- a/net/dns/mojo_host_resolver_impl.h +++ b/net/dns/mojo_host_resolver_impl.h @@ -10,27 +10,21 @@ #include "base/macros.h" #include "base/threading/thread_checker.h" #include "net/interfaces/host_resolver_service.mojom.h" -#include "net/log/net_log.h" namespace net { class HostResolver; -// MojoHostResolverImpl handles mojo host resolution requests. Inbound Mojo -// requests are sent to the HostResolver passed into the constructor. When -// destroyed, any outstanding resolver requests are cancelled. If a request's -// HostResolverRequestClient is shut down, the associated resolver request is -// cancelled. -class MojoHostResolverImpl { +// This is the implementation of the HostResolver Mojo interface. +// Inbound Mojo requests are sent to the HostResolver passed into the +// constructor. When destroyed, any outstanding resolver requests are cancelled. +// If a request's HostResolverRequestClient is shut down, the associated +// resolver request is cancelled. +class MojoHostResolverImpl : public interfaces::HostResolver { public: // |resolver| is expected to outlive |this|. - MojoHostResolverImpl(net::HostResolver* resolver, const BoundNetLog& net_log); - ~MojoHostResolverImpl(); - - void Resolve(interfaces::HostResolverRequestInfoPtr request_info, - interfaces::HostResolverRequestClientPtr client); - - bool request_in_progress() { return !pending_jobs_.empty(); } + explicit MojoHostResolverImpl(net::HostResolver* resolver); + ~MojoHostResolverImpl() override; private: class Job; @@ -38,12 +32,13 @@ class MojoHostResolverImpl { // Removes |job| from the set of pending jobs, and deletes it. void DeleteJob(Job* job); + // Overridden from net::interfaces::HostResolver: + void Resolve(interfaces::HostResolverRequestInfoPtr request_info, + interfaces::HostResolverRequestClientPtr client) override; + // Resolver for resolving incoming requests. Not owned. net::HostResolver* resolver_; - // The BoundNetLog to be passed to |resolver_| for all requests. - const BoundNetLog net_log_; - // All pending jobs, so they can be cancelled when this service is destroyed. // Owns all jobs. std::set pending_jobs_; diff --git a/net/dns/mojo_host_resolver_impl_unittest.cc b/net/dns/mojo_host_resolver_impl_unittest.cc index 4f7a05b47ee9..72f0f804b9c3 100644 --- a/net/dns/mojo_host_resolver_impl_unittest.cc +++ b/net/dns/mojo_host_resolver_impl_unittest.cc @@ -14,7 +14,6 @@ #include "net/base/net_util.h" #include "net/dns/mock_host_resolver.h" #include "net/dns/mojo_host_type_converters.h" -#include "net/log/net_log.h" #include "testing/gtest/include/gtest/gtest.h" #include "third_party/mojo/src/mojo/public/cpp/bindings/binding.h" #include "third_party/mojo/src/mojo/public/cpp/bindings/interface_request.h" @@ -131,8 +130,10 @@ class MojoHostResolverImplTest : public testing::Test { mock_host_resolver_.rules()->AddRule("chromium.org", "8.8.8.8"); mock_host_resolver_.rules()->AddSimulatedFailure("failure.fail"); - resolver_service_.reset( - new MojoHostResolverImpl(&mock_host_resolver_, BoundNetLog())); + resolver_service_.reset(new MojoHostResolverImpl(&mock_host_resolver_)); + resolver_service_binding_.reset( + new mojo::Binding(resolver_service_.get())); + resolver_service_binding_->Bind(mojo::GetProxy(&resolver_service_ptr_)); } interfaces::HostResolverRequestInfoPtr CreateRequest(const std::string& host, @@ -158,6 +159,9 @@ class MojoHostResolverImplTest : public testing::Test { CallbackMockHostResolver mock_host_resolver_; scoped_ptr resolver_service_; + + scoped_ptr> resolver_service_binding_; + interfaces::HostResolverPtr resolver_service_ptr_; }; TEST_F(MojoHostResolverImplTest, Resolve) { @@ -166,7 +170,7 @@ TEST_F(MojoHostResolverImplTest, Resolve) { interfaces::HostResolverRequestInfoPtr request = CreateRequest("example.com", 80, false); - resolver_service_->Resolve(request.Pass(), client_ptr.Pass()); + resolver_service_ptr_->Resolve(request.Pass(), client_ptr.Pass()); client.WaitForResult(); EXPECT_EQ(net::OK, client.error_); @@ -183,7 +187,7 @@ TEST_F(MojoHostResolverImplTest, ResolveSynchronous) { interfaces::HostResolverRequestInfoPtr request = CreateRequest("example.com", 80, false); - resolver_service_->Resolve(request.Pass(), client_ptr.Pass()); + resolver_service_ptr_->Resolve(request.Pass(), client_ptr.Pass()); client.WaitForResult(); EXPECT_EQ(net::OK, client.error_); @@ -202,10 +206,10 @@ TEST_F(MojoHostResolverImplTest, ResolveMultiple) { interfaces::HostResolverRequestInfoPtr request1 = CreateRequest("example.com", 80, false); - resolver_service_->Resolve(request1.Pass(), client1_ptr.Pass()); + resolver_service_ptr_->Resolve(request1.Pass(), client1_ptr.Pass()); interfaces::HostResolverRequestInfoPtr request2 = CreateRequest("chromium.org", 80, false); - resolver_service_->Resolve(request2.Pass(), client2_ptr.Pass()); + resolver_service_ptr_->Resolve(request2.Pass(), client2_ptr.Pass()); WaitForRequests(2); mock_host_resolver_.ResolveAllPending(); @@ -232,10 +236,10 @@ TEST_F(MojoHostResolverImplTest, ResolveDuplicate) { interfaces::HostResolverRequestInfoPtr request1 = CreateRequest("example.com", 80, false); - resolver_service_->Resolve(request1.Pass(), client1_ptr.Pass()); + resolver_service_ptr_->Resolve(request1.Pass(), client1_ptr.Pass()); interfaces::HostResolverRequestInfoPtr request2 = CreateRequest("example.com", 80, false); - resolver_service_->Resolve(request2.Pass(), client2_ptr.Pass()); + resolver_service_ptr_->Resolve(request2.Pass(), client2_ptr.Pass()); WaitForRequests(2); mock_host_resolver_.ResolveAllPending(); @@ -258,7 +262,7 @@ TEST_F(MojoHostResolverImplTest, ResolveFailure) { interfaces::HostResolverRequestInfoPtr request = CreateRequest("failure.fail", 80, false); - resolver_service_->Resolve(request.Pass(), client_ptr.Pass()); + resolver_service_ptr_->Resolve(request.Pass(), client_ptr.Pass()); client.WaitForResult(); EXPECT_EQ(net::ERR_NAME_NOT_RESOLVED, client.error_); @@ -274,7 +278,7 @@ TEST_F(MojoHostResolverImplTest, DestroyClient) { interfaces::HostResolverRequestInfoPtr request = CreateRequest("example.com", 80, false); - resolver_service_->Resolve(request.Pass(), client_ptr.Pass()); + resolver_service_ptr_->Resolve(request.Pass(), client_ptr.Pass()); WaitForRequests(1); client.reset(); @@ -284,4 +288,21 @@ TEST_F(MojoHostResolverImplTest, DestroyClient) { base::RunLoop().RunUntilIdle(); } +TEST_F(MojoHostResolverImplTest, DestroyService) { + interfaces::HostResolverRequestClientPtr client_ptr; + TestRequestClient client(mojo::GetProxy(&client_ptr)); + + mock_host_resolver_.set_ondemand_mode(true); + + interfaces::HostResolverRequestInfoPtr request = + CreateRequest("example.com", 80, false); + resolver_service_ptr_->Resolve(request.Pass(), client_ptr.Pass()); + WaitForRequests(1); + + resolver_service_binding_.reset(); + resolver_service_.reset(); + + client.WaitForConnectionError(); +} + } // namespace net diff --git a/net/interfaces/host_resolver_service.mojom b/net/interfaces/host_resolver_service.mojom index 63975154ec52..249fa0707d33 100644 --- a/net/interfaces/host_resolver_service.mojom +++ b/net/interfaces/host_resolver_service.mojom @@ -40,6 +40,15 @@ struct AddressList { array addresses; }; +interface HostResolver { + // Use a HostResolverRequestClient instead of returning a result so we can + // cancel in-flight requests by destroying the client. IPC requests in Mojo + // cannot be cancelled directly. + // TODO(amistry): Add BoundNetLog. + Resolve(HostResolverRequestInfo request_info, + HostResolverRequestClient client); +}; + interface HostResolverRequestClient { // |error| is a value in net::Error. ReportResult(int32 error, AddressList? result); diff --git a/net/interfaces/proxy_resolver_service.mojom b/net/interfaces/proxy_resolver_service.mojom index f6656fa1da08..133fe8b91d01 100644 --- a/net/interfaces/proxy_resolver_service.mojom +++ b/net/interfaces/proxy_resolver_service.mojom @@ -32,31 +32,27 @@ struct ProxyServer { interface ProxyResolver { // Use a ProxyResolverRequestClient instead of returning a result so we can // cancel in-flight requests by destroying the client. + // TODO(amistry): Add BoundNetLog. GetProxyForUrl(string url, ProxyResolverRequestClient client); }; interface ProxyResolverRequestClient { ReportResult(int32 error, array? proxy_servers); +}; - Alert(string error); - OnError(int32 line_number, string error); - - ResolveDns(HostResolverRequestInfo request_info, - HostResolverRequestClient client); +interface ProxyResolverErrorObserver { + OnPacScriptError(int32 line_number, string error); }; interface ProxyResolverFactory { + // TODO(amistry): Add NetLog. CreateResolver(string pac_script, ProxyResolver& resolver, + HostResolver host_resolver, + ProxyResolverErrorObserver? error_observer, ProxyResolverFactoryRequestClient client); }; interface ProxyResolverFactoryRequestClient { ReportResult(int32 error); - - Alert(string error); - OnError(int32 line_number, string error); - - ResolveDns(HostResolverRequestInfo request_info, - HostResolverRequestClient client); }; diff --git a/net/net.gyp b/net/net.gyp index 57c2b86b2f10..b2314994f0a1 100644 --- a/net/net.gyp +++ b/net/net.gyp @@ -320,7 +320,7 @@ 'dns/mojo_host_resolver_impl_unittest.cc', 'proxy/mojo_proxy_resolver_factory_impl_unittest.cc', 'proxy/mojo_proxy_resolver_impl_unittest.cc', - 'proxy/mojo_proxy_resolver_v8_tracing_bindings_unittest.cc', + 'proxy/proxy_resolver_error_observer_mojo_unittest.cc', 'proxy/proxy_resolver_factory_mojo_unittest.cc', 'proxy/proxy_service_mojo_unittest.cc', ], @@ -936,7 +936,8 @@ 'proxy/mojo_proxy_resolver_factory_impl.h', 'proxy/mojo_proxy_resolver_impl.cc', 'proxy/mojo_proxy_resolver_impl.h', - 'proxy/mojo_proxy_resolver_v8_tracing_bindings.h', + 'proxy/proxy_resolver_error_observer_mojo.cc', + 'proxy/proxy_resolver_error_observer_mojo.h', ], 'dependencies': [ 'mojo_type_converters', diff --git a/net/net.gypi b/net/net.gypi index faeaa4c13e5d..19f5e90466ab 100644 --- a/net/net.gypi +++ b/net/net.gypi @@ -1485,7 +1485,6 @@ 'proxy/dhcp_proxy_script_fetcher_win_unittest.cc', 'proxy/mojo_proxy_resolver_factory_impl_unittest.cc', 'proxy/mojo_proxy_resolver_impl_unittest.cc', - 'proxy/mojo_proxy_resolver_v8_tracing_bindings_unittest.cc', 'proxy/multi_threaded_proxy_resolver_unittest.cc', 'proxy/network_delegate_error_observer_unittest.cc', 'proxy/proxy_bypass_rules_unittest.cc', @@ -1495,6 +1494,7 @@ 'proxy/proxy_config_unittest.cc', 'proxy/proxy_info_unittest.cc', 'proxy/proxy_list_unittest.cc', + 'proxy/proxy_resolver_error_observer_mojo_unittest.cc', 'proxy/proxy_resolver_factory_mojo_unittest.cc', 'proxy/proxy_resolver_v8_tracing_unittest.cc', 'proxy/proxy_resolver_v8_tracing_wrapper_unittest.cc', diff --git a/net/proxy/in_process_mojo_proxy_resolver_factory.cc b/net/proxy/in_process_mojo_proxy_resolver_factory.cc index dfd9e7b2a033..cb1cf21aa911 100644 --- a/net/proxy/in_process_mojo_proxy_resolver_factory.cc +++ b/net/proxy/in_process_mojo_proxy_resolver_factory.cc @@ -29,8 +29,11 @@ scoped_ptr InProcessMojoProxyResolverFactory::CreateResolver( const mojo::String& pac_script, mojo::InterfaceRequest req, + interfaces::HostResolverPtr host_resolver, + interfaces::ProxyResolverErrorObserverPtr error_observer, interfaces::ProxyResolverFactoryRequestClientPtr client) { - factory_->CreateResolver(pac_script, req.Pass(), client.Pass()); + factory_->CreateResolver(pac_script, req.Pass(), host_resolver.Pass(), + error_observer.Pass(), client.Pass()); return nullptr; } diff --git a/net/proxy/in_process_mojo_proxy_resolver_factory.h b/net/proxy/in_process_mojo_proxy_resolver_factory.h index f130dca0fb94..86895e922ac7 100644 --- a/net/proxy/in_process_mojo_proxy_resolver_factory.h +++ b/net/proxy/in_process_mojo_proxy_resolver_factory.h @@ -24,6 +24,8 @@ class InProcessMojoProxyResolverFactory : public MojoProxyResolverFactory { scoped_ptr CreateResolver( const mojo::String& pac_script, mojo::InterfaceRequest req, + interfaces::HostResolverPtr host_resolver, + interfaces::ProxyResolverErrorObserverPtr error_observer, interfaces::ProxyResolverFactoryRequestClientPtr client) override; private: diff --git a/net/proxy/mojo_proxy_resolver_factory.h b/net/proxy/mojo_proxy_resolver_factory.h index 2163cb0cad3a..89b70f4ee895 100644 --- a/net/proxy/mojo_proxy_resolver_factory.h +++ b/net/proxy/mojo_proxy_resolver_factory.h @@ -23,6 +23,8 @@ class MojoProxyResolverFactory { virtual scoped_ptr CreateResolver( const mojo::String& pac_script, mojo::InterfaceRequest req, + interfaces::HostResolverPtr host_resolver, + interfaces::ProxyResolverErrorObserverPtr error_observer, interfaces::ProxyResolverFactoryRequestClientPtr client) = 0; protected: diff --git a/net/proxy/mojo_proxy_resolver_factory_impl.cc b/net/proxy/mojo_proxy_resolver_factory_impl.cc index a913ce25ead5..a67bc232bb2c 100644 --- a/net/proxy/mojo_proxy_resolver_factory_impl.cc +++ b/net/proxy/mojo_proxy_resolver_factory_impl.cc @@ -8,26 +8,44 @@ #include "base/stl_util.h" #include "net/base/net_errors.h" +#include "net/dns/host_resolver_mojo.h" #include "net/proxy/mojo_proxy_resolver_impl.h" -#include "net/proxy/mojo_proxy_resolver_v8_tracing_bindings.h" +#include "net/proxy/proxy_resolver_error_observer_mojo.h" #include "net/proxy/proxy_resolver_factory.h" -#include "net/proxy/proxy_resolver_v8_tracing.h" +#include "net/proxy/proxy_resolver_v8.h" +#include "net/proxy/proxy_resolver_v8_tracing_wrapper.h" namespace net { namespace { -// A class to manage the lifetime of a MojoProxyResolverImpl. An instance will -// remain while the message pipe for the mojo connection remains open. +scoped_ptr ReturnErrorObserver( + scoped_ptr error_observer) { + return error_observer; +} + +scoped_ptr CreateDefaultProxyResolver( + HostResolver* host_resolver, + scoped_ptr error_observer) { + return make_scoped_ptr(new ProxyResolverFactoryV8TracingWrapper( + host_resolver, nullptr, + base::Bind(&ReturnErrorObserver, base::Passed(&error_observer)))); +} + +// A class to manage the lifetime of a MojoProxyResolverImpl and a +// HostResolverMojo. An instance will remain while the message pipes for both +// mojo connections remain open. class MojoProxyResolverHolder { public: MojoProxyResolverHolder( - scoped_ptr proxy_resolver_impl, + scoped_ptr host_resolver, + scoped_ptr proxy_resolver_impl, mojo::InterfaceRequest request); private: // Mojo error handler. void OnConnectionError(); + scoped_ptr host_resolver_; MojoProxyResolverImpl mojo_proxy_resolver_; mojo::Binding binding_; @@ -35,12 +53,16 @@ class MojoProxyResolverHolder { }; MojoProxyResolverHolder::MojoProxyResolverHolder( - scoped_ptr proxy_resolver_impl, + scoped_ptr host_resolver, + scoped_ptr proxy_resolver_impl, mojo::InterfaceRequest request) - : mojo_proxy_resolver_(proxy_resolver_impl.Pass()), + : host_resolver_(host_resolver.Pass()), + mojo_proxy_resolver_(proxy_resolver_impl.Pass()), binding_(&mojo_proxy_resolver_, request.Pass()) { binding_.set_connection_error_handler(base::Bind( &MojoProxyResolverHolder::OnConnectionError, base::Unretained(this))); + host_resolver_->set_disconnect_callback(base::Bind( + &MojoProxyResolverHolder::OnConnectionError, base::Unretained(this))); } void MojoProxyResolverHolder::OnConnectionError() { @@ -53,8 +75,10 @@ class MojoProxyResolverFactoryImpl::Job { public: Job(MojoProxyResolverFactoryImpl* parent, const scoped_refptr& pac_script, - ProxyResolverV8TracingFactory* proxy_resolver_factory, + const MojoProxyResolverFactoryImpl::Factory& proxy_resolver_factory, mojo::InterfaceRequest request, + interfaces::HostResolverPtr host_resolver, + interfaces::ProxyResolverErrorObserverPtr error_observer, interfaces::ProxyResolverFactoryRequestClientPtr client); ~Job(); @@ -65,37 +89,39 @@ class MojoProxyResolverFactoryImpl::Job { void OnProxyResolverCreated(int error); MojoProxyResolverFactoryImpl* const parent_; - scoped_ptr proxy_resolver_impl_; + scoped_ptr host_resolver_; + scoped_ptr proxy_resolver_impl_; mojo::InterfaceRequest proxy_request_; - ProxyResolverV8TracingFactory* factory_; + scoped_ptr factory_; scoped_ptr request_; interfaces::ProxyResolverFactoryRequestClientPtr client_ptr_; - base::WeakPtrFactory - weak_factory_; - DISALLOW_COPY_AND_ASSIGN(Job); }; MojoProxyResolverFactoryImpl::Job::Job( MojoProxyResolverFactoryImpl* factory, const scoped_refptr& pac_script, - ProxyResolverV8TracingFactory* proxy_resolver_factory, + const MojoProxyResolverFactoryImpl::Factory& proxy_resolver_factory, mojo::InterfaceRequest request, + interfaces::HostResolverPtr host_resolver, + interfaces::ProxyResolverErrorObserverPtr error_observer, interfaces::ProxyResolverFactoryRequestClientPtr client) : parent_(factory), + host_resolver_(new HostResolverMojo( + host_resolver.Pass(), + base::Bind(&MojoProxyResolverFactoryImpl::Job::OnConnectionError, + base::Unretained(this)))), proxy_request_(request.Pass()), - factory_(proxy_resolver_factory), - client_ptr_(client.Pass()), - weak_factory_(client_ptr_.get()) { + factory_(proxy_resolver_factory.Run( + host_resolver_.get(), + ProxyResolverErrorObserverMojo::Create(error_observer.Pass()))), + client_ptr_(client.Pass()) { client_ptr_.set_connection_error_handler( base::Bind(&MojoProxyResolverFactoryImpl::Job::OnConnectionError, base::Unretained(this))); - factory_->CreateProxyResolverV8Tracing( - pac_script, make_scoped_ptr(new MojoProxyResolverV8TracingBindings< - interfaces::ProxyResolverFactoryRequestClient>( - weak_factory_.GetWeakPtr())), - &proxy_resolver_impl_, + factory_->CreateProxyResolver( + pac_script, &proxy_resolver_impl_, base::Bind(&MojoProxyResolverFactoryImpl::Job::OnProxyResolverCreated, base::Unretained(this)), &request_); @@ -110,9 +136,10 @@ void MojoProxyResolverFactoryImpl::Job::OnConnectionError() { void MojoProxyResolverFactoryImpl::Job::OnProxyResolverCreated(int error) { if (error == OK) { - // The MojoProxyResolverHolder will delete itself if |proxy_request_| - // encounters a connection error. - new MojoProxyResolverHolder(proxy_resolver_impl_.Pass(), + // The MojoProxyResolverHolder will delete itself if either + // |host_resolver_| or |proxy_request_| encounters a connection error. + new MojoProxyResolverHolder(host_resolver_.Pass(), + proxy_resolver_impl_.Pass(), proxy_request_.Pass()); } client_ptr_->ReportResult(error); @@ -120,15 +147,15 @@ void MojoProxyResolverFactoryImpl::Job::OnProxyResolverCreated(int error) { } MojoProxyResolverFactoryImpl::MojoProxyResolverFactoryImpl( - scoped_ptr proxy_resolver_factory, + const MojoProxyResolverFactoryImpl::Factory& proxy_resolver_factory, mojo::InterfaceRequest request) - : proxy_resolver_impl_factory_(proxy_resolver_factory.Pass()), + : proxy_resolver_impl_factory_(proxy_resolver_factory), binding_(this, request.Pass()) { } MojoProxyResolverFactoryImpl::MojoProxyResolverFactoryImpl( mojo::InterfaceRequest request) - : MojoProxyResolverFactoryImpl(ProxyResolverV8TracingFactory::Create(), + : MojoProxyResolverFactoryImpl(base::Bind(&CreateDefaultProxyResolver), request.Pass()) { } @@ -139,12 +166,15 @@ MojoProxyResolverFactoryImpl::~MojoProxyResolverFactoryImpl() { void MojoProxyResolverFactoryImpl::CreateResolver( const mojo::String& pac_script, mojo::InterfaceRequest request, + interfaces::HostResolverPtr host_resolver, + interfaces::ProxyResolverErrorObserverPtr error_observer, interfaces::ProxyResolverFactoryRequestClientPtr client) { // The Job will call RemoveJob on |this| when either the create request // finishes or |request| or |client| encounters a connection error. jobs_.insert(new Job( this, ProxyResolverScriptData::FromUTF8(pac_script.To()), - proxy_resolver_impl_factory_.get(), request.Pass(), client.Pass())); + proxy_resolver_impl_factory_, request.Pass(), host_resolver.Pass(), + error_observer.Pass(), client.Pass())); } void MojoProxyResolverFactoryImpl::RemoveJob(Job* job) { diff --git a/net/proxy/mojo_proxy_resolver_factory_impl.h b/net/proxy/mojo_proxy_resolver_factory_impl.h index dcc7b482bf62..f447759e14a3 100644 --- a/net/proxy/mojo_proxy_resolver_factory_impl.h +++ b/net/proxy/mojo_proxy_resolver_factory_impl.h @@ -9,19 +9,24 @@ #include "base/callback.h" #include "net/interfaces/proxy_resolver_service.mojom.h" +#include "net/proxy/proxy_resolver.h" #include "third_party/mojo/src/mojo/public/cpp/bindings/strong_binding.h" namespace net { class HostResolver; class ProxyResolverErrorObserver; -class ProxyResolverV8TracingFactory; +class ProxyResolverFactory; class MojoProxyResolverFactoryImpl : public interfaces::ProxyResolverFactory { public: + using Factory = base::Callback( + HostResolver*, + scoped_ptr)>; + explicit MojoProxyResolverFactoryImpl( mojo::InterfaceRequest request); MojoProxyResolverFactoryImpl( - scoped_ptr proxy_resolver_factory, + const Factory& proxy_resolver_factory, mojo::InterfaceRequest request); ~MojoProxyResolverFactoryImpl() override; @@ -33,11 +38,13 @@ class MojoProxyResolverFactoryImpl : public interfaces::ProxyResolverFactory { void CreateResolver( const mojo::String& pac_script, mojo::InterfaceRequest request, + interfaces::HostResolverPtr host_resolver, + interfaces::ProxyResolverErrorObserverPtr error_observer, interfaces::ProxyResolverFactoryRequestClientPtr client) override; void RemoveJob(Job* job); - const scoped_ptr proxy_resolver_impl_factory_; + const Factory proxy_resolver_impl_factory_; mojo::StrongBinding binding_; std::set jobs_; diff --git a/net/proxy/mojo_proxy_resolver_factory_impl_unittest.cc b/net/proxy/mojo_proxy_resolver_factory_impl_unittest.cc index 0262d647cc1c..162e4b0a7ad7 100644 --- a/net/proxy/mojo_proxy_resolver_factory_impl_unittest.cc +++ b/net/proxy/mojo_proxy_resolver_factory_impl_unittest.cc @@ -7,7 +7,7 @@ #include "base/strings/utf_string_conversions.h" #include "net/base/test_completion_callback.h" #include "net/proxy/mock_proxy_resolver.h" -#include "net/proxy/proxy_resolver_v8_tracing.h" +#include "net/proxy/proxy_resolver_error_observer.h" #include "net/test/event_waiter.h" #include "testing/gtest/include/gtest/gtest.h" #include "third_party/mojo/src/mojo/public/cpp/bindings/binding.h" @@ -17,7 +17,7 @@ namespace { const char kScriptData[] = "FooBarBaz"; -class FakeProxyResolver : public ProxyResolverV8Tracing { +class FakeProxyResolver : public MockAsyncProxyResolver { public: explicit FakeProxyResolver(const base::Closure& on_destruction) : on_destruction_(on_destruction) {} @@ -25,19 +25,6 @@ class FakeProxyResolver : public ProxyResolverV8Tracing { ~FakeProxyResolver() override { on_destruction_.Run(); } private: - // ProxyResolverV8Tracing overrides. - void GetProxyForURL(const GURL& url, - ProxyInfo* results, - const CompletionCallback& callback, - ProxyResolver::RequestHandle* request, - scoped_ptr bindings) override {} - - void CancelRequest(ProxyResolver::RequestHandle request) override {} - - LoadState GetLoadState(ProxyResolver::RequestHandle request) const override { - return LOAD_STATE_RESOLVING_PROXY_FOR_URL; - } - const base::Closure on_destruction_; }; @@ -48,44 +35,23 @@ enum Event { RESOLVER_DESTROYED, }; -class TestProxyResolverFactory : public ProxyResolverV8TracingFactory { +class TestProxyResolverFactory : public MockAsyncProxyResolverFactory { public: - struct PendingRequest { - scoped_ptr* resolver; - CompletionCallback callback; - }; - explicit TestProxyResolverFactory(EventWaiter* waiter) - : waiter_(waiter) {} + : MockAsyncProxyResolverFactory(true), waiter_(waiter) {} - void CreateProxyResolverV8Tracing( + int CreateProxyResolver( const scoped_refptr& pac_script, - scoped_ptr bindings, - scoped_ptr* resolver, + scoped_ptr* resolver, const CompletionCallback& callback, scoped_ptr* request) override { - requests_handled_++; waiter_->NotifyEvent(RESOLVER_CREATED); - EXPECT_EQ(base::ASCIIToUTF16(kScriptData), pac_script->utf16()); - EXPECT_TRUE(resolver); - pending_request_.reset(new PendingRequest); - pending_request_->resolver = resolver; - pending_request_->callback = callback; - - ASSERT_TRUE(bindings); - - bindings->Alert(base::ASCIIToUTF16("alert")); - bindings->OnError(10, base::ASCIIToUTF16("error")); - EXPECT_TRUE(bindings->GetHostResolver()); + return MockAsyncProxyResolverFactory::CreateProxyResolver( + pac_script, resolver, callback, request); } - size_t requests_handled() { return requests_handled_; } - const PendingRequest* pending_request() { return pending_request_.get(); } - private: EventWaiter* waiter_; - size_t requests_handled_ = 0; - scoped_ptr pending_request_; }; } // namespace @@ -95,13 +61,25 @@ class MojoProxyResolverFactoryImplTest public interfaces::ProxyResolverFactoryRequestClient { public: void SetUp() override { - mock_factory_ = new TestProxyResolverFactory(&waiter_); - new MojoProxyResolverFactoryImpl(make_scoped_ptr(mock_factory_), - mojo::GetProxy(&factory_)); + new MojoProxyResolverFactoryImpl( + base::Bind( + &MojoProxyResolverFactoryImplTest::CreateFakeProxyResolverFactory, + base::Unretained(this)), + mojo::GetProxy(&factory_)); + mock_factory_owner_.reset(new TestProxyResolverFactory(&waiter_)); + mock_factory_ = mock_factory_owner_.get(); } void OnConnectionError() { waiter_.NotifyEvent(CONNECTION_ERROR); } + scoped_ptr CreateFakeProxyResolverFactory( + HostResolver* host_resolver, + scoped_ptr error_observer) { + EXPECT_TRUE(host_resolver); + DCHECK(mock_factory_owner_); + return mock_factory_owner_.Pass(); + } + void OnFakeProxyInstanceDestroyed() { instances_destroyed_++; waiter_.NotifyEvent(RESOLVER_DESTROYED); @@ -109,13 +87,6 @@ class MojoProxyResolverFactoryImplTest void ReportResult(int32_t error) override { create_callback_.Run(error); } - void Alert(const mojo::String& message) override {} - - void OnError(int32_t line_number, const mojo::String& message) override {} - - void ResolveDns(interfaces::HostResolverRequestInfoPtr request_info, - interfaces::HostResolverRequestClientPtr client) override {} - protected: scoped_ptr mock_factory_owner_; TestProxyResolverFactory* mock_factory_; @@ -127,67 +98,193 @@ class MojoProxyResolverFactoryImplTest EventWaiter waiter_; }; +TEST_F(MojoProxyResolverFactoryImplTest, DisconnectHostResolver) { + interfaces::ProxyResolverPtr proxy_resolver; + interfaces::HostResolverPtr host_resolver; + mojo::InterfaceRequest host_resolver_request = + mojo::GetProxy(&host_resolver); + interfaces::ProxyResolverErrorObserverPtr error_observer; + mojo::GetProxy(&error_observer); + interfaces::ProxyResolverFactoryRequestClientPtr client_ptr; + mojo::Binding client_binding( + this, mojo::GetProxy(&client_ptr)); + factory_->CreateResolver( + mojo::String::From(kScriptData), mojo::GetProxy(&proxy_resolver), + host_resolver.Pass(), error_observer.Pass(), client_ptr.Pass()); + proxy_resolver.set_connection_error_handler( + base::Bind(&MojoProxyResolverFactoryImplTest::OnConnectionError, + base::Unretained(this))); + waiter_.WaitForEvent(RESOLVER_CREATED); + EXPECT_EQ(0, instances_destroyed_); + ASSERT_EQ(1u, mock_factory_->pending_requests().size()); + EXPECT_EQ(base::ASCIIToUTF16(kScriptData), + mock_factory_->pending_requests()[0]->script_data()->utf16()); + TestCompletionCallback create_callback; + create_callback_ = create_callback.callback(); + mock_factory_->pending_requests()[0]->CompleteNow( + OK, make_scoped_ptr(new FakeProxyResolver(base::Bind( + &MojoProxyResolverFactoryImplTest::OnFakeProxyInstanceDestroyed, + base::Unretained(this))))); + EXPECT_EQ(OK, create_callback.WaitForResult()); + host_resolver_request = mojo::InterfaceRequest(); + waiter_.WaitForEvent(CONNECTION_ERROR); + EXPECT_EQ(1, instances_destroyed_); +} + TEST_F(MojoProxyResolverFactoryImplTest, DisconnectProxyResolverClient) { interfaces::ProxyResolverPtr proxy_resolver; + interfaces::HostResolverPtr host_resolver; + mojo::InterfaceRequest host_resolver_request = + mojo::GetProxy(&host_resolver); + mojo::Binding binding(nullptr, &host_resolver); + binding.set_connection_error_handler( + base::Bind(&MojoProxyResolverFactoryImplTest::OnConnectionError, + base::Unretained(this))); + interfaces::ProxyResolverErrorObserverPtr error_observer; + mojo::GetProxy(&error_observer); interfaces::ProxyResolverFactoryRequestClientPtr client_ptr; mojo::Binding client_binding( this, mojo::GetProxy(&client_ptr)); - factory_->CreateResolver(mojo::String::From(kScriptData), - mojo::GetProxy(&proxy_resolver), client_ptr.Pass()); + factory_->CreateResolver( + mojo::String::From(kScriptData), mojo::GetProxy(&proxy_resolver), + host_resolver.Pass(), error_observer.Pass(), client_ptr.Pass()); proxy_resolver.set_connection_error_handler( base::Bind(&MojoProxyResolverFactoryImplTest::OnConnectionError, base::Unretained(this))); waiter_.WaitForEvent(RESOLVER_CREATED); EXPECT_EQ(0, instances_destroyed_); - ASSERT_EQ(1u, mock_factory_->requests_handled()); + ASSERT_EQ(1u, mock_factory_->pending_requests().size()); + EXPECT_EQ(base::ASCIIToUTF16(kScriptData), + mock_factory_->pending_requests()[0]->script_data()->utf16()); TestCompletionCallback create_callback; create_callback_ = create_callback.callback(); - ASSERT_TRUE(mock_factory_->pending_request()); - mock_factory_->pending_request()->resolver->reset( - new FakeProxyResolver(base::Bind( - &MojoProxyResolverFactoryImplTest::OnFakeProxyInstanceDestroyed, - base::Unretained(this)))); - mock_factory_->pending_request()->callback.Run(OK); + mock_factory_->pending_requests()[0]->CompleteNow( + OK, make_scoped_ptr(new FakeProxyResolver(base::Bind( + &MojoProxyResolverFactoryImplTest::OnFakeProxyInstanceDestroyed, + base::Unretained(this))))); EXPECT_EQ(OK, create_callback.WaitForResult()); proxy_resolver.reset(); + waiter_.WaitForEvent(CONNECTION_ERROR); + EXPECT_EQ(1, instances_destroyed_); +} + +TEST_F(MojoProxyResolverFactoryImplTest, DisconnectBoth) { + interfaces::ProxyResolverPtr proxy_resolver; + interfaces::HostResolverPtr host_resolver; + mojo::InterfaceRequest host_resolver_request = + mojo::GetProxy(&host_resolver); + interfaces::ProxyResolverErrorObserverPtr error_observer; + mojo::GetProxy(&error_observer); + interfaces::ProxyResolverFactoryRequestClientPtr client_ptr; + mojo::Binding client_binding( + this, mojo::GetProxy(&client_ptr)); + factory_->CreateResolver( + mojo::String::From(kScriptData), mojo::GetProxy(&proxy_resolver), + host_resolver.Pass(), error_observer.Pass(), client_ptr.Pass()); + proxy_resolver.set_connection_error_handler( + base::Bind(&MojoProxyResolverFactoryImplTest::OnConnectionError, + base::Unretained(this))); + waiter_.WaitForEvent(RESOLVER_CREATED); + EXPECT_EQ(0, instances_destroyed_); + ASSERT_EQ(1u, mock_factory_->pending_requests().size()); + EXPECT_EQ(base::ASCIIToUTF16(kScriptData), + mock_factory_->pending_requests()[0]->script_data()->utf16()); + TestCompletionCallback create_callback; + create_callback_ = create_callback.callback(); + mock_factory_->pending_requests()[0]->CompleteNow( + OK, make_scoped_ptr(new FakeProxyResolver(base::Bind( + &MojoProxyResolverFactoryImplTest::OnFakeProxyInstanceDestroyed, + base::Unretained(this))))); + EXPECT_EQ(OK, create_callback.WaitForResult()); + proxy_resolver.reset(); + host_resolver_request = mojo::InterfaceRequest(); waiter_.WaitForEvent(RESOLVER_DESTROYED); EXPECT_EQ(1, instances_destroyed_); } TEST_F(MojoProxyResolverFactoryImplTest, Error) { interfaces::ProxyResolverPtr proxy_resolver; + interfaces::HostResolverPtr host_resolver; + mojo::InterfaceRequest host_resolver_request = + mojo::GetProxy(&host_resolver); + interfaces::ProxyResolverErrorObserverPtr error_observer; + mojo::GetProxy(&error_observer); interfaces::ProxyResolverFactoryRequestClientPtr client_ptr; mojo::Binding client_binding( this, mojo::GetProxy(&client_ptr)); - factory_->CreateResolver(mojo::String::From(kScriptData), - mojo::GetProxy(&proxy_resolver), client_ptr.Pass()); + factory_->CreateResolver( + mojo::String::From(kScriptData), mojo::GetProxy(&proxy_resolver), + host_resolver.Pass(), error_observer.Pass(), client_ptr.Pass()); proxy_resolver.set_connection_error_handler( base::Bind(&MojoProxyResolverFactoryImplTest::OnConnectionError, base::Unretained(this))); waiter_.WaitForEvent(RESOLVER_CREATED); EXPECT_EQ(0, instances_destroyed_); - ASSERT_EQ(1u, mock_factory_->requests_handled()); + ASSERT_EQ(1u, mock_factory_->pending_requests().size()); + EXPECT_EQ(base::ASCIIToUTF16(kScriptData), + mock_factory_->pending_requests()[0]->script_data()->utf16()); TestCompletionCallback create_callback; create_callback_ = create_callback.callback(); - ASSERT_TRUE(mock_factory_->pending_request()); - mock_factory_->pending_request()->callback.Run(ERR_PAC_SCRIPT_FAILED); + mock_factory_->pending_requests()[0]->CompleteNow(ERR_PAC_SCRIPT_FAILED, + nullptr); EXPECT_EQ(ERR_PAC_SCRIPT_FAILED, create_callback.WaitForResult()); } +TEST_F(MojoProxyResolverFactoryImplTest, + DisconnectHostResolverDuringResolveCreation) { + interfaces::ProxyResolverPtr proxy_resolver; + interfaces::HostResolverPtr host_resolver; + mojo::InterfaceRequest host_resolver_request = + mojo::GetProxy(&host_resolver); + interfaces::ProxyResolverErrorObserverPtr error_observer; + mojo::GetProxy(&error_observer); + interfaces::ProxyResolverFactoryRequestClientPtr client_ptr; + mojo::Binding client_binding( + this, mojo::GetProxy(&client_ptr)); + factory_->CreateResolver( + mojo::String::From(kScriptData), mojo::GetProxy(&proxy_resolver), + host_resolver.Pass(), error_observer.Pass(), client_ptr.Pass()); + proxy_resolver.set_connection_error_handler( + base::Bind(&MojoProxyResolverFactoryImplTest::OnConnectionError, + base::Unretained(this))); + waiter_.WaitForEvent(RESOLVER_CREATED); + EXPECT_EQ(0, instances_destroyed_); + ASSERT_EQ(1u, mock_factory_->pending_requests().size()); + EXPECT_EQ(base::ASCIIToUTF16(kScriptData), + mock_factory_->pending_requests()[0]->script_data()->utf16()); + host_resolver_request = mojo::InterfaceRequest(); + TestCompletionCallback create_callback; + create_callback_ = create_callback.callback(); + waiter_.WaitForEvent(CONNECTION_ERROR); + EXPECT_EQ(ERR_PAC_SCRIPT_TERMINATED, create_callback.WaitForResult()); +} + TEST_F(MojoProxyResolverFactoryImplTest, DisconnectClientDuringResolverCreation) { interfaces::ProxyResolverPtr proxy_resolver; + interfaces::HostResolverPtr host_resolver; + mojo::InterfaceRequest host_resolver_request = + mojo::GetProxy(&host_resolver); + mojo::Binding binding(nullptr, &host_resolver); + binding.set_connection_error_handler( + base::Bind(&MojoProxyResolverFactoryImplTest::OnConnectionError, + base::Unretained(this))); + interfaces::ProxyResolverErrorObserverPtr error_observer; + mojo::GetProxy(&error_observer); interfaces::ProxyResolverFactoryRequestClientPtr client_ptr; mojo::Binding client_binding( this, mojo::GetProxy(&client_ptr)); - factory_->CreateResolver(mojo::String::From(kScriptData), - mojo::GetProxy(&proxy_resolver), client_ptr.Pass()); + factory_->CreateResolver( + mojo::String::From(kScriptData), mojo::GetProxy(&proxy_resolver), + host_resolver.Pass(), error_observer.Pass(), client_ptr.Pass()); proxy_resolver.set_connection_error_handler( base::Bind(&MojoProxyResolverFactoryImplTest::OnConnectionError, base::Unretained(this))); waiter_.WaitForEvent(RESOLVER_CREATED); EXPECT_EQ(0, instances_destroyed_); - ASSERT_EQ(1u, mock_factory_->requests_handled()); + ASSERT_EQ(1u, mock_factory_->pending_requests().size()); + EXPECT_EQ(base::ASCIIToUTF16(kScriptData), + mock_factory_->pending_requests()[0]->script_data()->utf16()); client_binding.Close(); waiter_.WaitForEvent(CONNECTION_ERROR); } @@ -195,11 +292,21 @@ TEST_F(MojoProxyResolverFactoryImplTest, TEST_F(MojoProxyResolverFactoryImplTest, DisconnectFactoryDuringResolverCreation) { interfaces::ProxyResolverPtr proxy_resolver; + interfaces::HostResolverPtr host_resolver; + mojo::InterfaceRequest host_resolver_request = + mojo::GetProxy(&host_resolver); + mojo::Binding binding(nullptr, &host_resolver); + binding.set_connection_error_handler( + base::Bind(&MojoProxyResolverFactoryImplTest::OnConnectionError, + base::Unretained(this))); + interfaces::ProxyResolverErrorObserverPtr error_observer; + mojo::GetProxy(&error_observer); interfaces::ProxyResolverFactoryRequestClientPtr client_ptr; mojo::Binding client_binding( this, mojo::GetProxy(&client_ptr)); - factory_->CreateResolver(mojo::String::From(kScriptData), - mojo::GetProxy(&proxy_resolver), client_ptr.Pass()); + factory_->CreateResolver( + mojo::String::From(kScriptData), mojo::GetProxy(&proxy_resolver), + host_resolver.Pass(), error_observer.Pass(), client_ptr.Pass()); proxy_resolver.set_connection_error_handler( base::Bind(&MojoProxyResolverFactoryImplTest::OnConnectionError, base::Unretained(this))); @@ -208,7 +315,9 @@ TEST_F(MojoProxyResolverFactoryImplTest, base::Unretained(this))); waiter_.WaitForEvent(RESOLVER_CREATED); EXPECT_EQ(0, instances_destroyed_); - ASSERT_EQ(1u, mock_factory_->requests_handled()); + ASSERT_EQ(1u, mock_factory_->pending_requests().size()); + EXPECT_EQ(base::ASCIIToUTF16(kScriptData), + mock_factory_->pending_requests()[0]->script_data()->utf16()); factory_.reset(); waiter_.WaitForEvent(CONNECTION_ERROR); } diff --git a/net/proxy/mojo_proxy_resolver_impl.cc b/net/proxy/mojo_proxy_resolver_impl.cc index 7c52c17901f2..754a388f51e2 100644 --- a/net/proxy/mojo_proxy_resolver_impl.cc +++ b/net/proxy/mojo_proxy_resolver_impl.cc @@ -8,11 +8,9 @@ #include "mojo/common/url_type_converters.h" #include "net/base/net_errors.h" #include "net/log/net_log.h" -#include "net/proxy/mojo_proxy_resolver_v8_tracing_bindings.h" #include "net/proxy/mojo_proxy_type_converters.h" #include "net/proxy/proxy_info.h" #include "net/proxy/proxy_resolver_script_data.h" -#include "net/proxy/proxy_resolver_v8_tracing.h" namespace net { @@ -42,13 +40,11 @@ class MojoProxyResolverImpl::Job { net::ProxyResolver::RequestHandle request_handle_; bool done_; - base::WeakPtrFactory weak_factory_; - DISALLOW_COPY_AND_ASSIGN(Job); }; MojoProxyResolverImpl::MojoProxyResolverImpl( - scoped_ptr resolver) + scoped_ptr resolver) : resolver_(resolver.Pass()) { } @@ -83,8 +79,7 @@ MojoProxyResolverImpl::Job::Job( client_(client.Pass()), url_(url), request_handle_(nullptr), - done_(false), - weak_factory_(client_.get()) { + done_(false) { } MojoProxyResolverImpl::Job::~Job() { @@ -93,11 +88,13 @@ MojoProxyResolverImpl::Job::~Job() { } void MojoProxyResolverImpl::Job::Start() { - resolver_->resolver_->GetProxyForURL( + int result = resolver_->resolver_->GetProxyForURL( url_, &result_, base::Bind(&Job::GetProxyDone, base::Unretained(this)), - &request_handle_, - make_scoped_ptr(new MojoProxyResolverV8TracingBindings< - interfaces::ProxyResolverRequestClient>(weak_factory_.GetWeakPtr()))); + &request_handle_, BoundNetLog()); + if (result != ERR_IO_PENDING) { + GetProxyDone(result); + return; + } client_.set_connection_error_handler(base::Bind( &MojoProxyResolverImpl::Job::OnConnectionError, base::Unretained(this))); resolver_->request_handle_to_job_.insert( diff --git a/net/proxy/mojo_proxy_resolver_impl.h b/net/proxy/mojo_proxy_resolver_impl.h index 53a1ff28fdde..2d801c8c4389 100644 --- a/net/proxy/mojo_proxy_resolver_impl.h +++ b/net/proxy/mojo_proxy_resolver_impl.h @@ -15,11 +15,10 @@ #include "net/proxy/proxy_resolver.h" namespace net { -class ProxyResolverV8Tracing; class MojoProxyResolverImpl : public interfaces::ProxyResolver { public: - explicit MojoProxyResolverImpl(scoped_ptr resolver); + explicit MojoProxyResolverImpl(scoped_ptr resolver); ~MojoProxyResolverImpl() override; @@ -33,7 +32,7 @@ class MojoProxyResolverImpl : public interfaces::ProxyResolver { void DeleteJob(Job* job); - scoped_ptr resolver_; + scoped_ptr resolver_; std::set resolve_jobs_; std::map request_handle_to_job_; diff --git a/net/proxy/mojo_proxy_resolver_impl_unittest.cc b/net/proxy/mojo_proxy_resolver_impl_unittest.cc index bfc777325a79..070177b2b4fd 100644 --- a/net/proxy/mojo_proxy_resolver_impl_unittest.cc +++ b/net/proxy/mojo_proxy_resolver_impl_unittest.cc @@ -12,7 +12,6 @@ #include "net/proxy/mock_proxy_resolver.h" #include "net/proxy/mojo_proxy_type_converters.h" #include "net/proxy/proxy_info.h" -#include "net/proxy/proxy_resolver_v8_tracing.h" #include "net/proxy/proxy_server.h" #include "net/test/event_waiter.h" #include "testing/gtest/include/gtest/gtest.h" @@ -41,10 +40,6 @@ class TestRequestClient : public interfaces::ProxyResolverRequestClient { // interfaces::ProxyResolverRequestClient override. void ReportResult(int32_t error, mojo::Array results) override; - void Alert(const mojo::String& message) override; - void OnError(int32_t line_number, const mojo::String& message) override; - void ResolveDns(interfaces::HostResolverRequestInfoPtr request_info, - interfaces::HostResolverRequestClientPtr client) override; // Mojo error handler. void OnConnectionError(); @@ -83,105 +78,87 @@ void TestRequestClient::ReportResult( done_ = true; } -void TestRequestClient::Alert(const mojo::String& message) { -} - -void TestRequestClient::OnError(int32_t line_number, - const mojo::String& message) { -} - -void TestRequestClient::ResolveDns( - interfaces::HostResolverRequestInfoPtr request_info, - interfaces::HostResolverRequestClientPtr client) { -} - void TestRequestClient::OnConnectionError() { event_waiter_.NotifyEvent(CONNECTION_ERROR); } -class MockProxyResolverV8Tracing : public ProxyResolverV8Tracing { +class CallbackMockProxyResolver : public MockAsyncProxyResolver { public: - struct Request { - GURL url; - ProxyInfo* results; - CompletionCallback callback; - bool cancelled = false; - }; - MockProxyResolverV8Tracing() {} + CallbackMockProxyResolver() {} + ~CallbackMockProxyResolver() override; - // ProxyResolverV8Tracing overrides. - void GetProxyForURL(const GURL& url, - ProxyInfo* results, - const CompletionCallback& callback, - ProxyResolver::RequestHandle* request, - scoped_ptr bindings) override; - void CancelRequest(ProxyResolver::RequestHandle request_handle) override; - LoadState GetLoadState(ProxyResolver::RequestHandle request) const override; + // MockAsyncProxyResolver overrides. + int GetProxyForURL(const GURL& url, + ProxyInfo* results, + const CompletionCallback& callback, + RequestHandle* request_handle, + const BoundNetLog& net_log) override; + void CancelRequest(RequestHandle request_handle) override; // Wait until the mock resolver has received a CancelRequest call. void WaitForCancel(); - const std::vector& pending_requests() { return pending_requests_; } + // Queues a proxy result to be returned synchronously. + void ReturnProxySynchronously(const ProxyInfo& result); private: base::Closure cancel_callback_; - std::vector pending_requests_; + scoped_ptr sync_result_; }; -void MockProxyResolverV8Tracing::GetProxyForURL( +CallbackMockProxyResolver::~CallbackMockProxyResolver() { + EXPECT_TRUE(pending_requests().empty()); +} + +int CallbackMockProxyResolver::GetProxyForURL( const GURL& url, ProxyInfo* results, const CompletionCallback& callback, - ProxyResolver::RequestHandle* request, - scoped_ptr bindings) { - pending_requests_.push_back(Request()); - auto& pending_request = pending_requests_.back(); - pending_request.url = url; - pending_request.results = results; - pending_request.callback = callback; - *request = - reinterpret_cast(pending_requests_.size()); + RequestHandle* request_handle, + const BoundNetLog& net_log) { + if (sync_result_) { + *results = *sync_result_; + sync_result_.reset(); + return OK; + } + return MockAsyncProxyResolver::GetProxyForURL(url, results, callback, + request_handle, net_log); } -void MockProxyResolverV8Tracing::CancelRequest( - ProxyResolver::RequestHandle request_handle) { - size_t id = reinterpret_cast(request_handle) - 1; - pending_requests_[id].cancelled = true; +void CallbackMockProxyResolver::CancelRequest(RequestHandle request_handle) { + MockAsyncProxyResolver::CancelRequest(request_handle); if (!cancel_callback_.is_null()) { cancel_callback_.Run(); cancel_callback_.Reset(); } } -LoadState MockProxyResolverV8Tracing::GetLoadState( - ProxyResolver::RequestHandle request) const { - return LOAD_STATE_RESOLVING_PROXY_FOR_URL; -} - -void MockProxyResolverV8Tracing::WaitForCancel() { - while (std::find_if(pending_requests_.begin(), pending_requests_.end(), - [](const Request& request) { - return request.cancelled; - }) != pending_requests_.end()) { +void CallbackMockProxyResolver::WaitForCancel() { + while (cancelled_requests().empty()) { base::RunLoop run_loop; cancel_callback_ = run_loop.QuitClosure(); run_loop.Run(); } } +void CallbackMockProxyResolver::ReturnProxySynchronously( + const ProxyInfo& result) { + sync_result_.reset(new ProxyInfo(result)); +} + } // namespace class MojoProxyResolverImplTest : public testing::Test { protected: void SetUp() override { - scoped_ptr mock_resolver( - new MockProxyResolverV8Tracing); + scoped_ptr mock_resolver( + new CallbackMockProxyResolver); mock_proxy_resolver_ = mock_resolver.get(); resolver_impl_.reset(new MojoProxyResolverImpl(mock_resolver.Pass())); resolver_ = resolver_impl_.get(); } - MockProxyResolverV8Tracing* mock_proxy_resolver_; + CallbackMockProxyResolver* mock_proxy_resolver_; scoped_ptr resolver_impl_; interfaces::ProxyResolver* resolver_; @@ -193,18 +170,18 @@ TEST_F(MojoProxyResolverImplTest, GetProxyForUrl) { resolver_->GetProxyForUrl("http://example.com", client_ptr.Pass()); ASSERT_EQ(1u, mock_proxy_resolver_->pending_requests().size()); - const MockProxyResolverV8Tracing::Request& request = + scoped_refptr request = mock_proxy_resolver_->pending_requests()[0]; - EXPECT_EQ(GURL("http://example.com"), request.url); + EXPECT_EQ(GURL("http://example.com"), request->url()); - request.results->UsePacString( + request->results()->UsePacString( "PROXY proxy.example.com:1; " "SOCKS4 socks4.example.com:2; " "SOCKS5 socks5.example.com:3; " "HTTPS https.example.com:4; " "QUIC quic.example.com:65000; " "DIRECT"); - request.callback.Run(OK); + request->CompleteNow(OK); client.WaitForResult(); EXPECT_EQ(OK, client.error()); @@ -234,16 +211,35 @@ TEST_F(MojoProxyResolverImplTest, GetProxyForUrl) { EXPECT_EQ(ProxyServer::SCHEME_DIRECT, servers[5].scheme()); } +TEST_F(MojoProxyResolverImplTest, GetProxyForUrlSynchronous) { + interfaces::ProxyResolverRequestClientPtr client_ptr; + TestRequestClient client(mojo::GetProxy(&client_ptr)); + + ProxyInfo result; + result.UsePacString("DIRECT"); + mock_proxy_resolver_->ReturnProxySynchronously(result); + resolver_->GetProxyForUrl("http://example.com", client_ptr.Pass()); + ASSERT_EQ(0u, mock_proxy_resolver_->pending_requests().size()); + client.WaitForResult(); + + EXPECT_EQ(OK, client.error()); + std::vector proxy_servers = + client.results().To>(); + ASSERT_EQ(1u, proxy_servers.size()); + ProxyServer& server = proxy_servers[0]; + EXPECT_TRUE(server.is_direct()); +} + TEST_F(MojoProxyResolverImplTest, GetProxyForUrlFailure) { interfaces::ProxyResolverRequestClientPtr client_ptr; TestRequestClient client(mojo::GetProxy(&client_ptr)); resolver_->GetProxyForUrl("http://example.com", client_ptr.Pass()); ASSERT_EQ(1u, mock_proxy_resolver_->pending_requests().size()); - const MockProxyResolverV8Tracing::Request& request = + scoped_refptr request = mock_proxy_resolver_->pending_requests()[0]; - EXPECT_EQ(GURL("http://example.com"), request.url); - request.callback.Run(ERR_FAILED); + EXPECT_EQ(GURL("http://example.com"), request->url()); + request->CompleteNow(ERR_FAILED); client.WaitForResult(); EXPECT_EQ(ERR_FAILED, client.error()); @@ -261,16 +257,16 @@ TEST_F(MojoProxyResolverImplTest, GetProxyForUrlMultiple) { resolver_->GetProxyForUrl("http://example.com", client_ptr1.Pass()); resolver_->GetProxyForUrl("https://example.com", client_ptr2.Pass()); ASSERT_EQ(2u, mock_proxy_resolver_->pending_requests().size()); - const MockProxyResolverV8Tracing::Request& request1 = + scoped_refptr request1 = mock_proxy_resolver_->pending_requests()[0]; - EXPECT_EQ(GURL("http://example.com"), request1.url); - const MockProxyResolverV8Tracing::Request& request2 = + EXPECT_EQ(GURL("http://example.com"), request1->url()); + scoped_refptr request2 = mock_proxy_resolver_->pending_requests()[1]; - EXPECT_EQ(GURL("https://example.com"), request2.url); - request1.results->UsePacString("HTTPS proxy.example.com:12345"); - request1.callback.Run(OK); - request2.results->UsePacString("SOCKS5 another-proxy.example.com:6789"); - request2.callback.Run(OK); + EXPECT_EQ(GURL("https://example.com"), request2->url()); + request1->results()->UsePacString("HTTPS proxy.example.com:12345"); + request1->CompleteNow(OK); + request2->results()->UsePacString("SOCKS5 another-proxy.example.com:6789"); + request2->CompleteNow(OK); client1.WaitForResult(); client2.WaitForResult(); @@ -300,10 +296,10 @@ TEST_F(MojoProxyResolverImplTest, DestroyClient) { resolver_->GetProxyForUrl("http://example.com", client_ptr.Pass()); ASSERT_EQ(1u, mock_proxy_resolver_->pending_requests().size()); - const MockProxyResolverV8Tracing::Request& request = + scoped_refptr request = mock_proxy_resolver_->pending_requests()[0]; - EXPECT_EQ(GURL("http://example.com"), request.url); - request.results->UsePacString("PROXY proxy.example.com:8080"); + EXPECT_EQ(GURL("http://example.com"), request->url()); + request->results()->UsePacString("PROXY proxy.example.com:8080"); client.reset(); mock_proxy_resolver_->WaitForCancel(); } @@ -314,6 +310,8 @@ TEST_F(MojoProxyResolverImplTest, DestroyService) { resolver_->GetProxyForUrl("http://example.com", client_ptr.Pass()); ASSERT_EQ(1u, mock_proxy_resolver_->pending_requests().size()); + scoped_refptr request = + mock_proxy_resolver_->pending_requests()[0]; resolver_impl_.reset(); client.event_waiter().WaitForEvent(TestRequestClient::CONNECTION_ERROR); } diff --git a/net/proxy/mojo_proxy_resolver_v8_tracing_bindings.h b/net/proxy/mojo_proxy_resolver_v8_tracing_bindings.h deleted file mode 100644 index f8d732993e93..000000000000 --- a/net/proxy/mojo_proxy_resolver_v8_tracing_bindings.h +++ /dev/null @@ -1,92 +0,0 @@ -// Copyright 2015 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef NET_PROXY_MOJO_PROXY_RESOLVER_V8_TRACING_BINDINGS_H_ -#define NET_PROXY_MOJO_PROXY_RESOLVER_V8_TRACING_BINDINGS_H_ - -#include "base/memory/weak_ptr.h" -#include "base/single_thread_task_runner.h" -#include "base/thread_task_runner_handle.h" -#include "mojo/common/common_type_converters.h" -#include "net/dns/host_resolver_mojo.h" -#include "net/interfaces/proxy_resolver_service.mojom.h" -#include "net/proxy/proxy_resolver_v8_tracing.h" - -namespace net { - -// An implementation of ProxyResolverV8Tracing::Bindings that forwards requests -// onto a Client mojo interface. Alert() and OnError() may be called from any -// thread; when they are called from another thread, the calls are proxied to -// the origin task runner. GetHostResolver() and GetBoundNetLog() may only be -// called from the origin task runner. -template -class MojoProxyResolverV8TracingBindings - : public ProxyResolverV8Tracing::Bindings, - public HostResolverMojo::Impl { - public: - explicit MojoProxyResolverV8TracingBindings(base::WeakPtr client) - : task_runner_(base::ThreadTaskRunnerHandle::Get()), - client_(client), - host_resolver_(this) {} - - // ProxyResolverV8Tracing::Bindings overrides. - void Alert(const base::string16& message) override { - if (!task_runner_->BelongsToCurrentThread()) { - task_runner_->PostTask( - FROM_HERE, - base::Bind(&MojoProxyResolverV8TracingBindings::AlertOnTaskRunner, - client_, message)); - return; - } - AlertOnTaskRunner(client_, message); - } - - void OnError(int line_number, const base::string16& message) override { - if (!task_runner_->BelongsToCurrentThread()) { - task_runner_->PostTask( - FROM_HERE, - base::Bind(&MojoProxyResolverV8TracingBindings::OnErrorOnTaskRunner, - client_, line_number, message)); - return; - } - OnErrorOnTaskRunner(client_, line_number, message); - } - - HostResolver* GetHostResolver() override { - DCHECK(task_runner_->BelongsToCurrentThread()); - return &host_resolver_; - } - - BoundNetLog GetBoundNetLog() override { return BoundNetLog(); } - - private: - static void AlertOnTaskRunner(base::WeakPtr client, - const base::string16& message) { - if (client) - client->Alert(mojo::String::From(message)); - } - - static void OnErrorOnTaskRunner(base::WeakPtr client, - int line_number, - const base::string16& message) { - if (client) - client->OnError(line_number, mojo::String::From(message)); - } - - // HostResolverMojo::Impl override. - void ResolveDns(interfaces::HostResolverRequestInfoPtr request_info, - interfaces::HostResolverRequestClientPtr client) { - DCHECK(task_runner_->BelongsToCurrentThread()); - DCHECK(client_); - client_->ResolveDns(request_info.Pass(), client.Pass()); - } - - scoped_refptr task_runner_; - base::WeakPtr client_; - HostResolverMojo host_resolver_; -}; - -} // namespace net - -#endif // NET_PROXY_MOJO_PROXY_RESOLVER_V8_TRACING_BINDINGS_H_ diff --git a/net/proxy/mojo_proxy_resolver_v8_tracing_bindings_unittest.cc b/net/proxy/mojo_proxy_resolver_v8_tracing_bindings_unittest.cc deleted file mode 100644 index c1af7a3d8526..000000000000 --- a/net/proxy/mojo_proxy_resolver_v8_tracing_bindings_unittest.cc +++ /dev/null @@ -1,91 +0,0 @@ -// Copyright 2015 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "net/proxy/mojo_proxy_resolver_v8_tracing_bindings.h" - -#include -#include -#include - -#include "base/strings/utf_string_conversions.h" -#include "base/threading/thread.h" -#include "net/test/event_waiter.h" -#include "testing/gtest/include/gtest/gtest.h" - -namespace net { - -class MojoProxyResolverV8TracingBindingsTest : public testing::Test { - public: - enum Event { - EVENT_ALERT, - EVENT_ERROR, - }; - MojoProxyResolverV8TracingBindingsTest() : weak_factory_(this) {} - - void SetUp() override { - bindings_.reset(new MojoProxyResolverV8TracingBindings< - MojoProxyResolverV8TracingBindingsTest>(weak_factory_.GetWeakPtr())); - } - - void Alert(const mojo::String& message) { - alerts_.push_back(message.To()); - waiter_.NotifyEvent(EVENT_ALERT); - } - - void OnError(int32_t line_number, const mojo::String& message) { - errors_.push_back(std::make_pair(line_number, message.To())); - waiter_.NotifyEvent(EVENT_ERROR); - } - - void ResolveDns(interfaces::HostResolverRequestInfoPtr request_info, - interfaces::HostResolverRequestClientPtr client) {} - - void SendAlertAndError() { - bindings_->Alert(base::ASCIIToUTF16("alert")); - bindings_->OnError(-1, base::ASCIIToUTF16("error")); - } - - protected: - scoped_ptr> bindings_; - - EventWaiter waiter_; - - std::vector alerts_; - std::vector> errors_; - - base::WeakPtrFactory weak_factory_; -}; - -TEST_F(MojoProxyResolverV8TracingBindingsTest, Basic) { - SendAlertAndError(); - - EXPECT_TRUE(bindings_->GetHostResolver()); - EXPECT_FALSE(bindings_->GetBoundNetLog().net_log()); - - ASSERT_EQ(1u, alerts_.size()); - EXPECT_EQ("alert", alerts_[0]); - ASSERT_EQ(1u, errors_.size()); - EXPECT_EQ(-1, errors_[0].first); - EXPECT_EQ("error", errors_[0].second); -} - -TEST_F(MojoProxyResolverV8TracingBindingsTest, CalledFromOtherThread) { - base::Thread thread("alert and error thread"); - thread.Start(); - thread.message_loop()->PostTask( - FROM_HERE, - base::Bind(&MojoProxyResolverV8TracingBindingsTest::SendAlertAndError, - base::Unretained(this))); - - waiter_.WaitForEvent(EVENT_ERROR); - - ASSERT_EQ(1u, alerts_.size()); - EXPECT_EQ("alert", alerts_[0]); - ASSERT_EQ(1u, errors_.size()); - EXPECT_EQ(-1, errors_[0].first); - EXPECT_EQ("error", errors_[0].second); -} - -} // namespace net diff --git a/net/proxy/proxy_resolver_error_observer_mojo.cc b/net/proxy/proxy_resolver_error_observer_mojo.cc new file mode 100644 index 000000000000..a63d5d99ec13 --- /dev/null +++ b/net/proxy/proxy_resolver_error_observer_mojo.cc @@ -0,0 +1,46 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "net/proxy/proxy_resolver_error_observer_mojo.h" + +#include "base/bind.h" +#include "base/location.h" +#include "base/thread_task_runner_handle.h" +#include "mojo/common/common_type_converters.h" + +namespace net { + +// static +scoped_ptr ProxyResolverErrorObserverMojo::Create( + interfaces::ProxyResolverErrorObserverPtr error_observer) { + if (!error_observer) + return nullptr; + + return scoped_ptr( + new ProxyResolverErrorObserverMojo(error_observer.Pass())); +} + +void ProxyResolverErrorObserverMojo::OnPACScriptError( + int line_number, + const base::string16& error) { + if (!task_runner_->RunsTasksOnCurrentThread()) { + task_runner_->PostTask( + FROM_HERE, base::Bind(&ProxyResolverErrorObserverMojo::OnPACScriptError, + weak_this_, line_number, error)); + return; + } + error_observer_->OnPacScriptError(line_number, mojo::String::From(error)); +} + +ProxyResolverErrorObserverMojo::ProxyResolverErrorObserverMojo( + interfaces::ProxyResolverErrorObserverPtr error_observer) + : error_observer_(error_observer.Pass()), + task_runner_(base::ThreadTaskRunnerHandle::Get()), + weak_factory_(this) { + weak_this_ = weak_factory_.GetWeakPtr(); +} + +ProxyResolverErrorObserverMojo::~ProxyResolverErrorObserverMojo() = default; + +} // namespace net diff --git a/net/proxy/proxy_resolver_error_observer_mojo.h b/net/proxy/proxy_resolver_error_observer_mojo.h new file mode 100644 index 000000000000..95a8ff9b1542 --- /dev/null +++ b/net/proxy/proxy_resolver_error_observer_mojo.h @@ -0,0 +1,43 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef NET_PROXY_PROXY_RESOLVER_ERROR_OBSERVER_MOJO_H_ +#define NET_PROXY_PROXY_RESOLVER_ERROR_OBSERVER_MOJO_H_ + +#include "base/memory/ref_counted.h" +#include "base/memory/scoped_ptr.h" +#include "base/memory/weak_ptr.h" +#include "base/single_thread_task_runner.h" +#include "net/interfaces/proxy_resolver_service.mojom.h" +#include "net/proxy/proxy_resolver_error_observer.h" + +namespace net { + +// An implementation of ProxyResolverErrorObserver that forwards errors to an +// interfaces::ProxyResolverErrorObserver mojo interface. +class ProxyResolverErrorObserverMojo : public ProxyResolverErrorObserver { + public: + static scoped_ptr Create( + interfaces::ProxyResolverErrorObserverPtr error_observer); + + void OnPACScriptError(int line_number, const base::string16& error) override; + + private: + explicit ProxyResolverErrorObserverMojo( + interfaces::ProxyResolverErrorObserverPtr error_observer); + ~ProxyResolverErrorObserverMojo() override; + + // |error_observer_| may only be accessed when running on |task_runner_|. + interfaces::ProxyResolverErrorObserverPtr error_observer_; + scoped_refptr task_runner_; + + // Creating a new WeakPtr is only valid on the original thread, but copying an + // existing WeakPtr is valid on any thread so keep |weak_this_| ready to copy. + base::WeakPtr weak_this_; + base::WeakPtrFactory weak_factory_; +}; + +} // namespace net + +#endif // NET_PROXY_PROXY_RESOLVER_ERROR_OBSERVER_MOJO_H_ diff --git a/net/proxy/proxy_resolver_error_observer_mojo_unittest.cc b/net/proxy/proxy_resolver_error_observer_mojo_unittest.cc new file mode 100644 index 000000000000..3e39e755701a --- /dev/null +++ b/net/proxy/proxy_resolver_error_observer_mojo_unittest.cc @@ -0,0 +1,105 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "net/proxy/proxy_resolver_error_observer_mojo.h" + +#include +#include + +#include "base/bind.h" +#include "base/location.h" +#include "base/single_thread_task_runner.h" +#include "base/strings/utf_string_conversions.h" +#include "base/threading/thread.h" +#include "mojo/common/common_type_converters.h" +#include "net/test/event_waiter.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace net { +namespace { + +class ErrorObserverClient : public interfaces::ProxyResolverErrorObserver { + public: + enum Event { + ERROR_RECEIVED, + }; + + explicit ErrorObserverClient( + mojo::InterfaceRequest request); + + EventWaiter& event_waiter() { return event_waiter_; } + const std::vector>& errors() const { + return errors_; + } + + private: + void OnPacScriptError(int32_t line_number, + const mojo::String& error) override; + + mojo::Binding binding_; + EventWaiter event_waiter_; + std::vector> errors_; + + DISALLOW_COPY_AND_ASSIGN(ErrorObserverClient); +}; + +ErrorObserverClient::ErrorObserverClient( + mojo::InterfaceRequest request) + : binding_(this, request.Pass()) { +} + +void ErrorObserverClient::OnPacScriptError(int32_t line_number, + const mojo::String& error) { + errors_.push_back(std::make_pair(line_number, error.To())); + event_waiter_.NotifyEvent(ERROR_RECEIVED); +} + +} // namespace + +class ProxyResolverErrorObserverMojoTest : public testing::Test { + public: + ProxyResolverErrorObserver& error_observer() { return *error_observer_; } + ErrorObserverClient& client() { return *client_; } + + private: + void SetUp() override { + interfaces::ProxyResolverErrorObserverPtr error_observer_ptr; + client_.reset(new ErrorObserverClient(mojo::GetProxy(&error_observer_ptr))); + error_observer_ = + ProxyResolverErrorObserverMojo::Create(error_observer_ptr.Pass()); + ASSERT_TRUE(error_observer_); + } + + scoped_ptr client_; + scoped_ptr error_observer_; +}; + +TEST_F(ProxyResolverErrorObserverMojoTest, NullHandle) { + EXPECT_FALSE(ProxyResolverErrorObserverMojo::Create( + interfaces::ProxyResolverErrorObserverPtr())); +} + +TEST_F(ProxyResolverErrorObserverMojoTest, ErrorReportedOnMainThread) { + base::string16 error(base::ASCIIToUTF16("error message")); + error_observer().OnPACScriptError(123, error); + client().event_waiter().WaitForEvent(ErrorObserverClient::ERROR_RECEIVED); + ASSERT_EQ(1u, client().errors().size()); + EXPECT_EQ(123, client().errors()[0].first); + EXPECT_EQ(error, client().errors()[0].second); +} + +TEST_F(ProxyResolverErrorObserverMojoTest, ErrorReportedOnAnotherThread) { + base::Thread other_thread("error reporting thread"); + base::string16 error(base::ASCIIToUTF16("error message")); + other_thread.Start(); + other_thread.task_runner()->PostTask( + FROM_HERE, base::Bind(&ProxyResolverErrorObserver::OnPACScriptError, + base::Unretained(&error_observer()), 123, error)); + client().event_waiter().WaitForEvent(ErrorObserverClient::ERROR_RECEIVED); + ASSERT_EQ(1u, client().errors().size()); + EXPECT_EQ(123, client().errors()[0].first); + EXPECT_EQ(error, client().errors()[0].second); +} + +} // namespace net diff --git a/net/proxy/proxy_resolver_factory_mojo.cc b/net/proxy/proxy_resolver_factory_mojo.cc index 32c1203e19b0..a1ff691626ff 100644 --- a/net/proxy/proxy_resolver_factory_mojo.cc +++ b/net/proxy/proxy_resolver_factory_mojo.cc @@ -10,7 +10,6 @@ #include "base/logging.h" #include "base/stl_util.h" #include "base/threading/thread_checker.h" -#include "base/values.h" #include "mojo/common/common_type_converters.h" #include "mojo/common/url_type_converters.h" #include "net/base/load_states.h" @@ -29,67 +28,37 @@ namespace net { namespace { -scoped_ptr NetLogErrorCallback( - int line_number, - const base::string16* message, - NetLogCaptureMode /* capture_mode */) { - scoped_ptr dict(new base::DictionaryValue()); - dict->SetInteger("line_number", line_number); - dict->SetString("message", *message); - return dict.Pass(); -} - -// A mixin that forwards logging to (Bound)NetLog and ProxyResolverErrorObserver -// and DNS requests to a MojoHostResolverImpl, which is implemented in terms of -// a HostResolver. -template -class ClientMixin : public ClientInterface { +class ErrorObserverHolder : public interfaces::ProxyResolverErrorObserver { public: - ClientMixin(HostResolver* host_resolver, - ProxyResolverErrorObserver* error_observer, - NetLog* net_log, - const BoundNetLog& bound_net_log) - : host_resolver_(host_resolver, bound_net_log), - error_observer_(error_observer), - net_log_(net_log), - bound_net_log_(bound_net_log) {} - - // Overridden from ClientInterface: - void Alert(const mojo::String& message) override { - base::string16 message_str = message.To(); - auto callback = NetLog::StringCallback("message", &message_str); - bound_net_log_.AddEvent(NetLog::TYPE_PAC_JAVASCRIPT_ALERT, callback); - if (net_log_) - net_log_->AddGlobalEntry(NetLog::TYPE_PAC_JAVASCRIPT_ALERT, callback); - } + ErrorObserverHolder( + scoped_ptr error_observer, + mojo::InterfaceRequest request); + ~ErrorObserverHolder() override; - void OnError(int32_t line_number, const mojo::String& message) override { - base::string16 message_str = message.To(); - auto callback = base::Bind(&NetLogErrorCallback, line_number, &message_str); - bound_net_log_.AddEvent(NetLog::TYPE_PAC_JAVASCRIPT_ERROR, callback); - if (net_log_) - net_log_->AddGlobalEntry(NetLog::TYPE_PAC_JAVASCRIPT_ERROR, callback); - if (error_observer_) - error_observer_->OnPACScriptError(line_number, message_str); - } - - void ResolveDns(interfaces::HostResolverRequestInfoPtr request_info, - interfaces::HostResolverRequestClientPtr client) override { - host_resolver_.Resolve(request_info.Pass(), client.Pass()); - } - - protected: - bool dns_request_in_progress() { - return host_resolver_.request_in_progress(); - } + void OnPacScriptError(int32_t line_number, + const mojo::String& error) override; private: - MojoHostResolverImpl host_resolver_; - ProxyResolverErrorObserver* const error_observer_; - NetLog* const net_log_; - const BoundNetLog bound_net_log_; + scoped_ptr error_observer_; + mojo::Binding binding_; + + DISALLOW_COPY_AND_ASSIGN(ErrorObserverHolder); }; +ErrorObserverHolder::ErrorObserverHolder( + scoped_ptr error_observer, + mojo::InterfaceRequest request) + : error_observer_(error_observer.Pass()), binding_(this, request.Pass()) { +} + +ErrorObserverHolder::~ErrorObserverHolder() = default; + +void ErrorObserverHolder::OnPacScriptError(int32_t line_number, + const mojo::String& error) { + DCHECK(error_observer_); + error_observer_->OnPACScriptError(line_number, error.To()); +} + // Implementation of ProxyResolver that connects to a Mojo service to evaluate // PAC scripts. This implementation only knows about Mojo services, and // therefore that service may live in or out of process. @@ -104,12 +73,13 @@ class ProxyResolverMojo : public ProxyResolver { // |host_resolver| as the DNS resolver, using |host_resolver_binding| to // communicate with it. When deleted, the closure contained within // |on_delete_callback_runner| will be run. + // TODO(amistry): Add NetLog. ProxyResolverMojo( interfaces::ProxyResolverPtr resolver_ptr, - HostResolver* host_resolver, + scoped_ptr host_resolver, + scoped_ptr> host_resolver_binding, scoped_ptr on_delete_callback_runner, - scoped_ptr error_observer, - NetLog* net_log); + scoped_ptr error_observer); ~ProxyResolverMojo() override; // ProxyResolver implementation: @@ -132,11 +102,12 @@ class ProxyResolverMojo : public ProxyResolver { // Connection to the Mojo proxy resolver. interfaces::ProxyResolverPtr mojo_proxy_resolver_ptr_; - HostResolver* host_resolver_; + // Mojo host resolver service and binding. + scoped_ptr mojo_host_resolver_; + scoped_ptr> + mojo_host_resolver_binding_; - scoped_ptr error_observer_; - - NetLog* net_log_; + scoped_ptr error_observer_; std::set pending_jobs_; @@ -147,21 +118,19 @@ class ProxyResolverMojo : public ProxyResolver { DISALLOW_COPY_AND_ASSIGN(ProxyResolverMojo); }; -class ProxyResolverMojo::Job - : public ClientMixin { +class ProxyResolverMojo::Job : public interfaces::ProxyResolverRequestClient { public: Job(ProxyResolverMojo* resolver, const GURL& url, ProxyInfo* results, - const CompletionCallback& callback, - const BoundNetLog& net_log); + const CompletionCallback& callback); ~Job() override; // Cancels the job and prevents the callback from being run. void Cancel(); // Returns the LoadState of this job. - LoadState GetLoadState(); + LoadState load_state() { return LOAD_STATE_RESOLVING_PROXY_FOR_URL; } private: // Mojo error handler. @@ -184,14 +153,8 @@ class ProxyResolverMojo::Job ProxyResolverMojo::Job::Job(ProxyResolverMojo* resolver, const GURL& url, ProxyInfo* results, - const CompletionCallback& callback, - const BoundNetLog& net_log) - : ClientMixin( - resolver->host_resolver_, - resolver->error_observer_.get(), - resolver->net_log_, - net_log), - resolver_(resolver), + const CompletionCallback& callback) + : resolver_(resolver), url_(url), results_(results), callback_(callback), @@ -217,11 +180,6 @@ void ProxyResolverMojo::Job::Cancel() { callback_.Reset(); } -LoadState ProxyResolverMojo::Job::GetLoadState() { - return dns_request_in_progress() ? LOAD_STATE_RESOLVING_HOST_IN_PROXY_SCRIPT - : LOAD_STATE_RESOLVING_PROXY_FOR_URL; -} - void ProxyResolverMojo::Job::OnConnectionError() { DCHECK(thread_checker_.CalledOnValidThread()); DVLOG(1) << "ProxyResolverMojo::Job::OnConnectionError"; @@ -247,14 +205,14 @@ void ProxyResolverMojo::Job::ReportResult( ProxyResolverMojo::ProxyResolverMojo( interfaces::ProxyResolverPtr resolver_ptr, - HostResolver* host_resolver, + scoped_ptr host_resolver, + scoped_ptr> host_resolver_binding, scoped_ptr on_delete_callback_runner, - scoped_ptr error_observer, - NetLog* net_log) + scoped_ptr error_observer) : mojo_proxy_resolver_ptr_(resolver_ptr.Pass()), - host_resolver_(host_resolver), + mojo_host_resolver_(host_resolver.Pass()), + mojo_host_resolver_binding_(host_resolver_binding.Pass()), error_observer_(error_observer.Pass()), - net_log_(net_log), on_delete_callback_runner_(on_delete_callback_runner.Pass()) { mojo_proxy_resolver_ptr_.set_connection_error_handler(base::Bind( &ProxyResolverMojo::OnConnectionError, base::Unretained(this))); @@ -291,7 +249,7 @@ int ProxyResolverMojo::GetProxyForURL(const GURL& url, if (!mojo_proxy_resolver_ptr_) return ERR_PAC_SCRIPT_TERMINATED; - Job* job = new Job(this, url, results, callback, net_log); + Job* job = new Job(this, url, results, callback); bool inserted = pending_jobs_.insert(job).second; DCHECK(inserted); *request = job; @@ -310,40 +268,42 @@ void ProxyResolverMojo::CancelRequest(RequestHandle request) { LoadState ProxyResolverMojo::GetLoadState(RequestHandle request) const { Job* job = static_cast(request); CHECK_EQ(1u, pending_jobs_.count(job)); - return job->GetLoadState(); + return job->load_state(); } } // namespace -// A Job to create a ProxyResolver instance. -// -// Note: a Job instance is not tied to a particular resolve request, and hence -// there is no per-request logging to be done (any netlog events are only sent -// globally) so this always uses an empty BoundNetLog. class ProxyResolverFactoryMojo::Job - : public ClientMixin, + : public interfaces::ProxyResolverFactoryRequestClient, public ProxyResolverFactory::Request { public: Job(ProxyResolverFactoryMojo* factory, const scoped_refptr& pac_script, scoped_ptr* resolver, - const CompletionCallback& callback, - scoped_ptr error_observer) - : ClientMixin( - factory->host_resolver_, - error_observer.get(), - factory->net_log_, - BoundNetLog()), - factory_(factory), + const CompletionCallback& callback) + : factory_(factory), resolver_(resolver), callback_(callback), binding_(this), - error_observer_(error_observer.Pass()) { + host_resolver_(new MojoHostResolverImpl(factory_->host_resolver_)), + host_resolver_binding_( + new mojo::Binding(host_resolver_.get())) { + interfaces::HostResolverPtr host_resolver_ptr; interfaces::ProxyResolverFactoryRequestClientPtr client_ptr; + interfaces::ProxyResolverErrorObserverPtr error_observer_ptr; binding_.Bind(mojo::GetProxy(&client_ptr)); + if (!factory_->error_observer_factory_.is_null()) { + scoped_ptr error_observer = + factory_->error_observer_factory_.Run(); + if (error_observer) { + error_observer_.reset(new ErrorObserverHolder( + error_observer.Pass(), mojo::GetProxy(&error_observer_ptr))); + } + } + host_resolver_binding_->Bind(mojo::GetProxy(&host_resolver_ptr)); on_delete_callback_runner_ = factory_->mojo_proxy_factory_->CreateResolver( mojo::String::From(pac_script->utf16()), mojo::GetProxy(&resolver_ptr_), - client_ptr.Pass()); + host_resolver_ptr.Pass(), error_observer_ptr.Pass(), client_ptr.Pass()); resolver_ptr_.set_connection_error_handler( base::Bind(&ProxyResolverFactoryMojo::Job::OnConnectionError, base::Unretained(this))); @@ -359,10 +319,10 @@ class ProxyResolverFactoryMojo::Job resolver_ptr_.set_connection_error_handler(mojo::Closure()); binding_.set_connection_error_handler(mojo::Closure()); if (error == OK) { - resolver_->reset( - new ProxyResolverMojo(resolver_ptr_.Pass(), factory_->host_resolver_, - on_delete_callback_runner_.Pass(), - error_observer_.Pass(), factory_->net_log_)); + resolver_->reset(new ProxyResolverMojo( + resolver_ptr_.Pass(), host_resolver_.Pass(), + host_resolver_binding_.Pass(), on_delete_callback_runner_.Pass(), + error_observer_.Pass())); } on_delete_callback_runner_.reset(); callback_.Run(error); @@ -373,21 +333,21 @@ class ProxyResolverFactoryMojo::Job const CompletionCallback callback_; interfaces::ProxyResolverPtr resolver_ptr_; mojo::Binding binding_; + scoped_ptr host_resolver_; + scoped_ptr> host_resolver_binding_; scoped_ptr on_delete_callback_runner_; - scoped_ptr error_observer_; + scoped_ptr error_observer_; }; ProxyResolverFactoryMojo::ProxyResolverFactoryMojo( MojoProxyResolverFactory* mojo_proxy_factory, HostResolver* host_resolver, const base::Callback()>& - error_observer_factory, - NetLog* net_log) + error_observer_factory) : ProxyResolverFactory(true), mojo_proxy_factory_(mojo_proxy_factory), host_resolver_(host_resolver), - error_observer_factory_(error_observer_factory), - net_log_(net_log) { + error_observer_factory_(error_observer_factory) { } ProxyResolverFactoryMojo::~ProxyResolverFactoryMojo() = default; @@ -403,10 +363,7 @@ int ProxyResolverFactoryMojo::CreateProxyResolver( pac_script->utf16().empty()) { return ERR_PAC_SCRIPT_FAILED; } - request->reset(new Job(this, pac_script, resolver, callback, - error_observer_factory_.is_null() - ? nullptr - : error_observer_factory_.Run())); + request->reset(new Job(this, pac_script, resolver, callback)); return ERR_IO_PENDING; } diff --git a/net/proxy/proxy_resolver_factory_mojo.h b/net/proxy/proxy_resolver_factory_mojo.h index 65e4866a99b4..8d12e5b23112 100644 --- a/net/proxy/proxy_resolver_factory_mojo.h +++ b/net/proxy/proxy_resolver_factory_mojo.h @@ -13,10 +13,9 @@ namespace net { class HostResolver; -class MojoProxyResolverFactory; -class NetLog; class ProxyResolverErrorObserver; class ProxyResolverScriptData; +class MojoProxyResolverFactory; // Implementation of ProxyResolverFactory that connects to a Mojo service to // create implementations of a Mojo proxy resolver to back a ProxyResolverMojo. @@ -26,8 +25,7 @@ class ProxyResolverFactoryMojo : public ProxyResolverFactory { MojoProxyResolverFactory* mojo_proxy_factory, HostResolver* host_resolver, const base::Callback()>& - error_observer_factory, - NetLog* net_log); + error_observer_factory); ~ProxyResolverFactoryMojo() override; // ProxyResolverFactory override. @@ -44,7 +42,6 @@ class ProxyResolverFactoryMojo : public ProxyResolverFactory { HostResolver* const host_resolver_; const base::Callback()> error_observer_factory_; - NetLog* const net_log_; DISALLOW_COPY_AND_ASSIGN(ProxyResolverFactoryMojo); }; diff --git a/net/proxy/proxy_resolver_factory_mojo_unittest.cc b/net/proxy/proxy_resolver_factory_mojo_unittest.cc index d0fd4ec1e591..0c3dbabfcc7b 100644 --- a/net/proxy/proxy_resolver_factory_mojo_unittest.cc +++ b/net/proxy/proxy_resolver_factory_mojo_unittest.cc @@ -14,20 +14,17 @@ #include "base/memory/scoped_vector.h" #include "base/run_loop.h" #include "base/stl_util.h" -#include "base/values.h" #include "mojo/common/common_type_converters.h" #include "net/base/load_states.h" #include "net/base/net_errors.h" #include "net/base/test_completion_callback.h" -#include "net/dns/host_resolver.h" -#include "net/log/test_net_log.h" +#include "net/log/net_log.h" #include "net/proxy/mojo_proxy_resolver_factory.h" #include "net/proxy/mojo_proxy_type_converters.h" #include "net/proxy/proxy_info.h" #include "net/proxy/proxy_resolver.h" #include "net/proxy/proxy_resolver_error_observer.h" #include "net/proxy/proxy_resolver_script_data.h" -#include "net/test/event_waiter.h" #include "testing/gtest/include/gtest/gtest.h" #include "third_party/mojo/src/mojo/public/cpp/bindings/binding.h" #include "url/gurl.h" @@ -46,7 +43,6 @@ struct CreateProxyResolverAction { DROP_RESOLVER, DROP_BOTH, WAIT_FOR_CLIENT_DISCONNECT, - MAKE_DNS_REQUEST, }; static CreateProxyResolverAction ReturnResult( @@ -90,14 +86,6 @@ struct CreateProxyResolverAction { return result; } - static CreateProxyResolverAction MakeDnsRequest( - const std::string& expected_pac_script) { - CreateProxyResolverAction result; - result.expected_pac_script = expected_pac_script; - result.action = MAKE_DNS_REQUEST; - return result; - } - std::string expected_pac_script; Action action = COMPLETE; Error error = OK; @@ -112,8 +100,6 @@ struct GetProxyForUrlAction { DISCONNECT, // Wait for the client pipe to be disconnected. WAIT_FOR_CLIENT_DISCONNECT, - // Make a DNS request. - MAKE_DNS_REQUEST, }; GetProxyForUrlAction() {} @@ -161,13 +147,6 @@ struct GetProxyForUrlAction { return result; } - static GetProxyForUrlAction MakeDnsRequest(const GURL& url) { - GetProxyForUrlAction result; - result.expected_url = url; - result.action = MAKE_DNS_REQUEST; - return result; - } - Action action = COMPLETE; Error error = OK; mojo::Array proxy_servers; @@ -248,37 +227,19 @@ void MockMojoProxyResolver::GetProxyForUrl( get_proxy_actions_.pop(); EXPECT_EQ(action.expected_url.spec(), url.To()); - client->Alert(url); - client->OnError(12345, url); switch (action.action) { - case GetProxyForUrlAction::COMPLETE: { + case GetProxyForUrlAction::COMPLETE: client->ReportResult(action.error, action.proxy_servers.Pass()); break; - } - case GetProxyForUrlAction::DROP: { + case GetProxyForUrlAction::DROP: client.reset(); break; - } - case GetProxyForUrlAction::DISCONNECT: { + case GetProxyForUrlAction::DISCONNECT: binding_.Close(); break; - } - case GetProxyForUrlAction::WAIT_FOR_CLIENT_DISCONNECT: { + case GetProxyForUrlAction::WAIT_FOR_CLIENT_DISCONNECT: ASSERT_FALSE(client.WaitForIncomingResponse()); break; - } - case GetProxyForUrlAction::MAKE_DNS_REQUEST: { - interfaces::HostResolverRequestInfoPtr request( - interfaces::HostResolverRequestInfo::New()); - request->host = url; - request->port = 12345; - interfaces::HostResolverRequestClientPtr dns_client; - mojo::GetProxy(&dns_client); - client->ResolveDns(request.Pass(), dns_client.Pass()); - blocked_clients_.push_back( - new interfaces::ProxyResolverRequestClientPtr(client.Pass())); - break; - } } WakeWaiter(); } @@ -293,8 +254,6 @@ class Request { int error() const { return error_; } const ProxyInfo& results() const { return results_; } - LoadState load_state() { return resolver_->GetLoadState(handle_); } - BoundTestNetLog& net_log() { return net_log_; } private: ProxyResolver* resolver_; @@ -303,7 +262,6 @@ class Request { ProxyResolver::RequestHandle handle_; int error_; TestCompletionCallback callback_; - BoundTestNetLog net_log_; }; Request::Request(ProxyResolver* resolver, const GURL& url) @@ -311,8 +269,9 @@ Request::Request(ProxyResolver* resolver, const GURL& url) } int Request::Resolve() { + BoundNetLog net_log; error_ = resolver_->GetProxyForURL(url_, &results_, callback_.callback(), - &handle_, net_log_.bound()); + &handle_, net_log); return error_; } @@ -343,6 +302,8 @@ class MockMojoProxyResolverFactory : public interfaces::ProxyResolverFactory { void CreateResolver( const mojo::String& pac_url, mojo::InterfaceRequest request, + interfaces::HostResolverPtr host_resolver, + interfaces::ProxyResolverErrorObserverPtr error_observer, interfaces::ProxyResolverFactoryRequestClientPtr client) override; void WakeWaiter(); @@ -394,54 +355,37 @@ void MockMojoProxyResolverFactory::ClearBlockedClients() { void MockMojoProxyResolverFactory::CreateResolver( const mojo::String& pac_script, mojo::InterfaceRequest request, + interfaces::HostResolverPtr host_resolver, + interfaces::ProxyResolverErrorObserverPtr error_observer, interfaces::ProxyResolverFactoryRequestClientPtr client) { ASSERT_FALSE(create_resolver_actions_.empty()); CreateProxyResolverAction action = create_resolver_actions_.front(); create_resolver_actions_.pop(); EXPECT_EQ(action.expected_pac_script, pac_script.To()); - client->Alert(pac_script); - client->OnError(12345, pac_script); switch (action.action) { - case CreateProxyResolverAction::COMPLETE: { + case CreateProxyResolverAction::COMPLETE: if (action.error == OK) resolver_->AddConnection(request.Pass()); client->ReportResult(action.error); break; - } - case CreateProxyResolverAction::DROP_CLIENT: { + case CreateProxyResolverAction::DROP_CLIENT: // Save |request| so its pipe isn't closed. blocked_resolver_requests_.push_back( new mojo::InterfaceRequest( request.Pass())); break; - } - case CreateProxyResolverAction::DROP_RESOLVER: { + case CreateProxyResolverAction::DROP_RESOLVER: // Save |client| so its pipe isn't closed. blocked_clients_.push_back( new interfaces::ProxyResolverFactoryRequestClientPtr(client.Pass())); break; - } - case CreateProxyResolverAction::DROP_BOTH: { + case CreateProxyResolverAction::DROP_BOTH: // Both |request| and |client| will be closed. break; - } - case CreateProxyResolverAction::WAIT_FOR_CLIENT_DISCONNECT: { + case CreateProxyResolverAction::WAIT_FOR_CLIENT_DISCONNECT: ASSERT_FALSE(client.WaitForIncomingResponse()); break; - } - case CreateProxyResolverAction::MAKE_DNS_REQUEST: { - interfaces::HostResolverRequestInfoPtr request( - interfaces::HostResolverRequestInfo::New()); - request->host = pac_script; - request->port = 12345; - interfaces::HostResolverRequestClientPtr dns_client; - mojo::GetProxy(&dns_client); - client->ResolveDns(request.Pass(), dns_client.Pass()); - blocked_clients_.push_back( - new interfaces::ProxyResolverFactoryRequestClientPtr(client.Pass())); - break; - } } WakeWaiter(); } @@ -456,53 +400,6 @@ void DeleteResolverFactoryRequestCallback( callback.Run(result); } -class MockHostResolver : public HostResolver { - public: - enum Event { - DNS_REQUEST, - }; - - // HostResolver overrides. - int Resolve(const RequestInfo& info, - RequestPriority priority, - AddressList* addresses, - const CompletionCallback& callback, - RequestHandle* request_handle, - const BoundNetLog& source_net_log) override { - waiter_.NotifyEvent(DNS_REQUEST); - return ERR_IO_PENDING; - } - int ResolveFromCache(const RequestInfo& info, - AddressList* addresses, - const BoundNetLog& source_net_log) override { - return ERR_DNS_CACHE_MISS; - } - void CancelRequest(RequestHandle req) override {} - HostCache* GetHostCache() override { return nullptr; } - - EventWaiter& waiter() { return waiter_; } - - private: - EventWaiter waiter_; -}; - -void CheckCapturedNetLogEntries(const std::string& expected_string, - const TestNetLogEntry::List& entries) { - ASSERT_EQ(2u, entries.size()); - EXPECT_EQ(NetLog::TYPE_PAC_JAVASCRIPT_ALERT, entries[0].type); - std::string message; - ASSERT_TRUE(entries[0].GetStringValue("message", &message)); - EXPECT_EQ(expected_string, message); - ASSERT_FALSE(entries[0].params->HasKey("line_number")); - message.clear(); - EXPECT_EQ(NetLog::TYPE_PAC_JAVASCRIPT_ERROR, entries[1].type); - ASSERT_TRUE(entries[1].GetStringValue("message", &message)); - EXPECT_EQ(expected_string, message); - int line_number = 0; - ASSERT_TRUE(entries[1].GetIntegerValue("line_number", &line_number)); - EXPECT_EQ(12345, line_number); -} - } // namespace class ProxyResolverFactoryMojoTest : public testing::Test, @@ -512,8 +409,8 @@ class ProxyResolverFactoryMojoTest : public testing::Test, mock_proxy_resolver_factory_.reset(new MockMojoProxyResolverFactory( &mock_proxy_resolver_, mojo::GetProxy(&factory_ptr_))); proxy_resolver_factory_mojo_.reset(new ProxyResolverFactoryMojo( - this, &host_resolver_, - base::Callback()>(), &net_log_)); + this, nullptr, + base::Callback()>())); } scoped_ptr MakeRequest(const GURL& url) { @@ -523,8 +420,11 @@ class ProxyResolverFactoryMojoTest : public testing::Test, scoped_ptr CreateResolver( const mojo::String& pac_script, mojo::InterfaceRequest req, + interfaces::HostResolverPtr host_resolver, + interfaces::ProxyResolverErrorObserverPtr error_observer, interfaces::ProxyResolverFactoryRequestClientPtr client) override { - factory_ptr_->CreateResolver(pac_script, req.Pass(), client.Pass()); + factory_ptr_->CreateResolver(pac_script, req.Pass(), host_resolver.Pass(), + error_observer.Pass(), client.Pass()); return make_scoped_ptr( new base::ScopedClosureRunner(on_delete_callback_.closure())); } @@ -559,8 +459,6 @@ class ProxyResolverFactoryMojoTest : public testing::Test, callback.Run(result); } - MockHostResolver host_resolver_; - TestNetLog net_log_; scoped_ptr mock_proxy_resolver_factory_; interfaces::ProxyResolverFactoryPtr factory_ptr_; scoped_ptr proxy_resolver_factory_mojo_; @@ -572,9 +470,6 @@ class ProxyResolverFactoryMojoTest : public testing::Test, TEST_F(ProxyResolverFactoryMojoTest, CreateProxyResolver) { CreateProxyResolver(); - TestNetLogEntry::List entries; - net_log_.GetEntries(&entries); - CheckCapturedNetLogEntries(kScriptData, entries); } TEST_F(ProxyResolverFactoryMojoTest, CreateProxyResolver_Empty) { @@ -705,42 +600,16 @@ TEST_F(ProxyResolverFactoryMojoTest, CreateProxyResolver_Cancel) { on_delete_callback_.WaitForResult(); } -TEST_F(ProxyResolverFactoryMojoTest, CreateProxyResolver_DnsRequest) { - mock_proxy_resolver_factory_->AddCreateProxyResolverAction( - CreateProxyResolverAction::MakeDnsRequest(kScriptData)); - - scoped_refptr pac_script( - ProxyResolverScriptData::FromUTF8(kScriptData)); - scoped_ptr request; - TestCompletionCallback callback; - EXPECT_EQ(ERR_IO_PENDING, proxy_resolver_factory_mojo_->CreateProxyResolver( - pac_script, &proxy_resolver_mojo_, - callback.callback(), &request)); - ASSERT_TRUE(request); - host_resolver_.waiter().WaitForEvent(MockHostResolver::DNS_REQUEST); - mock_proxy_resolver_factory_->ClearBlockedClients(); - callback.WaitForResult(); -} - TEST_F(ProxyResolverFactoryMojoTest, GetProxyForURL) { - const GURL url(kExampleUrl); mock_proxy_resolver_.AddGetProxyAction(GetProxyForUrlAction::ReturnServers( - url, ProxyServersFromPacString("DIRECT"))); + GURL(kExampleUrl), ProxyServersFromPacString("DIRECT"))); CreateProxyResolver(); - net_log_.Clear(); scoped_ptr request(MakeRequest(GURL(kExampleUrl))); EXPECT_EQ(ERR_IO_PENDING, request->Resolve()); EXPECT_EQ(OK, request->WaitForResult()); EXPECT_EQ("DIRECT", request->results().ToPacString()); - - TestNetLogEntry::List entries; - net_log_.GetEntries(&entries); - CheckCapturedNetLogEntries(url.spec(), entries); - entries.clear(); - request->net_log().GetEntries(&entries); - CheckCapturedNetLogEntries(url.spec(), entries); } TEST_F(ProxyResolverFactoryMojoTest, GetProxyForURL_MultipleResults) { @@ -871,21 +740,6 @@ TEST_F(ProxyResolverFactoryMojoTest, on_delete_callback_.WaitForResult(); } -TEST_F(ProxyResolverFactoryMojoTest, GetProxyForURL_DnsRequest) { - mock_proxy_resolver_.AddGetProxyAction( - GetProxyForUrlAction::MakeDnsRequest(GURL(kExampleUrl))); - CreateProxyResolver(); - - scoped_ptr request(MakeRequest(GURL(kExampleUrl))); - EXPECT_EQ(ERR_IO_PENDING, request->Resolve()); - EXPECT_EQ(LOAD_STATE_RESOLVING_PROXY_FOR_URL, request->load_state()); - - host_resolver_.waiter().WaitForEvent(MockHostResolver::DNS_REQUEST); - EXPECT_EQ(LOAD_STATE_RESOLVING_HOST_IN_PROXY_SCRIPT, request->load_state()); - mock_proxy_resolver_.ClearBlockedClients(); - request->WaitForResult(); -} - TEST_F(ProxyResolverFactoryMojoTest, DeleteResolver) { CreateProxyResolver(); proxy_resolver_mojo_.reset(); diff --git a/net/proxy/proxy_service_mojo.cc b/net/proxy/proxy_service_mojo.cc index 689b870e6de3..8939315853f1 100644 --- a/net/proxy/proxy_service_mojo.cc +++ b/net/proxy/proxy_service_mojo.cc @@ -37,8 +37,7 @@ ProxyService* CreateProxyServiceUsingMojoFactory( make_scoped_ptr(new ProxyResolverFactoryMojo( mojo_proxy_factory, host_resolver, base::Bind(&NetworkDelegateErrorObserver::Create, network_delegate, - base::ThreadTaskRunnerHandle::Get()), - net_log)), + base::ThreadTaskRunnerHandle::Get()))), net_log); // Configure fetchers to use for PAC script downloads and auto-detect. diff --git a/net/proxy/proxy_service_mojo_unittest.cc b/net/proxy/proxy_service_mojo_unittest.cc index 171f89be8c42..334bafaf142e 100644 --- a/net/proxy/proxy_service_mojo_unittest.cc +++ b/net/proxy/proxy_service_mojo_unittest.cc @@ -4,20 +4,16 @@ #include "net/proxy/proxy_service_mojo.h" -#include #include #include "base/callback_helpers.h" #include "base/memory/scoped_ptr.h" #include "base/strings/utf_string_conversions.h" -#include "base/values.h" #include "net/base/load_flags.h" #include "net/base/network_delegate_impl.h" #include "net/base/test_completion_callback.h" #include "net/dns/mock_host_resolver.h" #include "net/log/net_log.h" -#include "net/log/test_net_log.h" -#include "net/log/test_net_log_entry.h" #include "net/proxy/dhcp_proxy_script_fetcher.h" #include "net/proxy/in_process_mojo_proxy_resolver_factory.h" #include "net/proxy/mock_proxy_script_fetcher.h" @@ -25,7 +21,6 @@ #include "net/proxy/proxy_config_service_fixed.h" #include "net/proxy/proxy_service.h" #include "net/test/event_waiter.h" -#include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" #include "url/gurl.h" @@ -44,15 +39,10 @@ const char kDnsResolvePacScript[] = " return 'DIRECT';\n" " return 'QUIC bar:4321';\n" "}"; -const char kThrowingPacScript[] = +const char kErrorPacScript[] = "function FindProxyForURL(url, host) {\n" - " alert('alert: ' + host);\n" - " throw new Error('error: ' + url);\n" + " throw new Error('test error');\n" "}"; -const char kThrowingOnLoadPacScript[] = - "function FindProxyForURL(url, host) {}\n" - "alert('alert: foo');\n" - "throw new Error('error: http://foo');"; class TestNetworkDelegate : public NetworkDelegateImpl { public: @@ -71,52 +61,10 @@ class TestNetworkDelegate : public NetworkDelegateImpl { void TestNetworkDelegate::OnPACScriptError(int line_number, const base::string16& error) { event_waiter_.NotifyEvent(PAC_SCRIPT_ERROR); - EXPECT_EQ(3, line_number); - EXPECT_TRUE(base::UTF16ToUTF8(error).find("error: http://foo") != - std::string::npos); + EXPECT_EQ(2, line_number); + EXPECT_TRUE(base::UTF16ToUTF8(error).find("test error") != std::string::npos); } -void CheckCapturedNetLogEntries(const TestNetLogEntry::List& entries) { - ASSERT_GT(entries.size(), 2u); - size_t i = 0; - // ProxyService records its own NetLog entries, so skip forward until the - // expected event type. - while (i < entries.size() && - entries[i].type != NetLog::TYPE_PAC_JAVASCRIPT_ALERT) { - i++; - } - ASSERT_LT(i, entries.size()); - std::string message; - ASSERT_TRUE(entries[i].GetStringValue("message", &message)); - EXPECT_EQ("alert: foo", message); - ASSERT_FALSE(entries[i].params->HasKey("line_number")); - - while (i < entries.size() && - entries[i].type != NetLog::TYPE_PAC_JAVASCRIPT_ERROR) { - i++; - } - message.clear(); - ASSERT_TRUE(entries[i].GetStringValue("message", &message)); - EXPECT_THAT(message, testing::HasSubstr("error: http://foo")); - int line_number = 0; - ASSERT_TRUE(entries[i].GetIntegerValue("line_number", &line_number)); - EXPECT_EQ(3, line_number); -} - -class LoggingMockHostResolver : public MockHostResolver { - public: - int Resolve(const RequestInfo& info, - RequestPriority priority, - AddressList* addresses, - const CompletionCallback& callback, - RequestHandle* out_req, - const BoundNetLog& net_log) override { - net_log.AddEvent(NetLog::TYPE_HOST_RESOLVER_IMPL_JOB); - return MockHostResolver::Resolve(info, priority, addresses, callback, - out_req, net_log); - } -}; - } // namespace class ProxyServiceMojoTest : public testing::Test, @@ -130,23 +78,25 @@ class ProxyServiceMojoTest : public testing::Test, this, new ProxyConfigServiceFixed( ProxyConfig::CreateFromCustomPacURL(GURL(kPacUrl))), fetcher_, new DoNothingDhcpProxyScriptFetcher(), &mock_host_resolver_, - &net_log_, &network_delegate_)); + nullptr /* NetLog* */, &network_delegate_)); } scoped_ptr CreateResolver( const mojo::String& pac_script, mojo::InterfaceRequest req, + interfaces::HostResolverPtr host_resolver, + interfaces::ProxyResolverErrorObserverPtr error_observer, interfaces::ProxyResolverFactoryRequestClientPtr client) override { InProcessMojoProxyResolverFactory::GetInstance()->CreateResolver( - pac_script, req.Pass(), client.Pass()); + pac_script, req.Pass(), host_resolver.Pass(), error_observer.Pass(), + client.Pass()); return make_scoped_ptr( new base::ScopedClosureRunner(on_delete_closure_.closure())); } TestNetworkDelegate network_delegate_; - LoggingMockHostResolver mock_host_resolver_; + MockHostResolver mock_host_resolver_; MockProxyScriptFetcher* fetcher_; // Owned by |proxy_service_|. - TestNetLog net_log_; scoped_ptr proxy_service_; TestClosure on_delete_closure_; }; @@ -175,11 +125,10 @@ TEST_F(ProxyServiceMojoTest, Basic) { TEST_F(ProxyServiceMojoTest, DnsResolution) { ProxyInfo info; TestCompletionCallback callback; - BoundTestNetLog bound_net_log; EXPECT_EQ(ERR_IO_PENDING, proxy_service_->ResolveProxy(GURL("http://foo"), LOAD_NORMAL, &info, callback.callback(), nullptr, nullptr, - bound_net_log.bound())); + BoundNetLog())); // Proxy script fetcher should have a fetch triggered by the first // |ResolveProxy()| request. @@ -192,48 +141,9 @@ TEST_F(ProxyServiceMojoTest, DnsResolution) { EXPECT_EQ(1u, mock_host_resolver_.num_resolve()); proxy_service_.reset(); on_delete_closure_.WaitForResult(); - - TestNetLogEntry::List entries; - bound_net_log.GetEntries(&entries); - // There should be one entry with type TYPE_HOST_RESOLVER_IMPL_JOB. - EXPECT_EQ(1, std::count_if(entries.begin(), entries.end(), - [](const TestNetLogEntry& entry) { - return entry.type == - NetLog::TYPE_HOST_RESOLVER_IMPL_JOB; - })); } TEST_F(ProxyServiceMojoTest, Error) { - ProxyInfo info; - TestCompletionCallback callback; - BoundTestNetLog bound_net_log; - EXPECT_EQ(ERR_IO_PENDING, - proxy_service_->ResolveProxy(GURL("http://foo"), LOAD_NORMAL, &info, - callback.callback(), nullptr, nullptr, - bound_net_log.bound())); - - // Proxy script fetcher should have a fetch triggered by the first - // |ResolveProxy()| request. - EXPECT_TRUE(fetcher_->has_pending_request()); - EXPECT_EQ(GURL(kPacUrl), fetcher_->pending_request_url()); - fetcher_->NotifyFetchCompletion(OK, kThrowingPacScript); - - network_delegate_.event_waiter().WaitForEvent( - TestNetworkDelegate::PAC_SCRIPT_ERROR); - - EXPECT_EQ(OK, callback.WaitForResult()); - EXPECT_EQ("DIRECT", info.ToPacString()); - EXPECT_EQ(0u, mock_host_resolver_.num_resolve()); - - TestNetLogEntry::List entries; - bound_net_log.GetEntries(&entries); - CheckCapturedNetLogEntries(entries); - entries.clear(); - net_log_.GetEntries(&entries); - CheckCapturedNetLogEntries(entries); -} - -TEST_F(ProxyServiceMojoTest, ErrorOnInitialization) { ProxyInfo info; TestCompletionCallback callback; EXPECT_EQ(ERR_IO_PENDING, @@ -245,18 +155,10 @@ TEST_F(ProxyServiceMojoTest, ErrorOnInitialization) { // |ResolveProxy()| request. EXPECT_TRUE(fetcher_->has_pending_request()); EXPECT_EQ(GURL(kPacUrl), fetcher_->pending_request_url()); - fetcher_->NotifyFetchCompletion(OK, kThrowingOnLoadPacScript); + fetcher_->NotifyFetchCompletion(OK, kErrorPacScript); network_delegate_.event_waiter().WaitForEvent( TestNetworkDelegate::PAC_SCRIPT_ERROR); - - EXPECT_EQ(OK, callback.WaitForResult()); - EXPECT_EQ("DIRECT", info.ToPacString()); - EXPECT_EQ(0u, mock_host_resolver_.num_resolve()); - - TestNetLogEntry::List entries; - net_log_.GetEntries(&entries); - CheckCapturedNetLogEntries(entries); } } // namespace net