S13nSW: Set client id for shared workers

Store client_id supplied via ServiceWorkerContainer::SetController()
in ServiceWorkerProviderContext::ProviderStateForClient so that
we can pass the client_id to WorkerFetchContextImpl. This enables
us to set FetchEvent#clientId accordingly in shared workers.

Bug: 840668
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Ie03290b9d70c3fd0d2a74e83d7881057ec3e1b51
Reviewed-on: https://chromium-review.googlesource.com/1049210
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557802}

content/renderer/render_frame_impl.cc

@@ -3355,6 +3355,9 @@ RenderFrameImpl::CreateWorkerFetchContext() {
       provider->IsControlledByServiceWorker());
   worker_fetch_context->set_origin_url(
       GURL(frame_->GetDocument().Url()).GetOrigin());
+  if (provider_context)
+    worker_fetch_context->set_client_id(provider_context->client_id());
+
   for (auto& observer : observers_)
     observer.WillCreateWorkerFetchContext(worker_fetch_context.get());
   return std::move(worker_fetch_context);

content/renderer/service_worker/controller_service_worker_connector.cc

@@ -10,32 +10,27 @@
 
 namespace content {
 
-ControllerServiceWorkerConnector::ControllerServiceWorkerConnector(
-    mojom::ServiceWorkerContainerHost* container_host)
-    : container_host_(container_host) {}
-
 ControllerServiceWorkerConnector::ControllerServiceWorkerConnector(
     mojom::ServiceWorkerContainerHost* container_host,
     mojom::ControllerServiceWorkerPtr controller_ptr,
     const std::string& client_id)
-    : container_host_(container_host) {
-  ResetControllerConnection(std::move(controller_ptr), client_id);
+    : container_host_(container_host), client_id_(client_id) {
+  SetControllerServiceWorkerPtr(std::move(controller_ptr));
 }
 
 mojom::ControllerServiceWorker*
 ControllerServiceWorkerConnector::GetControllerServiceWorker(
     mojom::ControllerServiceWorkerPurpose purpose) {
   switch (state_) {
-    case State::kDisconnected:
+    case State::kDisconnected: {
       DCHECK(!controller_service_worker_);
       DCHECK(container_host_);
+      mojom::ControllerServiceWorkerPtr controller_ptr;
       container_host_->EnsureControllerServiceWorker(
-          mojo::MakeRequest(&controller_service_worker_), purpose);
-      controller_service_worker_.set_connection_error_handler(base::BindOnce(
-          &ControllerServiceWorkerConnector::OnControllerConnectionClosed,
-          base::Unretained(this)));
-      state_ = State::kConnected;
+          mojo::MakeRequest(&controller_ptr), purpose);
+      SetControllerServiceWorkerPtr(std::move(controller_ptr));
       return controller_service_worker_.get();
+    }
     case State::kConnected:
       DCHECK(controller_service_worker_.is_bound());
       return controller_service_worker_.get();
@@ -74,20 +69,22 @@ void ControllerServiceWorkerConnector::OnControllerConnectionClosed() {
 }
 
 void ControllerServiceWorkerConnector::ResetControllerConnection(
-    mojom::ControllerServiceWorkerPtr controller_ptr,
-    const std::string& client_id) {
+    mojom::ControllerServiceWorkerPtr controller_ptr) {
   if (state_ == State::kNoContainerHost)
     return;
+  SetControllerServiceWorkerPtr(std::move(controller_ptr));
+  if (!controller_service_worker_)
+    state_ = State::kNoController;
+}
+
+void ControllerServiceWorkerConnector::SetControllerServiceWorkerPtr(
+    mojom::ControllerServiceWorkerPtr controller_ptr) {
   controller_service_worker_ = std::move(controller_ptr);
   if (controller_service_worker_) {
-    DCHECK(client_id_.empty() || client_id_ == client_id);
-    client_id_ = client_id;
-    state_ = State::kConnected;
     controller_service_worker_.set_connection_error_handler(base::BindOnce(
         &ControllerServiceWorkerConnector::OnControllerConnectionClosed,
         base::Unretained(this)));
-  } else {
-    state_ = State::kNoController;
+    state_ = State::kConnected;
   }
 }
 

content/renderer/service_worker/controller_service_worker_connector.h

@@ -51,15 +51,10 @@ class CONTENT_EXPORT ControllerServiceWorkerConnector
     kNoContainerHost,
   };
 
-  // Use this ctor when no |controller_ptr| is available at creation time.
-  // |state_| is set to kDisconnected.
-  // TODO(bashi): Remove this one constructor. We need to update
-  // WorkerFetchContextImpl to remove this constructor.
-  explicit ControllerServiceWorkerConnector(
-      mojom::ServiceWorkerContainerHost* container_host);
-
-  // Use this ctor when |controller_ptr| is given by the browser at the
-  // creation time. |state_| is set to either kConnected or kNoController.
+  // This class should only be created if a controller exists for the client.
+  // |controller_ptr| may be nullptr if the caller does not yet have a Mojo
+  // connection to the controller. |state_| is set to kDisconnected in that
+  // case.
   ControllerServiceWorkerConnector(
       mojom::ServiceWorkerContainerHost* container_host,
       mojom::ControllerServiceWorkerPtr controller_ptr,
@@ -79,14 +74,16 @@ class CONTENT_EXPORT ControllerServiceWorkerConnector
   // Resets the controller connection with the given |controller_ptr|, this
   // can be called when a new controller is given, e.g. due to claim().
   void ResetControllerConnection(
-      mojom::ControllerServiceWorkerPtr controller_ptr,
-      const std::string& client_id);
+      mojom::ControllerServiceWorkerPtr controller_ptr);
 
   State state() const { return state_; }
 
   const std::string& client_id() const { return client_id_; }
 
  private:
+  void SetControllerServiceWorkerPtr(
+      mojom::ControllerServiceWorkerPtr controller_ptr);
+
   State state_ = State::kDisconnected;
 
   friend class base::RefCounted<ControllerServiceWorkerConnector>;

content/renderer/service_worker/service_worker_provider_context.cc

@@ -54,6 +54,10 @@ struct ServiceWorkerProviderContext::ProviderStateForClient {
   // Used when we create |subresource_loader_factory|.
   scoped_refptr<network::SharedURLLoaderFactory> fallback_loader_factory;
 
+  // S13nServiceWorker:
+  // The Client#id value of the client.
+  std::string client_id;
+
   // Tracks feature usage for UseCounter.
   std::set<blink::mojom::WebFeature> used_features;
 
@@ -209,6 +213,11 @@ ServiceWorkerProviderContext::used_features() const {
   return state_for_client_->used_features;
 }
 
+const std::string& ServiceWorkerProviderContext::client_id() const {
+  DCHECK(state_for_client_);
+  return state_for_client_->client_id;
+}
+
 void ServiceWorkerProviderContext::SetWebServiceWorkerProvider(
     base::WeakPtr<WebServiceWorkerProviderImpl> provider) {
   DCHECK(state_for_client_);
@@ -298,6 +307,10 @@ void ServiceWorkerProviderContext::SetController(
   state->controller_version_id =
       state->controller ? state->controller->version_id
                         : blink::mojom::kInvalidServiceWorkerVersionId;
+  // The client id should never change once set.
+  DCHECK(state->client_id.empty() ||
+         state->client_id == controller_info->client_id);
+  state->client_id = controller_info->client_id;
 
   // Propagate the controller to workers related to this provider.
   if (state->controller) {
@@ -334,8 +347,7 @@ void ServiceWorkerProviderContext::SetController(
       // factory (this part is inherently racy).
       state->controller_connector->ResetControllerConnection(
           mojom::ControllerServiceWorkerPtr(
-              std::move(controller_info->endpoint)),
-          controller_info->client_id);
+              std::move(controller_info->endpoint)));
     } else if (state->controller) {
       // Case (C): never had a controller, but got a new one now.
       // Set a new |state->controller_connector| so that subsequent resource

content/renderer/service_worker/service_worker_provider_context.h

@@ -127,6 +127,10 @@ class CONTENT_EXPORT ServiceWorkerProviderContext
   // For service worker clients. Returns the feature usage of its controller.
   const std::set<blink::mojom::WebFeature>& used_features() const;
 
+  // S13nServiceWorker:
+  // The Client#id value of the client.
+  const std::string& client_id() const;
+
   // For service worker clients. Sets a weak pointer back to the
   // WebServiceWorkerProviderImpl (which corresponds to ServiceWorkerContainer
   // in JavaScript) which has a strong reference to |this|. This allows us to

content/renderer/service_worker/service_worker_subresource_loader_unittest.cc

@@ -362,7 +362,7 @@ class ServiceWorkerSubresourceLoaderTest : public ::testing::Test {
   network::mojom::URLLoaderFactoryPtr CreateSubresourceLoaderFactory() {
     if (!connector_) {
       connector_ = base::MakeRefCounted<ControllerServiceWorkerConnector>(
-          &fake_container_host_);
+          &fake_container_host_, nullptr /*controller_ptr*/, "" /*client_id*/);
     }
     network::mojom::URLLoaderFactoryPtr service_worker_url_loader_factory;
     ServiceWorkerSubresourceLoaderFactory::Create(
@@ -570,7 +570,7 @@ TEST_F(ServiceWorkerSubresourceLoaderTest, NoController) {
   }
 
   // Make the connector have no controller.
-  connector_->ResetControllerConnection(nullptr, "" /*client_id*/);
+  connector_->ResetControllerConnection(nullptr);
   base::RunLoop().RunUntilIdle();
 
   {

content/renderer/service_worker/worker_fetch_context_impl.cc

@@ -313,6 +313,10 @@ void WorkerFetchContextImpl::set_origin_url(const GURL& origin_url) {
   origin_url_ = origin_url;
 }
 
+void WorkerFetchContextImpl::set_client_id(const std::string& client_id) {
+  client_id_ = client_id;
+}
+
 void WorkerFetchContextImpl::SetApplicationCacheHostID(int id) {
   appcache_host_id_ = id;
 }
@@ -344,7 +348,8 @@ void WorkerFetchContextImpl::ResetServiceWorkerURLLoaderFactory() {
   network::mojom::URLLoaderFactoryPtr service_worker_url_loader_factory;
   ServiceWorkerSubresourceLoaderFactory::Create(
       base::MakeRefCounted<ControllerServiceWorkerConnector>(
-          service_worker_container_host_.get()),
+          service_worker_container_host_.get(), nullptr /*controller_ptr*/,
+          client_id_),
       fallback_factory_, mojo::MakeRequest(&service_worker_url_loader_factory));
   url_loader_factory_->SetServiceWorkerURLLoaderFactory(
       std::move(service_worker_url_loader_factory));

content/renderer/service_worker/worker_fetch_context_impl.h

@@ -102,6 +102,7 @@ class CONTENT_EXPORT WorkerFetchContextImpl
   // https://w3c.github.io/webappsec-secure-contexts/
   void set_is_secure_context(bool flag);
   void set_origin_url(const GURL& origin_url);
+  void set_client_id(const std::string& client_id);
 
   using RewriteURLFunction = blink::WebURL (*)(const std::string&, bool);
   static void InstallRewriteURLFunction(RewriteURLFunction rewrite_url);
@@ -137,6 +138,12 @@ class CONTENT_EXPORT WorkerFetchContextImpl
   // Initialized on the worker thread when InitializeOnWorkerThread() is called.
   mojom::ServiceWorkerContainerHostPtr service_worker_container_host_;
 
+  // S13nServiceWorker:
+  // The Client#id value of the shared worker or dedicated worker (since
+  // dedicated workers are not yet service worker clients, it is the parent
+  // document's id in that case). Passed to ControllerServiceWorkerConnector.
+  std::string client_id_;
+
   // Initialized on the worker thread when InitializeOnWorkerThread() is called.
   std::unique_ptr<ResourceDispatcher> resource_dispatcher_;
 

content/renderer/shared_worker/embedded_shared_worker_stub.cc

@@ -358,6 +358,7 @@ EmbeddedSharedWorkerStub::CreateWorkerFetchContext(
     worker_fetch_context->set_is_controlled_by_service_worker(
         web_network_provider->HasControllerServiceWorker());
   }
+  worker_fetch_context->set_client_id(context->client_id());
   return std::move(worker_fetch_context);
 }
 

third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService

@@ -8,7 +8,6 @@ Bug(none) external/wpt/content-security-policy/inside-worker/dedicated-script.ht
 Bug(none) external/wpt/content-security-policy/inside-worker/dedicated-inheritance.html [ Failure ]
 Bug(none) external/wpt/html/browsers/offline/appcache/workers/appcache-worker.html [ Timeout ]
 crbug.com/829417 external/wpt/html/browsers/offline/appcache/workers/appcache-worker.https.html [ Timeout ]
-Bug(none) external/wpt/service-workers/service-worker/clients-get-client-types.https.html [ Failure ]
 Bug(none) external/wpt/service-workers/service-worker/fetch-request-xhr-sync-on-worker.https.html [ Timeout ]
 crbug.com/771118 external/wpt/service-workers/service-worker/mime-sniffing.https.html [ Failure ]
 Bug(none) http/tests/appcache/top-frame-3.html [ Pass Timeout ]