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

net/dns/mock_host_resolver.cc

 // Copyright (c) 2012 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/dns/mock_host_resolver.h"
 
 #include <string>
 #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"
 #include "base/stl_util.h"
 #include "base/strings/pattern.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/threading/platform_thread.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "net/base/ip_address.h"
@@ skipping 27 matching lines @@
     IPAddress ip_address;
     if (!ip_address.AssignFromIPLiteral(address)) {
       LOG(WARNING) << "Not a supported IP literal: " << address.as_string();
       return ERR_UNEXPECTED;
     }
     addrlist->push_back(IPEndPoint(ip_address, 0));
   }
   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;
   num_resolve_++;
   size_t id = next_request_id_++;
   int rv = ResolveFromIPLiteralOrCache(info, addresses);
   if (rv != ERR_DNS_CACHE_MISS) {
     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(
         FROM_HERE,
         base::Bind(&MockHostResolverBase::ResolveNow, AsWeakPtr(), id));
   }
 
   return ERR_IO_PENDING;
 }
 
 int MockHostResolverBase::ResolveFromCache(const RequestInfo& info,
                                            AddressList* addresses,
                                            const BoundNetLog& net_log) {
   num_resolve_from_cache_++;
   DCHECK(CalledOnValidThread());
   next_request_id_++;
   int rv = ResolveFromIPLiteralOrCache(info, addresses);
   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() {
   return cache_.get();
 }
 
 void MockHostResolverBase::ResolveAllPending() {
   DCHECK(CalledOnValidThread());
   DCHECK(ondemand_mode_);
   for (RequestMap::iterator i = requests_.begin(); i != requests_.end(); ++i) {
@@ skipping 41 matching lines @@
     const HostCache::Entry* entry = cache_->Lookup(key, base::TimeTicks::Now());
     if (entry) {
       rv = entry->error();
       if (rv == OK)
         *addresses = AddressList::CopyWithPort(entry->addresses(), info.port());
     }
   }
   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(),
                        info.host_resolver_flags());
     // Storing a failure with TTL 0 so that it overwrites previous value.
     base::TimeDelta ttl;
     if (rv == OK)
       ttl = base::TimeDelta::FromSeconds(kCacheEntryTTLSeconds);
     cache_->Set(key, HostCache::Entry(rv, addr), base::TimeTicks::Now(), ttl);
   }
   if (rv == OK)
     *addresses = AddressList::CopyWithPort(addr, info.port());
   return rv;
 }
 
 void MockHostResolverBase::ResolveNow(size_t id) {
   RequestMap::iterator it = requests_.find(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);
 }
 
 //-----------------------------------------------------------------------------
 
 struct RuleBasedHostResolverProc::Rule {
   enum ResolverType {
     kResolverTypeFail,
     kResolverTypeSystem,
     kResolverTypeIPLiteral,
   };
@@ skipping 188 matching lines @@
   // Next add a rules-based layer the use controls.
   return new RuleBasedHostResolverProc(catchall);
 }
 
 //-----------------------------------------------------------------------------
 
 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;
 }
 
 int HangingHostResolver::ResolveFromCache(const RequestInfo& info,
                                           AddressList* addresses,
                                           const BoundNetLog& net_log) {
   return ERR_DNS_CACHE_MISS;
 }
 
@@ skipping 13 matching lines @@
   CHECK_EQ(old_proc, current_proc_.get());
 }
 
 void ScopedDefaultHostResolverProc::Init(HostResolverProc* proc) {
   current_proc_ = proc;
   previous_proc_ = HostResolverProc::SetDefault(current_proc_.get());
   current_proc_->SetLastProc(previous_proc_.get());
 }
 
 }  // namespace net