BackgroundSyncManager tracks client registrations

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 "base/location.h"
 #include "base/single_thread_task_runner.h"
 #include "base/thread_task_runner_handle.h"
 #include "content/browser/background_sync/background_sync_metrics.h"
 #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"
 
 #if defined(OS_ANDROID)
 #include "content/browser/android/background_sync_launcher_android.h"
 #endif
 
+namespace content {
+
 namespace {
+
 const char kBackgroundSyncUserDataKey[] = "BackgroundSyncUserData";
+
+void PostErrorResponse(
+    BackgroundSyncStatus status,
+    const BackgroundSyncManager::StatusAndRegistrationCallback& callback) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::Bind(
+          callback, status,
+          base::Passed(scoped_ptr<ClientBackgroundSyncRegistration>().Pass())));
 }
 
-namespace content {
+}  // namespace
 
 BackgroundSyncManager::BackgroundSyncRegistrations::
     BackgroundSyncRegistrations()
     : next_id(BackgroundSyncRegistration::kInitialId) {
 }
 
 BackgroundSyncManager::BackgroundSyncRegistrations::
     ~BackgroundSyncRegistrations() {
 }
 
 // static
 scoped_ptr<BackgroundSyncManager> BackgroundSyncManager::Create(
     const scoped_refptr<ServiceWorkerContextWrapper>& service_worker_context) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   BackgroundSyncManager* sync_manager =
       new BackgroundSyncManager(service_worker_context);
   sync_manager->Init();
   return make_scoped_ptr(sync_manager);
 }
 
 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());
+
   service_worker_context_->RemoveObserver(this);
 }
 
 BackgroundSyncManager::RegistrationKey::RegistrationKey(
     const BackgroundSyncRegistration& registration)
     : RegistrationKey(registration.options()->tag,
                       registration.options()->periodicity) {
 }
 
 BackgroundSyncManager::RegistrationKey::RegistrationKey(
@@ skipping 17 matching lines @@
   BackgroundSyncMetrics::RegistrationCouldFire registration_could_fire =
       AreOptionConditionsMet(options)
           ? BackgroundSyncMetrics::REGISTRATION_COULD_FIRE
           : BackgroundSyncMetrics::REGISTRATION_COULD_NOT_FIRE;
 
   if (disabled_) {
     BackgroundSyncMetrics::CountRegister(
         options.periodicity, registration_could_fire,
         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()));
+    PostErrorResponse(BACKGROUND_SYNC_STATUS_STORAGE_ERROR, callback);
     return;
   }
 
   op_scheduler_.ScheduleOperation(
       base::Bind(&BackgroundSyncManager::RegisterImpl,
                  weak_ptr_factory_.GetWeakPtr(), sw_registration_id, options,
                  MakeStatusAndRegistrationCompletion(callback)));
 }
 
-void BackgroundSyncManager::Unregister(
-    int64 sw_registration_id,
-    const std::string& sync_registration_tag,
-    SyncPeriodicity periodicity,
-    BackgroundSyncRegistration::RegistrationId sync_registration_id,
-    const StatusCallback& callback) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-
-  if (disabled_) {
-    BackgroundSyncMetrics::CountUnregister(
-        periodicity, BACKGROUND_SYNC_STATUS_STORAGE_ERROR);
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::Bind(callback, BACKGROUND_SYNC_STATUS_STORAGE_ERROR));
-    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,
-      MakeStatusCompletion(callback)));
-}
-
 void BackgroundSyncManager::GetRegistration(
     int64 sw_registration_id,
     const std::string& sync_registration_tag,
     SyncPeriodicity periodicity,
     const StatusAndRegistrationCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (disabled_) {
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::Bind(callback, BACKGROUND_SYNC_STATUS_STORAGE_ERROR,
-                              BackgroundSyncRegistration()));
+    PostErrorResponse(BACKGROUND_SYNC_STATUS_STORAGE_ERROR, callback);
     return;
   }
 
   RegistrationKey registration_key(sync_registration_tag, periodicity);
 
   op_scheduler_.ScheduleOperation(base::Bind(
       &BackgroundSyncManager::GetRegistrationImpl,
       weak_ptr_factory_.GetWeakPtr(), sw_registration_id, registration_key,
       MakeStatusAndRegistrationCompletion(callback)));
 }
 
 void BackgroundSyncManager::GetRegistrations(
     int64 sw_registration_id,
     SyncPeriodicity periodicity,
     const StatusAndRegistrationsCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   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;
   }
 
   op_scheduler_.ScheduleOperation(
       base::Bind(&BackgroundSyncManager::GetRegistrationsImpl,
                  weak_ptr_factory_.GetWeakPtr(), sw_registration_id,
                  periodicity, MakeStatusAndRegistrationsCompletion(callback)));
 }
 
+bool BackgroundSyncManager::HasClientRegistration(
+    BackgroundSyncRegistration::RegistrationId sync_id) const {
+  return client_registration_ids_.Lookup(sync_id) != nullptr;
+}
+
+void BackgroundSyncManager::ReleaseClientRegistration(
+    BackgroundSyncRegistration::RegistrationId sync_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(client_registration_ids_.Lookup(sync_id));
+  client_registration_ids_.Remove(sync_id);
+}
+
 void BackgroundSyncManager::OnRegistrationDeleted(int64 registration_id,
                                                   const GURL& pattern) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   // 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, MakeEmptyCompletion()));
@@ skipping 77 matching lines @@
         const BackgroundSyncRegistrationProto& registration_proto =
             registrations_proto.registration(i);
 
         if (registration_proto.id() >= registrations->next_id) {
           corruption_detected = true;
           break;
         }
 
         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();
         options->periodicity = registration_proto.periodicity();
         options->min_period = registration_proto.min_period();
         options->network_state = registration_proto.network_state();
         options->power_state = registration_proto.power_state();
 
         registration->set_id(registration_proto.id());
         registration->set_sync_state(registration_proto.sync_state());
@@ skipping 32 matching lines @@
   BackgroundSyncMetrics::RegistrationCouldFire registration_could_fire =
       AreOptionConditionsMet(options)
           ? BackgroundSyncMetrics::REGISTRATION_COULD_FIRE
           : BackgroundSyncMetrics::REGISTRATION_COULD_NOT_FIRE;
 
   if (disabled_) {
     BackgroundSyncMetrics::CountRegister(
         options.periodicity, registration_could_fire,
         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()));
+    PostErrorResponse(BACKGROUND_SYNC_STATUS_STORAGE_ERROR, callback);
     return;
   }
 
   ServiceWorkerRegistration* sw_registration =
       service_worker_context_->GetLiveRegistration(sw_registration_id);
   if (!sw_registration || !sw_registration->active_version()) {
     BackgroundSyncMetrics::CountRegister(
         options.periodicity, registration_could_fire,
         BackgroundSyncMetrics::REGISTRATION_IS_NOT_DUPLICATE,
         BACKGROUND_SYNC_STATUS_NO_SERVICE_WORKER);
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE,
-        base::Bind(callback, BACKGROUND_SYNC_STATUS_NO_SERVICE_WORKER,
-                   BackgroundSyncRegistration()));
+    PostErrorResponse(BACKGROUND_SYNC_STATUS_NO_SERVICE_WORKER, callback);
     return;
   }
 
   if (!sw_registration->active_version()->HasWindowClients()) {
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::Bind(callback, BACKGROUND_SYNC_STATUS_NOT_ALLOWED,
-                              BackgroundSyncRegistration()));
+    PostErrorResponse(BACKGROUND_SYNC_STATUS_NOT_ALLOWED, callback);
     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;
     }
 
     // Record the duplicated registration
     BackgroundSyncMetrics::CountRegister(
         existing_registration->options()->periodicity, registration_could_fire,
         BackgroundSyncMetrics::REGISTRATION_IS_DUPLICATE,
         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(
     const base::Closure& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (disabled_) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
                                                   base::Bind(callback));
     return;
@@ skipping 37 matching lines @@
 void BackgroundSyncManager::DisableAndClearManagerClearedOne(
     const base::Closure& barrier_closure,
     ServiceWorkerStatusCode status) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   // The status doesn't matter at this point, there is nothing else to be done.
   base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
                                                 base::Bind(barrier_closure));
 }
 
-BackgroundSyncRegistration* BackgroundSyncManager::LookupRegistration(
+BackgroundSyncManager::RefCountedRegistration*
+BackgroundSyncManager::LookupRegistration(
     int64 sw_registration_id,
     const RegistrationKey& registration_key) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   SWIdToRegistrationsMap::iterator it =
       sw_to_registrations_map_.find(sw_registration_id);
   if (it == sw_to_registrations_map_.end())
     return nullptr;
 
   BackgroundSyncRegistrations& registrations = it->second;
   DCHECK_LE(BackgroundSyncRegistration::kInitialId, registrations.next_id);
   DCHECK(!registrations.origin.is_empty());
 
   auto key_and_registration_iter =
       registrations.registration_map.find(registration_key);
   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(
     int64 sw_registration_id,
     const ServiceWorkerStorage::StatusCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   // Serialize the data.
   const BackgroundSyncRegistrations& registrations =
       sw_to_registrations_map_[sw_registration_id];
   BackgroundSyncRegistrationsProto registrations_proto;
   registrations_proto.set_next_registration_id(registrations.next_id);
   registrations_proto.set_origin(registrations.origin.spec());
 
   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());
     registration_proto->set_sync_state(registration.sync_state());
     registration_proto->set_tag(registration.options()->tag);
     registration_proto->set_periodicity(registration.options()->periodicity);
     registration_proto->set_min_period(registration.options()->min_period);
     registration_proto->set_network_state(
         registration.options()->network_state);
     registration_proto->set_power_state(registration.options()->power_state);
   }
   std::string serialized;
   bool success = registrations_proto.SerializeToString(&serialized);
   DCHECK(success);
 
   StoreDataInBackend(sw_registration_id, registrations.origin,
                      kBackgroundSyncUserDataKey, serialized, callback);
 }
 
 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()));
+    PostErrorResponse(BACKGROUND_SYNC_STATUS_STORAGE_ERROR, callback);
     return;
   }
 
   if (status != SERVICE_WORKER_OK) {
     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).Pass())));
 }
 
 void BackgroundSyncManager::RemoveRegistrationFromMap(
     int64 sw_registration_id,
     const RegistrationKey& registration_key) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(LookupRegistration(sw_registration_id, registration_key));
 
   BackgroundSyncRegistrations* registrations =
       &sw_to_registrations_map_[sw_registration_id];
 
   registrations->registration_map.erase(registration_key);
 }
 
 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;
 }
 
 void BackgroundSyncManager::StoreDataInBackend(
     int64 sw_registration_id,
     const GURL& origin,
     const std::string& backend_key,
     const std::string& data,
     const ServiceWorkerStorage::StatusCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   service_worker_context_->StoreRegistrationUserData(
       sw_registration_id, origin, backend_key, data, callback);
 }
 
 void BackgroundSyncManager::GetDataFromBackend(
     const std::string& backend_key,
     const ServiceWorkerStorage::GetUserDataForAllRegistrationsCallback&
         callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   service_worker_context_->GetUserDataForAllRegistrations(backend_key,
                                                           callback);
 }
 
 void BackgroundSyncManager::FireOneShotSync(
-    const BackgroundSyncRegistration& registration,
+    const 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 will hold the reference until it is destroyed.
+  // The ~BackgroundSyncManager DCHECKS verify that this is uncommon.
+  SyncRegistrationPtr sync_registration =
+      mojo::ConvertTo<SyncRegistrationPtr>(*registration.value());
+  scoped_refptr<const RefCountedRegistration>* ptr =
+      new scoped_refptr<const RefCountedRegistration>(&registration);
+  sync_registration->id = client_registration_ids_.Add(ptr);

[michaeln, 2015/08/19 01:56:01]

Does this leak (for the lifetime of the profile) if the logic inside blink
(running on the renderer's worker thread) doesn't call
WebSomething->trackRegistration?

[jkarlin, 2015/08/19 12:47:05]

Yes. But we'd at least catch that it's happening (at least in tests) with the
destructor's DCHECK.

[michaeln, 2015/08/21 02:39:24]

I'd vote to improve upon this. There are a many things that can prevent the
logic on the worker thread from ever getting a chance to call the balancing
'track' method. See comments inline for some ideas on that.

+
+  active_version->DispatchSyncEvent(sync_registration.Pass(), callback);
+}
+
+void BackgroundSyncManager::Unregister(
+    int64 sw_registration_id,
+    SyncPeriodicity periodicity,
+    BackgroundSyncRegistration::RegistrationId sync_registration_id,
+    const StatusCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  if (disabled_) {
+    BackgroundSyncMetrics::CountUnregister(
+        periodicity, BACKGROUND_SYNC_STATUS_STORAGE_ERROR);
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(callback, BACKGROUND_SYNC_STATUS_STORAGE_ERROR));
+    return;
+  }
+
+  op_scheduler_.ScheduleOperation(base::Bind(
+      &BackgroundSyncManager::UnregisterImpl, weak_ptr_factory_.GetWeakPtr(),
+      sw_registration_id, periodicity, sync_registration_id,
+      MakeStatusCompletion(callback)));
 }
 
 void BackgroundSyncManager::UnregisterImpl(
     int64 sw_registration_id,
-    const RegistrationKey& registration_key,
+    SyncPeriodicity periodicity,
     BackgroundSyncRegistration::RegistrationId sync_registration_id,
-    SyncPeriodicity periodicity,
     const StatusCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (disabled_) {
     BackgroundSyncMetrics::CountUnregister(
         periodicity, BACKGROUND_SYNC_STATUS_STORAGE_ERROR);
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(callback, BACKGROUND_SYNC_STATUS_STORAGE_ERROR));
     return;
   }
 
-  const BackgroundSyncRegistration* existing_registration =
+  scoped_refptr<const RefCountedRegistration>* hosted_registration_ref =
+      client_registration_ids_.Lookup(sync_registration_id);
+  DCHECK(hosted_registration_ref);
+
+  const BackgroundSyncRegistration* hosted_registration =
+      (*hosted_registration_ref)->value();
+
+  RegistrationKey registration_key(hosted_registration->options()->tag,
+                                   periodicity);
+
+  const RefCountedRegistration* existing_registration =
       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(
         FROM_HERE, base::Bind(callback, BACKGROUND_SYNC_STATUS_NOT_FOUND));
     return;
   }
 
   RemoveRegistrationFromMap(sw_registration_id, registration_key);
 
   StoreRegistrations(sw_registration_id,
@@ skipping 33 matching lines @@
       FROM_HERE, base::Bind(callback, BACKGROUND_SYNC_STATUS_OK));
 }
 
 void BackgroundSyncManager::GetRegistrationImpl(
     int64 sw_registration_id,
     const RegistrationKey& registration_key,
     const StatusAndRegistrationCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (disabled_) {
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::Bind(callback, BACKGROUND_SYNC_STATUS_STORAGE_ERROR,
-                              BackgroundSyncRegistration()));
+    PostErrorResponse(BACKGROUND_SYNC_STATUS_STORAGE_ERROR, callback);
     return;
   }
 
-  const BackgroundSyncRegistration* out_registration =
+  const RefCountedRegistration* registration =
       LookupRegistration(sw_registration_id, registration_key);
-  if (!out_registration) {
-    base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::Bind(callback, BACKGROUND_SYNC_STATUS_NOT_FOUND,
-                              BackgroundSyncRegistration()));
+  if (!registration) {
+    PostErrorResponse(BACKGROUND_SYNC_STATUS_NOT_FOUND, callback);
     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(
     int64 sw_registration_id,
     SyncPeriodicity periodicity,
     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;
   }
 
   SWIdToRegistrationsMap::iterator it =
       sw_to_registrations_map_.find(sw_registration_id);
 
   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);
+      const 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(
     const BackgroundSyncRegistrationOptions& options) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   return network_observer_->NetworkSufficient(options.network_state) &&
          power_observer_->PowerSufficient(options.power_state);
 }
 
 bool BackgroundSyncManager::IsRegistrationReadyToFire(
@@ skipping 13 matching lines @@
 }
 
 void BackgroundSyncManager::SchedulePendingRegistrations() {
 #if defined(OS_ANDROID)
   bool keep_browser_alive_for_one_shot = false;
 
   for (const auto& sw_id_and_registrations : sw_to_registrations_map_) {
     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;
         } else {
           // TODO(jkarlin): Support keeping the browser alive for periodic
           // syncs.
         }
       }
     }
   }
@@ skipping 29 matching lines @@
     return;
   }
 
   // Find the registrations that are ready to run.
   std::vector<std::pair<int64, RegistrationKey>> sw_id_and_keys_to_fire;
 
   for (auto& sw_id_and_registrations : sw_to_registrations_map_) {
     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));
         // The state change is not saved to persistent storage because
         // if the sync event is killed mid-sync then it should return to
         // SYNC_STATE_PENDING.
         registration->set_sync_state(SYNC_STATE_FIRING);
       }
     }
   }
@@ skipping 12 matching lines @@
         sw_id_and_keys_to_fire.size(), base::Bind(callback));
 
     // Record the total time taken after all events have run to completion.
     base::Closure events_completed_barrier_closure =
         base::BarrierClosure(sw_id_and_keys_to_fire.size(),
                              base::Bind(&OnAllSyncEventsCompleted, start_time,
                                         sw_id_and_keys_to_fire.size()));
 
     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 =
+      const RefCountedRegistration* registration =
           LookupRegistration(service_worker_id, sw_id_and_key.second);
+      DCHECK(registration);
 
       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));
     }
   }
 
   SchedulePendingRegistrations();
 }
 
 void BackgroundSyncManager::FireReadyEventsDidFindRegistration(
     const RegistrationKey& registration_key,
     BackgroundSyncRegistration::RegistrationId registration_id,
     const base::Closure& event_fired_callback,
     const base::Closure& event_completed_callback,
     ServiceWorkerStatusCode service_worker_status,
     const scoped_refptr<ServiceWorkerRegistration>&
         service_worker_registration) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (service_worker_status != SERVICE_WORKER_OK) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(event_fired_callback));
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(event_completed_callback));
     return;
   }
 
-  BackgroundSyncRegistration* registration =
+  const RefCountedRegistration* registration =
       LookupRegistration(service_worker_registration->id(), registration_key);
+  DCHECK(registration);
 
   FireOneShotSync(
       *registration, service_worker_registration->active_version(),
       base::Bind(&BackgroundSyncManager::EventComplete,
                  weak_ptr_factory_.GetWeakPtr(), service_worker_registration,
                  service_worker_registration->id(), registration_key,
                  registration_id, event_completed_callback));
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE, base::Bind(event_fired_callback));
@@ skipping 25 matching lines @@
     ServiceWorkerStatusCode status_code,
     const base::Closure& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (disabled_) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
                                                   base::Bind(callback));
     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,
                                            status_code == SERVICE_WORKER_OK);
 
   if (registration->options()->periodicity == SYNC_ONE_SHOT) {
     if (status_code != SERVICE_WORKER_OK) {
       // TODO(jkarlin) Fire the sync event on the next page load controlled by
       // this registration. (crbug.com/479665)
       registration->set_sync_state(SYNC_STATE_FAILED);
@@ skipping 59 matching lines @@
 }
 
 void BackgroundSyncManager::OnStorageWipedImpl(const base::Closure& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   sw_to_registrations_map_.clear();
   disabled_ = false;
   InitImpl(callback);
 }
 
+scoped_ptr<ClientBackgroundSyncRegistration>
+BackgroundSyncManager::CreateClientRegistration(
+    const 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());

[michaeln, 2015/08/19 01:56:01]

Why is it necessary to copy the values? The refcounting logic works to keep a
particular instance alive.

Or maybe the better question is, why is it important to keep that particular
refcounted instance alive? It looks like POD data structure that could be passed
by value everywhere.

I'm not following exactaly, looks like your making copies of the POD as needed
and doing refcounting bookeeping?

[jkarlin, 2015/08/19 12:47:05]

Sorry, it's not obvious in this CL why all of this is happening. The description
describes it some. This particular refcounted instance needs to be kept alive
because in an upcoming CL clients will be able to ask about the status of
registrations that have completed and are no longer in the
sw_to_registrations_map_. The refcounted instance will have the final status.
The BackgroundSyncManager keeps a single refcounted instance for itself, but
gives the clients copies. Each copy has a unique id which maps to a
scoped_refptr to the BackgroundSyncManager's refcounted instance. The purpose of
this is to keep the refcounting within the BackgroundSyncManager. This way we
guarantee that when the BackgroundSyncManager is deleted all of its
registrations are deleted with it.

+
+  scoped_refptr<const RefCountedRegistration>* ptr =
+      new scoped_refptr<const RefCountedRegistration>(&registration);
+  out_registration->set_id(client_registration_ids_.Add(ptr));

[michaeln, 2015/08/19 01:56:01]

This confused me quite a bit.

(I think) in some cases 'id' refers to a persistent identifer for a stored
registration, i see the value being pulled out of a protobug. And in others
cases 'id' is a very transient valued. Multiple callers can GetRegisteration and
be returned something that corresponds to that same persistent registration,
each with a different 'id'.

[jkarlin, 2015/08/19 12:47:05]

Yes, each client registration has a unique id which the BackgroundSyncManager
maps to the real (protobuf) id. Added a comment.

+  return out_registration.Pass();
+}
+
 void BackgroundSyncManager::OnNetworkChanged() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   FireReadyEvents();
 }
 
 void BackgroundSyncManager::OnPowerChanged() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   FireReadyEvents();
 }
 
+// TODO(jkarlin): Figure out how to pass scoped_ptrs with this.
 template <typename CallbackT, typename... Params>
 void BackgroundSyncManager::CompleteOperationCallback(const CallbackT& callback,
                                                       Params... parameters) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   callback.Run(parameters...);
   op_scheduler_.CompleteOperationAndRunNext();
 }
 
+void BackgroundSyncManager::CompleteStatusAndRegistrationCallback(
+    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);
 
   return MakeClosureCompletion(base::Bind(base::DoNothing));
 }
 
 base::Closure BackgroundSyncManager::MakeClosureCompletion(
     const base::Closure& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   return base::Bind(
       &BackgroundSyncManager::CompleteOperationCallback<base::Closure>,
       weak_ptr_factory_.GetWeakPtr(), callback);
 }
 
 BackgroundSyncManager::StatusAndRegistrationCallback
 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
 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
 BackgroundSyncManager::MakeStatusCompletion(const StatusCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   return base::Bind(
       &BackgroundSyncManager::CompleteOperationCallback<StatusCallback,
                                                         BackgroundSyncStatus>,
       weak_ptr_factory_.GetWeakPtr(), callback);
 }
 
 }  // namespace content