[net] Ensure aborted HostResolverImpl::Jobs release slots in the dispatcher.

[net] Ensure aborted HostResolverImpl::Jobs release slots in the dispatcher.

Also re-introduces a missing life check in case a request callback deletes the HostResolver.

R=eroman@chromium.org,cbentzel@chromium.org
BUG=115399
TEST=./net_unittest --gtest_filter=HostResolverImplTest.CanceledRequestsReleaseJobSlots


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124783

[szym, 2012-03-02 18:29:38]

The new test times out before the (one-line) fix and should run smoothly after
the fix.

https://chromiumcodereview.appspot.com/9572018/diff/1/net/base/host_resolver_...
File net/base/host_resolver_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/9572018/diff/1/net/base/host_resolver_...
net/base/host_resolver_impl_unittest.cc:95: class WaitingHostResolverProc :
public HostResolverProc {
This is moved here from :470, removed the ctor variant with |manual_reset| since
CountingHostResolverProc better suits the
AbortOnlyExistingRequestsOnIPAddressChange test.

[cbentzel, 2012-03-02 18:47:24]

It looks like there are two changes - one for CancelRequest and one for
AbortAllInProgressJobs. Are both needed?

On Fri, Mar 2, 2012 at 1:29 PM, <szym@chromium.org> wrote:

> The new test times out before the (one-line) fix and should run smoothly
> after
> the fix.
>
>
> https://chromiumcodereview.**appspot.com/9572018/diff/1/**
>
net/base/host_resolver_impl_**unittest.cc<https://chromiumcodereview.appspot.com/9572018/diff/1/net/base/host_resolver_impl_unittest.cc>
> File net/base/host_resolver_impl_**unittest.cc (right):
>
> https://chromiumcodereview.**appspot.com/9572018/diff/1/**
>
net/base/host_resolver_impl_**unittest.cc#newcode95<https://chromiumcodereview.appspot.com/9572018/diff/1/net/base/host_resolver_impl_unittest.cc#newcode95>
> net/base/host_resolver_impl_**unittest.cc:95: class
> WaitingHostResolverProc : public HostResolverProc {
> This is moved here from :470, removed the ctor variant with
> |manual_reset| since CountingHostResolverProc better suits the
> AbortOnlyExistingRequestsOnIPA**ddressChange test.
>
>
https://chromiumcodereview.**appspot.com/9572018/<https://chromiumcodereview....
>

[szym, 2012-03-02 18:51:36]

On 2012/03/02 18:47:24, cbentzel wrote:
> It looks like there are two changes - one for CancelRequest and one for
> AbortAllInProgressJobs. Are both needed?

The check: 
  if (!self) return;
has been accidentally removed in:
http://codereview.chromium.org/9369045/diff/43074/net/base/host_resolver_impl.cc

I could split it off as a new CL, but it could lead to use-after-free, so I'd
rather fix it ASAP.

[szym, 2012-03-02 18:51:46]

On 2012/03/02 18:47:24, cbentzel wrote:
> It looks like there are two changes - one for CancelRequest and one for
> AbortAllInProgressJobs. Are both needed?

The check: 
  if (!self) return;
has been accidentally removed in:
http://codereview.chromium.org/9369045/diff/43074/net/base/host_resolver_impl.cc

I could split it off as a new CL, but it could lead to use-after-free, so I'd
rather fix it ASAP.

[cbentzel, 2012-03-02 19:05:02]

Going through the test now. We also did a lot of chatting over IM.

http://codereview.chromium.org/9572018/diff/1/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9572018/diff/1/net/base/host_resolver_impl.cc#...
net/base/host_resolver_impl.cc:1576: dispatcher_.OnJobFinished();
Does the ordering here matter? It's different in the AbortAllJobs case. Perhaps
we should move it into a helper function or make it part of abort?

[szym, 2012-03-02 19:09:30]

http://codereview.chromium.org/9572018/diff/1/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9572018/diff/1/net/base/host_resolver_impl.cc#...
net/base/host_resolver_impl.cc:1576: dispatcher_.OnJobFinished();
On 2012/03/02 19:05:02, cbentzel wrote:
> Does the ordering here matter? It's different in the AbortAllJobs case.
Perhaps
> we should move it into a helper function or make it part of abort?

I feel that the preferred order is Abort then dispatch. Although arguably since
Jobs cannot complete synchronously it might make more sense to dispatch then
Abort.

Most importantly, Abort could delete |this| and |dispatcher_|. In
AbortAllInProgressJobs we use a self-life check which we are forced to anyway
(since we are in a loop, and it is not safe to Abort a Job once we know we are
deleted, due to dependencies such as NetLog).

I am okay changing it to dispatch then Abort which would let us move the
self-check to the for loop condition.

[cbentzel, 2012-03-02 19:13:17]

On 2012/03/02 19:09:30, szym wrote:
> http://codereview.chromium.org/9572018/diff/1/net/base/host_resolver_impl.cc
> File net/base/host_resolver_impl.cc (right):
> 
>
http://codereview.chromium.org/9572018/diff/1/net/base/host_resolver_impl.cc#...
> net/base/host_resolver_impl.cc:1576: dispatcher_.OnJobFinished();
> On 2012/03/02 19:05:02, cbentzel wrote:
> > Does the ordering here matter? It's different in the AbortAllJobs case.
> Perhaps
> > we should move it into a helper function or make it part of abort?
> 
> I feel that the preferred order is Abort then dispatch. Although arguably
since
> Jobs cannot complete synchronously it might make more sense to dispatch then
> Abort.
> 
> Most importantly, Abort could delete |this| and |dispatcher_|. In
> AbortAllInProgressJobs we use a self-life check which we are forced to anyway
> (since we are in a loop, and it is not safe to Abort a Job once we know we are
> deleted, due to dependencies such as NetLog).
> 
> I am okay changing it to dispatch then Abort which would let us move the
> self-check to the for loop condition.

I like doing dispatch-then-Abort - or more specifically matching the ordering
that normal Job completion uses which is
dispatch-followed-by-CompleteRequests-and-deletion which mirrors Abort.

[szym, 2012-03-02 19:18:40]

Done.

On Fri, Mar 2, 2012 at 2:13 PM, <cbentzel@chromium.org> wrote:

> On 2012/03/02 19:09:30, szym wrote:
>
>> http://codereview.chromium.**org/9572018/diff/1/net/base/**
>>
host_resolver_impl.cc<http://codereview.chromium.org/9572018/diff/1/net/base/host_resolver_impl.cc>
>> File net/base/host_resolver_impl.cc (right):
>>
>
>
> http://codereview.chromium.**org/9572018/diff/1/net/base/**
>
host_resolver_impl.cc#**newcode1576<http://codereview.chromium.org/9572018/diff/1/net/base/host_resolver_impl.cc#newcode1576>
>
>> net/base/host_resolver_impl.**cc:1576: dispatcher_.OnJobFinished();
>> On 2012/03/02 19:05:02, cbentzel wrote:
>> > Does the ordering here matter? It's different in the AbortAllJobs case.
>> Perhaps
>> > we should move it into a helper function or make it part of abort?
>>
>
>  I feel that the preferred order is Abort then dispatch. Although arguably
>>
> since
>
>> Jobs cannot complete synchronously it might make more sense to dispatch
>> then
>> Abort.
>>
>
>  Most importantly, Abort could delete |this| and |dispatcher_|. In
>> AbortAllInProgressJobs we use a self-life check which we are forced to
>> anyway
>> (since we are in a loop, and it is not safe to Abort a Job once we know
>> we are
>> deleted, due to dependencies such as NetLog).
>>
>
>  I am okay changing it to dispatch then Abort which would let us move the
>> self-check to the for loop condition.
>>
>
> I like doing dispatch-then-Abort - or more specifically matching the
> ordering
> that normal Job completion uses which is
> dispatch-followed-by-**CompleteRequests-and-deletion which mirrors Abort.
>
>
>
>
http://codereview.chromium.**org/9572018/<http://codereview.chromium.org/9572...
>

[eroman, 2012-03-02 19:42:38]

lgtm

[cbentzel, 2012-03-02 19:45:27]

Still going through this some more.

Another reason I like doing the dispatch followed by AbortJob is that it is
effectively the reverse order of construction, which you typically want when
unwinding.

Creating a job is

1) Memory allocator and constructor for HostResolverImpl::Job
2) Insert into prioritized dispatcher
3) Add to the HostResolverImpl's job map.

Tearing down a job should go in reverse order

4) Remove from HostResolverImpl's job map
5) Remove from prioritized dispatcher
6) Call all of the request callbacks
7) Call Job destructor and free memory

This is generally the case now that you are consistent. 

I am worried about the prioritized_dispatcher insertion included in the Job
construction both because of potential virtual dispatch issues as well as
because it makes the creation ordering less consistent.

http://codereview.chromium.org/9572018/diff/1/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9572018/diff/1/net/base/host_resolver_impl.cc#...
net/base/host_resolver_impl.cc:1080: handle_ = resolver_->dispatcher_.Add(this,
priority);
One concern is that this could call Job::Dispatch immediately, which does
virtual dispatch. Nothing derives from this class so I believe it is safe, but
not sure.

[cbentzel, 2012-03-02 19:46:49]

http://codereview.chromium.org/9572018/diff/1004/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9572018/diff/1004/net/base/host_resolver_impl....
net/base/host_resolver_impl.cc:1575: if (job->is_running()) {
I asked over email - but is there a chance that a job is not |is_running()| but
is still treated like a non-queued job by the dispatcher? That would leak slots
as well.

[szym, 2012-03-02 19:54:04]

On 2012/03/02 19:46:49, cbentzel wrote:
>
http://codereview.chromium.org/9572018/diff/1004/net/base/host_resolver_impl.cc
> File net/base/host_resolver_impl.cc (right):
> 
>
http://codereview.chromium.org/9572018/diff/1004/net/base/host_resolver_impl....
> net/base/host_resolver_impl.cc:1575: if (job->is_running()) {
> I asked over email - but is there a chance that a job is not |is_running()|
but
> is still treated like a non-queued job by the dispatcher? That would leak
slots
> as well.

The PrioritizedDispatcher does not care about such Jobs. It only cares about
queued Jobs. Once they are started or evicted, the dispatcher keeps no track of
them other than the |num_running_jobs_| counter.

There was a NetLog-related bug where a Job that was cancelled because all of its
Requests were cancelled would not log the EndEvent.

[cbentzel, 2012-03-02 19:58:21]

On 2012/03/02 19:54:04, szym wrote:
> On 2012/03/02 19:46:49, cbentzel wrote:
> >
>
http://codereview.chromium.org/9572018/diff/1004/net/base/host_resolver_impl.cc
> > File net/base/host_resolver_impl.cc (right):
> > 
> >
>
http://codereview.chromium.org/9572018/diff/1004/net/base/host_resolver_impl....
> > net/base/host_resolver_impl.cc:1575: if (job->is_running()) {
> > I asked over email - but is there a chance that a job is not |is_running()|
> but
> > is still treated like a non-queued job by the dispatcher? That would leak
> slots
> > as well.
> 
> The PrioritizedDispatcher does not care about such Jobs. It only cares about
> queued Jobs. Once they are started or evicted, the dispatcher keeps no track
of
> them other than the |num_running_jobs_| counter.

Yes - I was just worried that we may not decrease this counter in certain corner
cases where we should. There's probably no timing where Job::Start is called
_and_ neither proc_task or dns_task are running _and_ a CancelRequest comes in.
But I don't know off the top of my head.

> 
> There was a NetLog-related bug where a Job that was cancelled because all of
its
> Requests were cancelled would not log the EndEvent.

[cbentzel, 2012-03-02 20:02:40]

LGTM - I saw the moving Schedule out of the constructor patch.

Thanks for getting a fix in quickly and adding the test.

http://codereview.chromium.org/9572018/diff/1004/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9572018/diff/1004/net/base/host_resolver_impl....
net/base/host_resolver_impl.cc:1577: job->Abort();
FYI: I don't think the Abort is needed here. ~Job appears to be doing all the
cleanup that is needed since there are no requests and associated callbacks tied
to this job.

[szym, 2012-03-02 20:03:21]

> Yes - I was just worried that we may not decrease this counter in certain
corner
> cases where we should. There's probably no timing where Job::Start is called
> _and_ neither proc_task or dns_task are running _and_ a CancelRequest comes
in.
> But I don't know off the top of my head.
> 

CancelRequest only affects the dispatcher if it's the last request for that Job.
This cannot happen from a callback. Otherwise a Job is_running from Start() to
CompleteRequests.

I moved the potential virtual dispatch to Schedule(). I'll clean this up in the
next CL.

[szym, 2012-03-02 20:05:43]

http://codereview.chromium.org/9572018/diff/1004/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/9572018/diff/1004/net/base/host_resolver_impl....
net/base/host_resolver_impl.cc:1577: job->Abort();
On 2012/03/02 20:02:40, cbentzel wrote:
> FYI: I don't think the Abort is needed here. ~Job appears to be doing all the
> cleanup that is needed since there are no requests and associated callbacks
tied
> to this job.

The cleaning up is now in a few places: Job::~Job, Job::CancelRequest,
HostResolverImpl::CancelRequest. Right now, ~Job is only supposed to take care
of the case when HostResolverImpl is destroyed. I'll refactor this in the next
CL.

[commit-bot: I haz the power, 2012-03-02 20:06:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/9572018/6002

[eroman, 2012-03-02 20:07:08]

Yeah, thanks for owning this and the really fast turn-around time! Can't believe
you were online last night at such a ridiculous time :-)

[szym, 2012-03-02 20:11:40]

On 2012/03/02 20:07:08, eroman wrote:
> Yeah, thanks for owning this and the really fast turn-around time! Can't
believe
> you were online last night at such a ridiculous time :-)

Thanks for the quick review. I'll be cleaning up the clean-up logic in a
followup CL, once this lands.

[commit-bot: I haz the power, 2012-03-03 00:04:57]

Change committed as 124783