Remove bad use of CancelableCallback from BrowingDataAppCacheHelper

chrome/browser/browsing_data/browsing_data_appcache_helper.cc

 // Copyright (c) 2012 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 "chrome/browser/browsing_data/browsing_data_appcache_helper.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "chrome/browser/browsing_data/browsing_data_helper.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/storage_partition.h"
+#include "net/base/completion_callback.h"
 
 using content::BrowserContext;
 using content::BrowserThread;
 
 BrowsingDataAppCacheHelper::BrowsingDataAppCacheHelper(
     BrowserContext* browser_context)
     : is_fetching_(false),
       appcache_service_(BrowserContext::GetDefaultStoragePartition(
                             browser_context)->GetAppCacheService()) {
 }
 
 void BrowsingDataAppCacheHelper::StartFetching(const base::Closure& callback) {
   if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
     DCHECK(!is_fetching_);
     DCHECK(!callback.is_null());
     is_fetching_ = true;
     info_collection_ = new content::AppCacheInfoCollection;
     completion_callback_ = callback;
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE,
         base::Bind(&BrowsingDataAppCacheHelper::StartFetching, this, callback));
     return;
   }
 
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  appcache_info_callback_.Reset(
+  net::CompletionCallback appcache_info_callback =
       base::Bind(&BrowsingDataAppCacheHelper::OnFetchComplete,
-                 base::Unretained(this)));
+                 base::Unretained(this));

[michaeln, 2015/01/12 22:46:58]

This assumes BrowsingDataAppCacheHelper can't be deleted prior to completion,
what makes that true? And if that is true, might as well have this bind closure
hold a ref to BrowsingDataAppCacheHelper instead of gambling with the unretained
ptr.

[dmichael (off chromium), 2015/01/13 00:02:01]

I was admittedly going with what I thought was the least behavior change without
taking the time to understand if the existing Unretained was safe, since this
isn't my primary goal. I realize now that the CancelableCallback might have been
expected to act like a callback bound with a WeakPtr. It isn't really safe
though because the callback happens on the IO thread while
BrowsingDataAppCacheHelper may be deleted on another thread, and
CancelableCallback doesn't do any kind of synchronization (it just relies on
WeakPtrFactory which also isn't thread-safe).

If bind a reference, it may extend the lifetime of |this|. Looking at
OnFetchComplete, it appears that could cause |completion_callback_| to be run
when it wouldn't today. I don't have enough context in this area...  do you
think that's OK, or if not, is there somebody more familiar with this class?

   appcache_service_->GetAllAppCacheInfo(info_collection_.get(),
-                                        appcache_info_callback_.callback());
+                                        appcache_info_callback);
 }
 
 void BrowsingDataAppCacheHelper::DeleteAppCacheGroup(
     const GURL& manifest_url) {
   if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE,
         base::Bind(&BrowsingDataAppCacheHelper::DeleteAppCacheGroup, this,
                    manifest_url));
     return;
@@ skipping 87 matching lines @@
   completion_callback.Run();
 }
 
 void CannedBrowsingDataAppCacheHelper::DeleteAppCacheGroup(
     const GURL& manifest_url) {
   info_collection_->infos_by_origin.erase(manifest_url.GetOrigin());
   BrowsingDataAppCacheHelper::DeleteAppCacheGroup(manifest_url);
 }
 
 CannedBrowsingDataAppCacheHelper::~CannedBrowsingDataAppCacheHelper() {}