Remove bad use of CancelableCallback from BrowingDataAppCacheHelper

chrome/browser/browsing_data/browsing_data_appcache_helper.cc

Index: chrome/browser/browsing_data/browsing_data_appcache_helper.cc
diff --git a/chrome/browser/browsing_data/browsing_data_appcache_helper.cc b/chrome/browser/browsing_data/browsing_data_appcache_helper.cc
index b11e486353a4390f7066b90d78c7df38337888a6..9cc4fcbaf7aac41b239eac43b10be7bd1400686b 100644
--- a/chrome/browser/browsing_data/browsing_data_appcache_helper.cc
+++ b/chrome/browser/browsing_data/browsing_data_appcache_helper.cc
@@ -10,6 +10,7 @@
 #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;
@@ -35,11 +36,11 @@ void BrowsingDataAppCacheHelper::StartFetching(const base::Closure& callback) {
   }
 
   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(