Always access ProxyResolverV8Tracing::Bindings from the origin thread.

If a ProxyResolverV8Tracing Job is cancelled, the Job may be deleted on
the worker thread. This causes the ProxyResolverV8Tracing::Bindings to
be deleted on the worker thread, violating the contract that it is
deleted on the origin thread. This changes the bindings interface to
only be accessed from the origin thread and deleted when the request
completes or is cancelled.

BUG=523908

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

Cr-Commit-Position: refs/heads/master@{#346349}

8 files changed, 114 insertions, 195 deletions

diff --git a/net/proxy/mojo_proxy_resolver_factory_impl.cc b/net/proxy/mojo_proxy_resolver_factory_impl.cc
index a913ce2..a24edc2 100644
--- a/net/proxy/mojo_proxy_resolver_factory_impl.cc
+++ b/net/proxy/mojo_proxy_resolver_factory_impl.cc
@@ -71,9 +71,6 @@ class MojoProxyResolverFactoryImpl::Job {
   scoped_ptr<net::ProxyResolverFactory::Request> request_;
   interfaces::ProxyResolverFactoryRequestClientPtr client_ptr_;
 
-  base::WeakPtrFactory<interfaces::ProxyResolverFactoryRequestClient>
-      weak_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(Job);
 };
 
@@ -86,15 +83,15 @@ MojoProxyResolverFactoryImpl::Job::Job(
     : parent_(factory),
       proxy_request_(request.Pass()),
       factory_(proxy_resolver_factory),
-      client_ptr_(client.Pass()),
-      weak_factory_(client_ptr_.get()) {
+      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<
+      pac_script,
+      make_scoped_ptr(new MojoProxyResolverV8TracingBindings<
                       interfaces::ProxyResolverFactoryRequestClient>(
-                      weak_factory_.GetWeakPtr())),
+          client_ptr_.get())),
       &proxy_resolver_impl_,
       base::Bind(&MojoProxyResolverFactoryImpl::Job::OnProxyResolverCreated,
                  base::Unretained(this)),
diff --git a/net/proxy/mojo_proxy_resolver_impl.cc b/net/proxy/mojo_proxy_resolver_impl.cc
index 9c6f0f1..c5ae034 100644
--- a/net/proxy/mojo_proxy_resolver_impl.cc
+++ b/net/proxy/mojo_proxy_resolver_impl.cc
@@ -40,8 +40,6 @@ class MojoProxyResolverImpl::Job {
   net::ProxyResolver::RequestHandle request_handle_;
   bool done_;
 
-  base::WeakPtrFactory<interfaces::ProxyResolverRequestClient> weak_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(Job);
 };
 
@@ -78,9 +76,7 @@ MojoProxyResolverImpl::Job::Job(
       client_(client.Pass()),
       url_(url),
       request_handle_(nullptr),
-      done_(false),
-      weak_factory_(client_.get()) {
-}
+      done_(false) {}
 
 MojoProxyResolverImpl::Job::~Job() {
   if (request_handle_ && !done_)
@@ -92,7 +88,7 @@ void MojoProxyResolverImpl::Job::Start() {
       url_, &result_, base::Bind(&Job::GetProxyDone, base::Unretained(this)),
       &request_handle_,
       make_scoped_ptr(new MojoProxyResolverV8TracingBindings<
-          interfaces::ProxyResolverRequestClient>(weak_factory_.GetWeakPtr())));
+                      interfaces::ProxyResolverRequestClient>(client_.get())));
   client_.set_connection_error_handler(base::Bind(
       &MojoProxyResolverImpl::Job::OnConnectionError, base::Unretained(this)));
 }
diff --git a/net/proxy/mojo_proxy_resolver_v8_tracing_bindings.h b/net/proxy/mojo_proxy_resolver_v8_tracing_bindings.h
index f8d7329..70ebe3c 100644
--- a/net/proxy/mojo_proxy_resolver_v8_tracing_bindings.h
+++ b/net/proxy/mojo_proxy_resolver_v8_tracing_bindings.h
@@ -5,9 +5,7 @@
 #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 "base/threading/thread_checker.h"
 #include "mojo/common/common_type_converters.h"
 #include "net/dns/host_resolver_mojo.h"
 #include "net/interfaces/proxy_resolver_service.mojom.h"
@@ -25,65 +23,42 @@ class MojoProxyResolverV8TracingBindings
     : public ProxyResolverV8Tracing::Bindings,
       public HostResolverMojo::Impl {
  public:
-  explicit MojoProxyResolverV8TracingBindings(base::WeakPtr<Client> client)
-      : task_runner_(base::ThreadTaskRunnerHandle::Get()),
-        client_(client),
-        host_resolver_(this) {}
+  explicit MojoProxyResolverV8TracingBindings(Client* client)
+      : client_(client), host_resolver_(this) {
+    DCHECK(client_);
+  }
 
   // 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);
+    DCHECK(thread_checker_.CalledOnValidThread());
+    client_->Alert(mojo::String::From(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);
+    DCHECK(thread_checker_.CalledOnValidThread());
+    client_->OnError(line_number, mojo::String::From(message));
   }
 
   HostResolver* GetHostResolver() override {
-    DCHECK(task_runner_->BelongsToCurrentThread());
+    DCHECK(thread_checker_.CalledOnValidThread());
     return &host_resolver_;
   }
 
-  BoundNetLog GetBoundNetLog() override { return BoundNetLog(); }
-
- private:
-  static void AlertOnTaskRunner(base::WeakPtr<Client> client,
-                                const base::string16& message) {
-    if (client)
-      client->Alert(mojo::String::From(message));
-  }
-
-  static void OnErrorOnTaskRunner(base::WeakPtr<Client> client,
-                                  int line_number,
-                                  const base::string16& message) {
-    if (client)
-      client->OnError(line_number, mojo::String::From(message));
+  BoundNetLog GetBoundNetLog() override {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    return BoundNetLog();
   }
 
+ private:
   // HostResolverMojo::Impl override.
   void ResolveDns(interfaces::HostResolverRequestInfoPtr request_info,
                   interfaces::HostResolverRequestClientPtr client) {
-    DCHECK(task_runner_->BelongsToCurrentThread());
-    DCHECK(client_);
+    DCHECK(thread_checker_.CalledOnValidThread());
     client_->ResolveDns(request_info.Pass(), client.Pass());
   }
 
-  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
-  base::WeakPtr<Client> client_;
+  base::ThreadChecker thread_checker_;
+  Client* client_;
   HostResolverMojo host_resolver_;
 };
 
diff --git a/net/proxy/mojo_proxy_resolver_v8_tracing_bindings_unittest.cc b/net/proxy/mojo_proxy_resolver_v8_tracing_bindings_unittest.cc
index c1af7a3..ce11b71 100644
--- a/net/proxy/mojo_proxy_resolver_v8_tracing_bindings_unittest.cc
+++ b/net/proxy/mojo_proxy_resolver_v8_tracing_bindings_unittest.cc
@@ -9,57 +9,44 @@
 #include <vector>
 
 #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) {}
+  MojoProxyResolverV8TracingBindingsTest() = default;
 
   void SetUp() override {
     bindings_.reset(new MojoProxyResolverV8TracingBindings<
-        MojoProxyResolverV8TracingBindingsTest>(weak_factory_.GetWeakPtr()));
+                    MojoProxyResolverV8TracingBindingsTest>(this));
   }
 
   void Alert(const mojo::String& message) {
     alerts_.push_back(message.To<std::string>());
-    waiter_.NotifyEvent(EVENT_ALERT);
   }
 
   void OnError(int32_t line_number, const mojo::String& message) {
     errors_.push_back(std::make_pair(line_number, message.To<std::string>()));
-    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<MojoProxyResolverV8TracingBindings<
       MojoProxyResolverV8TracingBindingsTest>> bindings_;
 
-  EventWaiter<Event> waiter_;
-
   std::vector<std::string> alerts_;
   std::vector<std::pair<int, std::string>> errors_;
 
-  base::WeakPtrFactory<MojoProxyResolverV8TracingBindingsTest> weak_factory_;
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MojoProxyResolverV8TracingBindingsTest);
 };
 
 TEST_F(MojoProxyResolverV8TracingBindingsTest, Basic) {
-  SendAlertAndError();
+  bindings_->Alert(base::ASCIIToUTF16("alert"));
+  bindings_->OnError(-1, base::ASCIIToUTF16("error"));
 
   EXPECT_TRUE(bindings_->GetHostResolver());
   EXPECT_FALSE(bindings_->GetBoundNetLog().net_log());
@@ -71,21 +58,4 @@ TEST_F(MojoProxyResolverV8TracingBindingsTest, Basic) {
   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_v8_tracing.cc b/net/proxy/proxy_resolver_v8_tracing.cc
index 01e1ef5..4d09355 100644
--- a/net/proxy/proxy_resolver_v8_tracing.cc
+++ b/net/proxy/proxy_resolver_v8_tracing.cc
@@ -193,8 +193,9 @@ class Job : public base::RefCountedThreadSafe<Job>,
   void HandleAlertOrError(bool is_alert, int line_number,
                           const base::string16& message);
   void DispatchBufferedAlertsAndErrors();
-  void DispatchAlertOrError(bool is_alert, int line_number,
-                            const base::string16& message);
+  void DispatchAlertOrErrorOnOriginThread(bool is_alert,
+                                          int line_number,
+                                          const base::string16& message);
 
   // The thread which called into ProxyResolverV8TracingImpl, and on which the
   // completion callback is expected to run.
@@ -395,6 +396,7 @@ void Job::Cancel() {
   // The worker thread might be blocked waiting for DNS.
   event_.Signal();
 
+  bindings_.reset();
   owned_self_reference_ = NULL;
 }
 
@@ -410,6 +412,7 @@ LoadState Job::GetLoadState() const {
 Job::~Job() {
   DCHECK(!pending_dns_);
   DCHECK(callback_.is_null());
+  DCHECK(!bindings_);
 }
 
 void Job::CheckIsOnWorkerThread() const {
@@ -463,6 +466,13 @@ void Job::NotifyCallerOnOriginLoop(int result) {
   if (cancelled_.IsSet())
     return;
 
+  DispatchBufferedAlertsAndErrors();
+
+  // This isn't the ordinary execution flow, however it is exercised by
+  // unit-tests.
+  if (cancelled_.IsSet())
+    return;
+
   DCHECK(!callback_.is_null());
   DCHECK(!pending_dns_);
 
@@ -474,6 +484,7 @@ void Job::NotifyCallerOnOriginLoop(int result) {
   ReleaseCallback();
   callback.Run(result);
 
+  bindings_.reset();
   owned_self_reference_ = NULL;
 }
 
@@ -530,7 +541,6 @@ void Job::ExecuteNonBlocking() {
   if (abandoned_)
     return;
 
-  DispatchBufferedAlertsAndErrors();
   NotifyCaller(result);
 }
 
@@ -704,11 +714,8 @@ void Job::DoDnsOperation() {
   // Check if the request was cancelled as a side-effect of calling into the
   // HostResolver. This isn't the ordinary execution flow, however it is
   // exercised by unit-tests.
-  if (cancelled_.IsSet()) {
-    if (!pending_dns_completed_synchronously_)
-      host_resolver()->CancelRequest(dns_request);
+  if (cancelled_.IsSet())
     return;
-  }
 
   if (pending_dns_completed_synchronously_) {
     OnDnsOperationComplete(result);
@@ -841,7 +848,9 @@ void Job::HandleAlertOrError(bool is_alert,
 
   if (blocking_dns_) {
     // In blocking DNS mode the events can be dispatched immediately.
-    DispatchAlertOrError(is_alert, line_number, message);
+    origin_runner_->PostTask(
+        FROM_HERE, base::Bind(&Job::DispatchAlertOrErrorOnOriginThread, this,
+                              is_alert, line_number, message));
     return;
   }
 
@@ -857,6 +866,7 @@ void Job::HandleAlertOrError(bool is_alert,
   // memory. Consider a script which does megabytes worth of alerts().
   // Avoid this by falling back to blocking mode.
   if (alerts_and_errors_byte_cost_ > kMaxAlertsAndErrorsBytes) {
+    alerts_and_errors_.clear();
     ScheduleRestartWithBlockingDns();
     return;
   }
@@ -866,28 +876,18 @@ void Job::HandleAlertOrError(bool is_alert,
 }
 
 void Job::DispatchBufferedAlertsAndErrors() {
-  CheckIsOnWorkerThread();
-  DCHECK(!blocking_dns_);
-  DCHECK(!abandoned_);
-
+  CheckIsOnOriginThread();
   for (size_t i = 0; i < alerts_and_errors_.size(); ++i) {
     const AlertOrError& x = alerts_and_errors_[i];
-    DispatchAlertOrError(x.is_alert, x.line_number, x.message);
+    DispatchAlertOrErrorOnOriginThread(x.is_alert, x.line_number, x.message);
   }
 }
 
-void Job::DispatchAlertOrError(bool is_alert,
-                               int line_number,
-                               const base::string16& message) {
-  CheckIsOnWorkerThread();
+void Job::DispatchAlertOrErrorOnOriginThread(bool is_alert,
+                                             int line_number,
+                                             const base::string16& message) {
+  CheckIsOnOriginThread();
 
-  // Note that the handling of cancellation is racy with regard to
-  // alerts/errors. The request might get cancelled shortly after this
-  // check! (There is no lock being held to guarantee otherwise).
-  //
-  // If this happens, then some information will be logged needlessly, however
-  // the Bindings are responsible for handling this case so it shouldn't cause
-  // problems.
   if (cancelled_.IsSet())
     return;
 
diff --git a/net/proxy/proxy_resolver_v8_tracing.h b/net/proxy/proxy_resolver_v8_tracing.h
index 0b0ab35..cd9ae6c 100644
--- a/net/proxy/proxy_resolver_v8_tracing.h
+++ b/net/proxy/proxy_resolver_v8_tracing.h
@@ -21,30 +21,24 @@ class NET_EXPORT ProxyResolverV8Tracing {
  public:
   // Bindings is an interface used by ProxyResolverV8Tracing to delegate
   // per-request functionality. Each instance will be destroyed on the origin
-  // thread of the ProxyResolverV8Tracing when the request completes or after
-  // the request is cancelled. In the cancellation case, the Bindings instance
-  // for a request may be destroyed after CancelRequest completes.
+  // thread of the ProxyResolverV8Tracing when the request completes or is
+  // cancelled. All methods will be invoked from the origin thread.
   class Bindings {
    public:
     Bindings() {}
     virtual ~Bindings() {}
 
-    // Invoked in response to an alert() call by the PAC script. This may be
-    // called after cancellation and from any thread.
+    // Invoked in response to an alert() call by the PAC script.
     virtual void Alert(const base::string16& message) = 0;
 
-    // Invoked in response to an error in the PAC script. This may be
-    // called after cancellation and from any thread.
+    // Invoked in response to an error in the PAC script.
     virtual void OnError(int line_number, const base::string16& message) = 0;
 
-    // Returns a HostResolver to use for DNS resolution. This will only be
-    // called from the origin thread and will never be called after
-    // cancellation.
+    // Returns a HostResolver to use for DNS resolution.
     virtual HostResolver* GetHostResolver() = 0;
 
     // Returns a BoundNetLog to be passed to the HostResolver returned by
-    // GetHostResolver(). This will only be called from the origin thread and
-    // will never be called after cancellation.
+    // GetHostResolver().
     virtual BoundNetLog GetBoundNetLog() = 0;
 
    private:
diff --git a/net/proxy/proxy_resolver_v8_tracing_unittest.cc b/net/proxy/proxy_resolver_v8_tracing_unittest.cc
index 6eda3b8..3bec370 100644
--- a/net/proxy/proxy_resolver_v8_tracing_unittest.cc
+++ b/net/proxy/proxy_resolver_v8_tracing_unittest.cc
@@ -12,6 +12,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/platform_thread.h"
+#include "base/threading/thread_checker.h"
 #include "base/values.h"
 #include "net/base/net_errors.h"
 #include "net/base/test_completion_callback.h"
@@ -19,6 +20,7 @@
 #include "net/dns/mock_host_resolver.h"
 #include "net/log/net_log.h"
 #include "net/proxy/proxy_info.h"
+#include "net/test/event_waiter.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
@@ -58,31 +60,32 @@ scoped_refptr<ProxyResolverScriptData> LoadScriptData(const char* filename) {
 class MockBindings {
  public:
   explicit MockBindings(HostResolver* host_resolver)
-      : host_resolver_(host_resolver), event_(true, false) {}
+      : host_resolver_(host_resolver) {}
 
   void Alert(const base::string16& message) {
-    base::AutoLock l(lock_);
     alerts_.push_back(base::UTF16ToASCII(message));
   }
   void OnError(int line_number, const base::string16& error) {
-    base::AutoLock l(lock_);
+    waiter_.NotifyEvent(EVENT_ERROR);
     errors_.push_back(std::make_pair(line_number, base::UTF16ToASCII(error)));
-    event_.Signal();
+    if (!error_callback_.is_null())
+      error_callback_.Run();
   }
 
   HostResolver* host_resolver() { return host_resolver_; }
 
   std::vector<std::string> GetAlerts() {
-    base::AutoLock l(lock_);
     return alerts_;
   }
 
   std::vector<std::pair<int, std::string>> GetErrors() {
-    base::AutoLock l(lock_);
     return errors_;
   }
 
-  void WaitForError() { event_.Wait(); }
+  void RunOnError(const base::Closure& callback) {
+    error_callback_ = callback;
+    waiter_.WaitForEvent(EVENT_ERROR);
+  }
 
   scoped_ptr<ProxyResolverV8Tracing::Bindings> CreateBindings() {
     return make_scoped_ptr(new ForwardingBindings(this));
@@ -91,33 +94,43 @@ class MockBindings {
  private:
   class ForwardingBindings : public ProxyResolverV8Tracing::Bindings {
    public:
-    ForwardingBindings(MockBindings* bindings) : bindings_(bindings) {}
+    explicit ForwardingBindings(MockBindings* bindings) : bindings_(bindings) {}
 
     // ProxyResolverV8Tracing::Bindings overrides.
     void Alert(const base::string16& message) override {
+      DCHECK(thread_checker_.CalledOnValidThread());
       bindings_->Alert(message);
     }
 
     void OnError(int line_number, const base::string16& error) override {
+      DCHECK(thread_checker_.CalledOnValidThread());
       bindings_->OnError(line_number, error);
     }
 
-    BoundNetLog GetBoundNetLog() override { return BoundNetLog(); }
+    BoundNetLog GetBoundNetLog() override {
+      DCHECK(thread_checker_.CalledOnValidThread());
+      return BoundNetLog();
+    }
 
     HostResolver* GetHostResolver() override {
+      DCHECK(thread_checker_.CalledOnValidThread());
       return bindings_->host_resolver();
     }
 
    private:
     MockBindings* bindings_;
+    base::ThreadChecker thread_checker_;
+  };
+
+  enum Event {
+    EVENT_ERROR,
   };
 
-  base::Lock lock_;
   std::vector<std::string> alerts_;
   std::vector<std::pair<int, std::string>> errors_;
   HostResolver* const host_resolver_;
-
-  base::WaitableEvent event_;
+  base::Closure error_callback_;
+  EventWaiter<Event> waiter_;
 };
 
 scoped_ptr<ProxyResolverV8Tracing> CreateResolver(
@@ -629,39 +642,29 @@ TEST_F(ProxyResolverV8TracingTest, CancelWhilePendingCompletionTask) {
 
   ProxyInfo proxy_info1;
   ProxyInfo proxy_info2;
-  ProxyInfo proxy_info3;
   ProxyResolver::RequestHandle request1;
   ProxyResolver::RequestHandle request2;
-  ProxyResolver::RequestHandle request3;
   TestCompletionCallback callback;
 
-  resolver->GetProxyForURL(GURL("http://foo/"), &proxy_info1,
+  resolver->GetProxyForURL(GURL("http://throw-an-error/"), &proxy_info1,
                            base::Bind(&CrashCallback), &request1,
                            mock_bindings.CreateBindings());
 
-  resolver->GetProxyForURL(GURL("http://throw-an-error"), &proxy_info2,
-                           callback.callback(), &request2,
-                           mock_bindings.CreateBindings());
-
   // Wait until the first request has finished running on the worker thread.
-  // (The second request will output an error).
-  mock_bindings.WaitForError();
-
-  // Cancel the first request, while it has a pending completion task on
+  // Cancel the first request, while it is running its completion task on
   // the origin thread.
-  resolver->CancelRequest(request1);
-
-  EXPECT_EQ(ERR_PAC_SCRIPT_FAILED, callback.WaitForResult());
+  mock_bindings.RunOnError(base::Bind(&ProxyResolverV8Tracing::CancelRequest,
+                                      base::Unretained(resolver.get()),
+                                      request1));
 
   // Start another request, to make sure it is able to complete.
   resolver->GetProxyForURL(GURL("http://i-have-no-idea-what-im-doing/"),
-                           &proxy_info3, callback.callback(), &request3,
+                           &proxy_info2, callback.callback(), &request2,
                            mock_bindings.CreateBindings());
 
   EXPECT_EQ(OK, callback.WaitForResult());
 
-  EXPECT_EQ("i-approve-this-message:42",
-            proxy_info3.proxy_server().ToURI());
+  EXPECT_EQ("i-approve-this-message:42", proxy_info2.proxy_server().ToURI());
 }
 
 // This implementation of HostResolver allows blocking until a resolve request
@@ -798,11 +801,6 @@ TEST_F(ProxyResolverV8TracingTest, CancelWhileBlockedInNonBlockingDns) {
       base::Bind(CancelRequestAndPause, resolver.get(), request));
 
   host_resolver.WaitUntilRequestIsReceived();
-
-  // At this point the host resolver ran Resolve(), and should have cancelled
-  // the request.
-
-  EXPECT_EQ(1, host_resolver.num_cancelled_requests());
 }
 
 // Cancel the request while there is a pending DNS request, however before
diff --git a/net/proxy/proxy_resolver_v8_tracing_wrapper_unittest.cc b/net/proxy/proxy_resolver_v8_tracing_wrapper_unittest.cc
index 7a77b82..c83f655 100644
--- a/net/proxy/proxy_resolver_v8_tracing_wrapper_unittest.cc
+++ b/net/proxy/proxy_resolver_v8_tracing_wrapper_unittest.cc
@@ -26,6 +26,7 @@
 #include "net/log/test_net_log_util.h"
 #include "net/proxy/proxy_info.h"
 #include "net/proxy/proxy_resolver_error_observer.h"
+#include "net/test/event_waiter.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
@@ -88,29 +89,31 @@ scoped_ptr<ProxyResolver> CreateResolver(
 
 class MockErrorObserver : public ProxyResolverErrorObserver {
  public:
-  MockErrorObserver() : event_(true, false) {}
-
   void OnPACScriptError(int line_number, const base::string16& error) override {
-    {
-      base::AutoLock l(lock_);
-      output += base::StringPrintf("Error: line %d: %s\n", line_number,
-                                   base::UTF16ToASCII(error).c_str());
-    }
-    event_.Signal();
+    output += base::StringPrintf("Error: line %d: %s\n", line_number,
+                                 base::UTF16ToASCII(error).c_str());
+    waiter_.NotifyEvent(EVENT_ERROR);
+    if (!error_callback_.is_null())
+      error_callback_.Run();
   }
 
   std::string GetOutput() {
-    base::AutoLock l(lock_);
     return output;
   }
 
-  void WaitForOutput() { event_.Wait(); }
+  void RunOnError(const base::Closure& callback) {
+    error_callback_ = callback;
+    waiter_.WaitForEvent(EVENT_ERROR);
+  }
 
  private:
-  base::Lock lock_;
+  enum Event {
+    EVENT_ERROR,
+  };
   std::string output;
 
-  base::WaitableEvent event_;
+  base::Closure error_callback_;
+  EventWaiter<Event> waiter_;
 };
 
 TEST_F(ProxyResolverV8TracingWrapperTest, Simple) {
@@ -723,40 +726,31 @@ TEST_F(ProxyResolverV8TracingWrapperTest, CancelWhilePendingCompletionTask) {
 
   ProxyInfo proxy_info1;
   ProxyInfo proxy_info2;
-  ProxyInfo proxy_info3;
   ProxyResolver::RequestHandle request1;
   ProxyResolver::RequestHandle request2;
-  ProxyResolver::RequestHandle request3;
   TestCompletionCallback callback;
 
-  int rv = resolver->GetProxyForURL(GURL("http://foo/"), &proxy_info1,
-                                    base::Bind(&CrashCallback), &request1,
-                                    BoundNetLog());
-  EXPECT_EQ(ERR_IO_PENDING, rv);
-
-  rv = resolver->GetProxyForURL(GURL("http://throw-an-error/"), &proxy_info2,
-                                callback.callback(), &request2, BoundNetLog());
+  int rv = resolver->GetProxyForURL(GURL("http://throw-an-error/"),
+                                    &proxy_info1, base::Bind(&CrashCallback),
+                                    &request1, BoundNetLog());
   EXPECT_EQ(ERR_IO_PENDING, rv);
 
   // Wait until the first request has finished running on the worker thread.
-  // (The second request will output an error).
-  error_observer->WaitForOutput();
-
   // Cancel the first request, while it has a pending completion task on
   // the origin thread.
-  resolver->CancelRequest(request1);
-
-  EXPECT_EQ(ERR_PAC_SCRIPT_FAILED, callback.WaitForResult());
+  error_observer->RunOnError(base::Bind(&ProxyResolver::CancelRequest,
+                                        base::Unretained(resolver.get()),
+                                        request1));
 
   // Start another request, to make sure it is able to complete.
   rv = resolver->GetProxyForURL(GURL("http://i-have-no-idea-what-im-doing/"),
-                                &proxy_info3, callback.callback(), &request3,
+                                &proxy_info2, callback.callback(), &request2,
                                 BoundNetLog());
   EXPECT_EQ(ERR_IO_PENDING, rv);
 
   EXPECT_EQ(OK, callback.WaitForResult());
 
-  EXPECT_EQ("i-approve-this-message:42", proxy_info3.proxy_server().ToURI());
+  EXPECT_EQ("i-approve-this-message:42", proxy_info2.proxy_server().ToURI());
 }
 
 // This implementation of HostResolver allows blocking until a resolve request
@@ -896,11 +890,6 @@ TEST_F(ProxyResolverV8TracingWrapperTest, CancelWhileBlockedInNonBlockingDns) {
       base::Bind(CancelRequestAndPause, resolver.get(), request));
 
   host_resolver.WaitUntilRequestIsReceived();
-
-  // At this point the host resolver ran Resolve(), and should have cancelled
-  // the request.
-
-  EXPECT_EQ(1, host_resolver.num_cancelled_requests());
 }
 
 // Cancel the request while there is a pending DNS request, however before