[ServiceWorker] Pass controller sw object ownership to ServiceWorkerContainer

Before this CL, after updated ServiceWorkerContainer#controller,
ServiceWorkerProviderContext still holds an extra
ServiceWorkerHandleReference retaining a reference to the
corresponding ServiceWorkerHandle in the browser process.

This is unnecessary because this extra ServiceWorkerHandleReference is
only used to provide the controller service worker version id, so this
CL just passes it to the ServiceWorkerContainer, to make the ownership
transfer process clearer.

This CL removes one usage to ServiceWorkerHandleReference::Create() to
enable us to eliminate ServiceWorkerHandleReference class later using
blink::mojom::ServiceWorkerObjectInfoPtr directly.

BUG=772713

Change-Id: I6657826355aa367604e5986751562dc413cc343e
Reviewed-on: https://chromium-review.googlesource.com/756818
Commit-Queue: Han Leon <leon.han@intel.com>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515390}

content/renderer/service_worker/service_worker_network_provider.cc

@@ -88,8 +88,8 @@ class WebServiceWorkerNetworkProviderForFrame
   }
 
   int64_t ControllerServiceWorkerID() override {
-    if (provider_->context() && provider_->context()->controller())
-      return provider_->context()->controller()->version_id();
+    if (provider_->context())
+      return provider_->context()->GetControllerVersionId();
     return blink::mojom::kInvalidServiceWorkerVersionId;
   }
 
@@ -247,7 +247,8 @@ int ServiceWorkerNetworkProvider::provider_id() const {
 }
 
 bool ServiceWorkerNetworkProvider::IsControlledByServiceWorker() const {
-  return context() && context()->controller();
+  return context() && context()->GetControllerVersionId() !=
+                          blink::mojom::kInvalidServiceWorkerVersionId;
 }
 
 // Creates an invalid instance (provider_id() returns

content/renderer/service_worker/service_worker_provider_context.cc

@@ -41,7 +41,10 @@ struct ServiceWorkerProviderContext::ControlleeState {
             std::move(default_loader_factory_getter)) {}
   ~ControlleeState() = default;
 
+  // |controller| will be set by SetController() and taken by TakeController().
   std::unique_ptr<ServiceWorkerHandleReference> controller;
+  // Keeps version id of the current controller service worker object.
+  int64_t controller_version_id = blink::mojom::kInvalidServiceWorkerVersionId;
 
   // S13nServiceWorker:
   // Used to intercept requests from the controllee and dispatch them
@@ -172,10 +175,17 @@ ServiceWorkerProviderContext::TakeRegistrationForServiceWorkerGlobalScope() {
   return info;
 }
 
-ServiceWorkerHandleReference* ServiceWorkerProviderContext::controller() {
+std::unique_ptr<ServiceWorkerHandleReference>
+ServiceWorkerProviderContext::TakeController() {
   DCHECK(main_thread_task_runner_->RunsTasksInCurrentSequence());
   DCHECK(controllee_state_);
-  return controllee_state_->controller.get();
+  return std::move(controllee_state_->controller);
+}
+
+int64_t ServiceWorkerProviderContext::GetControllerVersionId() {
+  DCHECK(main_thread_task_runner_->RunsTasksInCurrentSequence());
+  DCHECK(controllee_state_);
+  return controllee_state_->controller_version_id;
 }
 
 mojom::URLLoaderFactory*
@@ -255,7 +265,8 @@ void ServiceWorkerProviderContext::SetController(
   ServiceWorkerDispatcher* dispatcher =
       ServiceWorkerDispatcher::GetThreadSpecificInstance();
 
-  state->controller = dispatcher->Adopt(controller->Clone());
+  state->controller_version_id = controller->version_id;
+  state->controller = dispatcher->Adopt(std::move(controller));
 
   // Propagate the controller to workers related to this provider.
   if (state->controller) {
@@ -296,7 +307,7 @@ void ServiceWorkerProviderContext::SetController(
   // the controller from |this| via WebServiceWorkerProviderImpl::SetClient().
   if (state->web_service_worker_provider) {
     state->web_service_worker_provider->SetController(
-        std::move(controller), state->used_features,
+        std::move(state->controller), state->used_features,
         should_notify_controllerchange);
   }
 }

content/renderer/service_worker/service_worker_provider_context.h

@@ -104,9 +104,13 @@ class CONTENT_EXPORT ServiceWorkerProviderContext
   blink::mojom::ServiceWorkerRegistrationObjectInfoPtr
   TakeRegistrationForServiceWorkerGlobalScope();
 
-  // For service worker clients. The controller for
-  // ServiceWorkerContainer#controller.
-  ServiceWorkerHandleReference* controller();
+  // For service worker clients. Returns version id of the controller service
+  // worker object (ServiceWorkerContainer#controller).
+  int64_t GetControllerVersionId();
+
+  // For service worker clients. Takes the controller service worker object set
+  // by SetController() if any, otherwise returns nullptr.
+  std::unique_ptr<ServiceWorkerHandleReference> TakeController();
 
   // S13nServiceWorker:
   // For service worker clients. Returns URLLoaderFactory for loading

content/renderer/service_worker/service_worker_provider_context_unittest.cc

@@ -250,10 +250,10 @@ TEST_F(ServiceWorkerProviderContextTest, SetController) {
         CreateServiceWorkerRegistrationObjectInfo();
 
     // (2) In the case there are both SWProviderContext and SWProviderClient for
-    // the provider, the passed reference should be adopted and owned by the
-    // provider context. In addition, the new reference should be created for
-    // the provider client and immediately released due to limitation of the
-    // mock implementation.
+    // the provider, the passed reference should be adopted by the provider
+    // context and then be transfered ownership to the provider client, after
+    // that due to limitation of the mock implementation, the reference
+    // immediately gets released.
     mojom::ServiceWorkerContainerHostAssociatedPtrInfo host_ptr_info;
     mojom::ServiceWorkerContainerHostAssociatedRequest host_request =
         mojo::MakeRequest(&host_ptr_info);
@@ -277,11 +277,9 @@ TEST_F(ServiceWorkerProviderContextTest, SetController) {
     base::RunLoop().RunUntilIdle();
 
     EXPECT_TRUE(client->was_set_controller_called());
-    ASSERT_EQ(2UL, ipc_sink()->message_count());
-    EXPECT_EQ(ServiceWorkerHostMsg_IncrementServiceWorkerRefCount::ID,
-              ipc_sink()->GetMessageAt(0)->type());
+    ASSERT_EQ(1UL, ipc_sink()->message_count());
     EXPECT_EQ(ServiceWorkerHostMsg_DecrementServiceWorkerRefCount::ID,
-              ipc_sink()->GetMessageAt(1)->type());
+              ipc_sink()->GetMessageAt(0)->type());
   }
 }
 
@@ -310,7 +308,7 @@ TEST_F(ServiceWorkerProviderContextTest, SetController_Null) {
                                std::vector<blink::mojom::WebFeature>(), true);
   base::RunLoop().RunUntilIdle();
 
-  EXPECT_EQ(nullptr, provider_context->controller());
+  EXPECT_EQ(nullptr, provider_context->TakeController());
   EXPECT_TRUE(client->was_set_controller_called());
 }
 

content/renderer/service_worker/web_service_worker_provider_impl.cc

@@ -75,9 +75,11 @@ void WebServiceWorkerProviderImpl::SetClient(
   // for more context)
   GetDispatcher()->AddProviderClient(context_->provider_id(), client);
 
-  if (!context_->controller())
+  std::unique_ptr<ServiceWorkerHandleReference> controller =
+      context_->TakeController();
+  if (!controller)
     return;
-  SetController(context_->controller()->GetInfo(), context_->used_features(),
+  SetController(std::move(controller), context_->used_features(),
                 false /* notify_controllerchange */);
 }
 
@@ -197,23 +199,23 @@ bool WebServiceWorkerProviderImpl::ValidateScopeAndScriptURL(
 }
 
 void WebServiceWorkerProviderImpl::SetController(
-    blink::mojom::ServiceWorkerObjectInfoPtr info,
+    std::unique_ptr<ServiceWorkerHandleReference> controller,
     const std::set<uint32_t>& features,
     bool should_notify_controller_change) {
   blink::WebServiceWorkerProviderClient* provider_client =
       GetDispatcher()->GetProviderClient(context_->provider_id());
-  // The document may have been destroyed so that |provider_client| is null.
+  // The document may have been destroyed so that |provider_client| is null. In
+  // such case we just drop |controller| silently as we're just waiting to be
+  // destructed soon.
   if (!provider_client)
     return;
 
-  scoped_refptr<WebServiceWorkerImpl> controller =
-      GetDispatcher()->GetOrCreateServiceWorker(
-          ServiceWorkerHandleReference::Create(std::move(info),
-                                               thread_safe_sender_.get()));
   for (uint32_t feature : features)
     provider_client->CountFeature(feature);
-  provider_client->SetController(WebServiceWorkerImpl::CreateHandle(controller),
-                                 should_notify_controller_change);
+  provider_client->SetController(
+      WebServiceWorkerImpl::CreateHandle(
+          GetDispatcher()->GetOrCreateServiceWorker(std::move(controller))),
+      should_notify_controller_change);
 }
 
 int WebServiceWorkerProviderImpl::provider_id() const {

content/renderer/service_worker/web_service_worker_provider_impl.h

@@ -24,6 +24,7 @@ class WebServiceWorkerProviderClient;
 namespace content {
 
 class ServiceWorkerDispatcher;
+class ServiceWorkerHandleReference;
 class ServiceWorkerProviderContext;
 class ThreadSafeSender;
 
@@ -56,7 +57,7 @@ class CONTENT_EXPORT WebServiceWorkerProviderImpl
                                  blink::WebString* error_message) override;
   // Sets the ServiceWorkerContainer#controller for this provider. It's not
   // used when this WebServiceWorkerProvider is for a service worker context.
-  void SetController(blink::mojom::ServiceWorkerObjectInfoPtr info,
+  void SetController(std::unique_ptr<ServiceWorkerHandleReference> controller,
                      const std::set<uint32_t>& features,
                      bool should_notify_controller_change);
   // Posts a message to the ServiceWorkerContainer for this provider.

content/renderer/shared_worker/embedded_shared_worker_stub.cc

@@ -110,8 +110,8 @@ class WebServiceWorkerNetworkProviderForSharedWorker
   }
 
   int64_t ControllerServiceWorkerID() override {
-    if (provider_->context()->controller())
-      return provider_->context()->controller()->version_id();
+    if (provider_->context())
+      return provider_->context()->GetControllerVersionId();
     return blink::mojom::kInvalidServiceWorkerVersionId;
   }