[BackgroundSync] Handle storage failure

[BackgroundSync] Handle storage failure

This CL introduces proper failure handling in case the storage backend fails.

Changes:

1. If the ServiceWorker is unregistered - Remove the sync registrations from memory
2. If the ServiceWorkerStorage is wiped - Reinit the BackgroundSyncManager
3. If reading or writing from ServiceWorkerStorage fails (SERVICE_WORKER_ERROR_FAILURE) - Delete everything and disable the BackgroundSyncManager until browser restart.

Added unittests for above cases. I had to change the unittest to register serviceworkers via the ServiceWorkerContext instead of directly in Storage so that the ServiceWorkerContextObserver receives updates.

BUG=449443
Committed: https://crrev.com/9059304cb5c6f7f683ea30259eeacc5a429566c9
Cr-Commit-Position: refs/heads/master@{#323756}

[jkarlin, 2015-03-31 15:34:31]

Patchset #3 (id:40001) has been deleted

[jkarlin, 2015-03-31 15:38:09]

jkarlin@chromium.org changed reviewers:
+ jsbell@chromium.org

[jkarlin, 2015-03-31 15:39:11]

jsbell: PTAL at everything. In particular, is this the right way to handle
errors from CacheStorage?

[jsbell, 2015-03-31 16:33:13]

On 2015/03/31 15:39:11, jkarlin wrote:
> In particular, is this the right way to handle errors from CacheStorage?

This should be (and, from the CLs, appears to be) completely independent of
CacheStorage; it's tied to the SW registration system but that's independent of
CacheStorage as well. The OnStorageWiped corresponds to recovery on the SW
database.

Am I missing something?

(I'll go through the CL again in more detail shortly.)

[jkarlin, 2015-03-31 16:37:41]

On 2015/03/31 16:33:13, jsbell wrote:
> On 2015/03/31 15:39:11, jkarlin wrote:
> > In particular, is this the right way to handle errors from CacheStorage?
> 
> This should be (and, from the CLs, appears to be) completely independent of
> CacheStorage; it's tied to the SW registration system but that's independent
of
> CacheStorage as well. The OnStorageWiped corresponds to recovery on the SW
> database.
> 
> Am I missing something?
> 
> (I'll go through the CL again in more detail shortly.)

Bah, sorry. My eyes burned in on the word CacheStorage yesterday. I meant
ServiceWorkerStorage.

[jsbell, 2015-03-31 16:41:53]

On 2015/03/31 16:37:41, jkarlin wrote:
> Bah, sorry. My eyes burned in on the word CacheStorage yesterday. I meant
> ServiceWorkerStorage.

I know the feeling. :)

I'll still take a look, but if there's anything particularly tricky I may have
to defer to nhiroki or michaeln. From an initial pass it looked correct, though.

[jkarlin, 2015-03-31 16:43:43]

jkarlin@chromium.org changed reviewers:
+ michaeln@chromium.org

[jkarlin, 2015-03-31 16:44:48]

On 2015/03/31 16:41:53, jsbell wrote:
> On 2015/03/31 16:37:41, jkarlin wrote:
> > Bah, sorry. My eyes burned in on the word CacheStorage yesterday. I meant
> > ServiceWorkerStorage.
> 
> I know the feeling. :)
> 
> I'll still take a look, but if there's anything particularly tricky I may have
> to defer to nhiroki or michaeln. From an initial pass it looked correct,
though.

Adding michaeln@ too. Extra eyeballs on this is good.

[jkarlin, 2015-04-01 16:00:30]

Patchset #4 (id:80001) has been deleted

[jkarlin, 2015-04-01 17:15:54]

jsbell and michaeln, PTAL. Thanks!

[michaeln, 2015-04-01 21:37:31]

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:293:
DCHECK(disabled_);
Can BackgroundSyncManager::OnStorageWipedImpl happen while the op to run this
method works its way thru the scheduler? Seems like it could.

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:557: disabled_ =
false;
would it make sense to schedule an op to reinit?

[jsbell, 2015-04-01 21:54:54]

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:128: base::Closure
pending_callback =
Can you add a comment that this do-nothing callback is still necessary since
PendingClosure will kick the scheduler?

Or since this pattern occurs several times, maybe add a private helper that
creates these do-nothing closures so it can be inlined into the
ScheduleOperation call and the name might be self documenting?
(EmptyCompletion() or some such, dunno)

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:291: void
BackgroundSyncManager::DisableAndClearManagerImpl(
Out of curiosity, are there any relevant "I'm running as a scheduled operation"
assertions that can be made here, akin to the DCHECK_CURRENTLY_ON() ? Maybe we
can add that to CacheStorageScheduler at some point.

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:298: // may be
corrupt so reload the registrations from storage again.
Maybe clarify in the comment why memory is "corrupt" (i.e. its out of sync w/ sw
reg?)

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager_unittest.cc
(right):

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager_unittest.cc:603: //
restart or notification that the storage has been wiped.
Should we have a test for the OnStorageWiped path?

[jkarlin, 2015-04-02 17:57:56]

Thanks!

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:128: base::Closure
pending_callback =
On 2015/04/01 21:54:54, jsbell wrote:
> Can you add a comment that this do-nothing callback is still necessary since
> PendingClosure will kick the scheduler?
> 
> Or since this pattern occurs several times, maybe add a private helper that
> creates these do-nothing closures so it can be inlined into the
> ScheduleOperation call and the name might be self documenting?
> (EmptyCompletion() or some such, dunno)

Nice suggestion. I made such functions for all of the scheduleoperation calls. 

I'd use variadic template functions for this, but there is no good way to alias
them. So each call would wind up looking like.. MakeOperationCompletion<const
StatusAndRegistrationCallback&, const BackgroundSyncRegistration&,
ErrorType>(callback) which is a bit ugly.

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:291: void
BackgroundSyncManager::DisableAndClearManagerImpl(
On 2015/04/01 21:54:54, jsbell wrote:
> Out of curiosity, are there any relevant "I'm running as a scheduled
operation"
> assertions that can be made here, akin to the DCHECK_CURRENTLY_ON() ? Maybe we
> can add that to CacheStorageScheduler at some point.

There is a check that an operation is currently running, which the scheduler
checks on the CompleteOperationAndRunNext() call. Not sure that it's necessary
here as well. I've added a TODO to the scheduler to be able to verify that the
right operation (by identifier) is running, a la DCHECK_CURRENTLY_ON.

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:293:
DCHECK(disabled_);
On 2015/04/01 21:37:30, michaeln wrote:
> Can BackgroundSyncManager::OnStorageWipedImpl happen while the op to run this
> method works its way thru the scheduler? Seems like it could.

Yes, there is a race on disabled_ if DisableAndClearManagerImpl runs before
OnStorageWipedImpl gets to. Nice catch! The solution here is to run
DisableAndClearManager as part of the existing operation instead of making it
its own operation. This is much saner now. Done.

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:298: // may be
corrupt so reload the registrations from storage again.
On 2015/04/01 21:54:54, jsbell wrote:
> Maybe clarify in the comment why memory is "corrupt" (i.e. its out of sync w/
sw
> reg?)

Done.

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:557: disabled_ =
false;
On 2015/04/01 21:37:30, michaeln wrote:
> would it make sense to schedule an op to reinit?

Scheduling the op might allow some new requests to run before initing. Unless we
wait to set disabled_ to false until after the scheduled init runs. It seems
fine to run it now.

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager_unittest.cc
(right):

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager_unittest.cc:603: //
restart or notification that the storage has been wiped.
On 2015/04/01 21:54:54, jsbell wrote:
> Should we have a test for the OnStorageWiped path?

That's what this test is. Note the "ScheduleDeleteAndStartOver" call 2 lines
down, that results in an OnStorageWiped call.

The test above tests the restart path.

[jsbell, 2015-04-02 20:25:13]

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:291: void
BackgroundSyncManager::DisableAndClearManagerImpl(
On 2015/04/02 17:57:56, jkarlin wrote:
> On 2015/04/01 21:54:54, jsbell wrote:
> > Out of curiosity, are there any relevant "I'm running as a scheduled
> operation"
> > assertions that can be made here, akin to the DCHECK_CURRENTLY_ON() ? Maybe
we
> > can add that to CacheStorageScheduler at some point.
> 
> There is a check that an operation is currently running, which the scheduler
> checks on the CompleteOperationAndRunNext() call. Not sure that it's necessary
> here as well. I've added a TODO to the scheduler to be able to verify that the
> right operation (by identifier) is running, a la DCHECK_CURRENTLY_ON.

sgtm

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager_unittest.cc
(right):

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager_unittest.cc:603: //
restart or notification that the storage has been wiped.
Oops - missed that it was calling it on the SW context. :)

https://codereview.chromium.org/1048053002/diff/120001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_scheduler.cc (right):

https://codereview.chromium.org/1048053002/diff/120001/content/browser/cache_...
content/browser/cache_storage/cache_storage_scheduler.cc:43:
base::MessageLoopProxy::current()->PostTask(FROM_HERE, base::Bind(closure));
Thanks for this change - I was slightly worried that if any synchronous
operations were introduced it could hog the thread and/or blow out the stack.

Looks like the CacheStorageSchedulerTest cases need RunUntilIdle() sprinkles
now, though.

[michaeln, 2015-04-02 23:06:11]

lgtm (modulo green tests)

[michaeln, 2015-04-02 23:06:20]

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager.cc (right):

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:128: base::Closure
pending_callback =
On 2015/04/02 17:57:56, jkarlin wrote:
> On 2015/04/01 21:54:54, jsbell wrote:
> > Can you add a comment that this do-nothing callback is still necessary since
> > PendingClosure will kick the scheduler?
> > 
> > Or since this pattern occurs several times, maybe add a private helper that
> > creates these do-nothing closures so it can be inlined into the
> > ScheduleOperation call and the name might be self documenting?
> > (EmptyCompletion() or some such, dunno)
> 
> Nice suggestion. I made such functions for all of the scheduleoperation calls.

> 
> I'd use variadic template functions for this, but there is no good way to
alias
> them. So each call would wind up looking like.. MakeOperationCompletion<const
> StatusAndRegistrationCallback&, const BackgroundSyncRegistration&,
> ErrorType>(callback) which is a bit ugly.

Nice, those helpers help a lot.

https://codereview.chromium.org/1048053002/diff/100001/content/browser/backgr...
content/browser/background_sync/background_sync_manager.cc:293:
DCHECK(disabled_);
On 2015/04/02 17:57:56, jkarlin wrote:
> On 2015/04/01 21:37:30, michaeln wrote:
> > Can BackgroundSyncManager::OnStorageWipedImpl happen while the op to run
this
> > method works its way thru the scheduler? Seems like it could.
> 
> Yes, there is a race on disabled_ if DisableAndClearManagerImpl runs before
> OnStorageWipedImpl gets to. Nice catch! The solution here is to run
> DisableAndClearManager as part of the existing operation instead of making it
> its own operation. This is much saner now. Done.

yup, that works!

https://codereview.chromium.org/1048053002/diff/120001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager_unittest.cc
(right):

https://codereview.chromium.org/1048053002/diff/120001/content/browser/backgr...
content/browser/background_sync/background_sync_manager_unittest.cc:546: bool
register_called = false;
nit: maybe callback_called to match callback_error_

https://codereview.chromium.org/1048053002/diff/120001/content/browser/backgr...
content/browser/background_sync/background_sync_manager_unittest.cc:588: new
TestBackgroundSyncManager(helper_->context_wrapper()));
A comment about this emulating the restart case would have helped me.

[jkarlin, 2015-04-03 15:15:04]

Thanks!

https://codereview.chromium.org/1048053002/diff/120001/content/browser/backgr...
File content/browser/background_sync/background_sync_manager_unittest.cc
(right):

https://codereview.chromium.org/1048053002/diff/120001/content/browser/backgr...
content/browser/background_sync/background_sync_manager_unittest.cc:546: bool
register_called = false;
On 2015/04/02 23:06:20, michaeln wrote:
> nit: maybe callback_called to match callback_error_

Done.

https://codereview.chromium.org/1048053002/diff/120001/content/browser/backgr...
content/browser/background_sync/background_sync_manager_unittest.cc:588: new
TestBackgroundSyncManager(helper_->context_wrapper()));
On 2015/04/02 23:06:20, michaeln wrote:
> A comment about this emulating the restart case would have helped me.

Sorry about that. Done.

https://codereview.chromium.org/1048053002/diff/120001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_scheduler.cc (right):

https://codereview.chromium.org/1048053002/diff/120001/content/browser/cache_...
content/browser/cache_storage/cache_storage_scheduler.cc:43:
base::MessageLoopProxy::current()->PostTask(FROM_HERE, base::Bind(closure));
On 2015/04/02 20:25:13, jsbell wrote:
> Thanks for this change - I was slightly worried that if any synchronous
> operations were introduced it could hog the thread and/or blow out the stack.
> 
> Looks like the CacheStorageSchedulerTest cases need RunUntilIdle() sprinkles
> now, though.

Done.

[jsbell, 2015-04-03 16:38:03]

lgtm!

[jkarlin, 2015-04-03 16:42:47]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2015-04-03 16:42:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/1048053002/#ps140001
(title: "Address comments from PS5")

[commit-bot: I haz the power, 2015-04-03 16:43:16]

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

[commit-bot: I haz the power, 2015-04-03 16:47:07]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-04-03 20:38:21]

Patchset 6 (id:??) landed as
https://crrev.com/9059304cb5c6f7f683ea30259eeacc5a429566c9
Cr-Commit-Position: refs/heads/master@{#323756}