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

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

This cl has the following changes:
1) New class HostResolver::Request: it contains all the
necessary data for the actual request.

2) HostResolver::Resolve() takes an
std::unique_ptr<Request>* rather than a RequestHandle* now.

3) HostResolver::CancelRequest() method is removed. If
consumers want to cancel their requests, request.reset()
should be called.

BUG=486264
Committed: https://crrev.com/31452af8fb1192199be24016b3701feb09b31c2d
Cr-Commit-Position: refs/heads/master@{#408062}

[maksims (do not use this acc), 2016-07-04 12:17:32]

Description was changed from

==========
req

BUG=
==========

to

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

BUG=486264
==========

[maksims (do not use this acc), 2016-07-05 09:43:09]

Patchset #2 (id:20001) has been deleted

[maksims (do not use this acc), 2016-07-05 09:43:18]

Patchset #2 (id:40001) has been deleted

[maksims (do not use this acc), 2016-07-05 09:43:26]

Patchset #2 (id:60001) has been deleted

[maksims (do not use this acc), 2016-07-05 09:44:21]

Patchset #2 (id:80001) has been deleted

[maksims (do not use this acc), 2016-07-05 09:44:31]

Patchset #2 (id:100001) has been deleted

[maksims (do not use this acc), 2016-07-05 09:44:39]

Patchset #2 (id:120001) has been deleted

[maksims (do not use this acc), 2016-07-05 09:44:57]

Patchset #1 (id:1) has been deleted

[maksims (do not use this acc), 2016-07-05 09:46:23]

Patchset #1 (id:140001) has been deleted

[maksims (do not use this acc), 2016-07-05 09:47:23]

maksim.sisov@intel.com changed reviewers:
+ asanka@chromium.org, gauravsh@chromium.org, jam@chromium.org,
mmenke@chromium.org

[maksims (do not use this acc), 2016-07-05 09:47:53]

maksim.sisov@intel.com changed reviewers:
+ eroman@chromium.org

[maksims (do not use this acc), 2016-07-05 11:20:32]

Description was changed from

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

BUG=486264
==========

to

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

This cl has the following changes:
1) New class HostResolver::Request: it contains all the necessary data for the
actual request.

2) HostResolver::Resolve() takes an std::unique_ptr<Request>* rather than a
RequestHandle* now.

3) HostResolver::CancelRequest() method is removed. If consumers want to cancel
their requests, request.reset() should be called.

BUG=486264
==========

[maksims (do not use this acc), 2016-07-05 11:20:48]

Description was changed from

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

This cl has the following changes:
1) New class HostResolver::Request: it contains all the necessary data for the
actual request.

2) HostResolver::Resolve() takes an std::unique_ptr<Request>* rather than a
RequestHandle* now.

3) HostResolver::CancelRequest() method is removed. If consumers want to cancel
their requests, request.reset() should be called.

BUG=486264
==========

to

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

This cl has the following changes:
1) New class HostResolver::Request: it contains all the 
necessary data for the actual request.

2) HostResolver::Resolve() takes an 
std::unique_ptr<Request>* rather than a RequestHandle* now.

3) HostResolver::CancelRequest() method is removed. If 
consumers want to cancel their requests, request.reset() 
should be called.

BUG=486264
==========

[maksims (do not use this acc), 2016-07-05 11:21:08]

please take a look

[eroman, 2016-07-06 20:08:05]

(Most of these US-based reviewers were out on vacation)

Will try to get to this today or tomorrow.

[maksims (do not use this acc), 2016-07-07 05:08:12]

On 2016/07/06 20:08:05, eroman wrote:
> (Most of these US-based reviewers were out on vacation)
> 
> Will try to get to this today or tomorrow.

No hurry on this. Let's just wait Matt.

[jam, 2016-07-09 02:28:01]

which parts do you want me to look at?

[maksims (do not use this acc), 2016-07-11 04:55:51]

On 2016/07/09 02:28:01, jam wrote:
> which parts do you want me to look at?

Well, as much as you can. If I remember right, you are owner of
extensions/browser/*, net/dns/*, net/proxy/* , net/socket/*. You can review only
part of that and other reviewers can review other parts as well to reduce amount
of work done by one reviewer and be less error prone. I guess you could look at
net/socket and net/proxy, for example.

[eroman, 2016-07-11 19:59:40]

jam@ isn't needed on this review, I suggest removing him.

The majority of this CL just needs a //net OWNER -- which can be done by myself,
mmenke, or asanka, who are already on this review.

Once we sign off, the remaining approvals are for:

   chromeos/network/*  -- gauravsh (who is already on this review)

   extensions/browser/api/*  -- jam could stamp this as a top-level //src owner,
however I suggest adding someone from //extensions/OWNERS instead to keep it
more targetted.

[maksims (do not use this acc), 2016-07-12 05:22:45]

maksim.sisov@intel.com changed reviewers:
+ rdevlin.cronin@chromium.org

[maksims (do not use this acc), 2016-07-12 05:23:12]

On 2016/07/11 19:59:40, eroman wrote:
> jam@ isn't needed on this review, I suggest removing him.
> 
> The majority of this CL just needs a //net OWNER -- which can be done by
myself,
> mmenke, or asanka, who are already on this review.
> 
> Once we sign off, the remaining approvals are for:
> 
>    chromeos/network/*  -- gauravsh (who is already on this review)
> 
>    extensions/browser/api/*  -- jam could stamp this as a top-level //src
owner,
> however I suggest adding someone from //extensions/OWNERS instead to keep it
> more targetted.

+ Devlin@.

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

https://codereview.chromium.org/2116983002/diff/160001/extensions/browser/api...
File extensions/browser/api/dns/dns_api.cc (right):

https://codereview.chromium.org/2116983002/diff/160001/extensions/browser/api...
extensions/browser/api/dns/dns_api.cc:28: request_handle_(nullptr),
unnecessary (this is the default construction of std::unique_ptr)

https://codereview.chromium.org/2116983002/diff/160001/extensions/browser/api...
extensions/browser/api/dns/dns_api.cc:91: // destructor is not called
automatically.
I don't quite follow this comment.  Automatically when?  Is it not called here
after reset()?  I'd find that very surprising (and worrying).

https://codereview.chromium.org/2116983002/diff/160001/extensions/browser/api...
File extensions/browser/api/socket/socket_api.cc (right):

https://codereview.chromium.org/2116983002/diff/160001/extensions/browser/api...
extensions/browser/api/socket/socket_api.cc:335: request_handle_.reset();
ditto

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver.h
File net/dns/host_resolver.h (right):

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver....
net/dns/host_resolver.h:152: // If |out_req| is non-NULL, then |*out_req| will
be filled with a handle to
is out_req still allowed to be null?  It doesn't look like it.

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver....
net/dns/host_resolver.h:161: std::unique_ptr<Request>* out_req,
I'm not familiar with the implementation here at all, so more of a question than
a comment.

It looks like we now link the Job with the request - is there a lifetime
requirement related to the out_req?  e.g., if I pass in a stack-allocated
std::unique_ptr<Request> that's deleted right after the call to this, and the
Job finishes async, will that cause a crash?  We should document any lifetime
requirements here.

[mmenke, 2016-07-12 18:50:24]

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver.h
File net/dns/host_resolver.h (right):

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver....
net/dns/host_resolver.h:45: class NET_EXPORT Request {
Document this class.

(Owned by the consumer, deletion cancels the request)

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver....
net/dns/host_resolver.h:154: // completed.
This entire paragraph needs to be re-written.

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver....
net/dns/host_resolver.h:161: std::unique_ptr<Request>* out_req,
On 2016/07/12 15:04:51, Devlin wrote:
> I'm not familiar with the implementation here at all, so more of a question
than
> a comment.
> 
> It looks like we now link the Job with the request - is there a lifetime
> requirement related to the out_req?  e.g., if I pass in a stack-allocated
> std::unique_ptr<Request> that's deleted right after the call to this, and the
> Job finishes async, will that cause a crash?  We should document any lifetime
> requirements here.

No - it should be deleted the cancel the request.  IT was already implicitly
linked to the job before, this just makes cancellation less prone to bugginess.

This should be documented.

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver_...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:1419: return;  // no request found, might have
already been removed
Does this case ever happen?  Doesn't seem like it should, if we always use
request_.MarkAsCompleted() to remove them.

Then can replace all this with a DCHECK_NE(std::find(requests_.begin(),
requests_.end(), req), requests_.end());

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:1422: requests_[req_position] = nullptr;
Growing requests_ eternally over the lifetime of the job seems not great.  Can
we just just remove it from the list?

Need to make sure we only walk through requests, removing them, using something
like:

while (!requests_.empty()) {
  requests_.begin()->DoThingThatRemovesRequest();
}

but don't think it will be too complicated.

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:1507: if (requests_.empty()) {
We walk through everything in requests_ if requests_ has nothing in it?

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:2564: }
These methods should all be inlined, since all the other RequestImpl methods
are.

[maksims (do not use this acc), 2016-07-13 11:40:27]

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver_...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:1419: return;  // no request found, might have
already been removed
On 2016/07/12 18:50:23, mmenke wrote:
> Does this case ever happen?  Doesn't seem like it should, if we always use
> request_.MarkAsCompleted() to remove them.
> 
> Then can replace all this with a DCHECK_NE(std::find(requests_.begin(),
> requests_.end(), req), requests_.end());

right, I just wanted to be sure.

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:1422: requests_[req_position] = nullptr;
On 2016/07/12 18:50:23, mmenke wrote:
> Growing requests_ eternally over the lifetime of the job seems not great.  Can
> we just just remove it from the list?
> 
> Need to make sure we only walk through requests, removing them, using
something
> like:
> 
> while (!requests_.empty()) {
>   requests_.begin()->DoThingThatRemovesRequest();
> }
> 
> but don't think it will be too complicated.

The problem is that the request might have already been removed while we are
going through requests in Job::CompleteRequests(). Then "error: attempt to
compare a past-the-end iterator to a singular iterator" occurs. 

I guess that was the reason why requests were stored and just marked
completed/canceled instead of being removed.

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:1507: if (requests_.empty()) {
On 2016/07/12 18:50:23, mmenke wrote:
> We walk through everything in requests_ if requests_ has nothing in it?

no,no,no. my fault. sorry

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:2564: }
On 2016/07/12 18:50:24, mmenke wrote:
> These methods should all be inlined, since all the other RequestImpl methods
> are.

Nope, otherwise 
../../net/dns/host_resolver_impl.cc:545:11: error: member access into incomplete
type 'net::HostResolverImpl::Job'

../../net/dns/host_resolver_impl.h:172:9: note: forward declaration of
'net::HostResolverImpl::Job'
  class Job;

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

Matt, appreciate your help. It would be nice if you could explain me, what is
the reason for that?

https://codereview.chromium.org/2116983002/diff/160001/extensions/browser/api...
File extensions/browser/api/dns/dns_api.cc (right):

https://codereview.chromium.org/2116983002/diff/160001/extensions/browser/api...
extensions/browser/api/dns/dns_api.cc:91: // destructor is not called
automatically.
On 2016/07/12 15:04:51, Devlin wrote:
> I don't quite follow this comment.  Automatically when?  Is it not called here
> after reset()?  I'd find that very surprising (and worrying).

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]

https://codereview.chromium.org/2116983002/diff/160001/extensions/browser/api...
File extensions/browser/api/dns/dns_api.cc (right):

https://codereview.chromium.org/2116983002/diff/160001/extensions/browser/api...
extensions/browser/api/dns/dns_api.cc:91: // destructor is not called
automatically.
On 2016/07/14 12:02:47, maksims wrote:
> On 2016/07/12 15:04:51, Devlin wrote:
> > I don't quite follow this comment.  Automatically when?  Is it not called
here
> > after reset()?  I'd find that very surprising (and worrying).
> 
> 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...

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.

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver_...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:1422: requests_[req_position] = nullptr;
On 2016/07/13 11:40:27, maksims wrote:
> On 2016/07/12 18:50:23, mmenke wrote:
> > Growing requests_ eternally over the lifetime of the job seems not great. 
Can
> > we just just remove it from the list?
> > 
> > Need to make sure we only walk through requests, removing them, using
> something
> > like:
> > 
> > while (!requests_.empty()) {
> >   requests_.begin()->DoThingThatRemovesRequest();
> > }
> > 
> > but don't think it will be too complicated.
> 
> The problem is that the request might have already been removed while we are
> going through requests in Job::CompleteRequests(). Then "error: attempt to
> compare a past-the-end iterator to a singular iterator" occurs. 
> 
> I guess that was the reason why requests were stored and just marked
> completed/canceled instead of being removed.

Right, the problem is CancelRequest calls this method - which seems unnecessary,
it could just as easily use MarkAsCompleted, which should ensure this is never
called twice for the same job, right?

[maksims (do not use this acc), 2016-07-18 13:20:07]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 13:20:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 13:49:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-18 13:49:10]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[maksims (do not use this acc), 2016-07-18 14:42:09]

Patchset #3 (id:200001) has been deleted

[maksims (do not use this acc), 2016-07-19 07:13:10]

Patchset #2 (id:180001) has been deleted

[maksims (do not use this acc), 2016-07-19 07:17:04]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 07:17:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maksims (do not use this acc), 2016-07-19 09:41:00]

The CQ bit was unchecked by maksim.sisov@intel.com

[maksims (do not use this acc), 2016-07-19 12:06:36]

Patchset #2 (id:220001) has been deleted

[maksims (do not use this acc), 2016-07-19 12:06:37]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 12:06:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 13:51:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-19 13:51:06]

Dry run: This issue passed the CQ dry run.

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

dns_api and socket_api are fixed. No need to explicitly delete the request after
a resolve job is done

https://codereview.chromium.org/2116983002/diff/160001/extensions/browser/api...
File extensions/browser/api/dns/dns_api.cc (right):

https://codereview.chromium.org/2116983002/diff/160001/extensions/browser/api...
extensions/browser/api/dns/dns_api.cc:28: request_handle_(nullptr),
On 2016/07/12 15:04:51, Devlin wrote:
> unnecessary (this is the default construction of std::unique_ptr)

Done.

https://codereview.chromium.org/2116983002/diff/160001/extensions/browser/api...
extensions/browser/api/dns/dns_api.cc:91: // destructor is not called
automatically.
On 2016/07/14 18:25:31, mmenke wrote:
> On 2016/07/14 12:02:47, maksims wrote:
> > On 2016/07/12 15:04:51, Devlin wrote:
> > > I don't quite follow this comment.  Automatically when?  Is it not called
> here
> > > after reset()?  I'd find that very surprising (and worrying).
> > 
> > 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...
> 
> 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.

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

https://codereview.chromium.org/2116983002/diff/160001/extensions/browser/api...
extensions/browser/api/dns/dns_api.cc:91: // destructor is not called
automatically.
On 2016/07/12 15:04:51, Devlin wrote:
> I don't quite follow this comment.  Automatically when?  Is it not called here
> after reset()?  I'd find that very surprising (and worrying).

Done.

https://codereview.chromium.org/2116983002/diff/160001/extensions/browser/api...
extensions/browser/api/dns/dns_api.cc:91: // destructor is not called
automatically.
On 2016/07/14 12:02:47, maksims wrote:
> On 2016/07/12 15:04:51, Devlin wrote:
> > I don't quite follow this comment.  Automatically when?  Is it not called
here
> > after reset()?  I'd find that very surprising (and worrying).
> 
> 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...

Done.

https://codereview.chromium.org/2116983002/diff/160001/extensions/browser/api...
extensions/browser/api/dns/dns_api.cc:91: // destructor is not called
automatically.
On 2016/07/14 18:25:31, mmenke wrote:
> On 2016/07/14 12:02:47, maksims wrote:
> > On 2016/07/12 15:04:51, Devlin wrote:
> > > I don't quite follow this comment.  Automatically when?  Is it not called
> here
> > > after reset()?  I'd find that very surprising (and worrying).
> > 
> > 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...
> 
> 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.

Done.

https://codereview.chromium.org/2116983002/diff/160001/extensions/browser/api...
File extensions/browser/api/socket/socket_api.cc (right):

https://codereview.chromium.org/2116983002/diff/160001/extensions/browser/api...
extensions/browser/api/socket/socket_api.cc:335: request_handle_.reset();
On 2016/07/12 15:04:51, Devlin wrote:
> ditto

Done.

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver.h
File net/dns/host_resolver.h (right):

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver....
net/dns/host_resolver.h:45: class NET_EXPORT Request {
On 2016/07/12 18:50:23, mmenke wrote:
> Document this class.
> 
> (Owned by the consumer, deletion cancels the request)

Done.

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver....
net/dns/host_resolver.h:154: // completed.
On 2016/07/12 18:50:23, mmenke wrote:
> This entire paragraph needs to be re-written.

Done.

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver....
net/dns/host_resolver.h:161: std::unique_ptr<Request>* out_req,
On 2016/07/12 18:50:23, mmenke wrote:
> On 2016/07/12 15:04:51, Devlin wrote:
> > I'm not familiar with the implementation here at all, so more of a question
> than
> > a comment.
> > 
> > It looks like we now link the Job with the request - is there a lifetime
> > requirement related to the out_req?  e.g., if I pass in a stack-allocated
> > std::unique_ptr<Request> that's deleted right after the call to this, and
the
> > Job finishes async, will that cause a crash?  We should document any
lifetime
> > requirements here.
> 
> No - it should be deleted the cancel the request.  IT was already implicitly
> linked to the job before, this just makes cancellation less prone to
bugginess.
> 
> This should be documented.

Done.

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver_...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:1422: requests_[req_position] = nullptr;
It should be better now considering changed the request's implementation.

https://codereview.chromium.org/2116983002/diff/160001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:1507: if (requests_.empty()) {
Reverted as long as new implementation allows old way of doing things here.

[mmenke, 2016-07-19 19:03:57]

Wow!  I did not realize how many mock implementations of HostResolver we had.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver.h
File net/dns/host_resolver.h (right):

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver....
net/dns/host_resolver.h:55: // completion callback has already run or the
request was canceled.
This comment is outdated (There is no |req| or specified request,
ChangeRequestPriority can't be called after the request was cancelled, since the
way to cancel the request is now bug deleting it)

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver....
net/dns/host_resolver.h:163: // has completed, [out_req] is detached from the
job.
job is an internal implementation detail of the HostResolverImpl, so shouldn't
be mentioned in the HostResolver docs.  I think this paragraph can be combined
with the previous one, and say something like:

"|out_req| must be non-NULL. If the request is not completed synchronously, it
will be filled with a handle to the request. It must be completed or destroyed
before the HostResolver itself is destroyed."

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver....
net/dns/host_resolver.h:167: // request will not be completed.
Maybe delete "during a resolve job", and remove the second sentence?  Or maybe
replace with paragraph with something like "Deleting |out_req| will cancel the
request, and cause |callback| not to be invoked."

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:545: // Deletion cancels the outstanding request.
nit:  Comment not needed, since this is part of the HostResolver::Request
contract.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:1388: void DetachRequest(RequestImpl* request) {
I think this can be renamed CancelRequest again?  It's only called on
cancellation, not on success.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:1412: CHECK(it != requests_.end());
nit:  Use DCHECK, to avoid increasing binary size, unless you're particularly
worried about it failing, in which case, land it with with a CHECK, and switch
to to DCHECK fairly promptly..

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:1909: DCHECK_EQ(false, callback.is_null());
DCHECK(out_req);

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:1965: *out_req = std::move(req);
The old code let out_req by NULL, this code does not.  It looks like some
callers need still to be updated - see
https://cs.chromium.org/chromium/src/chrome/browser/devtools/device/port_forw...,
for instance.

Unfortunately, looks like some consumers of this class are not currently being
tested.  Should do a search for others - to do this, go to
https://cs.chromium.org/chromium/src/net/dns/host_resolver.h?sq=package:chrom...
and click on Resolve, and it will show you all callers at the bottom of the
screen, and you can look through them (Have to jump to each one, individually,
unfortunately).

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
File net/dns/host_resolver_impl_fuzzer.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl_fuzzer.cc:41: request_(),
this isn't needed.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl_fuzzer.cc:46: Cancel();
This is no longer needed - deleting this DnsRequest will delete request_, which
will cancel the request.  Which is one of the nice things about the new API.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl_fuzzer.cc:169: CHECK(!is_running_);
I'd suggest getting rid of is_running_, and DCHECK on request_ here instead. 
Also need to reset request_ in OnCallback, where is_running_ is set to false.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl_fuzzer.cc:176: request_.reset();
nit:  Can unconditionally reset the request here.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
File net/dns/host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl_unittest.cc:228: request_() {}
not needed.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl_unittest.cc:337: ;
Remove this line.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
File net/dns/host_resolver_mojo.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_mojo.cc:59: ~RequestImpl() override { job_.reset(); }
You can just give this an empty body - job_ will be destroyed since it's stored
in a unique_ptr.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_mojo.cc:96: (*request).reset(new
RequestImpl(std::move(job)));
nit:  request->reset(new RequestImpl(std::move(job));

https://codereview.chromium.org/2116983002/diff/240001/net/dns/mock_host_reso...
File net/dns/mock_host_resolver.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/dns/mock_host_reso...
net/dns/mock_host_resolver.cc:79: void OnResolveCancelled(MockHostResolverBase*
resolver_) {
This isn't used.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/mock_host_reso...
net/dns/mock_host_resolver.cc:108: requests_.clear();
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.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/single_request...
File net/dns/single_request_host_resolver.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/dns/single_request...
net/dns/single_request_host_resolver.cc:57: if
(!cur_request_callback_.is_null()) {
This check id no longer needed.  Clearing already empty objects is pretty
lightweight, and removing the condition marginally reduces code size.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/single_request...
File net/dns/single_request_host_resolver_unittest.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/dns/single_request...
net/dns/single_request_host_resolver_unittest.cc:48:
(*request).reset(outstanding_request_);
nit:  request->reset

https://codereview.chromium.org/2116983002/diff/240001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing.cc:342: pending_dns_() {
Not needed.

https://codereview.chromium.org/2116983002/diff/240001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing.cc:397: if (pending_dns_)
Can get rid of this check

https://codereview.chromium.org/2116983002/diff/240001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_v8_tracing_unittest.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:703: (*out_req).reset(new
RequestImpl(this));
out_req->reset

https://codereview.chromium.org/2116983002/diff/240001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_v8_tracing_wrapper_unittest.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing_wrapper_unittest.cc:789:
(*out_req).reset(new RequestImpl(this));
out_req->reset

https://codereview.chromium.org/2116983002/diff/240001/net/socket/socks5_clie...
File net/socket/socks5_client_socket_unittest.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/socket/socks5_clie...
net/socket/socks5_client_socket_unittest.cc:68:
std::unique_ptr<HostResolver::Request> request_;
This doesn't need to be a member of the class - can move this down into SetUp,
since it's only used there.

https://codereview.chromium.org/2116983002/diff/240001/net/socket/socks_clien...
File net/socket/socks_client_socket_unittest.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/socket/socks_clien...
net/socket/socks_client_socket_unittest.cc:108: class RequestImpl;
Can't have a private section before the public section, per google style guide..

https://codereview.chromium.org/2116983002/diff/240001/net/socket/socks_clien...
net/socket/socks_client_socket_unittest.cc:123:
(*out_req).reset(outstanding_request_);
out_req->reset

https://codereview.chromium.org/2116983002/diff/240001/net/socket/transport_c...
File net/socket/transport_client_socket_unittest.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/socket/transport_c...
net/socket/transport_client_socket_unittest.cc:99:
std::unique_ptr<HostResolver::Request> request_;
This can just be a local in SetUp.

[maksims (do not use this acc), 2016-07-20 06:18:52]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 06:19:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 06:38:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 06:38:01]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[maksims (do not use this acc), 2016-07-20 11:56:32]

Patchset #3 (id:260001) has been deleted

[maksims (do not use this acc), 2016-07-20 11:57:03]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 11:57:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 12:06:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 12:06:25]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[maksims (do not use this acc), 2016-07-20 15:11:45]

Patchset #3 (id:280001) has been deleted

[maksims (do not use this acc), 2016-07-20 15:42:12]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 15:42:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 15:50:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 15:50:06]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[maksims (do not use this acc), 2016-07-20 15:53:38]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 15:54:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 16:00:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 16:00:51]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[maksims (do not use this acc), 2016-07-21 05:57:36]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 05:58:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maksims (do not use this acc), 2016-07-21 07:11:48]

Patchset #4 (id:320001) has been deleted

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

Patchset #3 (id:300001) has been deleted

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

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver.h
File net/dns/host_resolver.h (right):

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver....
net/dns/host_resolver.h:55: // completion callback has already run or the
request was canceled.
On 2016/07/19 19:03:55, mmenke wrote:
> This comment is outdated (There is no |req| or specified request,
> ChangeRequestPriority can't be called after the request was cancelled, since
the
> way to cancel the request is now bug deleting it)

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver....
net/dns/host_resolver.h:163: // has completed, [out_req] is detached from the
job.
On 2016/07/19 19:03:55, mmenke wrote:
> job is an internal implementation detail of the HostResolverImpl, so shouldn't
> be mentioned in the HostResolver docs.  I think this paragraph can be combined
> with the previous one, and say something like:
> 
> "|out_req| must be non-NULL. If the request is not completed synchronously, it
> will be filled with a handle to the request. It must be completed or destroyed
> before the HostResolver itself is destroyed."

Requests can even be cancelled after the HostResolver itself is destroyed. There
are no restrictions for that.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver....
net/dns/host_resolver.h:167: // request will not be completed.
On 2016/07/19 19:03:55, mmenke wrote:
> Maybe delete "during a resolve job", and remove the second sentence?  Or maybe
> replace with paragraph with something like "Deleting |out_req| will cancel the
> request, and cause |callback| not to be invoked."

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:545: // Deletion cancels the outstanding request.
On 2016/07/19 19:03:55, mmenke wrote:
> nit:  Comment not needed, since this is part of the HostResolver::Request
> contract.

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:1388: void DetachRequest(RequestImpl* request) {
On 2016/07/19 19:03:55, mmenke wrote:
> I think this can be renamed CancelRequest again?  It's only called on
> cancellation, not on success.

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:1412: CHECK(it != requests_.end());
On 2016/07/19 19:03:55, mmenke wrote:
> nit:  Use DCHECK, to avoid increasing binary size, unless you're particularly
> worried about it failing, in which case, land it with with a CHECK, and switch
> to to DCHECK fairly promptly..

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:1909: DCHECK_EQ(false, callback.is_null());
On 2016/07/19 19:03:56, mmenke wrote:
> DCHECK(out_req);

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl.cc:1965: *out_req = std::move(req);
On 2016/07/19 19:03:56, mmenke wrote:
> The old code let out_req by NULL, this code does not.  It looks like some
> callers need still to be updated - see
>
https://cs.chromium.org/chromium/src/chrome/browser/devtools/device/port_forw...,
> for instance.
> 
> Unfortunately, looks like some consumers of this class are not currently being
> tested.  Should do a search for others - to do this, go to
>
https://cs.chromium.org/chromium/src/net/dns/host_resolver.h?sq=package:chrom...
> and click on Resolve, and it will show you all callers at the bottom of the
> screen, and you can look through them (Have to jump to each one, individually,
> unfortunately).

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
File net/dns/host_resolver_impl_fuzzer.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl_fuzzer.cc:41: request_(),
On 2016/07/19 19:03:56, mmenke wrote:
> this isn't needed.

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl_fuzzer.cc:46: Cancel();
On 2016/07/19 19:03:56, mmenke wrote:
> This is no longer needed - deleting this DnsRequest will delete request_,
which
> will cancel the request.  Which is one of the nice things about the new API.

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl_fuzzer.cc:169: CHECK(!is_running_);
On 2016/07/19 19:03:56, mmenke wrote:
> I'd suggest getting rid of is_running_, and DCHECK on request_ here instead. 
> Also need to reset request_ in OnCallback, where is_running_ is set to false.

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl_fuzzer.cc:176: request_.reset();
On 2016/07/19 19:03:56, mmenke wrote:
> nit:  Can unconditionally reset the request here.

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
File net/dns/host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl_unittest.cc:228: request_() {}
On 2016/07/19 19:03:56, mmenke wrote:
> not needed.

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_impl_unittest.cc:337: ;
On 2016/07/19 19:03:56, mmenke wrote:
> Remove this line.

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
File net/dns/host_resolver_mojo.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_mojo.cc:59: ~RequestImpl() override { job_.reset(); }
On 2016/07/19 19:03:56, mmenke wrote:
> You can just give this an empty body - job_ will be destroyed since it's
stored
> in a unique_ptr.

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/host_resolver_...
net/dns/host_resolver_mojo.cc:96: (*request).reset(new
RequestImpl(std::move(job)));
On 2016/07/19 19:03:56, mmenke wrote:
> nit:  request->reset(new RequestImpl(std::move(job));

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/mock_host_reso...
File net/dns/mock_host_resolver.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/dns/mock_host_reso...
net/dns/mock_host_resolver.cc:79: void OnResolveCancelled(MockHostResolverBase*
resolver_) {
On 2016/07/19 19:03:56, mmenke wrote:
> This isn't used.

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/mock_host_reso...
net/dns/mock_host_resolver.cc:108: requests_.clear();
On 2016/07/19 19:03:56, mmenke wrote:
> 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.

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/single_request...
File net/dns/single_request_host_resolver.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/dns/single_request...
net/dns/single_request_host_resolver.cc:57: if
(!cur_request_callback_.is_null()) {
On 2016/07/19 19:03:56, mmenke wrote:
> This check id no longer needed.  Clearing already empty objects is pretty
> lightweight, and removing the condition marginally reduces code size.

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/dns/single_request...
File net/dns/single_request_host_resolver_unittest.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/dns/single_request...
net/dns/single_request_host_resolver_unittest.cc:48:
(*request).reset(outstanding_request_);
On 2016/07/19 19:03:56, mmenke wrote:
> nit:  request->reset

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing.cc:342: pending_dns_() {
On 2016/07/19 19:03:56, mmenke wrote:
> Not needed.

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing.cc:397: if (pending_dns_)
On 2016/07/19 19:03:56, mmenke wrote:
> Can get rid of this check

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_v8_tracing_unittest.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing_unittest.cc:703: (*out_req).reset(new
RequestImpl(this));
On 2016/07/19 19:03:56, mmenke wrote:
> out_req->reset

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/socket/socks5_clie...
File net/socket/socks5_client_socket_unittest.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/socket/socks5_clie...
net/socket/socks5_client_socket_unittest.cc:68:
std::unique_ptr<HostResolver::Request> request_;
On 2016/07/19 19:03:56, mmenke wrote:
> This doesn't need to be a member of the class - can move this down into SetUp,
> since it's only used there.

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/socket/socks_clien...
File net/socket/socks_client_socket_unittest.cc (right):

https://codereview.chromium.org/2116983002/diff/240001/net/socket/socks_clien...
net/socket/socks_client_socket_unittest.cc:108: class RequestImpl;
On 2016/07/19 19:03:56, mmenke wrote:
> Can't have a private section before the public section, per google style
guide..

Done.

https://codereview.chromium.org/2116983002/diff/240001/net/socket/socks_clien...
net/socket/socks_client_socket_unittest.cc:123:
(*out_req).reset(outstanding_request_);
On 2016/07/19 19:03:56, mmenke wrote:
> out_req->reset

Done.

[commit-bot: I haz the power, 2016-07-21 07:13:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 07:13:09]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-07-21 16:00:41]

LGTM, modulo comments.

https://codereview.chromium.org/2116983002/diff/340001/chrome/browser/devtool...
File chrome/browser/devtools/device/port_forwarding_controller.cc (right):

https://codereview.chromium.org/2116983002/diff/340001/chrome/browser/devtool...
chrome/browser/devtools/device/port_forwarding_controller.cc:69:
std::unique_ptr<net::HostResolver::Request> request;
You don't need this - if result != ERR_IO_PENDING, then host_reolver_->Resolve
doesn't modify its request parameter, anyways.

https://codereview.chromium.org/2116983002/diff/340001/chrome/browser/devtool...
File chrome/browser/devtools/device/tcp_device_provider.cc (right):

https://codereview.chromium.org/2116983002/diff/340001/chrome/browser/devtool...
chrome/browser/devtools/device/tcp_device_provider.cc:39:
std::unique_ptr<net::HostResolver::Request> request;
Not needed (Note that there's also a bug below - you're passing request_ to
Resolve instead of request, but getting rid of request will result in fixing it,
anyways)

https://codereview.chromium.org/2116983002/diff/340001/chromeos/network/host_...
File chromeos/network/host_resolver_impl_chromeos_unittest.cc (right):

https://codereview.chromium.org/2116983002/diff/340001/chromeos/network/host_...
chromeos/network/host_resolver_impl_chromeos_unittest.cc:103:
std::unique_ptr<net::HostResolver::Request> request_;
This should be moved below all methods in this class.

https://codereview.chromium.org/2116983002/diff/340001/extensions/browser/api...
File extensions/browser/api/dns/dns_api.cc (right):

https://codereview.chromium.org/2116983002/diff/340001/extensions/browser/api...
extensions/browser/api/dns/dns_api.cc:28: request_(),
Not needed

https://codereview.chromium.org/2116983002/diff/340001/extensions/browser/api...
extensions/browser/api/dns/dns_api.cc:61:
std::unique_ptr<net::HostResolver::Request> request;
Per other comments, this isn't needed.

https://codereview.chromium.org/2116983002/diff/340001/extensions/browser/api...
File extensions/browser/api/socket/socket_api.cc (right):

https://codereview.chromium.org/2116983002/diff/340001/extensions/browser/api...
extensions/browser/api/socket/socket_api.cc:207:
std::unique_ptr<net::HostResolver::Request> request;
Not needed.

https://codereview.chromium.org/2116983002/diff/340001/net/tools/gdig/gdig.cc
File net/tools/gdig/gdig.cc (right):

https://codereview.chromium.org/2116983002/diff/340001/net/tools/gdig/gdig.cc...
net/tools/gdig/gdig.cc:469: std::unique_ptr<HostResolver::Request> request;
Not needed.

[mmenke, 2016-07-21 16:01:52]

On 2016/07/21 16:00:41, mmenke wrote:
> LGTM, modulo comments.

(And, for the record, I double-checked for missed Resolve() calls, and didn't
find any)

[mmenke, 2016-07-21 16:06:06]

mmenke@chromium.org changed reviewers:
- jam@chromium.org

[maksims (do not use this acc), 2016-07-22 10:14:26]

maksim.sisov@intel.com changed reviewers:
+ cpu@chromium.org
- asanka@chromium.org, rdevlin.cronin@chromium.org

[maksims (do not use this acc), 2016-07-22 10:16:00]

Would you please take a look? I think I still need your approvals.

cpu@:
chrome/browser/devtools/device/port_forwarding_controller.cc 
chrome/browser/devtools/device/tcp_device_provider.cc
extensions/browser/api/dns/dns_api.cc
extensions/browser/api/dns/dns_api.h
extensions/browser/api/socket/socket_api.cc
extensions/browser/api/socket/socket_api.h

gauravsh@:
chromeos/network/host_resolver_impl_chromeos.cc
chromeos/network/host_resolver_impl_chromeos.h
chromeos/network/host_resolver_impl_chromeos_unittest.cc

https://codereview.chromium.org/2116983002/diff/340001/chrome/browser/devtool...
File chrome/browser/devtools/device/port_forwarding_controller.cc (right):

https://codereview.chromium.org/2116983002/diff/340001/chrome/browser/devtool...
chrome/browser/devtools/device/port_forwarding_controller.cc:69:
std::unique_ptr<net::HostResolver::Request> request;
On 2016/07/21 16:00:40, mmenke wrote:
> You don't need this - if result != ERR_IO_PENDING, then host_reolver_->Resolve
> doesn't modify its request parameter, anyways.

Done.

https://codereview.chromium.org/2116983002/diff/340001/chrome/browser/devtool...
File chrome/browser/devtools/device/tcp_device_provider.cc (right):

https://codereview.chromium.org/2116983002/diff/340001/chrome/browser/devtool...
chrome/browser/devtools/device/tcp_device_provider.cc:39:
std::unique_ptr<net::HostResolver::Request> request;
On 2016/07/21 16:00:40, mmenke wrote:
> Not needed (Note that there's also a bug below - you're passing request_ to
> Resolve instead of request, but getting rid of request will result in fixing
it,
> anyways)

Ops...

https://codereview.chromium.org/2116983002/diff/340001/chromeos/network/host_...
File chromeos/network/host_resolver_impl_chromeos_unittest.cc (right):

https://codereview.chromium.org/2116983002/diff/340001/chromeos/network/host_...
chromeos/network/host_resolver_impl_chromeos_unittest.cc:103:
std::unique_ptr<net::HostResolver::Request> request_;
On 2016/07/21 16:00:40, mmenke wrote:
> This should be moved below all methods in this class.

Done.

https://codereview.chromium.org/2116983002/diff/340001/extensions/browser/api...
File extensions/browser/api/dns/dns_api.cc (right):

https://codereview.chromium.org/2116983002/diff/340001/extensions/browser/api...
extensions/browser/api/dns/dns_api.cc:28: request_(),
On 2016/07/21 16:00:40, mmenke wrote:
> Not needed

Done.

https://codereview.chromium.org/2116983002/diff/340001/extensions/browser/api...
extensions/browser/api/dns/dns_api.cc:61:
std::unique_ptr<net::HostResolver::Request> request;
On 2016/07/21 16:00:40, mmenke wrote:
> Per other comments, this isn't needed.

Done.

https://codereview.chromium.org/2116983002/diff/340001/extensions/browser/api...
File extensions/browser/api/socket/socket_api.cc (right):

https://codereview.chromium.org/2116983002/diff/340001/extensions/browser/api...
extensions/browser/api/socket/socket_api.cc:207:
std::unique_ptr<net::HostResolver::Request> request;
On 2016/07/21 16:00:40, mmenke wrote:
> Not needed.

Done.

https://codereview.chromium.org/2116983002/diff/340001/net/tools/gdig/gdig.cc
File net/tools/gdig/gdig.cc (right):

https://codereview.chromium.org/2116983002/diff/340001/net/tools/gdig/gdig.cc...
net/tools/gdig/gdig.cc:469: std::unique_ptr<HostResolver::Request> request;
On 2016/07/21 16:00:40, mmenke wrote:
> Not needed.

Done.

[maksims (do not use this acc), 2016-07-22 10:16:23]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 10:16:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maksims (do not use this acc), 2016-07-22 10:24:20]

Patchset #4 (id:360001) has been deleted

[maksims (do not use this acc), 2016-07-22 10:25:07]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-22 10:25:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 11:28:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-22 11:28:55]

Dry run: This issue passed the CQ dry run.

[maksims (do not use this acc), 2016-07-25 05:02:40]

Reminder:

cpu@:
chrome/browser/devtools/device/port_forwarding_controller.cc 
chrome/browser/devtools/device/tcp_device_provider.cc
extensions/browser/api/dns/dns_api.cc
extensions/browser/api/dns/dns_api.h
extensions/browser/api/socket/socket_api.cc
extensions/browser/api/socket/socket_api.h

gauravsh@:
chromeos/network/host_resolver_impl_chromeos.cc
chromeos/network/host_resolver_impl_chromeos.h
chromeos/network/host_resolver_impl_chromeos_unittest.cc

[gauravsh, 2016-07-25 17:01:07]

maksims: Sorry, I just saw this. For chromeos/network, you should get an OWNERS
approval from someone else (try
https://src.chromium.org/viewvc/chrome/trunk/src/chromeos/OWNERS). I haven't
worked on this project or touched this code in a long while. I am going to
submit a CL to remove myself as an OWNER. Thanks!


On 2016/07/25 05:02:40, maksims wrote:
> Reminder:
> 
> cpu@:
> chrome/browser/devtools/device/port_forwarding_controller.cc 
> chrome/browser/devtools/device/tcp_device_provider.cc
> extensions/browser/api/dns/dns_api.cc
> extensions/browser/api/dns/dns_api.h
> extensions/browser/api/socket/socket_api.cc
> extensions/browser/api/socket/socket_api.h
> 
> gauravsh@:
> chromeos/network/host_resolver_impl_chromeos.cc
> chromeos/network/host_resolver_impl_chromeos.h
> chromeos/network/host_resolver_impl_chromeos_unittest.cc

[gauravsh, 2016-07-25 17:01:09]

maksims: Sorry, I just saw this. For chromeos/network, you should get an OWNERS
approval from someone else (try
https://src.chromium.org/viewvc/chrome/trunk/src/chromeos/OWNERS). I haven't
worked on this project or touched this code in a long while. I am going to
submit a CL to remove myself as an OWNER. Thanks!


On 2016/07/25 05:02:40, maksims wrote:
> Reminder:
> 
> cpu@:
> chrome/browser/devtools/device/port_forwarding_controller.cc 
> chrome/browser/devtools/device/tcp_device_provider.cc
> extensions/browser/api/dns/dns_api.cc
> extensions/browser/api/dns/dns_api.h
> extensions/browser/api/socket/socket_api.cc
> extensions/browser/api/socket/socket_api.h
> 
> gauravsh@:
> chromeos/network/host_resolver_impl_chromeos.cc
> chromeos/network/host_resolver_impl_chromeos.h
> chromeos/network/host_resolver_impl_chromeos_unittest.cc

[stevenjb, 2016-07-25 17:15:55]

stevenjb@chromium.org changed reviewers:
+ stevenjb@chromium.org

[stevenjb, 2016-07-25 17:15:56]

chromeos/ RS LGTM

[mmenke, 2016-07-25 21:10:44]

On 2016/07/25 05:02:40, maksims wrote:
> Reminder:
> 
> cpu@:
> chrome/browser/devtools/device/port_forwarding_controller.cc 
> chrome/browser/devtools/device/tcp_device_provider.cc
> extensions/browser/api/dns/dns_api.cc
> extensions/browser/api/dns/dns_api.h
> extensions/browser/api/socket/socket_api.cc
> extensions/browser/api/socket/socket_api.h
> 
> gauravsh@:
> chromeos/network/host_resolver_impl_chromeos.cc
> chromeos/network/host_resolver_impl_chromeos.h
> chromeos/network/host_resolver_impl_chromeos_unittest.cc

Looks like cpu hasn't landed any code in chrome in 4 months.  Not sure he's
still active.

[maksims (do not use this acc), 2016-07-26 04:47:33]

maksim.sisov@intel.com changed reviewers:
+ brettw@chromium.org, jam@chromium.org
- cpu@chromium.org, gauravsh@chromium.org

[maksims (do not use this acc), 2016-07-26 04:48:25]

brettw@ or jam@, would you please review the following: 

> chrome/browser/devtools/device/port_forwarding_controller.cc 
> chrome/browser/devtools/device/tcp_device_provider.cc
> extensions/browser/api/dns/dns_api.cc
> extensions/browser/api/dns/dns_api.h
> extensions/browser/api/socket/socket_api.cc
> extensions/browser/api/socket/socket_api.h

[mmenke, 2016-07-26 11:49:52]

On 2016/07/26 04:48:25, maksims wrote:
> brettw@ or jam@, would you please review the following: 
> 
> > chrome/browser/devtools/device/port_forwarding_controller.cc 
> > chrome/browser/devtools/device/tcp_device_provider.cc
> > extensions/browser/api/dns/dns_api.cc
> > extensions/browser/api/dns/dns_api.h
> > extensions/browser/api/socket/socket_api.cc
> > extensions/browser/api/socket/socket_api.h

You should prefer not to use top-level owners.  They own a ton of files, and as
a result, should only be used as a last resort.  in this case, you should use an
extensions owner and a chrome/browser/devtools owner.

[maksims (do not use this acc), 2016-07-26 20:15:46]

maksim.sisov@intel.com changed reviewers:
+ dgozman@chromium.org, mek@chromium.org
- brettw@chromium.org, jam@chromium.org

[maksims (do not use this acc), 2016-07-26 20:16:53]

On 2016/07/26 11:49:52, mmenke wrote:
> On 2016/07/26 04:48:25, maksims wrote:
> > brettw@ or jam@, would you please review the following: 
> > 
> > > chrome/browser/devtools/device/port_forwarding_controller.cc 
> > > chrome/browser/devtools/device/tcp_device_provider.cc
> > > extensions/browser/api/dns/dns_api.cc
> > > extensions/browser/api/dns/dns_api.h
> > > extensions/browser/api/socket/socket_api.cc
> > > extensions/browser/api/socket/socket_api.h
> 
> You should prefer not to use top-level owners.  They own a ton of files, and
as
> a result, should only be used as a last resort.  in this case, you should use
an
> extensions owner and a chrome/browser/devtools owner.

Oh, I see.

dgozman, would you please review chrome/browser/devtools/device/* files?

mek, would like to ask you to look at extensions/browser/api/*. Thank you

[dgozman, 2016-07-26 20:28:16]

devtools lgtm

[Marijn Kruisselbrink, 2016-07-26 20:43:51]

extensions lgtm

[maksims (do not use this acc), 2016-07-26 20:44:36]

Thanks!

[maksims (do not use this acc), 2016-07-26 20:44:48]

The CQ bit was checked by maksim.sisov@intel.com

[maksims (do not use this acc), 2016-07-26 20:44:48]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2116983002/#ps380001
(title: "comments")

[commit-bot: I haz the power, 2016-07-26 20:45:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-26 20:53:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-26 20:53:42]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[maksims (do not use this acc), 2016-07-27 04:48:03]

The CQ bit was unchecked by maksim.sisov@intel.com

[maksims (do not use this acc), 2016-07-27 04:48:04]

The CQ bit was checked by maksim.sisov@intel.com

[maksims (do not use this acc), 2016-07-27 04:48:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, mek@chromium.org, dgozman@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2116983002/#ps400001
(title: "rebased")

[commit-bot: I haz the power, 2016-07-27 04:48:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 04:56:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-27 04:56:50]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[maksims (do not use this acc), 2016-07-27 05:02:08]

On 2016/07/26 11:49:52, mmenke wrote:
> On 2016/07/26 04:48:25, maksims wrote:
> > brettw@ or jam@, would you please review the following: 
> > 
> > > chrome/browser/devtools/device/port_forwarding_controller.cc 
> > > chrome/browser/devtools/device/tcp_device_provider.cc
> > > extensions/browser/api/dns/dns_api.cc
> > > extensions/browser/api/dns/dns_api.h
> > > extensions/browser/api/socket/socket_api.cc
> > > extensions/browser/api/socket/socket_api.h
> 
> You should prefer not to use top-level owners.  They own a ton of files, and
as
> a result, should only be used as a last resort.  in this case, you should use
an
> extensions owner and a chrome/browser/devtools owner.

Seems like somebody has written some new code while this cl has been under
review.

[maksims (do not use this acc), 2016-07-27 05:02:46]

> Seems like somebody has written some new code while this cl has been under
> review.

That's why commit does not work now.

[maksims (do not use this acc), 2016-07-27 05:22:07]

The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 05:22:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2016-07-27 05:23:31]

On 2016/07/27 05:02:46, maksims wrote:
> > Seems like somebody has written some new code while this cl has been under
> > review.
> 
> That's why commit does not work now.

Happens all the time.  Just update your local checkout with git pull (If you use
branches on top of a local master, have to update that branch one first). 
Resolve compile errors.  If the changes are trivial and you don't need new
owners to sign off, fine to just land.  If you want someone to take another look
at the changes, just ask.

[commit-bot: I haz the power, 2016-07-27 06:25:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-27 06:25:33]

Dry run: This issue passed the CQ dry run.

[maksims (do not use this acc), 2016-07-27 06:33:44]

The CQ bit was checked by maksim.sisov@intel.com

[maksims (do not use this acc), 2016-07-27 06:33:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, mek@chromium.org, dgozman@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2116983002/#ps420001
(title: "http_stream_factory_impl_job_controller_unittest RequestHandle* to
unique_ptr")

[commit-bot: I haz the power, 2016-07-27 06:34:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 06:38:25]

Description was changed from

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

This cl has the following changes:
1) New class HostResolver::Request: it contains all the 
necessary data for the actual request.

2) HostResolver::Resolve() takes an 
std::unique_ptr<Request>* rather than a RequestHandle* now.

3) HostResolver::CancelRequest() method is removed. If 
consumers want to cancel their requests, request.reset() 
should be called.

BUG=486264
==========

to

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

This cl has the following changes:
1) New class HostResolver::Request: it contains all the 
necessary data for the actual request.

2) HostResolver::Resolve() takes an 
std::unique_ptr<Request>* rather than a RequestHandle* now.

3) HostResolver::CancelRequest() method is removed. If 
consumers want to cancel their requests, request.reset() 
should be called.

BUG=486264
==========

[commit-bot: I haz the power, 2016-07-27 06:38:27]

Committed patchset #6 (id:420001)

[commit-bot: I haz the power, 2016-07-27 06:40:45]

Description was changed from

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

This cl has the following changes:
1) New class HostResolver::Request: it contains all the 
necessary data for the actual request.

2) HostResolver::Resolve() takes an 
std::unique_ptr<Request>* rather than a RequestHandle* now.

3) HostResolver::CancelRequest() method is removed. If 
consumers want to cancel their requests, request.reset() 
should be called.

BUG=486264
==========

to

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

This cl has the following changes:
1) New class HostResolver::Request: it contains all the
necessary data for the actual request.

2) HostResolver::Resolve() takes an
std::unique_ptr<Request>* rather than a RequestHandle* now.

3) HostResolver::CancelRequest() method is removed. If
consumers want to cancel their requests, request.reset()
should be called.

BUG=486264

Committed: https://crrev.com/31452af8fb1192199be24016b3701feb09b31c2d
Cr-Commit-Position: refs/heads/master@{#408062}
==========

[commit-bot: I haz the power, 2016-07-27 06:40:47]

Patchset 6 (id:??) landed as
https://crrev.com/31452af8fb1192199be24016b3701feb09b31c2d
Cr-Commit-Position: refs/heads/master@{#408062}

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

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

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

https://codereview.chromium.org/2116983002/diff/420001/net/proxy/proxy_resolv...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/2116983002/diff/420001/net/proxy/proxy_resolv...
net/proxy/proxy_resolver_v8_tracing.cc:287:
std::unique_ptr<HostResolver::Request> pending_dns_;
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