ServiceWorker: Ensure that ServiceWorkerDispatcher always adopts passed handle references (1)

content/child/service_worker/service_worker_dispatcher_unittest.cc

Index: content/child/service_worker/service_worker_dispatcher_unittest.cc
diff --git a/content/child/service_worker/service_worker_dispatcher_unittest.cc b/content/child/service_worker/service_worker_dispatcher_unittest.cc
index 6cd9621364d54dcbeeb5bbb45b7178208c15be3f..80d3465a6018f3ce6cecd655bb07101612d850d0 100644
--- a/content/child/service_worker/service_worker_dispatcher_unittest.cc
+++ b/content/child/service_worker/service_worker_dispatcher_unittest.cc
@@ -65,6 +65,15 @@ class ServiceWorkerDispatcherTest : public testing::Test {
     return ContainsKey(dispatcher_->registrations_, registration_handle_id);
   }
 
+  void OnAssociateRegistrationWithServiceWorker(
+      int thread_id,
+      int provider_id,
+      const ServiceWorkerRegistrationObjectInfo& info,
+      const ServiceWorkerVersionAttributes& attrs) {
+    dispatcher_->OnAssociateRegistrationWithServiceWorker(
+        thread_id, provider_id, info, attrs);
+  }
+
   void OnAssociateRegistration(int thread_id,
                                int provider_id,
                                const ServiceWorkerRegistrationObjectInfo& info,
@@ -72,6 +81,10 @@ class ServiceWorkerDispatcherTest : public testing::Test {
     dispatcher_->OnAssociateRegistration(thread_id, provider_id, info, attrs);
   }
 
+  void OnDisassociateRegistration(int thread_id, int provider_id) {
+    dispatcher_->OnDisassociateRegistration(thread_id, provider_id);
+  }
+
   void OnSetControllerServiceWorker(int thread_id,
                                     int provider_id,
                                     const ServiceWorkerObjectInfo& info,
@@ -126,7 +139,107 @@ class MockWebServiceWorkerProviderClientImpl
 };
 
 // TODO(nhiroki): Add tests for message handlers especially to receive reference
-// counts like OnAssociateRegistration().
+// counts.
+
+TEST_F(ServiceWorkerDispatcherTest, OnAssociateRegistrationWithServiceWorker) {
+  const int kProviderId = 10;
+
+  // Assume that these objects are passed from the browser process and own
+  // references to browser-side registration/worker representations.
+  ServiceWorkerRegistrationObjectInfo info;
+  ServiceWorkerVersionAttributes attrs;
+  CreateObjectInfoAndVersionAttributes(&info, &attrs);
+
+  // The passed references should be adopted but immediately destroyed because
+  // there is no provider context to own the references.
+  OnAssociateRegistrationWithServiceWorker(kDocumentMainThreadId, kProviderId,
+                                           info, attrs);
+  ASSERT_EQ(4UL, ipc_sink()->message_count());
+  EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID,
+            ipc_sink()->GetMessageAt(0)->type());
+  EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID,
+            ipc_sink()->GetMessageAt(1)->type());
+  EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID,
+            ipc_sink()->GetMessageAt(2)->type());
+  EXPECT_EQ(ServiceWorkerHostMsg_DecrementRegistrationRefCount::ID,
+            ipc_sink()->GetMessageAt(3)->type());
+  ipc_sink()->ClearMessages();
+
+  // Set up ServiceWorkerProviderContext for ServiceWorkerGlobalScope.
+  scoped_refptr<ServiceWorkerProviderContext> provider_context(
+      new ServiceWorkerProviderContext(kProviderId,
+                                       SERVICE_WORKER_PROVIDER_FOR_CONTROLLER,
+                                       thread_safe_sender()));
+
+  // The passed references should be adoped and owned by the provider context.

[falken, 2015/11/20 03:35:54]

nit: adopted (two places)

[nhiroki, 2015/11/20 05:14:59]

Done.

+  OnAssociateRegistrationWithServiceWorker(kDocumentMainThreadId, kProviderId,
+                                           info, attrs);
+  EXPECT_EQ(0UL, ipc_sink()->message_count());
+
+  // Destruction of the provider context should release references to the
+  // associated registration and its versions.
+  provider_context = nullptr;
+  ASSERT_EQ(4UL, ipc_sink()->message_count());
+  EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID,
+            ipc_sink()->GetMessageAt(0)->type());
+  EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID,
+            ipc_sink()->GetMessageAt(1)->type());
+  EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID,
+            ipc_sink()->GetMessageAt(2)->type());
+  EXPECT_EQ(ServiceWorkerHostMsg_DecrementRegistrationRefCount::ID,
+            ipc_sink()->GetMessageAt(3)->type());
+}
+
+TEST_F(ServiceWorkerDispatcherTest, OnAssociateRegistration) {
+  const int kProviderId = 10;
+
+  // Assume that these objects are passed from the browser process and own
+  // references to browser-side registration/worker representations.
+  ServiceWorkerRegistrationObjectInfo info;
+  ServiceWorkerVersionAttributes attrs;
+  CreateObjectInfoAndVersionAttributes(&info, &attrs);
+
+  // The passed references should be adopted but immediately destroyed because
+  // there is no provider context to own the references.
+  OnAssociateRegistrationWithServiceWorker(kDocumentMainThreadId, kProviderId,
+                                           info, attrs);

[falken, 2015/11/20 03:35:54]

How does this test differ from the one above it? The names don't give much clue;
they are both calling OnAssociateRegistrationWithServiceWorker.

[nhiroki, 2015/11/20 05:14:59]

Good catch! That should be OnAssociateRegistration. Fixed.

By the way, OnAssociateRegistration is now equal to
OnAssociateRegistrationWithServiceWorker and an operation is dispatched into
different implementations (ControllerDelegate vs. ControlleeDelegate) in
ServiceWorkerProviderContext depending on its provider type (please see my
comment on line 220), so we could simply merge them. I'll try it in a separate
CL.

[falken, 2015/11/20 05:26:25]

Agree with merging them.

+  ASSERT_EQ(4UL, ipc_sink()->message_count());
+  EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID,
+            ipc_sink()->GetMessageAt(0)->type());
+  EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID,
+            ipc_sink()->GetMessageAt(1)->type());
+  EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID,
+            ipc_sink()->GetMessageAt(2)->type());
+  EXPECT_EQ(ServiceWorkerHostMsg_DecrementRegistrationRefCount::ID,
+            ipc_sink()->GetMessageAt(3)->type());
+  ipc_sink()->ClearMessages();
+
+  // Set up ServiceWorkerProviderContext for a document context.
+  scoped_refptr<ServiceWorkerProviderContext> provider_context(
+      new ServiceWorkerProviderContext(kProviderId,
+                                       SERVICE_WORKER_PROVIDER_FOR_WINDOW,

[nhiroki, 2015/11/20 05:14:59]

fyi: This provider type is 
different from one in the other test (FOR_WINDOW vs. FOR_CONTROLLER).

+                                       thread_safe_sender()));
+
+  // The passed references should be adoped and only the registration reference
+  // should be owned by the provider context.
+  OnAssociateRegistrationWithServiceWorker(kDocumentMainThreadId, kProviderId,
+                                           info, attrs);
+  ASSERT_EQ(3UL, ipc_sink()->message_count());
+  EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID,
+            ipc_sink()->GetMessageAt(0)->type());
+  EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID,
+            ipc_sink()->GetMessageAt(1)->type());
+  EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID,
+            ipc_sink()->GetMessageAt(2)->type());
+  ipc_sink()->ClearMessages();
+
+  // Disassociating the provider context from the registration should release
+  // the reference.
+  OnDisassociateRegistration(kDocumentMainThreadId, kProviderId);
+  ASSERT_EQ(1UL, ipc_sink()->message_count());
+  EXPECT_EQ(ServiceWorkerHostMsg_DecrementRegistrationRefCount::ID,
+            ipc_sink()->GetMessageAt(0)->type());
+}
 
 TEST_F(ServiceWorkerDispatcherTest, OnSetControllerServiceWorker) {
   const int kProviderId = 10;