Change HostResolver::Resolve() to take an std::unique_ptr<Request>* rather than a RequestHandle*

net/proxy/proxy_resolver_v8_tracing.cc

 // Copyright (c) 2013 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_v8_tracing.h"
 
 #include <map>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 266 matching lines @@
   // Whether the current execution needs to be restarted in blocking mode.
   bool should_restart_with_blocking_dns_;
 
   // ---------------------------------------------------------------------------
   // State for pending DNS request.
   // ---------------------------------------------------------------------------
 
   // Handle to the outstanding request in the HostResolver, or NULL.
   // This is mutated and used on the origin thread, however it may be read by
   // the worker thread for some DCHECKS().
-  HostResolver::RequestHandle pending_dns_;
+  std::unique_ptr<HostResolver::Request> pending_dns_;

[jochen (gone - plz use gerrit), 2017/03/08 15:22:10]

std::unique_ptr cannot be read atomically, and given that we read it lock-less
from workers while writing (in Job::Cancel) from the foreground thread, this now
introduces a data race

 
   // Indicates if the outstanding DNS request completed synchronously. Written
   // on the origin thread, and read by the worker thread.
   bool pending_dns_completed_synchronously_;
 
   // These are the inputs to DoDnsOperation(). Written on the worker thread,
   // read by the origin thread.
   std::string pending_dns_host_;
   ResolveDnsOperation pending_dns_op_;
 
@@ skipping 33 matching lines @@
   DISALLOW_COPY_AND_ASSIGN(ProxyResolverV8TracingImpl);
 };
 
 Job::Job(const Job::Params* params,
          std::unique_ptr<ProxyResolverV8Tracing::Bindings> bindings)
     : origin_runner_(base::ThreadTaskRunnerHandle::Get()),
       params_(params),
       bindings_(std::move(bindings)),
       event_(base::WaitableEvent::ResetPolicy::MANUAL,
              base::WaitableEvent::InitialState::NOT_SIGNALED),
-      last_num_dns_(0),
-      pending_dns_(NULL) {
+      last_num_dns_(0) {
   CheckIsOnOriginThread();
 }
 
 void Job::StartCreateV8Resolver(
     const scoped_refptr<ProxyResolverScriptData>& script_data,
     std::unique_ptr<ProxyResolverV8>* resolver,
     const CompletionCallback& callback) {
   CheckIsOnOriginThread();
 
   resolver_out_ = resolver;
@@ skipping 34 matching lines @@
   // (f) The script execution completed entirely, and posted a task to the
   //     origin thread to notify the caller.
   //
   // |cancelled_| is read on both the origin thread and worker thread. The
   // code that runs on the worker thread is littered with checks on
   // |cancelled_| to break out early.
   cancelled_.Set();
 
   ReleaseCallback();
 
-  if (pending_dns_) {
-    host_resolver()->CancelRequest(pending_dns_);
-    pending_dns_ = NULL;
-  }
+  pending_dns_.reset();
 
   // The worker thread might be blocked waiting for DNS.
   event_.Signal();
 
   bindings_.reset();
   owned_self_reference_ = NULL;
 }
 
 LoadState Job::GetLoadState() const {
   CheckIsOnOriginThread();
@@ skipping 292 matching lines @@
   return true;
 }
 
 void Job::DoDnsOperation() {
   CheckIsOnOriginThread();
   DCHECK(!pending_dns_);
 
   if (cancelled_.IsSet())
     return;
 
-  HostResolver::RequestHandle dns_request = NULL;
+  std::unique_ptr<HostResolver::Request> dns_request;
   int result = host_resolver()->Resolve(
       MakeDnsRequestInfo(pending_dns_host_, pending_dns_op_), DEFAULT_PRIORITY,
       &pending_dns_addresses_, base::Bind(&Job::OnDnsOperationComplete, this),
       &dns_request, bindings_->GetBoundNetLog());
 
   pending_dns_completed_synchronously_ = result != ERR_IO_PENDING;
 
   // 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())
     return;
 
   if (pending_dns_completed_synchronously_) {
     OnDnsOperationComplete(result);
   } else {
     DCHECK(dns_request);
-    pending_dns_ = dns_request;
+    pending_dns_ = std::move(dns_request);
     // OnDnsOperationComplete() will be called by host resolver on completion.
   }
 
   if (!blocking_dns_) {
     // The worker thread always blocks waiting to see if the result can be
     // serviced from cache before restarting.
     event_.Signal();
   }
 }
 
 void Job::OnDnsOperationComplete(int result) {
   CheckIsOnOriginThread();
 
   DCHECK(!cancelled_.IsSet());
   DCHECK(pending_dns_completed_synchronously_ == (pending_dns_ == NULL));
 
   SaveDnsToLocalCache(pending_dns_host_, pending_dns_op_, result,
                       pending_dns_addresses_);
-  pending_dns_ = NULL;
+  pending_dns_.reset();
 
   if (blocking_dns_) {
     event_.Signal();
     return;
   }
 
   if (!blocking_dns_ && !pending_dns_completed_synchronously_) {
     // Restart. This time it should make more progress due to having
     // cached items.
     worker_task_runner()->PostTask(FROM_HERE,
@@ skipping 336 matching lines @@
 
 }  // namespace
 
 // static
 std::unique_ptr<ProxyResolverV8TracingFactory>
 ProxyResolverV8TracingFactory::Create() {
   return base::WrapUnique(new ProxyResolverV8TracingFactoryImpl());
 }
 
 }  // namespace net