Domain Reliability: Don't crash on shutdown with uploads pending

Domain Reliability: Don't crash on shutdown with uploads pending

This fixes a couple of bugs:

1. Makes sure the ChromeNetworkDelegate can't call in to a destroyed DomainReliabilityMonitor by moving ownership of the Monitor from ProfileImplIOData to ChromeNetworkDelegate.

2. Makes sure the uploader doesn't leave URL requests around once they're all supposed to be gone by adding a separate "shutdown" step that deletes existing requests and prohibits new ones.

BUG=644858
Committed: https://crrev.com/127604ea9f06489c24bf6ad47d5ea62b266d45b4
Cr-Commit-Position: refs/heads/master@{#439520}

[Julia Tuttle, 2016-09-14 22:14:19]

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

[commit-bot: I haz the power, 2016-09-14 22:14:59]

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-09-14 23:52:59]

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

[commit-bot: I haz the power, 2016-09-14 23:53:00]

Dry run: This issue passed the CQ dry run.

[Julia Tuttle, 2016-09-15 18:05:59]

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

[Julia Tuttle, 2016-09-15 18:06:00]

PTAL, davidben. I don't think there are any dependencies in the *other*
direction (calls from context_manager_ to uploader_), but I'd appreciate another
check.

[davidben, 2016-09-16 18:55:13]

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

[davidben, 2016-09-16 18:55:14]

Hrm. So the dependency goes Monitor -> Uploader -> URLRequest ->
ChromeNetworkDelegate -> Monitor -> ContextManager, right? It seems the bigger
issue is that the Monitor is still reachable from the ChromeNetworkDelegate
after/while it's being destroyed. By the time we call MaybeHandleHeader, we're
calling into a Monitor whose destructor has already run. We're just tearing down
its components now.

+mmenke is more familiar with ProfileIOData and ChromeNetworkDelegate lifetime
stuff than me. How do we usually handle this?

[mmenke, 2016-09-16 19:38:25]

On 2016/09/16 18:55:14, davidben wrote:
> Hrm. So the dependency goes Monitor -> Uploader -> URLRequest ->
> ChromeNetworkDelegate -> Monitor -> ContextManager, right? It seems the bigger
> issue is that the Monitor is still reachable from the ChromeNetworkDelegate
> after/while it's being destroyed. By the time we call MaybeHandleHeader, we're
> calling into a Monitor whose destructor has already run. We're just tearing
down
> its components now.
> 
> +mmenke is more familiar with ProfileIOData and ChromeNetworkDelegate lifetime
> stuff than me. How do we usually handle this?

By crashing (No, not really).

We generally don't have things that the ChromeNetworkDelegate calls into that
also create/destroy URLRequests, so we just tear everything that can make
requests before we tear down objects in the network stack.  SDCH, which is the
closest thing we have that does both, has a separate component that actually
makes the network requests, which can be torn down independently of the class to
store dictionaries in memory, and use them for decompression, so it's a simple
matter for the profile to just tear down the fetching component before anything
else.

As you say, this CL seems insufficient to the task of fixing this.  I'd rather
we try and mirror SDCH's approach, rather than NULLing out
ChromeNetworkDelegate's pointer, though I'm open to other ideas.

[mmenke, 2016-09-16 20:43:23]

On 2016/09/16 19:38:25, mmenke wrote:
> On 2016/09/16 18:55:14, davidben wrote:
> > Hrm. So the dependency goes Monitor -> Uploader -> URLRequest ->
> > ChromeNetworkDelegate -> Monitor -> ContextManager, right? It seems the
bigger
> > issue is that the Monitor is still reachable from the ChromeNetworkDelegate
> > after/while it's being destroyed. By the time we call MaybeHandleHeader,
we're
> > calling into a Monitor whose destructor has already run. We're just tearing
> down
> > its components now.
> > 
> > +mmenke is more familiar with ProfileIOData and ChromeNetworkDelegate
lifetime
> > stuff than me. How do we usually handle this?
> 
> By crashing (No, not really).
> 
> We generally don't have things that the ChromeNetworkDelegate calls into that
> also create/destroy URLRequests, so we just tear everything that can make
> requests before we tear down objects in the network stack.  SDCH, which is the
> closest thing we have that does both, has a separate component that actually
> makes the network requests, which can be torn down independently of the class
to
> store dictionaries in memory, and use them for decompression, so it's a simple
> matter for the profile to just tear down the fetching component before
anything
> else.
> 
> As you say, this CL seems insufficient to the task of fixing this.  I'd rather
> we try and mirror SDCH's approach, rather than NULLing out
> ChromeNetworkDelegate's pointer, though I'm open to other ideas.

Talked to Julia a bit about this - this issue would just go away if we could get
the ChromeURLRequestContextGetter over to Frankie...Erm...The Domain Reliability
Monitor stuff...  This problem should magically go away.  Since this would work
for anything else with this problem that hangs off the ProfileIOData, this seems
the best way to go to me.

[Julia Tuttle, 2016-09-22 14:29:26]

Description was changed from

==========
Domain Reliability: Make sure uploader_ is destroyed before context_manager_

BUG=644858
==========

to

==========
Domain Reliability: Don't crash on shutdown with uploads pending

This fixes a couple of bugs:

2. Makes sure the ChromeNetworkDelegate can't call in to a destroyed
DomainReliabilityMonitor by moving ownership of the Monitor from
ProfileImplIOData to ChromeNetworkDelegate.

3. Makes sure the uploader doesn't leave URL requests around once they're all
supposed to be gone by destroying the uploader in a separate "shutdown" step.

BUG=644858
==========

[Julia Tuttle, 2016-09-22 14:29:36]

Description was changed from

==========
Domain Reliability: Don't crash on shutdown with uploads pending

This fixes a couple of bugs:

2. Makes sure the ChromeNetworkDelegate can't call in to a destroyed
DomainReliabilityMonitor by moving ownership of the Monitor from
ProfileImplIOData to ChromeNetworkDelegate.

3. Makes sure the uploader doesn't leave URL requests around once they're all
supposed to be gone by destroying the uploader in a separate "shutdown" step.

BUG=644858
==========

to

==========
Domain Reliability: Don't crash on shutdown with uploads pending

This fixes a couple of bugs:

1. Makes sure the ChromeNetworkDelegate can't call in to a destroyed
DomainReliabilityMonitor by moving ownership of the Monitor from
ProfileImplIOData to ChromeNetworkDelegate.

2. Makes sure the uploader doesn't leave URL requests around once they're all
supposed to be gone by destroying the uploader in a separate "shutdown" step.

BUG=644858
==========

[Julia Tuttle, 2016-09-22 14:30:54]

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

[commit-bot: I haz the power, 2016-09-22 14:31:16]

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

[Julia Tuttle, 2016-09-22 14:35:34]

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

[commit-bot: I haz the power, 2016-09-22 14:36:08]

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-09-22 15:43:49]

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

[commit-bot: I haz the power, 2016-09-22 15:43:49]

Dry run: This issue passed the CQ dry run.

[Julia Tuttle, 2016-09-22 17:07:15]

PTAL davidben, mmenke.

I plan to add integration tests to prevent this kind of thing from happening in
the future, but since it's a top crasher, the fix is easy, and the tests aren't,
I'd like to get the fix landed separately.

[davidben, 2016-09-26 21:14:48]

Sorry about the delay. It seems there was some confusion whether it was me or
mmenke to review.

This looks good save for one comment below.

https://codereview.chromium.org/2336043007/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/2336043007/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:183:
io_data_->domain_reliability_monitor_ = domain_reliability_monitor.release();
This part and the WrapUnique below seems kind of off. It seems there's a window
where the monitor is unowned and then you convert a raw pointer into an owned
pointer.

mmenke probably should confirm this is sane, but perhaps we should put a
std::unique_ptr<Monitor> in the LazyParams, have that own it, and then
InitializeInternal below std::moves it?

[mmenke, 2016-09-26 21:30:09]

https://codereview.chromium.org/2336043007/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/2336043007/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:183:
io_data_->domain_reliability_monitor_ = domain_reliability_monitor.release();
On 2016/09/26 21:14:48, davidben wrote:
> This part and the WrapUnique below seems kind of off. It seems there's a
window
> where the monitor is unowned and then you convert a raw pointer into an owned
> pointer.
> 
> mmenke probably should confirm this is sane, but perhaps we should put a
> std::unique_ptr<Monitor> in the LazyParams, have that own it, and then
> InitializeInternal below std::moves it?

Yea, this seems like a problem, and adding it to LazyParams does seem like the
best solution here.

Hrm...  We're also not hooking up DomainReliabilityMonitor to isolated app
URLRequestContexts.  Should we be?

https://codereview.chromium.org/2336043007/diff/60001/components/domain_relia...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/2336043007/diff/60001/components/domain_relia...
components/domain_reliability/monitor.cc:161: uploader_.reset();
I don't see any checks for a null uploader anywhere?  Can deleting a URLRequest
cause DomainReliabilityMonitor to tell uploader_ to start a new URLRequest?

[Julia Tuttle, 2016-09-27 15:33:49]

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

[commit-bot: I haz the power, 2016-09-27 15:34:08]

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

[Julia Tuttle, 2016-09-27 16:13:24]

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

[commit-bot: I haz the power, 2016-09-27 16:13:39]

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

[Julia Tuttle, 2016-09-27 16:20:16]

PTAL, mmenke.

[Julia Tuttle, 2016-09-27 16:49:59]

https://codereview.chromium.org/2336043007/diff/60001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/2336043007/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:183:
io_data_->domain_reliability_monitor_ = domain_reliability_monitor.release();
On 2016/09/26 21:14:48, davidben wrote:
> This part and the WrapUnique below seems kind of off. It seems there's a
window
> where the monitor is unowned and then you convert a raw pointer into an owned
> pointer.
> 
> mmenke probably should confirm this is sane, but perhaps we should put a
> std::unique_ptr<Monitor> in the LazyParams, have that own it, and then
> InitializeInternal below std::moves it?

Done.

https://codereview.chromium.org/2336043007/diff/60001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:183:
io_data_->domain_reliability_monitor_ = domain_reliability_monitor.release();
On 2016/09/26 21:30:09, mmenke wrote:
> On 2016/09/26 21:14:48, davidben wrote:
> > This part and the WrapUnique below seems kind of off. It seems there's a
> window
> > where the monitor is unowned and then you convert a raw pointer into an
owned
> > pointer.
> > 
> > mmenke probably should confirm this is sane, but perhaps we should put a
> > std::unique_ptr<Monitor> in the LazyParams, have that own it, and then
> > InitializeInternal below std::moves it?
> 
> Yea, this seems like a problem, and adding it to LazyParams does seem like the
> best solution here.

Cool, done.

> Hrm...  We're also not hooking up DomainReliabilityMonitor to isolated app
> URLRequestContexts.  Should we be?

Not sure, but that'd be a separate CL.

https://codereview.chromium.org/2336043007/diff/60001/components/domain_relia...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/2336043007/diff/60001/components/domain_relia...
components/domain_reliability/monitor.cc:161: uploader_.reset();
On 2016/09/26 21:30:09, mmenke wrote:
> I don't see any checks for a null uploader anywhere?  Can deleting a
URLRequest
> cause DomainReliabilityMonitor to tell uploader_ to start a new URLRequest?

Not sure, but fixed with new Shutdown.

[Julia Tuttle, 2016-09-27 16:51:19]

Description was changed from

==========
Domain Reliability: Don't crash on shutdown with uploads pending

This fixes a couple of bugs:

1. Makes sure the ChromeNetworkDelegate can't call in to a destroyed
DomainReliabilityMonitor by moving ownership of the Monitor from
ProfileImplIOData to ChromeNetworkDelegate.

2. Makes sure the uploader doesn't leave URL requests around once they're all
supposed to be gone by destroying the uploader in a separate "shutdown" step.

BUG=644858
==========

to

==========
Domain Reliability: Don't crash on shutdown with uploads pending

This fixes a couple of bugs:

1. Makes sure the ChromeNetworkDelegate can't call in to a destroyed
DomainReliabilityMonitor by moving ownership of the Monitor from
ProfileImplIOData to ChromeNetworkDelegate.

2. Makes sure the uploader doesn't leave URL requests around once they're all
supposed to be gone by adding a separate "shutdown" step that deletes existing
requests and prohibits new ones.

BUG=644858
==========

[commit-bot: I haz the power, 2016-09-27 16:54:27]

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

[commit-bot: I haz the power, 2016-09-27 16:54:28]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-09-27 16:56:16]

Quick comments, haven't reviewed changes.

https://codereview.chromium.org/2336043007/diff/100001/components/domain_reli...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/2336043007/diff/100001/components/domain_reli...
components/domain_reliability/monitor.cc:159: uploader_->Shutdown();
On 2016/09/27 16:49:59, Julia Tuttle wrote:
> On 2016/09/26 21:30:09, mmenke wrote:
> > I don't see any checks for a null uploader anywhere?  Can deleting a
> URLRequest
> > cause DomainReliabilityMonitor to tell uploader_ to start a new URLRequest?
> 
> Not sure, but fixed with new Shutdown.

If it can, we should have a test for it.  If it can't, I'd like to know why.

https://codereview.chromium.org/2336043007/diff/100001/components/domain_reli...
File components/domain_reliability/uploader.cc (right):

https://codereview.chromium.org/2336043007/diff/100001/components/domain_reli...
components/domain_reliability/uploader.cc:78: if (discard_uploads_ || shutdown_)
{
Test coverage for this?

[Julia Tuttle, 2016-12-09 16:20:02]

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

[commit-bot: I haz the power, 2016-12-09 16:20:19]

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-12-09 17:23:38]

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

[commit-bot: I haz the power, 2016-12-09 17:23:39]

Dry run: This issue passed the CQ dry run.

[Julia Tuttle, 2016-12-09 17:50:40]

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

[commit-bot: I haz the power, 2016-12-09 17:51:09]

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

[Julia Tuttle, 2016-12-09 17:58:00]

PTAL, mmenke.

I now have a unittest that makes sure Uploader won't create any more requests
once it's shutdown.

[mmenke, 2016-12-09 18:07:06]

I'll look at this later today, but it seems like there really should be an
integration test for this, to ensure it's correctly hooked up.

[commit-bot: I haz the power, 2016-12-09 19:53:06]

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

[commit-bot: I haz the power, 2016-12-09 19:53:07]

Dry run: This issue passed the CQ dry run.

[Julia Tuttle, 2016-12-13 22:03:11]

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

[Julia Tuttle, 2016-12-13 22:03:49]

On 2016/12/09 18:07:06, mmenke wrote:
> I'll look at this later today, but it seems like there really should be an
> integration test for this, to ensure it's correctly hooked up.

Alright, I have swapped back in my crufty knowledge of browser tests and written
one that fails with the uploader changes removed and passes with them there.
PTAL.

[commit-bot: I haz the power, 2016-12-13 22:04:05]

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

[mmenke, 2016-12-13 22:06:37]

On 2016/12/13 22:03:49, Julia Tuttle wrote:
> On 2016/12/09 18:07:06, mmenke wrote:
> > I'll look at this later today, but it seems like there really should be an
> > integration test for this, to ensure it's correctly hooked up.
> 
> Alright, I have swapped back in my crufty knowledge of browser tests and
written
> one that fails with the uploader changes removed and passes with them there.
> PTAL.

Oops.  Forgot to get back to this, sorry about that.  Or it was just a really
long day.  :)  I'll start looking today, but probably won't get back to you
until tomorrow.

[commit-bot: I haz the power, 2016-12-13 22:29:32]

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

[commit-bot: I haz the power, 2016-12-13 22:29:33]

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

[Julia Tuttle, 2016-12-14 16:11:31]

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

[commit-bot: I haz the power, 2016-12-14 16:11:43]

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-12-14 16:24:36]

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

[commit-bot: I haz the power, 2016-12-14 16:24:37]

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_...)

[Julia Tuttle, 2016-12-14 16:28:35]

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

[commit-bot: I haz the power, 2016-12-14 16:28: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-12-14 17:40:38]

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

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

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-12-14 19:12:16]

Sorry for the delay.  Longer CLs in code that I know less well tend to drop to
the end of my review list.

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:889:
NOTREACHED();
include base/logging.h

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/domain_...
File chrome/browser/domain_reliability/browsertest.cc (right):

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/domain_...
chrome/browser/domain_reliability/browsertest.cc:18: class
DomainReliabilityBrowserTest : public InProcessBrowserTest {
Should probably call this file domain_reliability_browsertest (browsertest is
more common than browser_test, despite the fact that the "T" is generally
capitalized in test class names)

Or maybe clearer, go with DomainReliabilityServiceBrowserTest /
domain_reliability_service_browsertest .

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/domain_...
chrome/browser/domain_reliability/browsertest.cc:19: protected:
optional:  Suggest just making these public, as protected, while more accurate
about how their used, is less intuitive.

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/domain_...
chrome/browser/domain_reliability/browsertest.cc:28:
net::URLRequestHangingReadJob::AddUrlHandler();
See URLRequestFailedJob::GetMockHttp[s]Url(net::ERR_IO_PENDING);  (Oops, I
referred to this comment in an IM, but I hadn't published it yet)

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/domain_...
chrome/browser/domain_reliability/browsertest.cc:86: auto config =
base::MakeUnique<DomainReliabilityConfig>();
include base/memory/ptr_util.h

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/domain_...
chrome/browser/domain_reliability/browsertest.cc:87: config->origin =
GURL("https://localhost/");
include gurl.h

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/domain_...
chrome/browser/domain_reliability/browsertest.cc:97:
service->ForceUploadsForTesting();
Think you need a comment about what this actually tests.

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_impl_io_data.cc:469:
std::move(lazy_params_->domain_reliability_monitor);
optional: Don't think you need this line / variable, just grab it from
lazy_params_ twice.

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
File components/domain_reliability/dispatcher.cc (right):

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
components/domain_reliability/dispatcher.cc:89: tasks.insert(task.get());
std::set<Task*> tasks = tasks_;?

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
components/domain_reliability/dispatcher.cc:94: }
I assume we can't just do:

while(!tasks.empty()) {...}

because running one task may add another?  If that's the case, worth mentioning
this magic in the header?  Not clear what it does and does not run.

If that's not the case, suggest switching.

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
File components/domain_reliability/monitor.h (right):

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
components/domain_reliability/monitor.h:91: void Shutdown();
Needs description

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
components/domain_reliability/monitor.h:135: void ForceUploadsForTesting();
Maybe add a comment?

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
File components/domain_reliability/uploader.cc (right):

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
components/domain_reliability/uploader.cc:65: DCHECK(shutdown_);
include base/logging.h

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
components/domain_reliability/uploader.cc:167: upload_callbacks_;
A bit afield from this CL, but do you mind rename this map to upload_tasks_ or
something?  upload_callback_ doesn't like something that has the implication
"This owns URLFetchers."

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
File components/domain_reliability/uploader.h (right):

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
components/domain_reliability/uploader.h:68: virtual void Shutdown();
Needs a comment.

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
File components/domain_reliability/uploader_unittest.cc (right):

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
components/domain_reliability/uploader_unittest.cc:300:
TEST_F(DomainReliabilityUploaderTest, NoUploadAfterShutdown) {
Also have a test that checks that requests are cancelled at shutdown? (Create a
hanging request wait for request_count_ to reach 1, call Shutdown(), and then
call url_request_context_getter_->GetContext()->AssertNoURLRequests());

Since this test uses its own URLRequestJob subclass, count even add a way to
check that the created job was torn down instead.  Fine with either approach.

[Julia Tuttle, 2016-12-15 21:50:47]

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

[commit-bot: I haz the power, 2016-12-15 21:51:30]

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

[Julia Tuttle, 2016-12-15 21:53:00]

PTAL, mmenke.

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:889:
NOTREACHED();
On 2016/12/14 19:12:17, mmenke (Out Dec 17 to Jan 2) wrote:
> include base/logging.h

Done.

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/domain_...
File chrome/browser/domain_reliability/browsertest.cc (right):

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/domain_...
chrome/browser/domain_reliability/browsertest.cc:18: class
DomainReliabilityBrowserTest : public InProcessBrowserTest {
On 2016/12/14 19:12:17, mmenke (Out Dec 17 to Jan 2) wrote:
> Should probably call this file domain_reliability_browsertest (browsertest is
> more common than browser_test, despite the fact that the "T" is generally
> capitalized in test class names)
> 
> Or maybe clearer, go with DomainReliabilityServiceBrowserTest /
> domain_reliability_service_browsertest .

It's not a test of *just* the service, and Domain Reliability has the odd
convention of not including the domain_reliability_ prefix on *most* files. Is
there a reason for this one in particular to include it?

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/domain_...
chrome/browser/domain_reliability/browsertest.cc:19: protected:
On 2016/12/14 19:12:17, mmenke (Out Dec 17 to Jan 2) wrote:
> optional:  Suggest just making these public, as protected, while more accurate
> about how their used, is less intuitive.

Done.

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/domain_...
chrome/browser/domain_reliability/browsertest.cc:28:
net::URLRequestHangingReadJob::AddUrlHandler();
On 2016/12/14 19:12:17, mmenke (Out Dec 17 to Jan 2) wrote:
> See URLRequestFailedJob::GetMockHttp[s]Url(net::ERR_IO_PENDING);  (Oops, I
> referred to this comment in an IM, but I hadn't published it yet)

Oh! Good call. Done.

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/domain_...
chrome/browser/domain_reliability/browsertest.cc:86: auto config =
base::MakeUnique<DomainReliabilityConfig>();
On 2016/12/14 19:12:17, mmenke (Out Dec 17 to Jan 2) wrote:
> include base/memory/ptr_util.h

Done.

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/domain_...
chrome/browser/domain_reliability/browsertest.cc:87: config->origin =
GURL("https://localhost/");
On 2016/12/14 19:12:17, mmenke (Out Dec 17 to Jan 2) wrote:
> include gurl.h

Done.

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/domain_...
chrome/browser/domain_reliability/browsertest.cc:97:
service->ForceUploadsForTesting();
On 2016/12/14 19:12:17, mmenke (Out Dec 17 to Jan 2) wrote:
> Think you need a comment about what this actually tests.

Done.

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/2336043007/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_impl_io_data.cc:469:
std::move(lazy_params_->domain_reliability_monitor);
On 2016/12/14 19:12:17, mmenke (Out Dec 17 to Jan 2) wrote:
> optional: Don't think you need this line / variable, just grab it from
> lazy_params_ twice.

Done.

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
File components/domain_reliability/dispatcher.cc (right):

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
components/domain_reliability/dispatcher.cc:89: tasks.insert(task.get());
On 2016/12/14 19:12:17, mmenke (Out Dec 17 to Jan 2) wrote:
> std::set<Task*> tasks = tasks_;?

Doesn't work; tasks_ contains unique_ptr<Task> while tasks contains Task*.

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
components/domain_reliability/dispatcher.cc:94: }
On 2016/12/14 19:12:17, mmenke (Out Dec 17 to Jan 2) wrote:
> I assume we can't just do:
> 
> while(!tasks.empty()) {...}
> 
> because running one task may add another?  If that's the case, worth
mentioning
> this magic in the header?  Not clear what it does and does not run.
> 
> If that's not the case, suggest switching.

Yeah, same contract as RunEligibleTasks -- it runs only tasks that existed when
it was called. I'll document that.

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
File components/domain_reliability/monitor.h (right):

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
components/domain_reliability/monitor.h:91: void Shutdown();
On 2016/12/14 19:12:17, mmenke (Out Dec 17 to Jan 2) wrote:
> Needs description

Done.

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
components/domain_reliability/monitor.h:135: void ForceUploadsForTesting();
On 2016/12/14 19:12:17, mmenke (Out Dec 17 to Jan 2) wrote:
> Maybe add a comment?

Done.

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
File components/domain_reliability/uploader.cc (right):

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
components/domain_reliability/uploader.cc:65: DCHECK(shutdown_);
On 2016/12/14 19:12:17, mmenke (Out Dec 17 to Jan 2) wrote:
> include base/logging.h

Done.

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
components/domain_reliability/uploader.cc:167: upload_callbacks_;
On 2016/12/14 19:12:17, mmenke (Out Dec 17 to Jan 2) wrote:
> A bit afield from this CL, but do you mind rename this map to upload_tasks_ or
> something?  upload_callback_ doesn't like something that has the implication
> "This owns URLFetchers."

Sure, I'll just call it uploads_.

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
File components/domain_reliability/uploader.h (right):

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
components/domain_reliability/uploader.h:68: virtual void Shutdown();
On 2016/12/14 19:12:17, mmenke (Out Dec 17 to Jan 2) wrote:
> Needs a comment.

Done.

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
File components/domain_reliability/uploader_unittest.cc (right):

https://codereview.chromium.org/2336043007/diff/200001/components/domain_reli...
components/domain_reliability/uploader_unittest.cc:300:
TEST_F(DomainReliabilityUploaderTest, NoUploadAfterShutdown) {
On 2016/12/14 19:12:17, mmenke (Out Dec 17 to Jan 2) wrote:
> Also have a test that checks that requests are cancelled at shutdown? (Create
a
> hanging request wait for request_count_ to reach 1, call Shutdown(), and then
> call url_request_context_getter_->GetContext()->AssertNoURLRequests());
> 
> Since this test uses its own URLRequestJob subclass, count even add a way to
> check that the created job was torn down instead.  Fine with either approach.

Done.

[mmenke, 2016-12-15 22:16:24]

LGTM!

[Julia Tuttle, 2016-12-15 22:17:25]

The CQ bit was checked by juliatuttle@chromium.org

[commit-bot: I haz the power, 2016-12-15 22:18:06]

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-15 22:43:22]

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

[commit-bot: I haz the power, 2016-12-15 22:43:23]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Julia Tuttle, 2016-12-19 17:06:04]

juliatuttle@chromium.org changed reviewers:
+ mkwst@chromium.org

[Julia Tuttle, 2016-12-19 17:06:05]

mkwst, PTAL at browsing_data

[Mike West, 2016-12-19 17:06:34]

browsing_data LGTM!

[Julia Tuttle, 2016-12-19 17:07:11]

The CQ bit was checked by juliatuttle@chromium.org

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

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-19 19:13:08]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1482167231606940,
"parent_rev": "df58fdb7440eee6c0f6bb872b7d27831c44586a9", "commit_rev":
"f51acd8394bdce48f9a9a9fd012af080f107f6a3"}

[commit-bot: I haz the power, 2016-12-19 19:13:36]

Description was changed from

==========
Domain Reliability: Don't crash on shutdown with uploads pending

This fixes a couple of bugs:

1. Makes sure the ChromeNetworkDelegate can't call in to a destroyed
DomainReliabilityMonitor by moving ownership of the Monitor from
ProfileImplIOData to ChromeNetworkDelegate.

2. Makes sure the uploader doesn't leave URL requests around once they're all
supposed to be gone by adding a separate "shutdown" step that deletes existing
requests and prohibits new ones.

BUG=644858
==========

to

==========
Domain Reliability: Don't crash on shutdown with uploads pending

This fixes a couple of bugs:

1. Makes sure the ChromeNetworkDelegate can't call in to a destroyed
DomainReliabilityMonitor by moving ownership of the Monitor from
ProfileImplIOData to ChromeNetworkDelegate.

2. Makes sure the uploader doesn't leave URL requests around once they're all
supposed to be gone by adding a separate "shutdown" step that deletes existing
requests and prohibits new ones.

BUG=644858

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

[commit-bot: I haz the power, 2016-12-19 19:13:38]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-12-19 19:15:27]

Description was changed from

==========
Domain Reliability: Don't crash on shutdown with uploads pending

This fixes a couple of bugs:

1. Makes sure the ChromeNetworkDelegate can't call in to a destroyed
DomainReliabilityMonitor by moving ownership of the Monitor from
ProfileImplIOData to ChromeNetworkDelegate.

2. Makes sure the uploader doesn't leave URL requests around once they're all
supposed to be gone by adding a separate "shutdown" step that deletes existing
requests and prohibits new ones.

BUG=644858

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

to

==========
Domain Reliability: Don't crash on shutdown with uploads pending

This fixes a couple of bugs:

1. Makes sure the ChromeNetworkDelegate can't call in to a destroyed
DomainReliabilityMonitor by moving ownership of the Monitor from
ProfileImplIOData to ChromeNetworkDelegate.

2. Makes sure the uploader doesn't leave URL requests around once they're all
supposed to be gone by adding a separate "shutdown" step that deletes existing
requests and prohibits new ones.

BUG=644858

Committed: https://crrev.com/127604ea9f06489c24bf6ad47d5ea62b266d45b4
Cr-Commit-Position: refs/heads/master@{#439520}
==========

[commit-bot: I haz the power, 2016-12-19 19:15:29]

Patchset 13 (id:??) landed as
https://crrev.com/127604ea9f06489c24bf6ad47d5ea62b266d45b4
Cr-Commit-Position: refs/heads/master@{#439520}