Work around CancelableCompletionCallback threading restrictions.

Work around CancelableCompletionCallback threading restrictions.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=124020

[michaeln, 2012-02-24 03:45:27]

let's see what the trybots think

[mnaganov (inactive), 2012-02-24 09:51:54]

LGTM. This fixes the problem!

Thanks for reacting quickly!

[michaeln, 2012-02-24 19:49:04]

hi will, ptal at this small change, see the email thread below for details.

----------
From: Mikhail Naganov <mnaganov@google.com>
Date: Thu, Feb 23, 2012 at 11:24 AM
To: Michael Nordman <michaeln@google.com>
Cc: Kinuko Yasuda <kinuko@google.com>, John Knottenbelt <jknotten@google.com>

Hi Michael,

I've noticed a problem in Clank which is also can be reproduced in
Chrome, and it is related to the code you've submitted:
http://codereview.chromium.org/7031065

If you enable DCHECKS:

build/gyp_chromium -D dcheck_always_on=1
...compile...
out/Release/chrome --enable-dcheck

Then go to a site that populates AppCache, e.g.
http://htmlfive.appspot.com/static/stickies.html
Then go to settings > Advanced Settings > Clear browsing data..., make
sure that "Delete cookies and other site and plug-in data" is ticked,
and press "Clear browsing data"

Chrome will crash due to a dcheck failure:

[29286:29298:2992775210615:FATAL:weak_ptr.cc(21)] Check failed:
thread_checker_.CalledOnValidThread().

This happens because AppCacheQuotaClient binds
service_delete_callback_ in the constructor. Constructor is called on
the UI thread during Profile initialization. And internally
CancelableCallback has a Forwarder which uses a weak_ptr
(http://code.google.com/codesearch#OAMlx_jo-ck/src/base/cancelable_callback.h&...)

When one clears website data, QuotaManager's cleanup method is called
on the IO thread due to QuotaManager's requirements. For AppCache,
this drills down to AppCacheQuotaClient::DeleteOriginData which passes
service_delete_callback_ to AppCacheService, which then accesses the
callback while still being on the IO thread.

I realize that perhaps weak_ptr's threading requirements are too
strict, but the problem is we run Clank with DCHECK's enabled by
default, and we are hitting this issue. Can you please help in
resolving it? Do we really need service_delete_callback_ to be bound
at the client construction?

----------
From: Michael Nordman <michaeln@google.com>
Date: Thu, Feb 23, 2012 at 2:22 PM
To: Mikhail Naganov <mnaganov@google.com>
Cc: Kinuko Yasuda <kinuko@google.com>, John Knottenbelt <jknotten@google.com>


We can defer construction of the service_delete_callback_ till just
prior to first use. It's only used on the IO thread. There's really no
bug here afaict.

----------
From: Michael Nordman <michaeln@google.com>
Date: Thu, Feb 23, 2012 at 4:18 PM
To: Mikhail Naganov <mnaganov@google.com>
Cc: Kinuko Yasuda <kinuko@google.com>, John Knottenbelt <jknotten@google.com>


The base::Bind migration for all callback/closure related things makes
this a little messier. Where there once was a
scoped_refptr<CancelableCompletionCallback> whose construction could
be deferred, now there's an inline net::CancelableCompletionCallback
whose construction is less easily deferred, having a scoped_ptr to a
cancelable callback would work, but seems like the point of the
base::Bind changes was to eliminate that pattern for this closure
types.

Maybe we can expose a DetachFromThread() method on the
CancelableCallback class, and call that in the AppCacheQuotaClient
ctor after having constructed it?

[willchan no longer on Chromium, 2012-02-24 20:05:59]

+akalin, +darin in case they have opinions too.

I think this is minor enough that I don't care too strongly. That said, my
preference is to use a scoped_ptr here instead, and construct the
CancelableCompletionCallback on the IO thread. The reason is, although
base::Callback is copyable and what not, CancelableCompletionCallback is not.
Although it's built on top of the new callback infrastructure, I wouldn't treat
it exactly the same way I treat base::Callback, which internally refcounts,
while CancelableCompletionCallback does not.

[michaeln, 2012-02-27 20:16:47]

Updated to work around the restrictions using a scoped_ptr<> data member. I
don't care too strongly either, but fwiw, the one-line change would have been my
preference. Less error prone than making a lazy getter. If we run across other
occurrences of this type of problem with other consumers, consider adding the
DetachFromThread method on the CancelableCompletionCallback class.

On 2012/02/24 20:05:59, willchan wrote:
> +akalin, +darin in case they have opinions too.
> 
> I think this is minor enough that I don't care too strongly. That said, my
> preference is to use a scoped_ptr here instead, and construct the
> CancelableCompletionCallback on the IO thread. The reason is, although
> base::Callback is copyable and what not, CancelableCompletionCallback is not.
> Although it's built on top of the new callback infrastructure, I wouldn't
treat
> it exactly the same way I treat base::Callback, which internally refcounts,
> while CancelableCompletionCallback does not.

[mnaganov (inactive), 2012-02-27 22:02:10]

Provisional LGTM, if it doesn't crash at exit due to a check in WeakPtrFactory.

http://codereview.chromium.org/9456036/diff/18001/webkit/appcache/appcache_qu...
File webkit/appcache/appcache_quota_client.h (right):

http://codereview.chromium.org/9456036/diff/18001/webkit/appcache/appcache_qu...
webkit/appcache/appcache_quota_client.h:82:
scoped_ptr<net::CancelableCompletionCallback> service_delete_callback_;
Perhaps, mention that it is lazily created, or maybe reflect this in the name.

http://codereview.chromium.org/9456036/diff/18001/webkit/appcache/appcache_qu...
webkit/appcache/appcache_quota_client.h:82:
scoped_ptr<net::CancelableCompletionCallback> service_delete_callback_;
Does it play well regarding to destruction? WeakPtrFactory checks the thread in
the destructor, and CancellableCallback owns one. I'm not sure if we are really
running destructors at exit, but I think it's worth checking.

[michaeln, 2012-02-27 22:54:15]

http://codereview.chromium.org/9456036/diff/18001/webkit/appcache/appcache_qu...
File webkit/appcache/appcache_quota_client.h (right):

http://codereview.chromium.org/9456036/diff/18001/webkit/appcache/appcache_qu...
webkit/appcache/appcache_quota_client.h:82:
scoped_ptr<net::CancelableCompletionCallback> service_delete_callback_;
On 2012/02/27 22:02:10, Mikhail Naganov (Chromium) wrote:
> Does it play well regarding to destruction? WeakPtrFactory checks the thread
in
> the destructor, and CancellableCallback owns one. I'm not sure if we are
really
> running destructors at exit, but I think it's worth checking.

Aside from construction, this class is used deleted on the IO thread.

[mnaganov (inactive), 2012-02-28 09:55:58]

On 2012/02/27 22:54:15, michaeln wrote:
>
http://codereview.chromium.org/9456036/diff/18001/webkit/appcache/appcache_qu...
> File webkit/appcache/appcache_quota_client.h (right):
> 
>
http://codereview.chromium.org/9456036/diff/18001/webkit/appcache/appcache_qu...
> webkit/appcache/appcache_quota_client.h:82:
> scoped_ptr<net::CancelableCompletionCallback> service_delete_callback_;
> On 2012/02/27 22:02:10, Mikhail Naganov (Chromium) wrote:
> > Does it play well regarding to destruction? WeakPtrFactory checks the thread
> in
> > the destructor, and CancellableCallback owns one. I'm not sure if we are
> really
> > running destructors at exit, but I think it's worth checking.
> 
> Aside from construction, this class is used deleted on the IO thread.

Yes, with this patch all works well. When are you planning to commit it?

[michaeln, 2012-02-28 19:42:13]

Thnx for verifying it works for you. I'll commit it very soon. I had left it
open for comments, but none others seem to be forth coming :)

> Yes, with this patch all works well. When are you planning to commit it?