Skip to content

Commit

Permalink
Revert of Change the mojo ProxyResolver to use ProxyResolverV8Tracing…
Browse files Browse the repository at this point in the history 
…. (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}

[email protected],[email protected],[email protected]
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=467832

Review URL: https://codereview.chromium.org/1224083003 .

Cr-Commit-Position: refs/heads/master@{#338019}
  • Loading branch information
@saittam @JulienIsorce
saittam authored and JulienIsorce committed Jul 17, 2015
1 parent cf8c4b5 commit 13b22cf
Show file tree
Hide file tree
Showing 32 changed files with 805 additions and 864 deletions.
6 changes: 5 additions & 1 deletion chrome/browser/net/utility_process_mojo_proxy_resolver_factory.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ scoped_ptr<base::ScopedClosureRunner>
UtilityProcessMojoProxyResolverFactory::CreateResolver(
const mojo::String& pac_script,
mojo::InterfaceRequest<net::interfaces::ProxyResolver> req,
net::interfaces::HostResolverPtr host_resolver,
net::interfaces::ProxyResolverErrorObserverPtr error_observer,
net::interfaces::ProxyResolverFactoryRequestClientPtr client) {
DCHECK(thread_checker_.CalledOnValidThread());
if (!resolver_factory_)
Expand All @@ -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))));
Expand Down
2 changes: 2 additions & 0 deletions chrome/browser/net/utility_process_mojo_proxy_resolver_factory.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ class UtilityProcessMojoProxyResolverFactory
scoped_ptr<base::ScopedClosureRunner> CreateResolver(
const mojo::String& pac_script,
mojo::InterfaceRequest<net::interfaces::ProxyResolver> req,
net::interfaces::HostResolverPtr host_resolver,
net::interfaces::ProxyResolverErrorObserverPtr error_observer,
net::interfaces::ProxyResolverFactoryRequestClientPtr client) override;

private:
Expand Down
5 changes: 3 additions & 2 deletions net/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Expand Down Expand Up @@ -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",
]
Expand Down
19 changes: 15 additions & 4 deletions net/dns/host_resolver_mojo.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,16 @@ class HostResolverMojo::Job : public interfaces::HostResolverRequestClient {
base::WeakPtr<HostCache> 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;
Expand All @@ -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;
}

Expand All @@ -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) {
Expand Down
27 changes: 15 additions & 12 deletions net/dns/host_resolver_mojo.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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.
Expand All @@ -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<HostCache> host_cache_;
base::WeakPtrFactory<HostCache> host_cache_weak_factory_;
Expand Down
53 changes: 44 additions & 9 deletions net/dns/host_resolver_mojo_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<interfaces::HostResolver> request,
const base::Closure& resolver_connection_error_callback,
const base::Closure& request_connection_error_callback);
~MockMojoHostResolver() override;

void AddAction(scoped_ptr<HostResolverAction> action);
Expand All @@ -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<interfaces::HostResolver> binding_;
ScopedVector<HostResolverAction> actions_;
size_t results_returned_ = 0;
mojo::Array<interfaces::HostResolverRequestInfoPtr> requests_received_;
const base::Closure resolver_connection_error_callback_;
const base::Closure request_connection_error_callback_;
ScopedVector<MockMojoHostResolverRequest> requests_;
};

MockMojoHostResolver::MockMojoHostResolver(
mojo::InterfaceRequest<interfaces::HostResolver> 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<HostResolverAction> 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());
Expand All @@ -145,15 +161,24 @@ void MockMojoHostResolver::ResolveDns(
class HostResolverMojoTest : public testing::Test {
protected:
enum class ConnectionErrorSource {
RESOLVER,
REQUEST,
CLIENT,
};
using Waiter = EventWaiter<ConnectionErrorSource>;

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,
Expand Down Expand Up @@ -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"));
Expand Down
14 changes: 5 additions & 9 deletions net/dns/mojo_host_resolver_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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() {
Expand All @@ -60,7 +57,7 @@ void MojoHostResolverImpl::Resolve(
DCHECK(thread_checker_.CalledOnValidThread());
Job* job = new Job(this, resolver_,
request_info->To<net::HostResolver::RequestInfo>(),
net_log_, client.Pass());
client.Pass());
pending_jobs_.insert(job);
job->Start();
}
Expand All @@ -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(
Expand All @@ -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);
Expand Down
29 changes: 12 additions & 17 deletions net/dns/mojo_host_resolver_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,40 +10,35 @@
#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;

// 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<Job*> pending_jobs_;
Expand Down
Loading

0 comments on commit 13b22cf

Please sign in to comment.