Change threading in ServiceWorkerCacheStorage

Change threading in ServiceWorkerCacheStorage

ServiceWorkerCacheStorage currently expects to have its public
functions run by a SequencedTaskRunner.

This CL changes the functions to run on the calling thread and only
uses the TaskRunner for blocking operations.  This way, when calling
the cache and blob storage, we're on the calling thread (IO).

Also added some comments along the way.

BUG=392621
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289186

[jkarlin, 2014-08-11 18:26:49]

michaeln@chromium.org: Please review changes in everything.  Thanks!

falken@chromium.org: Please review changes in everything. Thanks!

gavinp@chromium.org: FYI

[michaeln, 2014-08-11 19:30:17]

i really like this change, overall this more closely matches how threading is
handled in other parts of sw system (and storage systems in general and i think
chromium in general)

some quick comments below about the patterns used to hop threads, i'll look more
closely at the method impls next

https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_cache_storage.cc (right):

https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.cc:111:
base::Unretained(this),  // should be weak_ptr?
unretained is no good... deriving CacheLoader from RefCountedThreadSafe works
well for this arrangement

https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.cc:372:
base::Unretained(this),
use of unretained here and in other callbacks can be problematic, i think having
ServiceWorkerCacheStorage vend weakptrs works well for this particular pattern

https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_cache_storage.h (right):

https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.h:26: //
|callback_loop| provided in the constructor.
please update the comment to describe the new rulez of threading engagement

https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.h:140:
base::ThreadChecker thread_checker_;
wdyt of calling this io_thread_checker_ ? it's real easy to get mixed up about
which thread is being used or supposed to be used, and naming this differently
could help to make it more clear that the public class methods, and the
callbacks provided to those methods, must be called on the IOThread.

[michaeln, 2014-08-11 19:33:39]

>
https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
> content/browser/service_worker/service_worker_cache_storage.h:140:
> base::ThreadChecker thread_checker_;
> wdyt of calling this io_thread_checker_ ? it's real easy to get mixed up about
> which thread is being used or supposed to be used, and naming this differently
> could help to make it more clear that the public class methods, and the
> callbacks provided to those methods, must be called on the IOThread.

actually, DCHECK_CURRENTLY_ON(BrowserThread::IO) makes that even more clear,
maybe nix the thread_checkers and use that instead

[michaeln, 2014-08-11 20:30:14]

some more comments inline

https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_cache_storage.cc (right):

https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.cc:151: // 1. Delete
the cache's directory if it exists. (CreateDeleteFiles)
(CreateDeleteFiles) s/b (CreateCacheDeleteFilesInPool)

https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.cc:171:
scoped_refptr<base::MessageLoopProxy> original_loop) {
maybe add DCHECKS for cache_task_runner_.RunTasksOnCurrentThread() in methods
that are supposed to be executed there

https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.cc:176:
base::Bind(&SimpleCacheLoader::LoadCache,
This operation take multiple hops to the cache_thread and back again and some
take only one. If all took exactly one, that might help reason about the
consistency of things and the ordering of operations and when things really
stick.

In this case, i think it could be done in one task on the cache thread, more or
less by calling LoadCreateDirectoryInPool() directly here.

https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.cc:202:
scoped_refptr<base::MessageLoopProxy> original_loop) {
can these be const refs (or a rawptr) to the MessageLoopProxy instead of pass by
value

[jkarlin, 2014-08-12 14:55:03]

Thanks!  Ready for another round.

https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_cache_storage.cc (right):

https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.cc:111:
base::Unretained(this),  // should be weak_ptr?
On 2014/08/11 19:30:17, michaeln wrote:
> unretained is no good... deriving CacheLoader from RefCountedThreadSafe works
> well for this arrangement

Done.

https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.cc:151: // 1. Delete
the cache's directory if it exists. (CreateDeleteFiles)
On 2014/08/11 20:30:14, michaeln wrote:
> (CreateDeleteFiles) s/b (CreateCacheDeleteFilesInPool)

Done.

https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.cc:171:
scoped_refptr<base::MessageLoopProxy> original_loop) {
On 2014/08/11 20:30:14, michaeln wrote:
> maybe add DCHECKS for cache_task_runner_.RunTasksOnCurrentThread() in methods
> that are supposed to be executed there

Great idea, thanks!

https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.cc:176:
base::Bind(&SimpleCacheLoader::LoadCache,
On 2014/08/11 20:30:14, michaeln wrote:
> This operation take multiple hops to the cache_thread and back again and some
> take only one. If all took exactly one, that might help reason about the
> consistency of things and the ordering of operations and when things really
> stick.
> 
> In this case, i think it could be done in one task on the cache thread, more
or
> less by calling LoadCreateDirectoryInPool() directly here.

Done.

https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.cc:202:
scoped_refptr<base::MessageLoopProxy> original_loop) {
On 2014/08/11 20:30:14, michaeln wrote:
> can these be const refs (or a rawptr) to the MessageLoopProxy instead of pass
by
> value

Thanks! Done.

https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.cc:372:
base::Unretained(this),
On 2014/08/11 19:30:17, michaeln wrote:
> use of unretained here and in other callbacks can be problematic, i think
having
> ServiceWorkerCacheStorage vend weakptrs works well for this particular pattern

Done.

https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
File content/browser/service_worker/service_worker_cache_storage.h (right):

https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.h:26: //
|callback_loop| provided in the constructor.
On 2014/08/11 19:30:17, michaeln wrote:
> please update the comment to describe the new rulez of threading engagement

Done.

https://codereview.chromium.org/460683002/diff/140001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.h:140:
base::ThreadChecker thread_checker_;
On 2014/08/11 19:30:17, michaeln wrote:
> wdyt of calling this io_thread_checker_ ? it's real easy to get mixed up about
> which thread is being used or supposed to be used, and naming this differently
> could help to make it more clear that the public class methods, and the
> callbacks provided to those methods, must be called on the IOThread.

Done.  Used DCHECK_CURRENTLY_ON(BrowserThread::IO) as you suggested.

[michaeln, 2014-08-12 23:34:32]

lgtm (but please rename the method to obj->GetAsWeakPtr() for consistency with
chromium)

https://codereview.chromium.org/460683002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_cache.h (right):

https://codereview.chromium.org/460683002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_cache.h:25: static
scoped_ptr<ServiceWorkerCache> CreateMemoryCache(
nice change on the rv types

https://codereview.chromium.org/460683002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_cache.h:42:
base::WeakPtr<ServiceWorkerCache> GetWeakPtr() {
typicically a methods on classes that vend weakptrs to themselves are named
GetAsWeakPtr()

[jkarlin, 2014-08-12 23:52:55]

https://codereview.chromium.org/460683002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_cache.h (right):

https://codereview.chromium.org/460683002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_cache.h:25: static
scoped_ptr<ServiceWorkerCache> CreateMemoryCache(
On 2014/08/12 23:34:32, michaeln wrote:
> nice change on the rv types

Acknowledged.

https://codereview.chromium.org/460683002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_cache.h:42:
base::WeakPtr<ServiceWorkerCache> GetWeakPtr() {
On 2014/08/12 23:34:32, michaeln wrote:
> typicically a methods on classes that vend weakptrs to themselves are named
> GetAsWeakPtr()

Done.

[jkarlin, 2014-08-13 00:36:00]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-08-13 00:46:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jkarlin@chromium.org/460683002/210001

[commit-bot: I haz the power, 2014-08-13 03:33:23]

Change committed as 289186

[kinuko, 2014-08-23 06:07:44]

A few drive-by questions, likely just minor ones...

https://codereview.chromium.org/460683002/diff/210001/content/browser/service...
File content/browser/service_worker/service_worker_cache_storage.cc (right):

https://codereview.chromium.org/460683002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.cc:270: CacheMap*
caches,
This is not used and passing the rawptr here feels unsafe?

https://codereview.chromium.org/460683002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.cc:643: cache_ptr));
 // cache is owned by this->CacheMap and won't be deleted
This comment confused me, since we're passing WeakPtr here contrary to what the
comment says. Is it because DeleteCache could come in the mid of WriteIndex, or
could there be some other cases? (If the former is the case can you make the
comment clearer?)

https://codereview.chromium.org/460683002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.cc:652:
callback.Run(false, CACHE_STORAGE_ERROR_STORAGE);
Is returning STORAGE_ERROR expected here?

[jkarlin, 2014-08-25 18:30:02]

Thanks Kinuko.  Addressed in https://codereview.chromium.org/503823002

https://codereview.chromium.org/460683002/diff/210001/content/browser/service...
File content/browser/service_worker/service_worker_cache_storage.cc (right):

https://codereview.chromium.org/460683002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.cc:270: CacheMap*
caches,
On 2014/08/23 06:07:44, kinuko (slow to respond) wrote:
> This is not used and passing the rawptr here feels unsafe?

Done.

https://codereview.chromium.org/460683002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.cc:643: cache_ptr));
 // cache is owned by this->CacheMap and won't be deleted
On 2014/08/23 06:07:43, kinuko (slow to respond) wrote:
> This comment confused me, since we're passing WeakPtr here contrary to what
the
> comment says. Is it because DeleteCache could come in the mid of WriteIndex,
or
> could there be some other cases? (If the former is the case can you make the
> comment clearer?)

Fixed.  Thanks.

https://codereview.chromium.org/460683002/diff/210001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage.cc:652:
callback.Run(false, CACHE_STORAGE_ERROR_STORAGE);
On 2014/08/23 06:07:43, kinuko (slow to respond) wrote:
> Is returning STORAGE_ERROR expected here?

Done.