[BackgroundSyncManager] Remove origin argument from public BackgroundSyncManager methods

The origin can be determined from the live ServiceWorkerRegistration,
use that instead of requiring the renderer to pass it.

Also, verify that the live registration is active to prevent
BackgroundSyncManager::Register from being called prematurely.

BUG=482012

Review URL: https://codereview.chromium.org/1110993003

Cr-Commit-Position: refs/heads/master@{#327371}

content/browser/background_sync/background_sync_manager.cc

@@ -62,7 +62,6 @@ BackgroundSyncManager::RegistrationKey::RegistrationKey(
 }
 
 void BackgroundSyncManager::Register(
-    const GURL& origin,
     int64 sw_registration_id,
     const BackgroundSyncRegistration& sync_registration,
     const StatusAndRegistrationCallback& callback) {
@@ -79,12 +78,11 @@ void BackgroundSyncManager::Register(
 
   op_scheduler_.ScheduleOperation(base::Bind(
       &BackgroundSyncManager::RegisterImpl, weak_ptr_factory_.GetWeakPtr(),
-      origin, sw_registration_id, sync_registration,
+      sw_registration_id, sync_registration,
       MakeStatusAndRegistrationCompletion(callback)));
 }
 
 void BackgroundSyncManager::Unregister(
-    const GURL& origin,
     int64 sw_registration_id,
     const std::string& sync_registration_tag,
     SyncPeriodicity periodicity,
@@ -102,12 +100,11 @@ void BackgroundSyncManager::Unregister(
 
   op_scheduler_.ScheduleOperation(base::Bind(
       &BackgroundSyncManager::UnregisterImpl, weak_ptr_factory_.GetWeakPtr(),
-      origin, sw_registration_id, registration_key, sync_registration_id,
+      sw_registration_id, registration_key, sync_registration_id,
       MakeStatusCompletion(callback)));
 }
 
 void BackgroundSyncManager::GetRegistration(
-    const GURL& origin,
     int64 sw_registration_id,
     const std::string sync_registration_tag,
     SyncPeriodicity periodicity,
@@ -125,8 +122,8 @@ void BackgroundSyncManager::GetRegistration(
 
   op_scheduler_.ScheduleOperation(base::Bind(
       &BackgroundSyncManager::GetRegistrationImpl,
-      weak_ptr_factory_.GetWeakPtr(), origin, sw_registration_id,
-      registration_key, MakeStatusAndRegistrationCompletion(callback)));
+      weak_ptr_factory_.GetWeakPtr(), sw_registration_id, registration_key,
+      MakeStatusAndRegistrationCompletion(callback)));
 }
 
 void BackgroundSyncManager::OnRegistrationDeleted(int64 registration_id,
@@ -248,7 +245,6 @@ void BackgroundSyncManager::InitDidGetDataFromBackend(
 }
 
 void BackgroundSyncManager::RegisterImpl(
-    const GURL& origin,
     int64 sw_registration_id,
     const BackgroundSyncRegistration& sync_registration,
     const StatusAndRegistrationCallback& callback) {
@@ -273,7 +269,18 @@ void BackgroundSyncManager::RegisterImpl(
       &sw_to_registrations_map_[sw_registration_id];
   new_registration.id = registrations->next_id++;
 
-  AddRegistrationToMap(sw_registration_id, origin, new_registration);
+  ServiceWorkerRegistration* sw_registration =
+      service_worker_context_->GetLiveRegistration(sw_registration_id);
+  if (!sw_registration || !sw_registration->active_version()) {
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE, base::Bind(callback, ERROR_TYPE_NO_SERVICE_WORKER,
+                              BackgroundSyncRegistration()));
+    return;
+  }
+
+  AddRegistrationToMap(sw_registration_id,
+                       sw_registration->pattern().GetOrigin(),
+                       new_registration);
 
   StoreRegistrations(
       sw_registration_id,
@@ -454,7 +461,6 @@ void BackgroundSyncManager::GetDataFromBackend(
 }
 
 void BackgroundSyncManager::UnregisterImpl(
-    const GURL& origin,
     int64 sw_registration_id,
     const RegistrationKey& registration_key,
     BackgroundSyncRegistration::RegistrationId sync_registration_id,
@@ -506,7 +512,6 @@ void BackgroundSyncManager::UnregisterDidStore(
 }
 
 void BackgroundSyncManager::GetRegistrationImpl(
-    const GURL& origin,
     int64 sw_registration_id,
     const RegistrationKey& registration_key,
     const StatusAndRegistrationCallback& callback) {

content/browser/background_sync/background_sync_manager.h

@@ -45,7 +45,8 @@ class CONTENT_EXPORT BackgroundSyncManager
   enum ErrorType {
     ERROR_TYPE_OK = 0,
     ERROR_TYPE_STORAGE,
-    ERROR_TYPE_NOT_FOUND
+    ERROR_TYPE_NOT_FOUND,
+    ERROR_TYPE_NO_SERVICE_WORKER
   };
 
   // TODO(jkarlin): Remove this and use the struct from IPC messages once it
@@ -85,8 +86,7 @@ class CONTENT_EXPORT BackgroundSyncManager
   // with ErrorTypeOK and the accepted registration on success. The accepted
   // registration will have a unique id. It may also have altered parameters if
   // the user or UA chose different parameters than those supplied.
-  void Register(const GURL& origin,
-                int64 sw_registration_id,
+  void Register(int64 sw_registration_id,
                 const BackgroundSyncRegistration& sync_registration,
                 const StatusAndRegistrationCallback& callback);
 
@@ -95,7 +95,6 @@ class CONTENT_EXPORT BackgroundSyncManager
   // ErrorTypeNotFound if no match is found. Calls |callback| with ErrorTypeOK
   // on success.
   void Unregister(
-      const GURL& origin,
       int64 sw_registration_id,
       const std::string& sync_registration_tag,
       SyncPeriodicity periodicity,
@@ -106,8 +105,7 @@ class CONTENT_EXPORT BackgroundSyncManager
   // |sw_registration_id| with periodicity |periodicity|. Calls
   // |callback| with ErrorTypeNotFound if it doesn't exist. Calls |callback|
   // with ErrorTypeOK on success.
-  void GetRegistration(const GURL& origin,
-                       int64 sw_registration_id,
+  void GetRegistration(int64 sw_registration_id,
                        const std::string sync_registration_tag,
                        SyncPeriodicity periodicity,
                        const StatusAndRegistrationCallback& callback);
@@ -206,8 +204,7 @@ class CONTENT_EXPORT BackgroundSyncManager
       ServiceWorkerStatusCode status);
 
   // Register callbacks
-  void RegisterImpl(const GURL& origin,
-                    int64 sw_registration_id,
+  void RegisterImpl(int64 sw_registration_id,
                     const BackgroundSyncRegistration& sync_registration,
                     const StatusAndRegistrationCallback& callback);
   void RegisterDidStore(int64 sw_registration_id,
@@ -217,7 +214,6 @@ class CONTENT_EXPORT BackgroundSyncManager
 
   // Unregister callbacks
   void UnregisterImpl(
-      const GURL& origin,
       int64 sw_registration_id,
       const RegistrationKey& registration_key,
       BackgroundSyncRegistration::RegistrationId sync_registration_id,
@@ -228,8 +224,7 @@ class CONTENT_EXPORT BackgroundSyncManager
       ServiceWorkerStatusCode status);
 
   // GetRegistration callbacks
-  void GetRegistrationImpl(const GURL& origin,
-                           int64 sw_registration_id,
+  void GetRegistrationImpl(int64 sw_registration_id,
                            const RegistrationKey& registration_key,
                            const StatusAndRegistrationCallback& callback);
 

content/browser/background_sync/background_sync_manager_unittest.cc

@@ -20,7 +20,6 @@ namespace content {
 
 namespace {
 
-const char kOrigin[] = "https://example.com";
 const char kPattern1[] = "https://example.com/a";
 const char kPattern2[] = "https://example.com/b";
 const char kScript1[] = "https://example.com/a/script.js";
@@ -37,6 +36,14 @@ void RegisterServiceWorkerCallback(bool* called,
   *store_registration_id = registration_id;
 }
 
+void FindServiceWorkerRegistrationCallback(
+    scoped_refptr<ServiceWorkerRegistration>* out_registration,
+    ServiceWorkerStatusCode status,
+    const scoped_refptr<ServiceWorkerRegistration>& registration) {
+  EXPECT_EQ(SERVICE_WORKER_OK, status) << ServiceWorkerStatusToString(status);
+  *out_registration = registration;
+}
+
 void UnregisterServiceWorkerCallback(bool* called,
                                      ServiceWorkerStatusCode code) {
   EXPECT_EQ(SERVICE_WORKER_OK, code);
@@ -155,7 +162,10 @@ class BackgroundSyncManagerTest : public testing::Test {
     // Wait for storage to finish initializing before registering service
     // workers.
     base::RunLoop().RunUntilIdle();
+    RegisterServiceWorkers();
+  }
 
+  void RegisterServiceWorkers() {
     bool called_1 = false;
     bool called_2 = false;
     helper_->context()->RegisterServiceWorker(
@@ -170,6 +180,19 @@ class BackgroundSyncManagerTest : public testing::Test {
     base::RunLoop().RunUntilIdle();
     EXPECT_TRUE(called_1);
     EXPECT_TRUE(called_2);
+
+    // Hang onto the registrations as they need to be "live" when
+    // calling BackgroundSyncMasnager::Register.
+    helper_->context_wrapper()->FindRegistrationForId(
+        sw_registration_id_1_, GURL(kPattern1).GetOrigin(),
+        base::Bind(FindServiceWorkerRegistrationCallback, &sw_registration_1_));
+
+    helper_->context_wrapper()->FindRegistrationForId(
+        sw_registration_id_2_, GURL(kPattern1).GetOrigin(),
+        base::Bind(FindServiceWorkerRegistrationCallback, &sw_registration_2_));
+    base::RunLoop().RunUntilIdle();
+    EXPECT_TRUE(sw_registration_1_);
+    EXPECT_TRUE(sw_registration_2_);
   }
 
   void StatusAndRegistrationCallback(
@@ -208,7 +231,7 @@ class BackgroundSyncManagerTest : public testing::Test {
           sync_registration) {
     bool was_called = false;
     background_sync_manager_->Register(
-        GURL(kOrigin), sw_registration_id, sync_registration,
+        sw_registration_id, sync_registration,
         base::Bind(&BackgroundSyncManagerTest::StatusAndRegistrationCallback,
                    base::Unretained(this), &was_called));
     base::RunLoop().RunUntilIdle();
@@ -228,7 +251,7 @@ class BackgroundSyncManagerTest : public testing::Test {
           sync_registration) {
     bool was_called = false;
     background_sync_manager_->Unregister(
-        GURL(kOrigin), sw_registration_id, sync_registration.tag,
+        sw_registration_id, sync_registration.tag,
         sync_registration.periodicity, sync_registration.id,
         base::Bind(&BackgroundSyncManagerTest::StatusCallback,
                    base::Unretained(this), &was_called));
@@ -249,7 +272,7 @@ class BackgroundSyncManagerTest : public testing::Test {
           sync_registration) {
     bool was_called = false;
     background_sync_manager_->GetRegistration(
-        GURL(kOrigin), sw_registration_id, sync_registration.tag,
+        sw_registration_id, sync_registration.tag,
         sync_registration.periodicity,
         base::Bind(&BackgroundSyncManagerTest::StatusAndRegistrationCallback,
                    base::Unretained(this), &was_called));
@@ -289,6 +312,8 @@ class BackgroundSyncManagerTest : public testing::Test {
 
   int64 sw_registration_id_1_;
   int64 sw_registration_id_2_;
+  scoped_refptr<ServiceWorkerRegistration> sw_registration_1_;
+  scoped_refptr<ServiceWorkerRegistration> sw_registration_2_;
 
   BackgroundSyncManager::BackgroundSyncRegistration sync_reg_1_;
   BackgroundSyncManager::BackgroundSyncRegistration sync_reg_2_;
@@ -311,6 +336,20 @@ TEST_F(BackgroundSyncManagerTest, RegistractionIntact) {
       callback_registration_.id);
 }
 
+TEST_F(BackgroundSyncManagerTest, RegisterWithoutLiveSWRegistration) {
+  sw_registration_1_ = nullptr;
+  EXPECT_FALSE(Register(sync_reg_1_));
+  EXPECT_EQ(BackgroundSyncManager::ERROR_TYPE_NO_SERVICE_WORKER,
+            callback_error_);
+}
+
+TEST_F(BackgroundSyncManagerTest, RegisterWithoutActiveSWRegistration) {
+  sw_registration_1_->UnsetVersion(sw_registration_1_->active_version());
+  EXPECT_FALSE(Register(sync_reg_1_));
+  EXPECT_EQ(BackgroundSyncManager::ERROR_TYPE_NO_SERVICE_WORKER,
+            callback_error_);
+}
+
 TEST_F(BackgroundSyncManagerTest, RegisterExistingKeepsId) {
   EXPECT_TRUE(Register(sync_reg_1_));
   BackgroundSyncManager::BackgroundSyncRegistration first_registration =
@@ -507,16 +546,15 @@ TEST_F(BackgroundSyncManagerTest, SequentialOperations) {
   bool unregister_called = false;
   bool get_registration_called = false;
   manager->Register(
-      GURL(kOrigin), sw_registration_id_1_, sync_reg_1_,
+      sw_registration_id_1_, sync_reg_1_,
       base::Bind(&BackgroundSyncManagerTest::StatusAndRegistrationCallback,
                  base::Unretained(this), &register_called));
-  manager->Unregister(GURL(kOrigin), sw_registration_id_1_, sync_reg_1_.tag,
+  manager->Unregister(sw_registration_id_1_, sync_reg_1_.tag,
                       sync_reg_1_.periodicity, kExpectedInitialId,
                       base::Bind(&BackgroundSyncManagerTest::StatusCallback,
                                  base::Unretained(this), &unregister_called));
   manager->GetRegistration(
-      GURL(kOrigin), sw_registration_id_1_, sync_reg_1_.tag,
-      sync_reg_1_.periodicity,
+      sw_registration_id_1_, sync_reg_1_.tag, sync_reg_1_.periodicity,
       base::Bind(&BackgroundSyncManagerTest::StatusAndRegistrationCallback,
                  base::Unretained(this), &get_registration_called));
 
@@ -569,7 +607,7 @@ TEST_F(BackgroundSyncManagerTest,
   manager->set_delay_backend(true);
   bool callback_called = false;
   manager->Register(
-      GURL(kOrigin), sw_registration_id_1_, sync_reg_2_,
+      sw_registration_id_1_, sync_reg_2_,
       base::Bind(&BackgroundSyncManagerTest::StatusAndRegistrationCallback,
                  base::Unretained(this), &callback_called));
 
@@ -630,13 +668,7 @@ TEST_F(BackgroundSyncManagerTest, DisabledManagerWorksAfterDeleteAndStartOver) {
   helper_->context()->ScheduleDeleteAndStartOver();
   base::RunLoop().RunUntilIdle();
 
-  bool called = false;
-  helper_->context()->RegisterServiceWorker(
-      GURL(kPattern1), GURL(kScript1), NULL,
-      base::Bind(&RegisterServiceWorkerCallback, &called,
-                 &sw_registration_id_1_));
-  base::RunLoop().RunUntilIdle();
-  EXPECT_TRUE(called);
+  RegisterServiceWorkers();
 
   EXPECT_TRUE(Register(sync_reg_2_));
   EXPECT_FALSE(GetRegistration(sync_reg_1_));