Make EmbeddedWorkerInstanceClient independent from Renderer

This is practically a revert of https://crrev.com/c/1053042.
The previous change was made to ensure user agent is set for
RenderThreadImpl before creating service workers. However,
service worker code path doesn't rely on RenderThreadImpl's
user agent. User agent is passed as a part of
EmbeddedWorkerStartParams. We don't need to use mojo::Renderer
interface to create EmbeddedWorkerInstanceClient.

This change allows us to choose the thread to create
EmbeddedWorkerInstanceClient, which is a requirement for
off-the-main-thread service worker startup.

Bug: 692909
Change-Id: I1bdfa22e588193f184eb76c4a93b3912f92ec157
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1730889
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#687528}

content/browser/service_worker/embedded_worker_instance.cc

@@ -28,7 +28,6 @@
 #include "content/browser/service_worker/service_worker_script_loader_factory.h"
 #include "content/browser/url_loader_factory_getter.h"
 #include "content/common/content_switches_internal.h"
-#include "content/common/renderer.mojom.h"
 #include "content/common/url_schemes.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
@@ -189,8 +188,7 @@ void SetupOnUIThread(int embedded_worker_id,
   // the process. If the process dies, |client_|'s connection error callback
   // will be called on the IO thread.
   if (request.is_pending()) {
-    rph->GetRendererInterface()->SetUpEmbeddedWorkerChannelForServiceWorker(
-        std::move(request));
+    BindInterface(rph, std::move(request));
   }
 
   // Register to DevTools and update params accordingly.

content/browser/service_worker/embedded_worker_instance.h

@@ -128,10 +128,11 @@ class CONTENT_EXPORT EmbeddedWorkerInstance
   // Starts the worker. It is invalid to call this when the worker is not in
   // STOPPED status.
   //
-  // |sent_start_callback| is invoked once the Start IPC is sent, or if an error
-  // prevented that from happening. The callback is not invoked in some cases,
-  // e.g., when Stop() is called and aborts the start procedure. Note that when
-  // the callback is invoked with kOk status, the service worker has not yet
+  // |sent_start_callback| is invoked once the Start IPC is sent, and in some
+  // cases may be invoked if an error prevented that from happening. It's not
+  // invoked in some cases, like if the Mojo connection fails to connect, or
+  // when Stop() is called and aborts the start procedure. Note that when the
+  // callback is invoked with kOk status, the service worker has not yet
   // finished starting. Observe OnStarted()/OnStopped() for when start completed
   // or failed.
   void Start(blink::mojom::EmbeddedWorkerStartParamsPtr params,

content/browser/service_worker/embedded_worker_instance_unittest.cc

@@ -553,25 +553,15 @@ TEST_F(EmbeddedWorkerInstanceTest, FailToSendStartIPC) {
   auto worker = std::make_unique<EmbeddedWorkerInstance>(pair.second.get());
   worker->AddObserver(this);
 
-  // Attempt to start the worker. From the browser process's point of view, the
-  // start IPC was sent.
-  base::Optional<blink::ServiceWorkerStatusCode> status;
-  base::RunLoop loop;
-  worker->Start(CreateStartParams(pair.second),
-                ReceiveStatus(&status, loop.QuitClosure()));
-  loop.Run();
-  EXPECT_EQ(blink::ServiceWorkerStatusCode::kOk, status.value());
-
-  // But the renderer should not receive the message and the binding is broken.
-  // Worker should handle the failure of binding on the remote side as detach.
+  // Attempt to start the worker. Pass DoNothing() as the |sent_start_callback|
+  // as it won't be called when mojo IPC fails to connect.
+  worker->Start(CreateStartParams(pair.second), base::DoNothing());
   base::RunLoop().RunUntilIdle();
 
-  ASSERT_EQ(3u, events_.size());
-  EXPECT_EQ(PROCESS_ALLOCATED, events_[0].type);
-  EXPECT_EQ(START_WORKER_MESSAGE_SENT, events_[1].type);
-  EXPECT_EQ(DETACHED, events_[2].type);
-  EXPECT_EQ(EmbeddedWorkerStatus::STARTING, events_[2].status.value());
-  EXPECT_EQ(EmbeddedWorkerStatus::STOPPED, worker->status());
+  // Worker should handle the failure of binding on the remote side as detach.
+  ASSERT_EQ(1u, events_.size());
+  EXPECT_EQ(DETACHED, events_[0].type);
+  EXPECT_EQ(EmbeddedWorkerStatus::STARTING, events_[0].status.value());
 }
 
 TEST_F(EmbeddedWorkerInstanceTest, RemoveRemoteInterface) {

content/browser/service_worker/embedded_worker_test_helper.cc

@@ -13,7 +13,6 @@
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/browser/service_worker/service_worker_test_utils.h"
-#include "content/common/renderer.mojom.h"
 #include "content/public/test/mock_render_process_host.h"
 #include "content/public/test/test_browser_context.h"
 #include "content/test/fake_network_url_loader_factory.h"
@@ -24,75 +23,6 @@
 
 namespace content {
 
-class EmbeddedWorkerTestHelper::MockRendererInterface : public mojom::Renderer {
- public:
-  // |helper| must outlive this.
-  explicit MockRendererInterface(EmbeddedWorkerTestHelper* helper)
-      : helper_(helper) {}
-
-  void AddBinding(mojom::RendererAssociatedRequest request) {
-    bindings_.AddBinding(this, std::move(request));
-  }
-
- private:
-  void CreateView(mojom::CreateViewParamsPtr) override { NOTREACHED(); }
-  void DestroyView(int32_t) override { NOTREACHED(); }
-  void CreateFrame(mojom::CreateFrameParamsPtr) override { NOTREACHED(); }
-  void SetUpEmbeddedWorkerChannelForServiceWorker(
-      blink::mojom::EmbeddedWorkerInstanceClientRequest client_request)
-      override {
-    helper_->OnInstanceClientRequest(std::move(client_request));
-  }
-  void CreateFrameProxy(
-      int32_t routing_id,
-      int32_t render_view_routing_id,
-      int32_t opener_routing_id,
-      int32_t parent_routing_id,
-      const FrameReplicationState& replicated_state,
-      const base::UnguessableToken& devtools_frame_token) override {
-    NOTREACHED();
-  }
-  void OnNetworkConnectionChanged(
-      net::NetworkChangeNotifier::ConnectionType type,
-      double max_bandwidth_mbps) override {
-    NOTREACHED();
-  }
-  void OnNetworkQualityChanged(net::EffectiveConnectionType type,
-                               base::TimeDelta http_rtt,
-                               base::TimeDelta transport_rtt,
-                               double bandwidth_kbps) override {
-    NOTREACHED();
-  }
-  void SetWebKitSharedTimersSuspended(bool suspend) override { NOTREACHED(); }
-  void SetUserAgent(const std::string& user_agent) override { NOTREACHED(); }
-  void SetUserAgentMetadata(const blink::UserAgentMetadata& metadata) override {
-    NOTREACHED();
-  }
-  void UpdateScrollbarTheme(
-      mojom::UpdateScrollbarThemeParamsPtr params) override {
-    NOTREACHED();
-  }
-  void OnSystemColorsChanged(int32_t aqua_color_variant,
-                             const std::string& highlight_text_color,
-                             const std::string& highlight_color) override {
-    NOTREACHED();
-  }
-  void UpdateSystemColorInfo(
-      mojom::UpdateSystemColorInfoParamsPtr params) override {
-    NOTREACHED();
-  }
-  void PurgePluginListCache(bool reload_pages) override { NOTREACHED(); }
-  void SetProcessState(mojom::RenderProcessState process_state) override {
-    NOTREACHED();
-  }
-  void SetSchedulerKeepActive(bool keep_active) override { NOTREACHED(); }
-  void SetIsLockedToSite() override { NOTREACHED(); }
-  void EnableV8LowMemoryMode() override { NOTREACHED(); }
-
-  EmbeddedWorkerTestHelper* helper_;
-  mojo::AssociatedBindingSet<mojom::Renderer> bindings_;
-};
-
 EmbeddedWorkerTestHelper::EmbeddedWorkerTestHelper(
     const base::FilePath& user_data_directory)
     : browser_context_(std::make_unique<TestBrowserContext>()),
@@ -120,25 +50,14 @@ EmbeddedWorkerTestHelper::EmbeddedWorkerTestHelper(
   wrapper_->process_manager()->SetNewProcessIdForTest(new_render_process_id());
   wrapper_->InitializeResourceContext(browser_context_->GetResourceContext());
 
-  // Install a mocked mojom::Renderer interface to catch requests to
-  // establish Mojo connection for EWInstanceClient.
-  mock_renderer_interface_ = std::make_unique<MockRendererInterface>(this);
-
-  auto renderer_interface_ptr =
-      std::make_unique<mojom::RendererAssociatedPtr>();
-  mock_renderer_interface_->AddBinding(
-      mojo::MakeRequestAssociatedWithDedicatedPipe(
-          renderer_interface_ptr.get()));
-  render_process_host_->OverrideRendererInterfaceForTesting(
-      std::move(renderer_interface_ptr));
-
-  auto new_renderer_interface_ptr =
-      std::make_unique<mojom::RendererAssociatedPtr>();
-  mock_renderer_interface_->AddBinding(
-      mojo::MakeRequestAssociatedWithDedicatedPipe(
-          new_renderer_interface_ptr.get()));
-  new_render_process_host_->OverrideRendererInterfaceForTesting(
-      std::move(new_renderer_interface_ptr));
+  render_process_host_->OverrideBinderForTesting(
+      blink::mojom::EmbeddedWorkerInstanceClient::Name_,
+      base::BindRepeating(&EmbeddedWorkerTestHelper::OnInstanceClientRequest,
+                          base::Unretained(this)));
+  new_render_process_host_->OverrideBinderForTesting(
+      blink::mojom::EmbeddedWorkerInstanceClient::Name_,
+      base::BindRepeating(&EmbeddedWorkerTestHelper::OnInstanceClientRequest,
+                          base::Unretained(this)));
 
   default_network_loader_factory_ =
       std::make_unique<FakeNetworkURLLoaderFactory>();
@@ -170,7 +89,9 @@ void EmbeddedWorkerTestHelper::AddPendingServiceWorker(
 }
 
 void EmbeddedWorkerTestHelper::OnInstanceClientRequest(
-    blink::mojom::EmbeddedWorkerInstanceClientRequest request) {
+    mojo::ScopedMessagePipeHandle request_handle) {
+  blink::mojom::EmbeddedWorkerInstanceClientRequest request(
+      std::move(request_handle));
   std::unique_ptr<FakeEmbeddedWorkerInstanceClient> client;
   if (!pending_embedded_worker_instance_clients_.empty()) {
     // Use the instance client that was registered for this message.

content/browser/service_worker/embedded_worker_test_helper.h

@@ -131,10 +131,9 @@ class EmbeddedWorkerTestHelper {
   // The following are exposed to public so the fake embedded worker and service
   // worker implementations and their subclasses can call them.
 
-  // Called when |request| is received. It takes the object from a previous
-  // AddPending*() call if any and calls Create*() otherwise.
-  void OnInstanceClientRequest(
-      blink::mojom::EmbeddedWorkerInstanceClientRequest request);
+  // Called when |request_handle| is received. It takes the object from a
+  // previous AddPending*() call if any and calls Create*() otherwise.
+  void OnInstanceClientRequest(mojo::ScopedMessagePipeHandle request_handle);
   void OnServiceWorkerRequest(blink::mojom::ServiceWorkerRequest request);
 
   // Called by the fakes to destroy themselves.
@@ -155,16 +154,12 @@ class EmbeddedWorkerTestHelper {
   virtual std::unique_ptr<FakeServiceWorker> CreateServiceWorker();
 
  private:
-  class MockRendererInterface;
-
   std::unique_ptr<TestBrowserContext> browser_context_;
   std::unique_ptr<MockRenderProcessHost> render_process_host_;
   std::unique_ptr<MockRenderProcessHost> new_render_process_host_;
 
   scoped_refptr<ServiceWorkerContextWrapper> wrapper_;
 
-  std::unique_ptr<MockRendererInterface> mock_renderer_interface_;
-
   base::queue<std::unique_ptr<FakeEmbeddedWorkerInstanceClient>>
       pending_embedded_worker_instance_clients_;
   base::flat_set<std::unique_ptr<FakeEmbeddedWorkerInstanceClient>,

content/common/renderer.mojom

@@ -11,8 +11,8 @@ import "mojo/public/mojom/base/generic_pending_receiver.mojom";
 import "mojo/public/mojom/base/time.mojom";
 import "mojo/public/mojom/base/unguessable_token.mojom";
 import "services/network/public/mojom/network_types.mojom";
+import "third_party/blink/public/mojom/manifest/manifest.mojom";
 import "third_party/blink/public/mojom/renderer_preferences.mojom";
-import "third_party/blink/public/mojom/service_worker/embedded_worker.mojom";
 import "third_party/blink/public/mojom/user_agent/user_agent_metadata.mojom";
 import "ui/gfx/geometry/mojom/geometry.mojom";
 
@@ -205,13 +205,6 @@ interface Renderer {
                    FrameReplicationState replication_state,
                    mojo_base.mojom.UnguessableToken devtools_frame_token);
 
-  // Tells the renderer to create an EmbeddedWorkerInstanceClient, which is what
-  // manages service worker startup and shutdown.
-  // TODO(shimazu): Send all params for starting service worker to reduce the
-  // number of IPCs.
-  SetUpEmbeddedWorkerChannelForServiceWorker(
-      blink.mojom.EmbeddedWorkerInstanceClient& client_request);
-
   // Tells the renderer that the network type has changed so that
   // navigator.onLine and navigator.connection can be updated.
   OnNetworkConnectionChanged(NetworkConnectionType connection_type,

content/public/app/content_renderer_manifest.cc

@@ -21,6 +21,7 @@ const service_manager::Manifest& GetContentRendererManifest() {
               std::set<const char*>{
                   "blink.mojom.CodeCacheHost",
                   "blink.mojom.CrashMemoryMetricsReporter",
+                  "blink.mojom.EmbeddedWorkerInstanceClient",
                   "blink.mojom.LeakDetector",
                   "blink.mojom.OomIntervention",
                   "blink.mojom.SharedWorkerFactory",

content/renderer/render_thread_impl.cc

@@ -796,6 +796,12 @@ void RenderThreadImpl::Init() {
   registry->AddInterface(base::BindRepeating(CreateResourceUsageReporter,
                                              weak_factory_.GetWeakPtr()),
                          base::ThreadTaskRunnerHandle::Get());
+  // TODO(bashi): Use the IO task runner to start service worker on the IO
+  // thread.
+  registry->AddInterface(
+      base::BindRepeating(&EmbeddedWorkerInstanceClientImpl::Create,
+                          GetWebMainThreadScheduler()->DefaultTaskRunner()),
+      GetWebMainThreadScheduler()->DefaultTaskRunner());
 
   GetServiceManagerConnection()->AddConnectionFilter(
       std::make_unique<SimpleConnectionFilter>(std::move(registry)));
@@ -2089,13 +2095,6 @@ void RenderThreadImpl::CreateFrameProxy(
       replicated_state, devtools_frame_token);
 }
 
-void RenderThreadImpl::SetUpEmbeddedWorkerChannelForServiceWorker(
-    blink::mojom::EmbeddedWorkerInstanceClientRequest client_request) {
-  EmbeddedWorkerInstanceClientImpl::Create(
-      std::move(client_request),
-      GetWebMainThreadScheduler()->DefaultTaskRunner());
-}
-
 void RenderThreadImpl::OnNetworkConnectionChanged(
     net::NetworkChangeNotifier::ConnectionType type,
     double max_bandwidth_mbps) {

content/renderer/render_thread_impl.h

@@ -516,9 +516,6 @@ class CONTENT_EXPORT RenderThreadImpl
       int32_t parent_routing_id,
       const FrameReplicationState& replicated_state,
       const base::UnguessableToken& devtools_frame_token) override;
-  void SetUpEmbeddedWorkerChannelForServiceWorker(
-      blink::mojom::EmbeddedWorkerInstanceClientRequest client_request)
-      override;
   void OnNetworkConnectionChanged(
       net::NetworkChangeNotifier::ConnectionType type,
       double max_bandwidth_mbps) override;

content/renderer/service_worker/embedded_worker_instance_client_impl.cc

@@ -27,8 +27,8 @@ namespace content {
 
 // static
 void EmbeddedWorkerInstanceClientImpl::Create(
-    blink::mojom::EmbeddedWorkerInstanceClientRequest request,
-    scoped_refptr<base::SingleThreadTaskRunner> initiator_thread_task_runner) {
+    scoped_refptr<base::SingleThreadTaskRunner> initiator_thread_task_runner,
+    blink::mojom::EmbeddedWorkerInstanceClientRequest request) {
   // This won't be leaked because the lifetime will be managed internally.
   // See the class documentation for detail.
   // We can't use MakeStrongBinding because must give the worker thread

content/renderer/service_worker/embedded_worker_instance_client_impl.h

@@ -45,8 +45,8 @@ class CONTENT_EXPORT EmbeddedWorkerInstanceClientImpl
   // TODO(shimazu): Create a service worker's execution context by this method
   // instead of just creating an instance of EmbeddedWorkerInstanceClient.
   static void Create(
-      blink::mojom::EmbeddedWorkerInstanceClientRequest request,
-      scoped_refptr<base::SingleThreadTaskRunner> initiator_task_runner);
+      scoped_refptr<base::SingleThreadTaskRunner> initiator_task_runner,
+      blink::mojom::EmbeddedWorkerInstanceClientRequest request);
 
   ~EmbeddedWorkerInstanceClientImpl() override;