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

extensions/browser/api/dns/dns_api.cc

 // Copyright 2014 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 "extensions/browser/api/dns/dns_api.h"
 
 #include "base/bind.h"
 #include "base/values.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/resource_context.h"
 #include "extensions/browser/api/dns/host_resolver_wrapper.h"
 #include "extensions/common/api/dns.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/net_errors.h"
 #include "net/log/net_log.h"
 
 using content::BrowserThread;
 using extensions::api::dns::ResolveCallbackResolveInfo;
 
 namespace Resolve = extensions::api::dns::Resolve;
 
 namespace extensions {
 
 DnsResolveFunction::DnsResolveFunction()
     : resource_context_(NULL),
       response_(false),
-      request_handle_(new net::HostResolver::RequestHandle()),
+      request_handle_(nullptr),

[Devlin, 2016/07/12 15:04:51]

unnecessary (this is the default construction of std::unique_ptr)

[maksims (do not use this acc), 2016/07/19 15:00:25]

Done.

       addresses_(new net::AddressList) {}
 
 DnsResolveFunction::~DnsResolveFunction() {}
 
 bool DnsResolveFunction::RunAsync() {
   std::unique_ptr<Resolve::Params> params(Resolve::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
 
   hostname_ = params->hostname;
   resource_context_ = browser_context()->GetResourceContext();
@@ skipping 11 matching lines @@
 
   net::HostResolver* host_resolver =
       HostResolverWrapper::GetInstance()->GetHostResolver(resource_context_);
   DCHECK(host_resolver);
 
   // Yes, we are passing zero as the port. There are some interesting but not
   // presently relevant reasons why HostResolver asks for the port of the
   // hostname you'd like to resolve, even though it doesn't use that value in
   // determining its answer.
   net::HostPortPair host_port_pair(hostname_, 0);
-
   net::HostResolver::RequestInfo request_info(host_port_pair);
   int resolve_result = host_resolver->Resolve(
-      request_info,
-      net::DEFAULT_PRIORITY,
-      addresses_.get(),
-      base::Bind(&DnsResolveFunction::OnLookupFinished, this),
-      request_handle_.get(),
+      request_info, net::DEFAULT_PRIORITY, addresses_.get(),
+      base::Bind(&DnsResolveFunction::OnLookupFinished, this), &request_handle_,
       net::BoundNetLog());
 
   // Balanced in OnLookupFinished.
   AddRef();
 
   if (resolve_result != net::ERR_IO_PENDING)
     OnLookupFinished(resolve_result);
 }
 
 void DnsResolveFunction::RespondOnUIThread() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   SendResponse(response_);
 }
 
 void DnsResolveFunction::OnLookupFinished(int resolve_result) {
   std::unique_ptr<ResolveCallbackResolveInfo> resolve_info(
       new ResolveCallbackResolveInfo());
   resolve_info->result_code = resolve_result;
   if (resolve_result == net::OK) {
     DCHECK(!addresses_->empty());
     resolve_info->address.reset(
         new std::string(addresses_->front().ToStringWithoutPort()));
   }
   results_ = Resolve::Results::Create(*resolve_info);
   response_ = true;
 
+  // TODO(maksims): investigate why std::unique_ptr<HostResolver::Request>'s
+  // destructor is not called automatically.

[Devlin, 2016/07/12 15:04:51]

I don't quite follow this comment.  Automatically when?  Is it not called here
after reset()?  I'd find that very surprising (and worrying).

[maksims (do not use this acc), 2016/07/14 12:02:47]

No, I mean that DnsResolveFunction's destructor is not called until request_ is
reseted for some reason. And there is a memory leak then. I cannot find what
exactly causes the trouble. It's a temp workaround. 

Matt, would you please take a look into that? I cannot find the root cause.
Log is here.
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

[mmenke, 2016/07/14 18:25:31]

Without digging into it, my guess is that the test is leaking the
DnsResolveFunction, and there's some suppression that leak.

In the old code, DnsResolveFunction::WorkOnIOThread doesn't allocate any memory
of its own - it's all owned by the HostResolver, which used to destroy requests
on completion itself.  The new code allocates memory owned by the
DnsResolveFunction there, so if it leaks, the handle is, too.  

The old suppression wouldn't cover this because it only covers stuff allocated
in DnsResolveFunction's constructor.

Anyhow, just a theory.  I'm not at all familiar with this code.

[maksims (do not use this acc), 2016/07/19 15:00:25]

Done.

[maksims (do not use this acc), 2016/07/19 15:00:25]

Done.

[maksims (do not use this acc), 2016/07/19 15:00:25]

I just the implementation of ::RequestImpl and it solved the issue.

[maksims (do not use this acc), 2016/07/19 15:00:25]

Done.

+  request_handle_.reset();
+
   bool post_task_result = BrowserThread::PostTask(
       BrowserThread::UI,
       FROM_HERE,
       base::Bind(&DnsResolveFunction::RespondOnUIThread, this));
   DCHECK(post_task_result);
 
   Release();  // Added in WorkOnIOThread().
 }
 
 }  // namespace extensions