BackgroundSyncManager tracks client registrations

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 018bbae5ab270ecfc3905a191a9fbb48dd63cb93..a1f94faf1813e5fe3ec44a87cc9fdde6943935c9 100644
--- a/content/browser/background_sync/background_sync_manager.cc
+++ b/content/browser/background_sync/background_sync_manager.cc
@@ -13,6 +13,7 @@
 #include "content/browser/background_sync/background_sync_network_observer.h"
 #include "content/browser/background_sync/background_sync_power_observer.h"
 #include "content/browser/background_sync/background_sync_registration_options.h"
+#include "content/browser/background_sync/client_background_sync_registration.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/browser/service_worker/service_worker_storage.h"
 #include "content/public/browser/browser_thread.h"
@@ -50,6 +51,13 @@ scoped_ptr<BackgroundSyncManager> BackgroundSyncManager::Create(
 BackgroundSyncManager::~BackgroundSyncManager() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
+  // This could race in the extremely unlikely case that the
+  // BackgroundSyncManager is deleted before a dispatched sync request is able
+  // to call BackgroundSyncServiceImpl::TrackRegistration.
+  // TODO(jkarlin): Stop the RenderProcessHost mojo registry when
+  // StoragePartition is gone.
+  DCHECK(client_registration_ids_.IsEmpty());

[iclelland, 2015/08/14 14:44:18]

What happens if a renderer crashed (for unrelated reasons), while still holding
on to a reference to a ClientBackgroundSyncRegistration, at some time before the
manager is deleted? Will this code escalate that to a full browser crash?

[jkarlin, 2015/08/17 17:14:49]

The BackgroundSyncServiceImpl tracks all of the references that the renderer
has. If the renderer crashes its mojo pipe will die and the
BackgroundSyncServiceImpl will die, releasing the references.

[iclelland, 2015/08/17 19:22:02]

That seems sound, thanks.

+
   service_worker_context_->RemoveObserver(this);
 }
 
@@ -88,8 +96,10 @@ void BackgroundSyncManager::Register(
         BackgroundSyncMetrics::REGISTRATION_IS_NOT_DUPLICATE,
         BACKGROUND_SYNC_STATUS_STORAGE_ERROR);
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::Bind(callback, BACKGROUND_SYNC_STATUS_STORAGE_ERROR,
-                              BackgroundSyncRegistration()));
+        FROM_HERE,
+        base::Bind(callback, BACKGROUND_SYNC_STATUS_STORAGE_ERROR,
+                   base::Passed(
+                       scoped_ptr<ClientBackgroundSyncRegistration>().Pass())));

[iclelland, 2015/08/14 14:44:18]

This change is repeated so many times in this file -- would it makes sense to
have a function to encapsulate the entire PostTask call for storage errors?

[jkarlin, 2015/08/17 17:14:49]

Good idea. Done.

     return;
   }
 
@@ -101,7 +111,6 @@ void BackgroundSyncManager::Register(
 
 void BackgroundSyncManager::Unregister(

[iclelland, 2015/08/14 14:44:18]

This method was made private in the header file; does it need to be moved
elsewhere in the .cc file to match?

[jkarlin, 2015/08/17 17:18:15]

Done.

     int64 sw_registration_id,
-    const std::string& sync_registration_tag,
     SyncPeriodicity periodicity,
     BackgroundSyncRegistration::RegistrationId sync_registration_id,
     const StatusCallback& callback) {
@@ -115,11 +124,9 @@ void BackgroundSyncManager::Unregister(
     return;
   }
 
-  RegistrationKey registration_key(sync_registration_tag, periodicity);
-
   op_scheduler_.ScheduleOperation(base::Bind(
       &BackgroundSyncManager::UnregisterImpl, weak_ptr_factory_.GetWeakPtr(),
-      sw_registration_id, registration_key, sync_registration_id, periodicity,
+      sw_registration_id, periodicity, sync_registration_id,
       MakeStatusCompletion(callback)));
 }
 
@@ -132,8 +139,10 @@ void BackgroundSyncManager::GetRegistration(
 
   if (disabled_) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::Bind(callback, BACKGROUND_SYNC_STATUS_STORAGE_ERROR,
-                              BackgroundSyncRegistration()));
+        FROM_HERE,
+        base::Bind(callback, BACKGROUND_SYNC_STATUS_STORAGE_ERROR,
+                   base::Passed(
+                       scoped_ptr<ClientBackgroundSyncRegistration>().Pass())));
     return;
   }
 
@@ -153,8 +162,12 @@ void BackgroundSyncManager::GetRegistrations(
 
   if (disabled_) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::Bind(callback, BACKGROUND_SYNC_STATUS_STORAGE_ERROR,
-                              std::vector<BackgroundSyncRegistration>()));
+        FROM_HERE,
+        base::Bind(
+            callback, BACKGROUND_SYNC_STATUS_STORAGE_ERROR,
+            base::Passed(
+                scoped_ptr<ScopedVector<ClientBackgroundSyncRegistration>>()
+                    .Pass())));
     return;
   }
 
@@ -164,6 +177,17 @@ void BackgroundSyncManager::GetRegistrations(
                  periodicity, MakeStatusAndRegistrationsCompletion(callback)));
 }
 
+void BackgroundSyncManager::ReleaseRegistration(
+    BackgroundSyncRegistration::RegistrationId sync_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  if (!client_registration_ids_.Lookup(sync_id)) {
+    // TODO(jkarlin): Abort the render process.

[iclelland, 2015/08/14 14:44:18]

What happens right now (by not aborting?) Is anything left in an inconsistent
state, or will the renderer clean itself up once it believes that the
registration has been released?

[jkarlin, 2015/08/17 17:14:49]

I made some changes to BackgroundSyncServiceImpl::TrackRegistration so that this
can be a DCHECK instead. Done.

+    LOG(ERROR) << "Client attempted to release a non-existant registration";
+    return;
+  }
+  client_registration_ids_.Remove(sync_id);
+}
+
 void BackgroundSyncManager::OnRegistrationDeleted(int64 registration_id,
                                                   const GURL& pattern) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
@@ -261,8 +285,11 @@ void BackgroundSyncManager::InitDidGetDataFromBackend(
 
         RegistrationKey registration_key(registration_proto.tag(),
                                          registration_proto.periodicity());
-        BackgroundSyncRegistration* registration =
-            &registrations->registration_map[registration_key];
+
+        scoped_refptr<RefCountedRegistration> ref_registration(
+            new RefCountedRegistration());
+        registrations->registration_map[registration_key] = ref_registration;
+        BackgroundSyncRegistration* registration = ref_registration->value();
 
         BackgroundSyncRegistrationOptions* options = registration->options();
         options->tag = registration_proto.tag();
@@ -316,8 +343,10 @@ void BackgroundSyncManager::RegisterImpl(
         BackgroundSyncMetrics::REGISTRATION_IS_NOT_DUPLICATE,
         BACKGROUND_SYNC_STATUS_STORAGE_ERROR);
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::Bind(callback, BACKGROUND_SYNC_STATUS_STORAGE_ERROR,
-                              BackgroundSyncRegistration()));
+        FROM_HERE,
+        base::Bind(callback, BACKGROUND_SYNC_STATUS_STORAGE_ERROR,
+                   base::Passed(
+                       scoped_ptr<ClientBackgroundSyncRegistration>().Pass())));
     return;
   }
 
@@ -331,28 +360,33 @@ void BackgroundSyncManager::RegisterImpl(
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE,
         base::Bind(callback, BACKGROUND_SYNC_STATUS_NO_SERVICE_WORKER,
-                   BackgroundSyncRegistration()));
+                   base::Passed(
+                       scoped_ptr<ClientBackgroundSyncRegistration>().Pass())));
     return;
   }
 
   if (!sw_registration->active_version()->HasWindowClients()) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::Bind(callback, BACKGROUND_SYNC_STATUS_NOT_ALLOWED,
-                              BackgroundSyncRegistration()));
+        FROM_HERE,
+        base::Bind(callback, BACKGROUND_SYNC_STATUS_NOT_ALLOWED,
+                   base::Passed(
+                       scoped_ptr<ClientBackgroundSyncRegistration>().Pass())));
     return;
   }
 
-  BackgroundSyncRegistration* existing_registration =
+  RefCountedRegistration* existing_registration_ref =
       LookupRegistration(sw_registration_id, RegistrationKey(options));
-  if (existing_registration &&
-      existing_registration->options()->Equals(options)) {
+  if (existing_registration_ref &&
+      existing_registration_ref->value()->options()->Equals(options)) {
+    BackgroundSyncRegistration* existing_registration =
+        existing_registration_ref->value();
     if (existing_registration->sync_state() == SYNC_STATE_FAILED) {
       existing_registration->set_sync_state(SYNC_STATE_PENDING);
       StoreRegistrations(
           sw_registration_id,
           base::Bind(&BackgroundSyncManager::RegisterDidStore,
                      weak_ptr_factory_.GetWeakPtr(), sw_registration_id,
-                     *existing_registration, callback));
+                     make_scoped_refptr(existing_registration_ref), callback));
       return;
     }
 
@@ -363,27 +397,33 @@ void BackgroundSyncManager::RegisterImpl(
         BACKGROUND_SYNC_STATUS_OK);
 
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::Bind(callback, BACKGROUND_SYNC_STATUS_OK,
-                              *existing_registration));
+        FROM_HERE,
+        base::Bind(
+            callback, BACKGROUND_SYNC_STATUS_OK,
+            base::Passed(
+                CreateClientRegistration(existing_registration_ref).Pass())));
     return;
   }
 
-  BackgroundSyncRegistration new_registration;
-  *new_registration.options() = options;
+  scoped_refptr<RefCountedRegistration> new_ref_registration(
+      new RefCountedRegistration());
+  BackgroundSyncRegistration* new_registration = new_ref_registration->value();
+
+  *new_registration->options() = options;
 
   BackgroundSyncRegistrations* registrations =
       &sw_to_registrations_map_[sw_registration_id];
-  new_registration.set_id(registrations->next_id++);
+  new_registration->set_id(registrations->next_id++);
 
   AddRegistrationToMap(sw_registration_id,
                        sw_registration->pattern().GetOrigin(),
-                       new_registration);
+                       new_ref_registration);
 
   StoreRegistrations(
       sw_registration_id,
       base::Bind(&BackgroundSyncManager::RegisterDidStore,
                  weak_ptr_factory_.GetWeakPtr(), sw_registration_id,
-                 new_registration, callback));
+                 new_ref_registration, callback));
 }
 
 void BackgroundSyncManager::DisableAndClearManager(
@@ -441,7 +481,8 @@ void BackgroundSyncManager::DisableAndClearManagerClearedOne(
                                                 base::Bind(barrier_closure));
 }
 
-BackgroundSyncRegistration* BackgroundSyncManager::LookupRegistration(
+BackgroundSyncManager::RefCountedRegistration*
+BackgroundSyncManager::LookupRegistration(
     int64 sw_registration_id,
     const RegistrationKey& registration_key) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
@@ -460,7 +501,7 @@ BackgroundSyncRegistration* BackgroundSyncManager::LookupRegistration(
   if (key_and_registration_iter == registrations.registration_map.end())
     return nullptr;
 
-  return &key_and_registration_iter->second;
+  return key_and_registration_iter->second.get();
 }
 
 void BackgroundSyncManager::StoreRegistrations(
@@ -477,7 +518,7 @@ void BackgroundSyncManager::StoreRegistrations(
 
   for (const auto& key_and_registration : registrations.registration_map) {
     const BackgroundSyncRegistration& registration =
-        key_and_registration.second;
+        *key_and_registration.second->value();
     BackgroundSyncRegistrationProto* registration_proto =
         registrations_proto.add_registration();
     registration_proto->set_id(registration.id());
@@ -499,27 +540,32 @@ void BackgroundSyncManager::StoreRegistrations(
 
 void BackgroundSyncManager::RegisterDidStore(
     int64 sw_registration_id,
-    const BackgroundSyncRegistration& new_registration,
+    const scoped_refptr<RefCountedRegistration>& new_registration_ref,
     const StatusAndRegistrationCallback& callback,
     ServiceWorkerStatusCode status) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
+  const BackgroundSyncRegistration* new_registration =
+      new_registration_ref->value();
+
   // For UMA, determine here whether the sync could fire immediately
   BackgroundSyncMetrics::RegistrationCouldFire registration_could_fire =
-      AreOptionConditionsMet(*new_registration.options())
+      AreOptionConditionsMet(*new_registration->options())
           ? BackgroundSyncMetrics::REGISTRATION_COULD_FIRE
           : BackgroundSyncMetrics::REGISTRATION_COULD_NOT_FIRE;
 
   if (status == SERVICE_WORKER_ERROR_NOT_FOUND) {
     // The service worker registration is gone.
     BackgroundSyncMetrics::CountRegister(
-        new_registration.options()->periodicity, registration_could_fire,
+        new_registration->options()->periodicity, registration_could_fire,
         BackgroundSyncMetrics::REGISTRATION_IS_NOT_DUPLICATE,
         BACKGROUND_SYNC_STATUS_STORAGE_ERROR);
     sw_to_registrations_map_.erase(sw_registration_id);
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::Bind(callback, BACKGROUND_SYNC_STATUS_STORAGE_ERROR,
-                              BackgroundSyncRegistration()));
+        FROM_HERE,
+        base::Bind(callback, BACKGROUND_SYNC_STATUS_STORAGE_ERROR,
+                   base::Passed(
+                       scoped_ptr<ClientBackgroundSyncRegistration>().Pass())));
     return;
   }
 
@@ -527,23 +573,26 @@ void BackgroundSyncManager::RegisterDidStore(
     LOG(ERROR) << "BackgroundSync failed to store registration due to backend "
                   "failure.";
     BackgroundSyncMetrics::CountRegister(
-        new_registration.options()->periodicity, registration_could_fire,
+        new_registration->options()->periodicity, registration_could_fire,
         BackgroundSyncMetrics::REGISTRATION_IS_NOT_DUPLICATE,
         BACKGROUND_SYNC_STATUS_STORAGE_ERROR);
-    DisableAndClearManager(base::Bind(callback,
-                                      BACKGROUND_SYNC_STATUS_STORAGE_ERROR,
-                                      BackgroundSyncRegistration()));
+    DisableAndClearManager(base::Bind(
+        callback, BACKGROUND_SYNC_STATUS_STORAGE_ERROR,
+        base::Passed(scoped_ptr<ClientBackgroundSyncRegistration>().Pass())));
     return;
   }
 
   BackgroundSyncMetrics::CountRegister(
-      new_registration.options()->periodicity, registration_could_fire,
+      new_registration->options()->periodicity, registration_could_fire,
       BackgroundSyncMetrics::REGISTRATION_IS_NOT_DUPLICATE,
       BACKGROUND_SYNC_STATUS_OK);
   FireReadyEvents();
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
-      base::Bind(callback, BACKGROUND_SYNC_STATUS_OK, new_registration));
+      base::Bind(
+          callback, BACKGROUND_SYNC_STATUS_OK,
+          base::Passed(
+              CreateClientRegistration(new_registration_ref.get()).Pass())));
 }
 
 void BackgroundSyncManager::RemoveRegistrationFromMap(
@@ -561,15 +610,15 @@ void BackgroundSyncManager::RemoveRegistrationFromMap(
 void BackgroundSyncManager::AddRegistrationToMap(
     int64 sw_registration_id,
     const GURL& origin,
-    const BackgroundSyncRegistration& sync_registration) {
+    const scoped_refptr<RefCountedRegistration>& sync_registration) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  DCHECK(sync_registration.IsValid());
+  DCHECK(sync_registration->value()->IsValid());
 
   BackgroundSyncRegistrations* registrations =
       &sw_to_registrations_map_[sw_registration_id];
   registrations->origin = origin;
 
-  RegistrationKey registration_key(sync_registration);
+  RegistrationKey registration_key(*sync_registration->value());
   registrations->registration_map[registration_key] = sync_registration;
 }
 
@@ -596,20 +645,31 @@ void BackgroundSyncManager::GetDataFromBackend(
 }
 
 void BackgroundSyncManager::FireOneShotSync(
-    const BackgroundSyncRegistration& registration,
+    RefCountedRegistration* registration,
     const scoped_refptr<ServiceWorkerVersion>& active_version,
     const ServiceWorkerVersion::StatusCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  active_version->DispatchSyncEvent(
-      mojo::ConvertTo<SyncRegistrationPtr>(registration), callback);
+  // The ServiceWorkerVersion doesn't know when the client (javascript) is done
+  // with the registration so don't give it a ClientBackgroundSyncRegistration.
+  // The render process must call BackgroundSyncServiceImpl::TrackRegistration
+  // which creates the corresponding ClientBackgroundSyncRegistration. In the
+  // unlikely event that the render process is killed before TrackRegistration
+  // is called, the manager might will hold the reference until it is destroyed.

[iclelland, 2015/08/14 14:44:18]

"might will"

[jkarlin, 2015/08/17 17:14:49]

Done.

+  // The ~BackgroundSyncManager DCHECKS verify that this is uncommon.
+  SyncRegistrationPtr sync_registration =
+      mojo::ConvertTo<SyncRegistrationPtr>(*registration->value());
+  scoped_refptr<RefCountedRegistration>* ptr =
+      new scoped_refptr<RefCountedRegistration>(registration);
+  sync_registration->id = client_registration_ids_.Add(ptr);
+
+  active_version->DispatchSyncEvent(sync_registration.Pass(), callback);
 }
 
 void BackgroundSyncManager::UnregisterImpl(
     int64 sw_registration_id,
-    const RegistrationKey& registration_key,
-    BackgroundSyncRegistration::RegistrationId sync_registration_id,
     SyncPeriodicity periodicity,
+    BackgroundSyncRegistration::RegistrationId sync_registration_id,
     const StatusCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
@@ -621,10 +681,28 @@ void BackgroundSyncManager::UnregisterImpl(
     return;
   }
 
-  const BackgroundSyncRegistration* existing_registration =
+  scoped_refptr<RefCountedRegistration>* hosted_registration_ref =
+      client_registration_ids_.Lookup(sync_registration_id);
+
+  if (!hosted_registration_ref) {
+    LOG(ERROR) << "Client attempted to access a nonexistent registration";
+    BackgroundSyncMetrics::CountUnregister(periodicity,
+                                           BACKGROUND_SYNC_STATUS_NOT_ALLOWED);
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(callback, BACKGROUND_SYNC_STATUS_NOT_ALLOWED));
+    return;
+  }
+
+  BackgroundSyncRegistration* hosted_registration =
+      (*hosted_registration_ref)->value();
+
+  RegistrationKey registration_key(hosted_registration->options()->tag,
+                                   periodicity);
+  RefCountedRegistration* existing_registration =

[iclelland, 2015/08/14 14:44:18]

const removed?

[jkarlin, 2015/08/17 17:14:49]

Done.

       LookupRegistration(sw_registration_id, registration_key);
+
   if (!existing_registration ||
-      existing_registration->id() != sync_registration_id) {
+      existing_registration->value()->id() != hosted_registration->id()) {
     BackgroundSyncMetrics::CountUnregister(periodicity,
                                            BACKGROUND_SYNC_STATUS_NOT_FOUND);
     base::ThreadTaskRunnerHandle::Get()->PostTask(
@@ -679,23 +757,28 @@ void BackgroundSyncManager::GetRegistrationImpl(
 
   if (disabled_) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::Bind(callback, BACKGROUND_SYNC_STATUS_STORAGE_ERROR,
-                              BackgroundSyncRegistration()));
+        FROM_HERE,
+        base::Bind(callback, BACKGROUND_SYNC_STATUS_STORAGE_ERROR,
+                   base::Passed(
+                       scoped_ptr<ClientBackgroundSyncRegistration>().Pass())));
     return;
   }
 
-  const BackgroundSyncRegistration* out_registration =
+  RefCountedRegistration* registration =

[iclelland, 2015/08/14 14:44:18]

const removed?

Actually there are a lot of these here -- is there an issue with RefCounted
objects not being const'able? You've defined an explicit const value() method,
so I didn't think so, but it's been pretty consistently removed in this file.

[jkarlin, 2015/08/17 17:14:49]

Thanks, I've added const where possible.

       LookupRegistration(sw_registration_id, registration_key);
-  if (!out_registration) {
+  if (!registration) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::Bind(callback, BACKGROUND_SYNC_STATUS_NOT_FOUND,
-                              BackgroundSyncRegistration()));
+        FROM_HERE,
+        base::Bind(callback, BACKGROUND_SYNC_STATUS_NOT_FOUND,
+                   base::Passed(
+                       scoped_ptr<ClientBackgroundSyncRegistration>().Pass())));
     return;
   }
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
-      base::Bind(callback, BACKGROUND_SYNC_STATUS_OK, *out_registration));
+      base::Bind(callback, BACKGROUND_SYNC_STATUS_OK,
+                 base::Passed(CreateClientRegistration(registration).Pass())));
 }
 
 void BackgroundSyncManager::GetRegistrationsImpl(
@@ -704,12 +787,13 @@ void BackgroundSyncManager::GetRegistrationsImpl(
     const StatusAndRegistrationsCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  std::vector<BackgroundSyncRegistration> out_registrations;
+  scoped_ptr<ScopedVector<ClientBackgroundSyncRegistration>> out_registrations(
+      new ScopedVector<ClientBackgroundSyncRegistration>());
 
   if (disabled_) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(callback, BACKGROUND_SYNC_STATUS_STORAGE_ERROR,
-                              out_registrations));
+                              base::Passed(out_registrations.Pass())));
     return;
   }
 
@@ -719,16 +803,17 @@ void BackgroundSyncManager::GetRegistrationsImpl(
   if (it != sw_to_registrations_map_.end()) {
     const BackgroundSyncRegistrations& registrations = it->second;
     for (const auto& tag_and_registration : registrations.registration_map) {
-      const BackgroundSyncRegistration& registration =
-          tag_and_registration.second;
-      if (registration.options()->periodicity == periodicity)
-        out_registrations.push_back(registration);
+      RefCountedRegistration* registration = tag_and_registration.second.get();
+      if (registration->value()->options()->periodicity == periodicity) {
+        out_registrations->push_back(
+            CreateClientRegistration(registration).release());
+      }
     }
   }
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
-      base::Bind(callback, BACKGROUND_SYNC_STATUS_OK, out_registrations));
+      FROM_HERE, base::Bind(callback, BACKGROUND_SYNC_STATUS_OK,
+                            base::Passed(out_registrations.Pass())));
 }
 
 bool BackgroundSyncManager::AreOptionConditionsMet(
@@ -762,7 +847,7 @@ void BackgroundSyncManager::SchedulePendingRegistrations() {
     for (const auto& key_and_registration :
          sw_id_and_registrations.second.registration_map) {
       const BackgroundSyncRegistration& registration =
-          key_and_registration.second;
+          *key_and_registration.second->value();
       if (registration.sync_state() == SYNC_STATE_PENDING) {
         if (registration.options()->periodicity == SYNC_ONE_SHOT) {
           keep_browser_alive_for_one_shot = true;
@@ -812,7 +897,8 @@ void BackgroundSyncManager::FireReadyEventsImpl(const base::Closure& callback) {
     const int64 service_worker_id = sw_id_and_registrations.first;
     for (auto& key_and_registration :
          sw_id_and_registrations.second.registration_map) {
-      BackgroundSyncRegistration* registration = &key_and_registration.second;
+      BackgroundSyncRegistration* registration =
+          key_and_registration.second->value();
       if (IsRegistrationReadyToFire(*registration)) {
         sw_id_and_keys_to_fire.push_back(
             std::make_pair(service_worker_id, key_and_registration.first));
@@ -845,14 +931,14 @@ void BackgroundSyncManager::FireReadyEventsImpl(const base::Closure& callback) {
 
     for (const auto& sw_id_and_key : sw_id_and_keys_to_fire) {
       int64 service_worker_id = sw_id_and_key.first;
-      const BackgroundSyncRegistration* registration =
+      RefCountedRegistration* registration =
           LookupRegistration(service_worker_id, sw_id_and_key.second);
 
       service_worker_context_->FindRegistrationForId(
           service_worker_id, sw_to_registrations_map_[service_worker_id].origin,
           base::Bind(&BackgroundSyncManager::FireReadyEventsDidFindRegistration,
                      weak_ptr_factory_.GetWeakPtr(), sw_id_and_key.second,
-                     registration->id(), events_fired_barrier_closure,
+                     registration->value()->id(), events_fired_barrier_closure,
                      events_completed_barrier_closure));
     }
   }
@@ -877,11 +963,11 @@ void BackgroundSyncManager::FireReadyEventsDidFindRegistration(
     return;
   }
 
-  BackgroundSyncRegistration* registration =
+  RefCountedRegistration* registration =
       LookupRegistration(service_worker_registration->id(), registration_key);
 
   FireOneShotSync(
-      *registration, service_worker_registration->active_version(),
+      registration, service_worker_registration->active_version(),
       base::Bind(&BackgroundSyncManager::EventComplete,
                  weak_ptr_factory_.GetWeakPtr(), service_worker_registration,
                  service_worker_registration->id(), registration_key,
@@ -924,14 +1010,17 @@ void BackgroundSyncManager::EventCompleteImpl(
     return;
   }
 
-  BackgroundSyncRegistration* registration =
+  RefCountedRegistration* registration_ref =
       LookupRegistration(service_worker_id, key);
-  if (!registration || registration->id() != sync_registration_id) {
+  if (!registration_ref ||
+      registration_ref->value()->id() != sync_registration_id) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
                                                   base::Bind(callback));
     return;
   }
 
+  BackgroundSyncRegistration* registration = registration_ref->value();
+
   // The event ran to completion, we should count it, no matter what happens
   // from here.
   BackgroundSyncMetrics::RecordEventResult(registration->options()->periodicity,
@@ -1011,6 +1100,20 @@ void BackgroundSyncManager::OnStorageWipedImpl(const base::Closure& callback) {
   InitImpl(callback);
 }
 
+scoped_ptr<ClientBackgroundSyncRegistration>
+BackgroundSyncManager::CreateClientRegistration(
+    RefCountedRegistration* registration) {
+  scoped_ptr<ClientBackgroundSyncRegistration> out_registration(
+      new ClientBackgroundSyncRegistration(this));
+  *out_registration->options() = *registration->value()->options();
+  out_registration->set_sync_state(registration->value()->sync_state());
+
+  scoped_refptr<RefCountedRegistration>* ptr =
+      new scoped_refptr<RefCountedRegistration>(registration);
+  out_registration->set_id(client_registration_ids_.Add(ptr));
+  return out_registration.Pass();
+}
+
 void BackgroundSyncManager::OnNetworkChanged() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
@@ -1032,6 +1135,26 @@ void BackgroundSyncManager::CompleteOperationCallback(const CallbackT& callback,
   op_scheduler_.CompleteOperationAndRunNext();
 }
 
+void BackgroundSyncManager::CompleteStatusAndRegistrationCallback(

[iclelland, 2015/08/14 14:44:18]

Why are these two functions specified? I don't see why they're not just
instantiations of the CompleteOperationCallback anymore

[jkarlin, 2015/08/17 17:14:49]

Because they're passing scoped_ptrs that need to be passed. There is some magic
template way to do it (that's how bind works with base::Passed etc.) but I don't
know what it is. Added TODO though.

[iclelland, 2015/08/17 19:22:02]

That makes sense -- thanks for the explanation. I'm sure you're right, that
there is some template magic that would fix it, and that it's probably fine to
not spend any time on it right now.

+    StatusAndRegistrationCallback callback,
+    BackgroundSyncStatus status,
+    scoped_ptr<ClientBackgroundSyncRegistration> result) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  callback.Run(status, result.Pass());
+  op_scheduler_.CompleteOperationAndRunNext();
+}
+
+void BackgroundSyncManager::CompleteStatusAndRegistrationsCallback(
+    StatusAndRegistrationsCallback callback,
+    BackgroundSyncStatus status,
+    scoped_ptr<ScopedVector<ClientBackgroundSyncRegistration>> results) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  callback.Run(status, results.Pass());
+  op_scheduler_.CompleteOperationAndRunNext();
+}
+
 base::Closure BackgroundSyncManager::MakeEmptyCompletion() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
@@ -1052,10 +1175,9 @@ BackgroundSyncManager::MakeStatusAndRegistrationCompletion(
     const StatusAndRegistrationCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  return base::Bind(&BackgroundSyncManager::CompleteOperationCallback<
-                        StatusAndRegistrationCallback, BackgroundSyncStatus,
-                        const BackgroundSyncRegistration&>,
-                    weak_ptr_factory_.GetWeakPtr(), callback);
+  return base::Bind(
+      &BackgroundSyncManager::CompleteStatusAndRegistrationCallback,
+      weak_ptr_factory_.GetWeakPtr(), callback);
 }
 
 BackgroundSyncManager::StatusAndRegistrationsCallback
@@ -1063,10 +1185,9 @@ BackgroundSyncManager::MakeStatusAndRegistrationsCompletion(
     const StatusAndRegistrationsCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  return base::Bind(&BackgroundSyncManager::CompleteOperationCallback<
-                        StatusAndRegistrationsCallback, BackgroundSyncStatus,
-                        const std::vector<BackgroundSyncRegistration>&>,
-                    weak_ptr_factory_.GetWeakPtr(), callback);
+  return base::Bind(
+      &BackgroundSyncManager::CompleteStatusAndRegistrationsCallback,
+      weak_ptr_factory_.GetWeakPtr(), callback);
 }
 
 BackgroundSyncManager::StatusCallback