[BackgroundSync] Handle storage failure

content/browser/background_sync/background_sync_manager.cc

Index: content/browser/background_sync/background_sync_manager.cc
diff --git a/content/browser/background_sync/background_sync_manager.cc b/content/browser/background_sync/background_sync_manager.cc
index 7ea194f1af92dd463d56a59fbc1527c99842d81f..1075e39fa3261bbab20146c94fea6886aa394b29 100644
--- a/content/browser/background_sync/background_sync_manager.cc
+++ b/content/browser/background_sync/background_sync_manager.cc
@@ -46,6 +46,7 @@ scoped_ptr<BackgroundSyncManager> BackgroundSyncManager::Create(
 }
 
 BackgroundSyncManager::~BackgroundSyncManager() {
+  service_worker_context_->RemoveObserver(this);
 }
 
 void BackgroundSyncManager::Register(
@@ -57,6 +58,13 @@ void BackgroundSyncManager::Register(
   DCHECK_EQ(BackgroundSyncRegistration::kInvalidRegistrationId,
             sync_registration.id);
 
+  if (disabled_) {
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(callback, ERROR_TYPE_STORAGE, BackgroundSyncRegistration()));
+    return;
+  }
+
   StatusAndRegistrationCallback pending_callback =
       base::Bind(&BackgroundSyncManager::PendingStatusAndRegistrationCallback,
                  weak_ptr_factory_.GetWeakPtr(), callback);
@@ -74,6 +82,12 @@ void BackgroundSyncManager::Unregister(
     const StatusCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
+  if (disabled_) {
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE, base::Bind(callback, ERROR_TYPE_STORAGE));
+    return;
+  }
+
   StatusCallback pending_callback =
       base::Bind(&BackgroundSyncManager::PendingStatusCallback,
                  weak_ptr_factory_.GetWeakPtr(), callback);
@@ -91,6 +105,13 @@ void BackgroundSyncManager::GetRegistration(
     const StatusAndRegistrationCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
+  if (disabled_) {
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(callback, ERROR_TYPE_STORAGE, BackgroundSyncRegistration()));
+    return;
+  }
+
   StatusAndRegistrationCallback pending_callback =
       base::Bind(&BackgroundSyncManager::PendingStatusAndRegistrationCallback,
                  weak_ptr_factory_.GetWeakPtr(), callback);
@@ -101,33 +122,81 @@ void BackgroundSyncManager::GetRegistration(
                  sync_registration_name, pending_callback));
 }
 
+void BackgroundSyncManager::OnRegistrationDeleted(int64 registration_id,
+                                                  const GURL& pattern) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  base::Closure pending_callback =

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

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)

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

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.

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

Nice, those helpers help a lot.

+      base::Bind(&BackgroundSyncManager::PendingClosure,
+                 weak_ptr_factory_.GetWeakPtr(), base::Bind(base::DoNothing));
+
+  // Operations already in the queue will either fail when they write to storage
+  // or return stale results based on registrations loaded in memory. This is
+  // inconsequential since the service worker is gone.
+  op_scheduler_.ScheduleOperation(base::Bind(
+      &BackgroundSyncManager::OnRegistrationDeletedImpl,
+      weak_ptr_factory_.GetWeakPtr(), registration_id, pending_callback));
+}
+
+void BackgroundSyncManager::OnStorageWiped() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  base::Closure pending_callback =
+      base::Bind(&BackgroundSyncManager::PendingClosure,
+                 weak_ptr_factory_.GetWeakPtr(), base::Bind(base::DoNothing));
+
+  // Operations already in the queue will either fail when they write to storage
+  // or return stale results based on registrations loaded in memory. This is
+  // inconsequential since the service workers are gone.
+  op_scheduler_.ScheduleOperation(
+      base::Bind(&BackgroundSyncManager::OnStorageWipedImpl,
+                 weak_ptr_factory_.GetWeakPtr(), pending_callback));
+}
+
 BackgroundSyncManager::BackgroundSyncManager(
     const scoped_refptr<ServiceWorkerContextWrapper>& service_worker_context)
-    : service_worker_context_(service_worker_context), weak_ptr_factory_(this) {
+    : service_worker_context_(service_worker_context),
+      disabled_(false),
+      weak_ptr_factory_(this) {
+  service_worker_context_->AddObserver(this);
 }
 
 void BackgroundSyncManager::Init() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(!op_scheduler_.ScheduledOperations());
+  DCHECK(!disabled_);
+
+  base::Closure pending_callback =
+      base::Bind(&BackgroundSyncManager::PendingClosure,
+                 weak_ptr_factory_.GetWeakPtr(), base::Bind(base::DoNothing));
 
   op_scheduler_.ScheduleOperation(base::Bind(&BackgroundSyncManager::InitImpl,
-                                             weak_ptr_factory_.GetWeakPtr()));
+                                             weak_ptr_factory_.GetWeakPtr(),
+                                             pending_callback));
 }
 
-void BackgroundSyncManager::InitImpl() {
+void BackgroundSyncManager::InitImpl(const base::Closure& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
+  if (disabled_) {
+    base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(callback));
+    return;
+  }
+
   GetDataFromBackend(
       kBackgroundSyncUserDataKey,
       base::Bind(&BackgroundSyncManager::InitDidGetDataFromBackend,
-                 weak_ptr_factory_.GetWeakPtr()));
+                 weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 void BackgroundSyncManager::InitDidGetDataFromBackend(
+    const base::Closure& callback,
     const std::vector<std::pair<int64, std::string>>& user_data,
     ServiceWorkerStatusCode status) {
-  if (status != SERVICE_WORKER_OK && status != SERVICE_WORKER_ERROR_NOT_FOUND)
-    LOG(ERROR) << "Background Sync Failed to load from backend.";
+  if (status != SERVICE_WORKER_OK && status != SERVICE_WORKER_ERROR_NOT_FOUND) {
+    LOG(ERROR) << "BackgroundSync failed to init due to backend failure.";
+    DisableAndClearManager();
+    base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(callback));
+    return;
+  }
 
   bool corruption_detected = false;
   for (const std::pair<int64, std::string>& data : user_data) {
@@ -163,12 +232,12 @@ void BackgroundSyncManager::InitDidGetDataFromBackend(
 
   if (corruption_detected) {
     LOG(ERROR) << "Corruption detected in background sync backend";
-    sw_to_registrations_map_.clear();
+    DisableAndClearManager();
   }
 
   // TODO(jkarlin): Call the scheduling algorithm here.
 
-  op_scheduler_.CompleteOperationAndRunNext();
+  base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(callback));
 }
 
 void BackgroundSyncManager::RegisterImpl(
@@ -176,6 +245,13 @@ void BackgroundSyncManager::RegisterImpl(
     int64 sw_registration_id,
     const BackgroundSyncRegistration& sync_registration,
     const StatusAndRegistrationCallback& callback) {
+  if (disabled_) {
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(callback, ERROR_TYPE_STORAGE, BackgroundSyncRegistration()));
+    return;
+  }
+
   BackgroundSyncRegistration existing_registration;
   if (LookupRegistration(sw_registration_id, sync_registration.name,
                          &existing_registration)) {
@@ -198,7 +274,60 @@ void BackgroundSyncManager::RegisterImpl(
       origin, sw_registration_id,
       base::Bind(&BackgroundSyncManager::RegisterDidStore,
                  weak_ptr_factory_.GetWeakPtr(), sw_registration_id,
-                 new_registration, existing_registration, callback));
+                 new_registration, callback));
+}
+
+void BackgroundSyncManager::DisableAndClearManager() {
+  disabled_ = true;
+  base::Closure pending_callback =
+      base::Bind(&BackgroundSyncManager::PendingClosure,
+                 weak_ptr_factory_.GetWeakPtr(), base::Bind(base::DoNothing));
+
+  op_scheduler_.ScheduleOperation(
+      base::Bind(&BackgroundSyncManager::DisableAndClearManagerImpl,
+                 weak_ptr_factory_.GetWeakPtr(), pending_callback));
+}
+
+void BackgroundSyncManager::DisableAndClearManagerImpl(

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

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.

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

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.

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

sgtm

+    const base::Closure& callback) {
+  DCHECK(disabled_);

[michaeln, 2015/04/01 21:37:30]

Can BackgroundSyncManager::OnStorageWipedImpl happen while the op to run this
method works its way thru the scheduler? Seems like it could.

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

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.

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

yup, that works!

+
+  sw_to_registrations_map_.clear();
+
+  // Delete all backend entries. The memory representation of registered syncs
+  // may be corrupt so reload the registrations from storage again.

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

Maybe clarify in the comment why memory is "corrupt" (i.e. its out of sync w/ sw
reg?)

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

Done.

+  GetDataFromBackend(
+      kBackgroundSyncUserDataKey,
+      base::Bind(&BackgroundSyncManager::DisableAndClearDidGetRegistrations,
+                 weak_ptr_factory_.GetWeakPtr(), callback));
+}
+
+void BackgroundSyncManager::DisableAndClearDidGetRegistrations(
+    const base::Closure& callback,
+    const std::vector<std::pair<int64, std::string>>& user_data,
+    ServiceWorkerStatusCode status) {
+  if (status != SERVICE_WORKER_OK || user_data.empty()) {
+    base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(callback));
+    return;
+  }
+
+  base::Closure barrier_closure =
+      base::BarrierClosure(user_data.size(), base::Bind(callback));
+
+  for (const auto& sw_id_and_regs : user_data) {
+    service_worker_context_->context()->storage()->ClearUserData(
+        sw_id_and_regs.first, kBackgroundSyncUserDataKey,
+        base::Bind(&BackgroundSyncManager::DisableAndClearManagerClearedOne,
+                   weak_ptr_factory_.GetWeakPtr(), barrier_closure));
+  }
+}
+
+void BackgroundSyncManager::DisableAndClearManagerClearedOne(
+    const base::Closure& barrier_closure,
+    ServiceWorkerStatusCode status) {
+  // The status doesn't matter at this point, there is nothing else to be done.
+  base::MessageLoop::current()->PostTask(FROM_HERE,
+                                         base::Bind(barrier_closure));
 }
 
 bool BackgroundSyncManager::LookupRegistration(
@@ -255,18 +384,21 @@ void BackgroundSyncManager::StoreRegistrations(
 void BackgroundSyncManager::RegisterDidStore(
     int64 sw_registration_id,
     const BackgroundSyncRegistration& new_registration,
-    const BackgroundSyncRegistration& previous_registration,
     const StatusAndRegistrationCallback& callback,
     ServiceWorkerStatusCode status) {
+  if (status == SERVICE_WORKER_ERROR_NOT_FOUND) {
+    // The registration is gone.
+    sw_to_registrations_map_.erase(sw_registration_id);
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(callback, ERROR_TYPE_STORAGE, BackgroundSyncRegistration()));
+    return;
+  }
+
   if (status != SERVICE_WORKER_OK) {
-    // Restore the previous state.
-    if (previous_registration.id !=
-        BackgroundSyncRegistration::kInvalidRegistrationId) {
-      AddRegistrationToMap(sw_registration_id, previous_registration);
-    } else {
-      RemoveRegistrationFromMap(sw_registration_id, new_registration.name,
-                                nullptr);
-    }
+    LOG(ERROR) << "BackgroundSync failed to store registration due to backend "
+                  "failure.";
+    DisableAndClearManager();
     base::MessageLoop::current()->PostTask(
         FROM_HERE,
         base::Bind(callback, ERROR_TYPE_STORAGE, BackgroundSyncRegistration()));
@@ -334,6 +466,12 @@ void BackgroundSyncManager::UnregisterImpl(
     const std::string& sync_registration_name,
     BackgroundSyncRegistration::RegistrationId sync_registration_id,
     const StatusCallback& callback) {
+  if (disabled_) {
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE, base::Bind(callback, ERROR_TYPE_STORAGE));
+    return;
+  }
+
   BackgroundSyncRegistration existing_registration;
   if (!LookupRegistration(sw_registration_id, sync_registration_name,
                           &existing_registration) ||
@@ -359,9 +497,17 @@ void BackgroundSyncManager::UnregisterDidStore(
     const BackgroundSyncRegistration& old_sync_registration,
     const StatusCallback& callback,
     ServiceWorkerStatusCode status) {
+  if (status == SERVICE_WORKER_ERROR_NOT_FOUND) {
+    // ServiceWorker was unregistered.
+    sw_to_registrations_map_.erase(sw_registration_id);
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE, base::Bind(callback, ERROR_TYPE_STORAGE));
+    return;
+  }
+
   if (status != SERVICE_WORKER_OK) {
-    // Restore the previous state.
-    AddRegistrationToMap(sw_registration_id, old_sync_registration);
+    LOG(ERROR) << "BackgroundSync failed to unregister due to backend failure.";
+    DisableAndClearManager();
     base::MessageLoop::current()->PostTask(
         FROM_HERE, base::Bind(callback, ERROR_TYPE_STORAGE));
     return;
@@ -377,6 +523,13 @@ void BackgroundSyncManager::GetRegistrationImpl(
     int64 sw_registration_id,
     const std::string sync_registration_name,
     const StatusAndRegistrationCallback& callback) {
+  if (disabled_) {
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(callback, ERROR_TYPE_STORAGE, BackgroundSyncRegistration()));
+    return;
+  }
+
   BackgroundSyncRegistration out_registration;
   if (!LookupRegistration(sw_registration_id, sync_registration_name,
                           &out_registration)) {
@@ -390,6 +543,21 @@ void BackgroundSyncManager::GetRegistrationImpl(
       FROM_HERE, base::Bind(callback, ERROR_TYPE_OK, out_registration));
 }
 
+void BackgroundSyncManager::OnRegistrationDeletedImpl(
+    int64 registration_id,
+    const base::Closure& callback) {
+  // The backend (ServiceWorkerStorage) will delete the data, so just delete the
+  // memory representation here.
+  sw_to_registrations_map_.erase(registration_id);
+  base::MessageLoop::current()->PostTask(FROM_HERE, base::Bind(callback));
+}
+
+void BackgroundSyncManager::OnStorageWipedImpl(const base::Closure& callback) {
+  sw_to_registrations_map_.clear();
+  disabled_ = false;

[michaeln, 2015/04/01 21:37:30]

would it make sense to schedule an op to reinit?

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

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.

+  InitImpl(callback);
+}
+
 void BackgroundSyncManager::PendingStatusAndRegistrationCallback(
     const StatusAndRegistrationCallback& callback,
     ErrorType error,
@@ -411,4 +579,12 @@ void BackgroundSyncManager::PendingStatusCallback(
     op_scheduler_.CompleteOperationAndRunNext();
 }
 
+void BackgroundSyncManager::PendingClosure(const base::Closure& callback) {
+  // The callback might delete this object, so hang onto a weak ptr to find out.
+  base::WeakPtr<BackgroundSyncManager> manager = weak_ptr_factory_.GetWeakPtr();
+  callback.Run();
+  if (manager)
+    op_scheduler_.CompleteOperationAndRunNext();
+}
+
 }  // namespace content