Remove bad use of CancelableCallback from BrowingDataAppCacheHelper

Remove bad use of CancelableCallback from BrowingDataAppCacheHelper

BrowsingDataAppCacheHelper no longer needs to be able to cancel its callback,
so remove the "CancelableCallback" member. The current code is also not
correct usage of CancelableCallback, because the callback can be deleted on a
different thread than where it was created. This was making a DCHECK fail when
trying to fix a problem in CancelableCallback (see
https://code.google.com/p/chromium/issues/detail?id=433583#c1).

BUG=433583
R=michaeln@chromium.org

Committed: https://crrev.com/91d2f864f83131a7390ab78b0e19ffedf7c7e514
Cr-Commit-Position: refs/heads/master@{#311369}

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

https://codereview.chromium.org/851553003/diff/1/chrome/browser/browsing_data...
File chrome/browser/browsing_data/browsing_data_appcache_helper.cc (right):

https://codereview.chromium.org/851553003/diff/1/chrome/browser/browsing_data...
chrome/browser/browsing_data/browsing_data_appcache_helper.cc:41:
base::Unretained(this));
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:02]

https://codereview.chromium.org/851553003/diff/1/chrome/browser/browsing_data...
File chrome/browser/browsing_data/browsing_data_appcache_helper.cc (right):

https://codereview.chromium.org/851553003/diff/1/chrome/browser/browsing_data...
chrome/browser/browsing_data/browsing_data_appcache_helper.cc:41:
base::Unretained(this));
On 2015/01/12 22:46:58, michaeln wrote:
> 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.
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?

[michaeln, 2015-01-13 01:24:09]

> 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?


LocalDataContainer is the primary consumer of this class and it looks like it
can delete the appcachehelper while the call is inflight, so the Unretained ptr
is definitely not ok. But the callback it provides to the helper is bound to a
weakptr<LocalDataContainer>, so it should be safe to call after
~LocalDataContainer is gone. None of the other test callsites require
destruction to cancel the callback either.

Looks like the best option is to base::Bind with a
RefCountedThreadSafe<BrowsingDataAppCacheHelper> instead of that rawptr.

[dmichael (off chromium), 2015-01-13 17:06:32]

Done, thanks for checking. Note that base::Bind/base::Callback knows enough to
take a ref on a type that inherits RefCounted or RefCountedThreadSafe, so we
only need to pass |this| directly.

[michaeln, 2015-01-13 21:46:27]

lgtm

https://codereview.chromium.org/851553003/diff/20001/chrome/browser/browsing_...
File chrome/browser/browsing_data/browsing_data_appcache_helper.cc (right):

https://codereview.chromium.org/851553003/diff/20001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_appcache_helper.cc:40:
base::Bind(&BrowsingDataAppCacheHelper::OnFetchComplete, this);
nit: its real common to put the base::Bind call inline in the param list (as on
line 34)

[dmichael (off chromium), 2015-01-13 23:25:23]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2015-01-13 23:26:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/851553003/40001

[commit-bot: I haz the power, 2015-01-14 00:29:49]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-14 00:30:33]

Patchset 3 (id:??) landed as
https://crrev.com/91d2f864f83131a7390ab78b0e19ffedf7c7e514
Cr-Commit-Position: refs/heads/master@{#311369}