[BackgroundSync] Handle storage failure

content/browser/background_sync/background_sync_manager.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/background_sync/background_sync_manager.h"
 
 #include "base/barrier_closure.h"
 #include "base/bind.h"
 #include "content/browser/background_sync/background_sync.pb.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
@@ skipping 28 matching lines @@
 // static
 scoped_ptr<BackgroundSyncManager> BackgroundSyncManager::Create(
     const scoped_refptr<ServiceWorkerContextWrapper>& service_worker_context) {
   BackgroundSyncManager* sync_manager =
       new BackgroundSyncManager(service_worker_context);
   sync_manager->Init();
   return make_scoped_ptr(sync_manager);
 }
 
 BackgroundSyncManager::~BackgroundSyncManager() {
+  service_worker_context_->RemoveObserver(this);
 }
 
 void BackgroundSyncManager::Register(
     const GURL& origin,
     int64 sw_registration_id,
     const BackgroundSyncRegistration& sync_registration,
     const StatusAndRegistrationCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   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);
 
   op_scheduler_.ScheduleOperation(base::Bind(
       &BackgroundSyncManager::RegisterImpl, weak_ptr_factory_.GetWeakPtr(),
       origin, sw_registration_id, sync_registration, pending_callback));
 }
 
 void BackgroundSyncManager::Unregister(
     const GURL& origin,
     int64 sw_registration_id,
     const std::string& sync_registration_name,
     BackgroundSyncRegistration::RegistrationId sync_registration_id,
     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);
 
   op_scheduler_.ScheduleOperation(base::Bind(
       &BackgroundSyncManager::UnregisterImpl, weak_ptr_factory_.GetWeakPtr(),
       origin, sw_registration_id, sync_registration_name, sync_registration_id,
       pending_callback));
 }
 
 void BackgroundSyncManager::GetRegistration(
     const GURL& origin,
     int64 sw_registration_id,
     const std::string sync_registration_name,
     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);
 
   op_scheduler_.ScheduleOperation(
       base::Bind(&BackgroundSyncManager::GetRegistrationImpl,
                  weak_ptr_factory_.GetWeakPtr(), origin, sw_registration_id,
                  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) {
     BackgroundSyncRegistrationsProto registrations_proto;
     if (registrations_proto.ParseFromString(data.second)) {
       sw_to_registrations_map_[data.first] = BackgroundSyncRegistrations(
           registrations_proto.next_registration_id());
       BackgroundSyncRegistrations* registrations =
           &sw_to_registrations_map_[data.first];
 
@@ skipping 15 matching lines @@
             registration;
       }
     }
 
     if (corruption_detected)
       break;
   }
 
   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(
     const GURL& origin,
     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)) {
     if (existing_registration.Equals(sync_registration)) {
       base::MessageLoop::current()->PostTask(
           FROM_HERE,
           base::Bind(callback, ERROR_TYPE_OK, existing_registration));
       return;
     }
   }
 
   BackgroundSyncRegistration new_registration = sync_registration;
   BackgroundSyncRegistrations* registrations =
       &sw_to_registrations_map_[sw_registration_id];
   new_registration.id = registrations->next_id++;
 
   AddRegistrationToMap(sw_registration_id, new_registration);
 
   StoreRegistrations(
       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(
     int64 sw_registration_id,
     const std::string& sync_registration_name,
     BackgroundSyncRegistration* existing_registration) {
   SWIdToRegistrationsMap::iterator it =
       sw_to_registrations_map_.find(sw_registration_id);
   if (it == sw_to_registrations_map_.end())
     return false;
@@ skipping 36 matching lines @@
   bool success = registrations_proto.SerializeToString(&serialized);
   DCHECK(success);
 
   StoreDataInBackend(sw_registration_id, origin, kBackgroundSyncUserDataKey,
                      serialized, callback);
 }
 
 void BackgroundSyncManager::RegisterDidStore(
     int64 sw_registration_id,
     const BackgroundSyncRegistration& new_registration,
-    const BackgroundSyncRegistration& previous_registration,
     const StatusAndRegistrationCallback& callback,
     ServiceWorkerStatusCode status) {
-  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);
-    }
+  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) {
+    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()));
     return;
   }
 
   // TODO(jkarlin): Run the registration algorithm.
   base::MessageLoop::current()->PostTask(
       FROM_HERE, base::Bind(callback, ERROR_TYPE_OK, new_registration));
 }
 
 void BackgroundSyncManager::RemoveRegistrationFromMap(
     int64 sw_registration_id,
@@ skipping 44 matching lines @@
   service_worker_context_->context()->storage()->GetUserDataForAllRegistrations(
       key, callback);
 }
 
 void BackgroundSyncManager::UnregisterImpl(
     const GURL& origin,
     int64 sw_registration_id,
     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) ||
       existing_registration.id != sync_registration_id) {
     base::MessageLoop::current()->PostTask(
         FROM_HERE, base::Bind(callback, ERROR_TYPE_NOT_FOUND));
     return;
   }
 
   BackgroundSyncRegistration old_sync_registration;
   RemoveRegistrationFromMap(sw_registration_id, sync_registration_name,
                             &old_sync_registration);
 
   StoreRegistrations(
       origin, sw_registration_id,
       base::Bind(&BackgroundSyncManager::UnregisterDidStore,
                  weak_ptr_factory_.GetWeakPtr(), sw_registration_id,
                  old_sync_registration, callback));
 }
 
 void BackgroundSyncManager::UnregisterDidStore(
     int64 sw_registration_id,
     const BackgroundSyncRegistration& old_sync_registration,
     const StatusCallback& callback,
     ServiceWorkerStatusCode status) {
-  if (status != SERVICE_WORKER_OK) {
-    // Restore the previous state.
-    AddRegistrationToMap(sw_registration_id, old_sync_registration);
+  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) {
+    LOG(ERROR) << "BackgroundSync failed to unregister due to backend failure.";
+    DisableAndClearManager();
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE, base::Bind(callback, ERROR_TYPE_STORAGE));
+    return;
   }
 
   // TODO(jkarlin): Run the registration algorithm.
   base::MessageLoop::current()->PostTask(FROM_HERE,
                                          base::Bind(callback, ERROR_TYPE_OK));
 }
 
 void BackgroundSyncManager::GetRegistrationImpl(
     const GURL& origin,
     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)) {
     base::MessageLoop::current()->PostTask(
         FROM_HERE, base::Bind(callback, ERROR_TYPE_NOT_FOUND,
                               BackgroundSyncRegistration()));
     return;
   }
 
   base::MessageLoop::current()->PostTask(
       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,
     const BackgroundSyncRegistration& sync_registration) {
   // 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(error, sync_registration);
   if (manager)
     op_scheduler_.CompleteOperationAndRunNext();
 }
 
 void BackgroundSyncManager::PendingStatusCallback(
     const StatusCallback& callback,
     ErrorType error) {
   // 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(error);
   if (manager)
     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