Remove stl_util's deletion functions from net/dns/.

Remove stl_util's deletion functions from net/dns/.

BUG=555865
Committed: https://crrev.com/9e72eb481e3038de9381c268cce159c4f41d91c0
Cr-Commit-Position: refs/heads/master@{#437655}

[Avi (use Gerrit), 2016-10-02 03:24:57]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-02 03:25:05]

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-10-02 03:46:40]

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

[commit-bot: I haz the power, 2016-10-02 03:46:40]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Avi (use Gerrit), 2016-10-28 14:51:05]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 14:51: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-10-28 14:54:24]

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

[commit-bot: I haz the power, 2016-10-28 14:54:24]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Avi (use Gerrit), 2016-10-28 16:14:49]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 16:15:05]

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-10-28 16:28:47]

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

[commit-bot: I haz the power, 2016-10-28 16:28:47]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[Avi (use Gerrit), 2016-11-07 00:57:45]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 00:57: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-11-07 01:08:58]

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

[commit-bot: I haz the power, 2016-11-07 01:08:59]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[Avi (use Gerrit), 2016-11-09 17:11:46]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 17:11:59]

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

[Avi (use Gerrit), 2016-11-09 17:36:57]

Description was changed from

==========
Remove stl_util's deletion functions from net/dns/.

BUG=555865
==========

to

==========
Remove stl_util's deletion functions from net/dns/.

BUG=555865
==========

[Avi (use Gerrit), 2016-11-09 17:36:57]

avi@chromium.org changed reviewers:
+ davidben@chromium.org

[commit-bot: I haz the power, 2016-11-09 18:17:37]

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

[commit-bot: I haz the power, 2016-11-09 18:17:38]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-11-09 19:09:50]

ptal

[davidben, 2016-11-09 22:22:03]

https://codereview.chromium.org/2389613002/diff/40001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.h (right):

https://codereview.chromium.org/2389613002/diff/40001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.h:266: // Removes |job| from |jobs_|, only if it
exists, but does not delete it.
Perhaps it'd be better for it to return std::unique_ptr<Job>? Then
CompleteRequests can be changed to:

    std::unique_ptr<Job> self_deleter = resolver_->RemoveJob(this);

https://codereview.chromium.org/2389613002/diff/40001/net/dns/mojo_host_resol...
File net/dns/mojo_host_resolver_impl.cc (right):

https://codereview.chromium.org/2389613002/diff/40001/net/dns/mojo_host_resol...
net/dns/mojo_host_resolver_impl.cc:76: [job](const std::unique_ptr<Job>& ptr) {
return ptr.get() == job; });
Same issue as always. :-) std::find_if on a std::set is the worst of all worlds.
std::find_if on a std::vector or std::map<T*, std::unique_ptr<T>> are both
strictly better than this pattern.

But, try jobs willing, I propose we use this pattern (and probably fix all the
old instances prior CLs left around). The one nuisance is this pattern can't
temporarily orphan jobs as HostResolverImpl does. But I feel like most of the
time, this works fine:

https://codereview.chromium.org/2489113002

[Avi (use Gerrit), 2016-11-09 23:09:22]

I'm removing the MojoHostResolverImpl files in favor of your patch.

https://codereview.chromium.org/2389613002/diff/40001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.h (right):

https://codereview.chromium.org/2389613002/diff/40001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.h:266: // Removes |job| from |jobs_|, only if it
exists, but does not delete it.
On 2016/11/09 22:22:03, davidben wrote:
> Perhaps it'd be better for it to return std::unique_ptr<Job>? Then
> CompleteRequests can be changed to:
> 
>     std::unique_ptr<Job> self_deleter = resolver_->RemoveJob(this);

Done.

[Avi (use Gerrit), 2016-11-09 23:09:25]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 23:09:52]

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-11-09 23:17:31]

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

[commit-bot: I haz the power, 2016-11-09 23:17:33]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[davidben, 2016-11-09 23:17:54]

lgtm assuming comment below fixes try bots. :-)

https://codereview.chromium.org/2389613002/diff/60001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/2389613002/diff/60001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2307: }
return nullptr?

[Avi (use Gerrit), 2016-11-09 23:21:54]

On 2016/11/09 23:17:54, davidben wrote:
> lgtm assuming comment below fixes try bots. :-)
> 
>
https://codereview.chromium.org/2389613002/diff/60001/net/dns/host_resolver_i...
> File net/dns/host_resolver_impl.cc (right):
> 
>
https://codereview.chromium.org/2389613002/diff/60001/net/dns/host_resolver_i...
> net/dns/host_resolver_impl.cc:2307: }
> return nullptr?

Yes, I should try compiling it first :)

[Avi (use Gerrit), 2016-11-09 23:21:55]

On 2016/11/09 23:17:54, davidben wrote:
> lgtm assuming comment below fixes try bots. :-)
> 
>
https://codereview.chromium.org/2389613002/diff/60001/net/dns/host_resolver_i...
> File net/dns/host_resolver_impl.cc (right):
> 
>
https://codereview.chromium.org/2389613002/diff/60001/net/dns/host_resolver_i...
> net/dns/host_resolver_impl.cc:2307: }
> return nullptr?

Yes, I should try compiling it first :)

[Avi (use Gerrit), 2016-11-09 23:24:53]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2016-11-09 23:24:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/2389613002/#ps80001
(title: "fix")

[commit-bot: I haz the power, 2016-11-09 23:25:23]

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

[Avi (use Gerrit), 2016-11-10 00:45:59]

The CQ bit was unchecked by avi@chromium.org

[Avi (use Gerrit), 2016-11-10 01:16:08]

https://codereview.chromium.org/2389613002/diff/80001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/2389613002/diff/80001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:1897: jobs_.clear();
This is not safe.

This will destroy the map with the Job objects, which, in their destructors will
call back into RemoveJob, which will mess with the map, re-entrantly.

Rethinking this.

[Avi (use Gerrit), 2016-11-11 01:42:49]

https://codereview.chromium.org/2389613002/diff/40001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.h (right):

https://codereview.chromium.org/2389613002/diff/40001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.h:266: // Removes |job| from |jobs_|, only if it
exists, but does not delete it.
On 2016/11/09 23:09:22, Avi wrote:
> On 2016/11/09 22:22:03, davidben wrote:
> > Perhaps it'd be better for it to return std::unique_ptr<Job>? Then
> > CompleteRequests can be changed to:
> > 
> >     std::unique_ptr<Job> self_deleter = resolver_->RemoveJob(this);
> 
> Done.

Oh, boy. But the job might not be in jobs_, because it might be in the middle of
being aborted (see AbortAllInProgressJobs and how it removes the jobs from
jobs_).

The ownership is a bit nuts here. Still working out a better solution.

[Avi (use Gerrit), 2016-11-11 01:50:03]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 01:50:35]

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-11-11 02:22:18]

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

[commit-bot: I haz the power, 2016-11-11 02:22:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2016-11-11 02:40:17]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 02:40:51]

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-11-11 02:55:28]

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

[commit-bot: I haz the power, 2016-11-11 02:55:29]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[Avi (use Gerrit), 2016-11-11 03:04:45]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 03:05:12]

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-11-11 04:28:22]

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

[commit-bot: I haz the power, 2016-11-11 04:28:24]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-11-11 05:01:01]

Some messing around and I'm back roughly to where I was at. PTAL.

[Avi (use Gerrit), 2016-11-22 16:59:36]

On 2016/11/11 05:01:01, Avi wrote:
> Some messing around and I'm back roughly to where I was at. PTAL.

davidben: ping

[davidben, 2016-12-08 21:09:57]

Sorry about the delay. It's been a crazy couple of weeks.

So, I'm not following why patch set 5 is unsafe while patch set 9 is fine. They
seem to be identical as far as risks of jobs_ reentrancy goes. I'm probably just
missing something.

[davidben, 2016-12-08 21:16:19]

On 2016/12/08 21:09:57, davidben wrote:
> Sorry about the delay. It's been a crazy couple of weeks.
> 
> So, I'm not following why patch set 5 is unsafe while patch set 9 is fine.
They
> seem to be identical as far as risks of jobs_ reentrancy goes. I'm probably
just
> missing something.

I'm also not seeing where HostResolverImpl::Job::~Job calls into RemoveJob. If
it did, wouldn't the existing code have a similar problem, because we'd be
erasing from jobs_ while we are iterating over it?

[Avi (use Gerrit), 2016-12-09 18:38:37]

I was wrong in patch set 5; please ignore my comments in it. Here's what's going
on:

In your very first response to this CL you wrote:

> Perhaps it'd be better for it to return std::unique_ptr<Job>? Then
> CompleteRequests can be changed to:
> 
>     std::unique_ptr<Job> self_deleter = resolver_->RemoveJob(this);

It turns out that that doesn't work because the jobs_ container isn't guaranteed
to actually hold ownership of the job passed into RemoveJob.

See AbortAllInProgressJobs. It removes all affected jobs from the jobs_
container, then calls Abort on them, which calls CompleteRequests, which would
then fail because they're not in the jobs_ container to take ownership from.

So I'm putting things back to the way they were before I tried to implement that
fix you suggested. I did all the other fixes you requested.

[davidben, 2016-12-09 18:53:57]

On 2016/12/09 18:38:37, Avi wrote:
> I was wrong in patch set 5; please ignore my comments in it. Here's what's
going
> on:
> 
> In your very first response to this CL you wrote:
> 
> > Perhaps it'd be better for it to return std::unique_ptr<Job>? Then
> > CompleteRequests can be changed to:
> > 
> >     std::unique_ptr<Job> self_deleter = resolver_->RemoveJob(this);
> 
> It turns out that that doesn't work because the jobs_ container isn't
guaranteed
> to actually hold ownership of the job passed into RemoveJob.
> 
> See AbortAllInProgressJobs. It removes all affected jobs from the jobs_
> container, then calls Abort on them, which calls CompleteRequests, which would
> then fail because they're not in the jobs_ container to take ownership from.
> 
> So I'm putting things back to the way they were before I tried to implement
that
> fix you suggested. I did all the other fixes you requested.

Wow, yuck. lgtm. Where by "g" I mean "terrible but it was scary before so
okay...".

Did you find it broke due to an existing unit test or something else? (If there
isn't a unit test which covers it, let's file a bug to add one and I'll try to
find someone familiar enough with it to do it. Asynchronously from this CL, of
course.)

[Avi (use Gerrit), 2016-12-09 18:57:41]

On 2016/12/09 18:53:57, davidben wrote:
> On 2016/12/09 18:38:37, Avi wrote:
> > I was wrong in patch set 5; please ignore my comments in it. Here's what's
> going
> > on:
> > 
> > In your very first response to this CL you wrote:
> > 
> > > Perhaps it'd be better for it to return std::unique_ptr<Job>? Then
> > > CompleteRequests can be changed to:
> > > 
> > >     std::unique_ptr<Job> self_deleter = resolver_->RemoveJob(this);
> > 
> > It turns out that that doesn't work because the jobs_ container isn't
> guaranteed
> > to actually hold ownership of the job passed into RemoveJob.
> > 
> > See AbortAllInProgressJobs. It removes all affected jobs from the jobs_
> > container, then calls Abort on them, which calls CompleteRequests, which
would
> > then fail because they're not in the jobs_ container to take ownership from.
> > 
> > So I'm putting things back to the way they were before I tried to implement
> that
> > fix you suggested. I did all the other fixes you requested.
> 
> Wow, yuck. lgtm. Where by "g" I mean "terrible but it was scary before so
> okay...".
> 
> Did you find it broke due to an existing unit test or something else? (If
there
> isn't a unit test which covers it, let's file a bug to add one and I'll try to
> find someone familiar enough with it to do it. Asynchronously from this CL, of
> course.)

Yes, when I made the change you suggested, quite a few tests turned red. It was
only when investigating those red tests that I figured this out.

[Avi (use Gerrit), 2016-12-09 18:57:48]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2016-12-09 18:58:31]

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-12-09 21:10:59]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481309868989240,
"parent_rev": "2cef54680b0033d4fcf357c3b1ccd6d669fcd0d3", "commit_rev":
"9d55513e5f47d49cce10dad12f670f5f4d6e00ec"}

[commit-bot: I haz the power, 2016-12-09 21:11:35]

Description was changed from

==========
Remove stl_util's deletion functions from net/dns/.

BUG=555865
==========

to

==========
Remove stl_util's deletion functions from net/dns/.

BUG=555865

Review-Url: https://codereview.chromium.org/2389613002
==========

[commit-bot: I haz the power, 2016-12-09 21:11:36]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-12-12 14:40:55]

Description was changed from

==========
Remove stl_util's deletion functions from net/dns/.

BUG=555865

Review-Url: https://codereview.chromium.org/2389613002
==========

to

==========
Remove stl_util's deletion functions from net/dns/.

BUG=555865

Committed: https://crrev.com/9e72eb481e3038de9381c268cce159c4f41d91c0
Cr-Commit-Position: refs/heads/master@{#437655}
==========

[commit-bot: I haz the power, 2016-12-12 14:40:57]

Patchset 9 (id:??) landed as
https://crrev.com/9e72eb481e3038de9381c268cce159c4f41d91c0
Cr-Commit-Position: refs/heads/master@{#437655}