Fix two ContentHashFetcherTest.* tests' flakiness.

extensions/browser/content_hash_fetcher.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "extensions/browser/content_hash_fetcher.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <memory>
@@ skipping 190 matching lines @@
                      base::TaskPriority::USER_VISIBLE),
       base::Bind(&ContentHashFetcherJob::LoadVerifiedContents, this,
                  verified_contents_path),
       base::Bind(&ContentHashFetcherJob::DoneCheckingForVerifiedContents,
                  this));
 }
 
 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() {
   base::AutoLock autolock(cancelled_lock_);
   bool result = cancelled_;
   return result;
 }
 
 ContentHashFetcherJob::~ContentHashFetcherJob() {
 }
@@ skipping 216 matching lines @@
                       timer.Elapsed());
   return result;
 }
 
 void ContentHashFetcherJob::DispatchCallback() {
   {
     base::AutoLock autolock(cancelled_lock_);
     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);
 }
 
 // ----
 
 ContentHashFetcher::ContentHashFetcher(
     net::URLRequestContextGetter* context_getter,
     ContentVerifierDelegate* delegate,
     const FetchCallback& callback)
     : context_getter_(context_getter),
@@ skipping 65 matching lines @@
 
   for (JobMap::iterator i = jobs_.begin(); i != jobs_.end(); ++i) {
     if (i->second.get() == job) {
       jobs_.erase(i);
       break;
     }
   }
 }
 
 }  // namespace extensions