Fix two ContentHashFetcherTest.* tests' flakiness.

extensions/browser/content_hash_fetcher.cc

Index: extensions/browser/content_hash_fetcher.cc
diff --git a/extensions/browser/content_hash_fetcher.cc b/extensions/browser/content_hash_fetcher.cc
index 4ed26fafb1692ca4f39ede9564d179bb7fd9775c..ecf4bbcd75a6508edca144a53a1d7026616ed0c6 100644
--- a/extensions/browser/content_hash_fetcher.cc
+++ b/extensions/browser/content_hash_fetcher.cc
@@ -208,6 +208,7 @@ void ContentHashFetcherJob::Start() {
 void ContentHashFetcherJob::Cancel() {
   base::AutoLock autolock(cancelled_lock_);
   cancelled_ = true;
+  url_fetcher_.reset();

[Devlin, 2017/04/13 15:12:18]

nit: We can get rid of cancelled_ now and use !url_fetcher_ instead.

 }
 
 bool ContentHashFetcherJob::IsCancelled() {
@@ -444,6 +445,7 @@ void ContentHashFetcherJob::DispatchCallback() {
     if (cancelled_)
       return;
   }
+  url_fetcher_.reset();

[lazyboy, 2017/04/13 02:06:35]

btw, this is the reset that's necessary for the test to pass.

[Devlin, 2017/04/13 15:12:18]

Where is the reference to ContentHashFetcherJob in URLFetcher?  I only see it
passed in as the URLFetcherDelegate (as a raw ptr, which wouldn't add a
reference), and don't see any callbacks.  What am I missing?

[lazyboy, 2017/04/13 16:06:24]

You're right, I got too excited yesterday and didn't properly check that the
URLFetcherDelegate ptr is raw ptr :)
The fix here occurs not because of ref, but because we kill url_fetcher_ early,
when the task runner check here
https://cs.chromium.org/chromium/src/net/url_request/url_fetcher_core.cc?rcl=...
holds true:
"""
void URLFetcherCore::Stop() {
  if (delegate_task_runner_)  // May be NULL in tests.
    DCHECK(delegate_task_runner_->RunsTasksOnCurrentThread());
...
"""
This is the check that fails when the test fails.
So, I need to reword the CL description... WDYT?

[Devlin, 2017/04/13 16:24:45]

Hmm...

It makes sense why that would solve the problem, but then the issue is that
ContentHashFetcherJob is still potentially leaking.  It *looks* like the only
refs should be held by the callbacks it creates internally (which should all be
cleaned up by the time it finishes) and by the ContentHashFetcher itself (which
should release the ref in JobFinished()).

Two thoughts:
1. If that's right, the ContentHashFetcherJob should be released, and is only
not destructed immediately because of the DeleteSoon() call (as opposed to
`delete`).  If this is the case, using a base::RunLoop().RunUntilIdle() call at
the end of the test (or even right after the job finishes) should solve the
problem.  It would be good to check that that's the case, since otherwise we may
have a leak on our hands (which could be bad for a few reasons).
2. Assuming that the problem is just that job is living a bit too long, then
we're left with two possible fixes: this, and the
base::RunLoop().RunUntilIdle().  Unfortunately, I don't love either one, since
they both seem targeted at fixing the test, rather than a potential problem in
production (though I'm not sure this can ever actually happen in production? 
Perhaps not).  This one deletes an object early because a thread is destructed
in the test tear down, and the other ensures this whole object is deleted before
tear down.  I'm fine with either one, but we should probably add a comment
explaining whichever path we choose.

   callback_.Run(this);
 }