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

net/dns/mock_host_resolver.cc

Index: net/dns/mock_host_resolver.cc
diff --git a/net/dns/mock_host_resolver.cc b/net/dns/mock_host_resolver.cc
index 01030aa353740634a8401d7f072e2a1991365f32..90298884e94471dc316353bee67c091c497b90d5 100644
--- a/net/dns/mock_host_resolver.cc
+++ b/net/dns/mock_host_resolver.cc
@@ -8,6 +8,7 @@
 #include <vector>
 
 #include "base/bind.h"
+#include "base/callback_helpers.h"
 #include "base/location.h"
 #include "base/memory/ref_counted.h"
 #include "base/single_thread_task_runner.h"
@@ -55,25 +56,63 @@ int ParseAddressList(const std::string& host_list,
   return OK;
 }
 
-struct MockHostResolverBase::Request {
-  Request(const RequestInfo& req_info,
-          AddressList* addr,
-          const CompletionCallback& cb)
-      : info(req_info), addresses(addr), callback(cb) {}
-  RequestInfo info;
-  AddressList* addresses;
-  CompletionCallback callback;
+class MockHostResolverBase::RequestImpl : public HostResolver::Request {
+ public:
+  RequestImpl(const RequestInfo& req_info,
+              AddressList* addr,
+              const CompletionCallback& cb,
+              MockHostResolverBase* resolver,
+              size_t id)
+      : info_(req_info),
+        addresses_(addr),
+        callback_(cb),
+        resolver_(resolver),
+        id_(id) {}
+
+  ~RequestImpl() override {
+    if (resolver_)
+      resolver_->DetachRequest(id_);
+  }
+
+  void ChangeRequestPriority(RequestPriority priority) override {}
+
+  void OnResolveCancelled(MockHostResolverBase* resolver_) {

[mmenke, 2016/07/19 19:03:56]

This isn't used.

[maksims (do not use this acc), 2016/07/21 07:12:46]

Done.

+    DCHECK_EQ(resolver_, resolver_);
+    resolver_ = nullptr;
+    addresses_ = nullptr;
+    callback_.Reset();
+  }
+
+  void OnResolveCompleted(MockHostResolverBase* resolver, int error) {
+    DCHECK_EQ(resolver_, resolver);
+    resolver_ = nullptr;
+    addresses_ = nullptr;
+    base::ResetAndReturn(&callback_).Run(error);
+  }
+
+  RequestInfo info() { return info_; }
+
+  AddressList* addresses() { return addresses_; }
+
+ private:
+  RequestInfo info_;
+  AddressList* addresses_;
+  CompletionCallback callback_;
+  MockHostResolverBase* resolver_;
+  size_t id_;
+
+  DISALLOW_COPY_AND_ASSIGN(RequestImpl);
 };
 
 MockHostResolverBase::~MockHostResolverBase() {
-  STLDeleteValues(&requests_);
+  requests_.clear();

[mmenke, 2016/07/19 19:03:56]

This isn't right - if there are any requests alive after the MockHostResolver
base is destroyed, they'll still call into DetachRequest in their destructor,
and we'll crash.

This either needs to call OnResolveCancelled on all requests, or
DCHECK(requests.empty());.

If tests are passing, I'd go with the DCHECK.

[maksims (do not use this acc), 2016/07/21 07:12:46]

Done.

 }
 
 int MockHostResolverBase::Resolve(const RequestInfo& info,
                                   RequestPriority priority,
                                   AddressList* addresses,
                                   const CompletionCallback& callback,
-                                  RequestHandle* handle,
+                                  std::unique_ptr<Request>* request,
                                   const BoundNetLog& net_log) {
   DCHECK(CalledOnValidThread());
   last_request_priority_ = priority;
@@ -84,13 +123,13 @@ int MockHostResolverBase::Resolve(const RequestInfo& info,
     return rv;
   }
   if (synchronous_mode_) {
-    return ResolveProc(id, info, addresses);
+    return ResolveProc(info, addresses);
   }
   // Store the request for asynchronous resolution
-  Request* req = new Request(info, addresses, callback);
-  requests_[id] = req;
-  if (handle)
-    *handle = reinterpret_cast<RequestHandle>(id);
+  std::unique_ptr<RequestImpl> req(
+      new RequestImpl(info, addresses, callback, this, id));
+  requests_[id] = req.get();
+  *request = std::move(req);
 
   if (!ondemand_mode_) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
@@ -111,17 +150,10 @@ int MockHostResolverBase::ResolveFromCache(const RequestInfo& info,
   return rv;
 }
 
-void MockHostResolverBase::CancelRequest(RequestHandle handle) {
-  DCHECK(CalledOnValidThread());
-  size_t id = reinterpret_cast<size_t>(handle);
+void MockHostResolverBase::DetachRequest(size_t id) {
   RequestMap::iterator it = requests_.find(id);
-  if (it != requests_.end()) {
-    std::unique_ptr<Request> req(it->second);
-    requests_.erase(it);
-  } else {
-    NOTREACHED() << "CancelRequest must NOT be called after request is "
-        "complete or canceled.";
-  }
+  CHECK(it != requests_.end());
+  requests_.erase(it);
 }
 
 HostCache* MockHostResolverBase::GetHostCache() {
@@ -183,15 +215,11 @@ int MockHostResolverBase::ResolveFromIPLiteralOrCache(const RequestInfo& info,
   return rv;
 }
 
-int MockHostResolverBase::ResolveProc(size_t id,
-                                      const RequestInfo& info,
+int MockHostResolverBase::ResolveProc(const RequestInfo& info,
                                       AddressList* addresses) {
   AddressList addr;
-  int rv = rules_->Resolve(info.hostname(),
-                           info.address_family(),
-                           info.host_resolver_flags(),
-                           &addr,
-                           NULL);
+  int rv = rules_->Resolve(info.hostname(), info.address_family(),
+                           info.host_resolver_flags(), &addr, nullptr);
   if (cache_.get()) {
     HostCache::Key key(info.hostname(),
                        info.address_family(),
@@ -212,11 +240,11 @@ void MockHostResolverBase::ResolveNow(size_t id) {
   if (it == requests_.end())
     return;  // was canceled
 
-  std::unique_ptr<Request> req(it->second);
+  RequestImpl* req = it->second;
   requests_.erase(it);
-  int rv = ResolveProc(id, req->info, req->addresses);
-  if (!req->callback.is_null())
-    req->callback.Run(rv);
+
+  int error = ResolveProc(req->info(), req->addresses());
+  req->OnResolveCompleted(this, error);
 }
 
 //-----------------------------------------------------------------------------
@@ -425,7 +453,7 @@ int HangingHostResolver::Resolve(const RequestInfo& info,
                                  RequestPriority priority,
                                  AddressList* addresses,
                                  const CompletionCallback& callback,
-                                 RequestHandle* out_req,
+                                 std::unique_ptr<Request>* request,
                                  const BoundNetLog& net_log) {
   return ERR_IO_PENDING;
 }