ServiceWorker: Make APIs return the existing ServiceWorkerRegistration object if available

This CL reverts following CLs:
- https://codereview.chromium.org/1307133003/
- https://codereview.chromium.org/1311103002/

These CLs made APIs that return ServiceWorkerRegistration object (ie. register,
getRegistration and getRegistrations) always coin a new JS object, but the
changes were based on misinterpretation of the spec.

To correct them, this CL reverts the changes and makes APIs return the existing
object if available as specified in the spec.

Spec: https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#navigator-service-worker-getRegistration

BUG=523904

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

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

3 files changed, 116 insertions, 56 deletions

diff --git a/content/child/service_worker/service_worker_dispatcher.cc b/content/child/service_worker/service_worker_dispatcher.cc
index fbf1e31..489506d 100644
--- a/content/child/service_worker/service_worker_dispatcher.cc
+++ b/content/child/service_worker/service_worker_dispatcher.cc
@@ -294,23 +294,56 @@ WebServiceWorkerImpl* ServiceWorkerDispatcher::GetServiceWorker(
 }
 
 scoped_refptr<WebServiceWorkerRegistrationImpl>
-ServiceWorkerDispatcher::CreateRegistration(
+ServiceWorkerDispatcher::GetOrCreateRegistration(
     const ServiceWorkerRegistrationObjectInfo& info,
     const ServiceWorkerVersionAttributes& attrs) {
-  return CreateRegistrationInternal(
-      ServiceWorkerRegistrationHandleReference::Create(
-          info, thread_safe_sender_.get()),
-      attrs, false /* adopt_handle */);
+  RegistrationObjectMap::iterator found = registrations_.find(info.handle_id);
+  if (found != registrations_.end())
+    return found->second;
+
+  // WebServiceWorkerRegistrationImpl constructor calls
+  // AddServiceWorkerRegistration.
+  scoped_refptr<WebServiceWorkerRegistrationImpl> registration(
+      new WebServiceWorkerRegistrationImpl(
+          ServiceWorkerRegistrationHandleReference::Create(
+              info, thread_safe_sender_.get())));
+
+  bool adopt_handle = false;
+  registration->SetInstalling(GetServiceWorker(attrs.installing, adopt_handle));
+  registration->SetWaiting(GetServiceWorker(attrs.waiting, adopt_handle));
+  registration->SetActive(GetServiceWorker(attrs.active, adopt_handle));
+  return registration.Pass();
 }
 
 scoped_refptr<WebServiceWorkerRegistrationImpl>
-ServiceWorkerDispatcher::AdoptRegistration(
+ServiceWorkerDispatcher::GetOrAdoptRegistration(
     const ServiceWorkerRegistrationObjectInfo& info,
     const ServiceWorkerVersionAttributes& attrs) {
-  return CreateRegistrationInternal(
-      ServiceWorkerRegistrationHandleReference::Adopt(
-          info, thread_safe_sender_.get()),
-      attrs, true /* adopt_handle */);
+  RegistrationObjectMap::iterator found = registrations_.find(info.handle_id);
+  if (found != registrations_.end()) {
+    ServiceWorkerHandleReference::Adopt(attrs.installing,
+                                        thread_safe_sender_.get());
+    ServiceWorkerHandleReference::Adopt(attrs.waiting,
+                                        thread_safe_sender_.get());
+    ServiceWorkerHandleReference::Adopt(attrs.active,
+                                        thread_safe_sender_.get());
+    ServiceWorkerRegistrationHandleReference::Adopt(info,
+                                                    thread_safe_sender_.get());
+    return found->second;
+  }
+
+  // WebServiceWorkerRegistrationImpl constructor calls
+  // AddServiceWorkerRegistration.
+  scoped_refptr<WebServiceWorkerRegistrationImpl> registration(
+      new WebServiceWorkerRegistrationImpl(
+          ServiceWorkerRegistrationHandleReference::Adopt(
+              info, thread_safe_sender_.get())));
+
+  bool adopt_handle = true;
+  registration->SetInstalling(GetServiceWorker(attrs.installing, adopt_handle));
+  registration->SetWaiting(GetServiceWorker(attrs.waiting, adopt_handle));
+  registration->SetActive(GetServiceWorker(attrs.active, adopt_handle));
+  return registration.Pass();
 }
 
 // We can assume that this message handler is called before the worker context
@@ -372,7 +405,7 @@ void ServiceWorkerDispatcher::OnRegistered(
   if (!callbacks)
     return;
 
-  callbacks->onSuccess(AdoptRegistration(info, attrs)->CreateHandle());
+  callbacks->onSuccess(GetOrAdoptRegistration(info, attrs)->CreateHandle());
   pending_registration_callbacks_.Remove(request_id);
 }
 
@@ -434,7 +467,7 @@ void ServiceWorkerDispatcher::OnDidGetRegistration(
 
   scoped_refptr<WebServiceWorkerRegistrationImpl> registration;
   if (info.handle_id != kInvalidServiceWorkerHandleId)
-    registration = AdoptRegistration(info, attrs);
+    registration = GetOrAdoptRegistration(info, attrs);
 
   callbacks->onSuccess(registration ? registration->CreateHandle() : nullptr);
   pending_get_registration_callbacks_.Remove(request_id);
@@ -472,7 +505,8 @@ void ServiceWorkerDispatcher::OnDidGetRegistrations(
       // WebServiceWorkerGetRegistrationsCallbacks cannot receive an array of
       // WebPassOwnPtr<WebServiceWorkerRegistration::Handle>, so create leaky
       // handles instead.
-      (*registrations)[i] = AdoptRegistration(info, attr)->CreateLeakyHandle();
+      (*registrations)[i] =
+          GetOrAdoptRegistration(info, attr)->CreateLeakyHandle();
     }
   }
 
@@ -499,7 +533,7 @@ void ServiceWorkerDispatcher::OnDidGetRegistrationForReady(
   if (!callbacks)
     return;
 
-  callbacks->onSuccess(AdoptRegistration(info, attrs)->CreateHandle());
+  callbacks->onSuccess(GetOrAdoptRegistration(info, attrs)->CreateHandle());
   get_for_ready_callbacks_.Remove(request_id);
 }
 
@@ -735,19 +769,4 @@ void ServiceWorkerDispatcher::RemoveServiceWorkerRegistration(
   registrations_.erase(registration_handle_id);
 }
 
-scoped_refptr<WebServiceWorkerRegistrationImpl>
-ServiceWorkerDispatcher::CreateRegistrationInternal(
-    scoped_ptr<ServiceWorkerRegistrationHandleReference> handle_ref,
-    const ServiceWorkerVersionAttributes& attrs,
-    bool adopt_handle) {
-  // WebServiceWorkerRegistrationImpl constructor calls
-  // AddServiceWorkerRegistration.
-  scoped_refptr<WebServiceWorkerRegistrationImpl> registration(
-      new WebServiceWorkerRegistrationImpl(handle_ref.Pass()));
-  registration->SetInstalling(GetServiceWorker(attrs.installing, adopt_handle));
-  registration->SetWaiting(GetServiceWorker(attrs.waiting, adopt_handle));
-  registration->SetActive(GetServiceWorker(attrs.active, adopt_handle));
-  return registration.Pass();
-}
-
 }  // namespace content
diff --git a/content/child/service_worker/service_worker_dispatcher.h b/content/child/service_worker/service_worker_dispatcher.h
index a845ee4..32fc133 100644
--- a/content/child/service_worker/service_worker_dispatcher.h
+++ b/content/child/service_worker/service_worker_dispatcher.h
@@ -133,11 +133,17 @@ class CONTENT_EXPORT ServiceWorkerDispatcher : public WorkerThread::Observer {
       const ServiceWorkerObjectInfo& info,
       bool adopt_handle);
 
-  // Returns a new registration filled in with version attributes.
-  scoped_refptr<WebServiceWorkerRegistrationImpl> CreateRegistration(
+  // Returns the existing registration or a newly created one. When a new one is
+  // created, increments interprocess references to the registration and its
+  // versions via ServiceWorker(Registration)HandleReference.
+  scoped_refptr<WebServiceWorkerRegistrationImpl> GetOrCreateRegistration(
       const ServiceWorkerRegistrationObjectInfo& info,
       const ServiceWorkerVersionAttributes& attrs);
-  scoped_refptr<WebServiceWorkerRegistrationImpl> AdoptRegistration(
+
+  // Returns the existing registration or a newly created one. Always adopts
+  // interprocess references to the registration and its versions via
+  // ServiceWorker(Registration)HandleReference.
+  scoped_refptr<WebServiceWorkerRegistrationImpl> GetOrAdoptRegistration(
       const ServiceWorkerRegistrationObjectInfo& info,
       const ServiceWorkerVersionAttributes& attrs);
 
@@ -264,11 +270,6 @@ class CONTENT_EXPORT ServiceWorkerDispatcher : public WorkerThread::Observer {
   void RemoveServiceWorkerRegistration(
       int registration_handle_id);
 
-  scoped_refptr<WebServiceWorkerRegistrationImpl> CreateRegistrationInternal(
-      scoped_ptr<ServiceWorkerRegistrationHandleReference> handle_ref,
-      const ServiceWorkerVersionAttributes& attrs,
-      bool adopt_handle);
-
   RegistrationCallbackMap pending_registration_callbacks_;
   UpdateCallbackMap pending_update_callbacks_;
   UnregistrationCallbackMap pending_unregistration_callbacks_;
diff --git a/content/child/service_worker/service_worker_dispatcher_unittest.cc b/content/child/service_worker/service_worker_dispatcher_unittest.cc
index 40f8f63..0a66d2a 100644
--- a/content/child/service_worker/service_worker_dispatcher_unittest.cc
+++ b/content/child/service_worker/service_worker_dispatcher_unittest.cc
@@ -133,20 +133,43 @@ TEST_F(ServiceWorkerDispatcherTest, GetServiceWorker) {
   EXPECT_EQ(0UL, ipc_sink()->message_count());
 }
 
-TEST_F(ServiceWorkerDispatcherTest, CreateRegistration) {
+TEST_F(ServiceWorkerDispatcherTest, GetOrCreateRegistration) {
   ServiceWorkerRegistrationObjectInfo info;
   ServiceWorkerVersionAttributes attrs;
   CreateObjectInfoAndVersionAttributes(&info, &attrs);
 
-  // Should return a registration object newly created with adopting refcount.
-  scoped_refptr<WebServiceWorkerRegistrationImpl> registration(
-      dispatcher()->AdoptRegistration(info, attrs));
-  EXPECT_TRUE(registration);
-  EXPECT_EQ(info.registration_id, registration->registration_id());
+  // Should return a registration object newly created with incrementing
+  // the refcounts.
+  scoped_refptr<WebServiceWorkerRegistrationImpl> registration1(
+      dispatcher()->GetOrCreateRegistration(info, attrs));
+  EXPECT_TRUE(registration1);
+  EXPECT_TRUE(ContainsRegistration(info.handle_id));
+  EXPECT_EQ(info.registration_id, registration1->registration_id());
+  ASSERT_EQ(4UL, ipc_sink()->message_count());
+  EXPECT_EQ(ServiceWorkerHostMsg_IncrementRegistrationRefCount::ID,
+            ipc_sink()->GetMessageAt(0)->type());
+  EXPECT_EQ(ServiceWorkerHostMsg_IncrementServiceWorkerRefCount::ID,
+            ipc_sink()->GetMessageAt(1)->type());
+  EXPECT_EQ(ServiceWorkerHostMsg_IncrementServiceWorkerRefCount::ID,
+            ipc_sink()->GetMessageAt(2)->type());
+  EXPECT_EQ(ServiceWorkerHostMsg_IncrementServiceWorkerRefCount::ID,
+            ipc_sink()->GetMessageAt(3)->type());
+
+  ipc_sink()->ClearMessages();
+
+  // Should return the same registration object without incrementing the
+  // refcounts.
+  scoped_refptr<WebServiceWorkerRegistrationImpl> registration2(
+      dispatcher()->GetOrCreateRegistration(info, attrs));
+  EXPECT_TRUE(registration2);
+  EXPECT_EQ(registration1, registration2);
   EXPECT_EQ(0UL, ipc_sink()->message_count());
 
-  // The registration dtor decrements the refcount.
-  registration = nullptr;
+  ipc_sink()->ClearMessages();
+
+  // The registration dtor decrements the refcounts.
+  registration1 = nullptr;
+  registration2 = nullptr;
   ASSERT_EQ(4UL, ipc_sink()->message_count());
   EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID,
             ipc_sink()->GetMessageAt(0)->type());
@@ -156,28 +179,45 @@ TEST_F(ServiceWorkerDispatcherTest, CreateRegistration) {
             ipc_sink()->GetMessageAt(2)->type());
   EXPECT_EQ(ServiceWorkerHostMsg_DecrementRegistrationRefCount::ID,
             ipc_sink()->GetMessageAt(3)->type());
+}
 
-  ipc_sink()->ClearMessages();
+TEST_F(ServiceWorkerDispatcherTest, GetOrAdoptRegistration) {
+  ServiceWorkerRegistrationObjectInfo info;
+  ServiceWorkerVersionAttributes attrs;
+  CreateObjectInfoAndVersionAttributes(&info, &attrs);
 
-  // Should return a registration object newly created with incrementing
-  // refcount.
-  registration = dispatcher()->CreateRegistration(info, attrs);
-  EXPECT_TRUE(registration);
+  // Should return a registration object newly created with adopting the
+  // refcounts.
+  scoped_refptr<WebServiceWorkerRegistrationImpl> registration1(
+      dispatcher()->GetOrAdoptRegistration(info, attrs));
+  EXPECT_TRUE(registration1);
   EXPECT_TRUE(ContainsRegistration(info.handle_id));
+  EXPECT_EQ(info.registration_id, registration1->registration_id());
+  EXPECT_EQ(0UL, ipc_sink()->message_count());
+
+  ipc_sink()->ClearMessages();
+
+  // Should return the same registration object without incrementing the
+  // refcounts.
+  scoped_refptr<WebServiceWorkerRegistrationImpl> registration2(
+      dispatcher()->GetOrAdoptRegistration(info, attrs));
+  EXPECT_TRUE(registration2);
+  EXPECT_EQ(registration1, registration2);
   ASSERT_EQ(4UL, ipc_sink()->message_count());
-  EXPECT_EQ(ServiceWorkerHostMsg_IncrementRegistrationRefCount::ID,
+  EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID,
             ipc_sink()->GetMessageAt(0)->type());
-  EXPECT_EQ(ServiceWorkerHostMsg_IncrementServiceWorkerRefCount::ID,
+  EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID,
             ipc_sink()->GetMessageAt(1)->type());
-  EXPECT_EQ(ServiceWorkerHostMsg_IncrementServiceWorkerRefCount::ID,
+  EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID,
             ipc_sink()->GetMessageAt(2)->type());
-  EXPECT_EQ(ServiceWorkerHostMsg_IncrementServiceWorkerRefCount::ID,
+  EXPECT_EQ(ServiceWorkerHostMsg_DecrementRegistrationRefCount::ID,
             ipc_sink()->GetMessageAt(3)->type());
 
   ipc_sink()->ClearMessages();
 
-  // The registration dtor decrements the refcount.
-  registration = nullptr;
+  // The registration dtor decrements the refcounts.
+  registration1 = nullptr;
+  registration2 = nullptr;
   ASSERT_EQ(4UL, ipc_sink()->message_count());
   EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID,
             ipc_sink()->GetMessageAt(0)->type());