Lacros: Move mojo handshake to earlier stage.

Lacros used to use mojo_base::mojo::Binder to establish the
Mojo connection between Lacros and Ash chromes,
preferring the plataform independent way.
However, it is too late for passing initializing parameters,
which needs to be passed before starting the main message loop.

This CL directly binds the LacrosChromeService,
so that parameters can be available at earlier timing.

Bug: 1115092
Test: Ran Lacros locally.
Change-Id: I5345fa4bfe57b9a922e2d6c389c579266893d03b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2353976
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#801311}

chrome/app/BUILD.gn

@@ -2,6 +2,7 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import("//build/config/chromeos/ui_mode.gni")
 import("//build/config/locales.gni")
 import("//build/config/ui.gni")
 import("//chrome/common/features.gni")
@@ -115,6 +116,7 @@ static_library("test_support") {
 
   deps = [
     "//base",
+    "//build:lacros_buildflags",
     "//chrome/app:shutdown_signal_handlers",
     "//chrome/browser",
     "//chrome/browser/policy:path_parser",
@@ -170,6 +172,10 @@ static_library("test_support") {
     ]
   }
 
+  if (chromeos_is_browser_only) {
+    deps += [ "//chromeos/lacros" ]
+  }
+
   if (enable_plugins && enable_nacl) {
     deps += [
       "//components/nacl/browser",

chrome/app/DEPS

@@ -11,6 +11,7 @@ include_rules = [
   "+chrome/utility/chrome_content_utility_client.h",
   "+chromeos/constants",
   "+chromeos/hugepage_text/hugepage_text.h",
+  "+chromeos/lacros",
   "+chromeos/memory",
   "+components/browser_watcher",
   "+components/component_updater",

chrome/app/chrome_main_delegate.cc

@@ -175,6 +175,11 @@
 #include "components/gwp_asan/client/gwp_asan.h"  // nogncheck
 #endif
 
+#if BUILDFLAG(IS_LACROS)
+#include "chrome/browser/lacros/lacros_chrome_service_delegate_impl.h"
+#include "chromeos/lacros/lacros_chrome_service_impl.h"
+#endif
+
 base::LazyInstance<ChromeContentGpuClient>::DestructorAtExit
     g_chrome_content_gpu_client = LAZY_INSTANCE_INITIALIZER;
 base::LazyInstance<ChromeContentRendererClient>::DestructorAtExit
@@ -548,6 +553,13 @@ void ChromeMainDelegate::PostEarlyInitialization(bool is_running_tests) {
   chromeos::InitializeFeatureListDependentDBus();
 #endif
 
+#if BUILDFLAG(IS_LACROS)
+  // LacrosChromeServiceImpl instance is needs the sequence of the main thread,
+  // and needs to be created earlier than incoming Mojo invitation handling.
+  lacros_chrome_service_ = std::make_unique<chromeos::LacrosChromeServiceImpl>(
+      std::make_unique<LacrosChromeServiceDelegateImpl>());
+#endif
+
 #if defined(OS_ANDROID)
   startup_data_->CreateProfilePrefService();
   net::NetworkChangeNotifier::SetFactory(

chrome/app/chrome_main_delegate.h

@@ -11,6 +11,7 @@
 #include "base/macros.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
+#include "build/lacros_buildflags.h"
 #include "chrome/browser/startup_data.h"
 #include "chrome/common/chrome_content_client.h"
 #include "content/public/app/content_main_delegate.h"
@@ -19,6 +20,10 @@ namespace base {
 class CommandLine;
 }
 
+namespace chromeos {
+class LacrosChromeServiceImpl;
+}
+
 namespace tracing {
 class TracingSamplerProfiler;
 }
@@ -83,6 +88,10 @@ class ChromeMainDelegate : public content::ContentMainDelegate {
   // the reporting pipeline.
   std::unique_ptr<HeapProfilerController> heap_profiler_controller_;
 
+#if BUILDFLAG(IS_LACROS)
+  std::unique_ptr<chromeos::LacrosChromeServiceImpl> lacros_chrome_service_;
+#endif
+
   DISALLOW_COPY_AND_ASSIGN(ChromeMainDelegate);
 };
 

chrome/browser/BUILD.gn

@@ -4106,8 +4106,6 @@ static_library("browser") {
   if (chromeos_is_browser_only) {
     assert(enable_native_notifications)
     sources += [
-      "chrome_browser_main_extra_parts_lacros.cc",
-      "chrome_browser_main_extra_parts_lacros.h",
       "lacros/lacros_chrome_service_delegate_impl.cc",
       "lacros/lacros_chrome_service_delegate_impl.h",
       "metrics/lacros_metrics_provider.cc",

chrome/browser/chrome_browser_main_extra_parts_lacros.cc

@@ -1,19 +0,0 @@
-// Copyright 2020 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "chrome/browser/chrome_browser_main_extra_parts_lacros.h"
-
-#include "chrome/browser/lacros/lacros_chrome_service_delegate_impl.h"
-#include "chromeos/lacros/lacros_chrome_service_impl.h"
-
-ChromeBrowserMainExtraPartsLacros::ChromeBrowserMainExtraPartsLacros() =
-    default;
-
-ChromeBrowserMainExtraPartsLacros::~ChromeBrowserMainExtraPartsLacros() =
-    default;
-
-void ChromeBrowserMainExtraPartsLacros::PostCreateThreads() {
-  lacros_chrome_service_ = std::make_unique<chromeos::LacrosChromeServiceImpl>(
-      std::make_unique<LacrosChromeServiceDelegateImpl>());
-}

chrome/browser/chrome_browser_main_extra_parts_lacros.h

@@ -1,33 +0,0 @@
-// Copyright 2020 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef CHROME_BROWSER_CHROME_BROWSER_MAIN_EXTRA_PARTS_LACROS_H_
-#define CHROME_BROWSER_CHROME_BROWSER_MAIN_EXTRA_PARTS_LACROS_H_
-
-#include <memory>
-
-#include "chrome/browser/chrome_browser_main_extra_parts.h"
-
-namespace chromeos {
-class LacrosChromeServiceImpl;
-}
-
-// Startup and shutdown code for lacros-chrome.
-class ChromeBrowserMainExtraPartsLacros : public ChromeBrowserMainExtraParts {
- public:
-  ChromeBrowserMainExtraPartsLacros();
-  ChromeBrowserMainExtraPartsLacros(const ChromeBrowserMainExtraPartsLacros&) =
-      delete;
-  ChromeBrowserMainExtraPartsLacros& operator=(
-      const ChromeBrowserMainExtraPartsLacros&) = delete;
-  ~ChromeBrowserMainExtraPartsLacros() override;
-
- private:
-  // ChromeBrowserMainExtraParts:
-  void PostCreateThreads() override;
-
-  std::unique_ptr<chromeos::LacrosChromeServiceImpl> lacros_chrome_service_;
-};
-
-#endif  // CHROME_BROWSER_CHROME_BROWSER_MAIN_EXTRA_PARTS_LACROS_H_

chrome/browser/chrome_content_browser_client.cc

@@ -632,7 +632,6 @@
 #endif
 
 #if BUILDFLAG(IS_LACROS)
-#include "chrome/browser/chrome_browser_main_extra_parts_lacros.h"
 #include "chromeos/lacros/lacros_chrome_service_impl.h"
 #endif
 
@@ -1380,10 +1379,6 @@ ChromeContentBrowserClient::CreateBrowserMainParts(
   main_parts->AddParts(std::make_unique<ChromeBrowserMainExtraPartsAsh>());
 #endif
 
-#if BUILDFLAG(IS_LACROS)
-  main_parts->AddParts(std::make_unique<ChromeBrowserMainExtraPartsLacros>());
-#endif
-
 #if defined(USE_X11) || defined(USE_OZONE)
   main_parts->AddParts(std::make_unique<ChromeBrowserMainExtraPartsOzone>());
 #endif
@@ -5816,11 +5811,11 @@ bool ChromeContentBrowserClient::IsOriginTrialRequiredForAppCache(
 }
 
 void ChromeContentBrowserClient::BindBrowserControlInterface(
-    mojo::GenericPendingReceiver receiver) {
+    mojo::ScopedMessagePipeHandle pipe) {
 #if BUILDFLAG(IS_LACROS)
-  if (auto r = receiver.As<crosapi::mojom::LacrosChromeService>()) {
-    chromeos::LacrosChromeServiceImpl::Get()->BindReceiver(std::move(r));
-  }
+  chromeos::LacrosChromeServiceImpl::Get()->BindReceiver(
+      mojo::PendingReceiver<crosapi::mojom::LacrosChromeService>(
+          std::move(pipe)));
 #endif
 }
 

chrome/browser/chrome_content_browser_client.h

@@ -683,8 +683,7 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
 
   bool IsOriginTrialRequiredForAppCache(
       content::BrowserContext* browser_context) override;
-  void BindBrowserControlInterface(
-      mojo::GenericPendingReceiver receiver) override;
+  void BindBrowserControlInterface(mojo::ScopedMessagePipeHandle pipe) override;
   bool ShouldInheritCrossOriginEmbedderPolicyImplicitly(
       const GURL& url) override;
   bool ShouldAllowInsecurePrivateNetworkRequests(

chrome/browser/chromeos/crosapi/browser_manager.cc

@@ -35,7 +35,6 @@
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/platform/platform_channel.h"
 #include "mojo/public/cpp/system/invitation.h"
-#include "mojo/public/mojom/base/binder.mojom.h"
 
 // TODO(crbug.com/1101667): Currently, this source has log spamming
 // by LOG(WARNING) for non critical errors to make it easy
@@ -223,6 +222,21 @@ bool BrowserManager::Start() {
   mojo::PlatformChannel channel;
   channel.PrepareToPassRemoteEndpoint(&options, &command_line);
 
+  // Queue messages to establish the mojo connection,
+  // so that the passed IPC is available already when lacros-chrome accepts
+  // the invitation.
+  // TODO(crbug.com/1115092): Pass the initialization parameter over
+  // mojo connection.
+  mojo::OutgoingInvitation invitation;
+  lacros_chrome_service_.Bind(
+      mojo::PendingRemote<crosapi::mojom::LacrosChromeService>(
+          invitation.AttachMessagePipe(0), /*version=*/0));
+  lacros_chrome_service_.set_disconnect_handler(base::BindOnce(
+      &BrowserManager::OnMojoDisconnected, weak_factory_.GetWeakPtr()));
+  lacros_chrome_service_->RequestAshChromeServiceReceiver(
+      base::BindOnce(&BrowserManager::OnAshChromeServiceReceiverReceived,
+                     weak_factory_.GetWeakPtr()));
+
   // Create the lacros-chrome subprocess.
   base::RecordAction(base::UserMetricsAction("Lacros.Launch"));
   // If lacros_process_ already exists, because it does not call waitpid(2),
@@ -235,22 +249,11 @@ bool BrowserManager::Start() {
   state_ = State::STARTING;
   LOG(WARNING) << "Launched lacros-chrome with pid " << lacros_process_.Pid();
 
-  // Invite the lacros-chrome to the mojo universe, and bind
-  // LacrosChromeService and AshChromeService interfaces to each other.
+  // Invite the lacros-chrome to the mojo universe.
   channel.RemoteProcessLaunchAttempted();
-  mojo::OutgoingInvitation invitation;
-  mojo::Remote<mojo_base::mojom::Binder> binder(
-      mojo::PendingRemote<mojo_base::mojom::Binder>(
-          invitation.AttachMessagePipe(0), /*version=*/0));
   mojo::OutgoingInvitation::Send(std::move(invitation),
                                  lacros_process_.Handle(),
                                  channel.TakeLocalEndpoint());
-  binder->Bind(lacros_chrome_service_.BindNewPipeAndPassReceiver());
-  lacros_chrome_service_.set_disconnect_handler(base::BindOnce(
-      &BrowserManager::OnMojoDisconnected, weak_factory_.GetWeakPtr()));
-  lacros_chrome_service_->RequestAshChromeServiceReceiver(
-      base::BindOnce(&BrowserManager::OnAshChromeServiceReceiverReceived,
-                     weak_factory_.GetWeakPtr()));
   return true;
 }
 

content/app/content_main_runner_impl.cc

@@ -90,7 +90,7 @@
 #include "mojo/public/cpp/platform/platform_channel.h"
 #include "mojo/public/cpp/system/dynamic_library_support.h"
 #include "mojo/public/cpp/system/invitation.h"
-#include "mojo/public/mojom/base/binder.mojom.h"
+#include "mojo/public/cpp/system/message_pipe.h"
 #include "ppapi/buildflags/buildflags.h"
 #include "sandbox/policy/sandbox_type.h"
 #include "sandbox/policy/switches.h"
@@ -394,24 +394,6 @@ void PreSandboxInit() {
 
 #endif  // defined(OS_LINUX) || defined(OS_CHROMEOS)
 
-class ControlInterfaceBinderImpl : public mojo_base::mojom::Binder {
- public:
-  ControlInterfaceBinderImpl() = default;
-  ~ControlInterfaceBinderImpl() override = default;
-
-  // mojo_base::mojom::Binder:
-  void Bind(mojo::GenericPendingReceiver receiver) override {
-    GetContentClient()->browser()->BindBrowserControlInterface(
-        std::move(receiver));
-  }
-};
-
-void RunControlInterfaceBinder(mojo::ScopedMessagePipeHandle pipe) {
-  mojo::MakeSelfOwnedReceiver(
-      std::make_unique<ControlInterfaceBinderImpl>(),
-      mojo::PendingReceiver<mojo_base::mojom::Binder>(std::move(pipe)));
-}
-
 }  // namespace
 
 class ContentClientCreator {
@@ -969,7 +951,8 @@ int ContentMainRunnerImpl::RunServiceManager(MainFunctionParams& main_params,
           mojo::PlatformChannel::RecoverPassedEndpointFromCommandLine(
               command_line);
       auto invitation = mojo::IncomingInvitation::Accept(std::move(endpoint));
-      RunControlInterfaceBinder(invitation.ExtractMessagePipe(0));
+      GetContentClient()->browser()->BindBrowserControlInterface(
+          invitation.ExtractMessagePipe(0));
     }
 
     download::SetIOTaskRunner(

content/browser/launch_as_mojo_client_browsertest.cc

@@ -20,7 +20,6 @@
 #include "mojo/public/cpp/bindings/remote.h"
 #include "mojo/public/cpp/platform/platform_channel.h"
 #include "mojo/public/cpp/system/invitation.h"
-#include "mojo/public/mojom/base/binder.mojom.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 #if defined(USE_OZONE)
@@ -93,15 +92,12 @@ class LaunchAsMojoClientBrowserTest : public ContentBrowserTest {
     channel.RemoteProcessLaunchAttempted();
 
     mojo::OutgoingInvitation invitation;
-    mojo::Remote<mojo_base::mojom::Binder> binder(
-        mojo::PendingRemote<mojo_base::mojom::Binder>(
+    mojo::Remote<mojom::ShellController> controller(
+        mojo::PendingRemote<mojom::ShellController>(
             invitation.AttachMessagePipe(0), /*version=*/0));
     mojo::OutgoingInvitation::Send(std::move(invitation),
                                    content_shell_process_.Handle(),
                                    channel.TakeLocalEndpoint());
-
-    mojo::Remote<mojom::ShellController> controller;
-    binder->Bind(controller.BindNewPipeAndPassReceiver());
     return controller;
   }
 

content/public/browser/content_browser_client.cc

@@ -1120,7 +1120,7 @@ bool ContentBrowserClient::IsOriginTrialRequiredForAppCache(
 }
 
 void ContentBrowserClient::BindBrowserControlInterface(
-    mojo::GenericPendingReceiver receiver) {}
+    mojo::ScopedMessagePipeHandle pipe) {}
 
 bool ContentBrowserClient::ShouldInheritCrossOriginEmbedderPolicyImplicitly(
     const GURL& url) {

content/public/browser/content_browser_client.h

@@ -41,6 +41,7 @@
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/remote.h"
+#include "mojo/public/cpp/system/message_pipe.h"
 #include "ppapi/buildflags/buildflags.h"
 #include "services/network/public/mojom/network_context.mojom-forward.h"
 #include "services/network/public/mojom/restricted_cookie_manager.mojom-forward.h"
@@ -1881,8 +1882,7 @@ class CONTENT_EXPORT ContentBrowserClient {
   // External applications and services may launch the browser in a mode which
   // exposes browser control interfaces via Mojo. Any such interface binding
   // request received from an external client is passed to this method.
-  virtual void BindBrowserControlInterface(
-      mojo::GenericPendingReceiver receiver);
+  virtual void BindBrowserControlInterface(mojo::ScopedMessagePipeHandle pipe);
 
   // Returns true when a context (e.g., iframe) whose URL is |url| should
   // inherit the parent COEP value implicitly, similar to "blob:"

content/shell/browser/shell_content_browser_client.cc

@@ -440,12 +440,10 @@ ShellContentBrowserClient::GetNetworkContextsParentDirectory() {
 }
 
 void ShellContentBrowserClient::BindBrowserControlInterface(
-    mojo::GenericPendingReceiver receiver) {
-  if (auto r = receiver.As<mojom::ShellController>()) {
-    mojo::MakeSelfOwnedReceiver(std::make_unique<ShellControllerImpl>(),
-                                std::move(r));
-    return;
-  }
+    mojo::ScopedMessagePipeHandle pipe) {
+  mojo::MakeSelfOwnedReceiver(
+      std::make_unique<ShellControllerImpl>(),
+      mojo::PendingReceiver<mojom::ShellController>(std::move(pipe)));
 }
 
 ShellBrowserContext* ShellContentBrowserClient::browser_context() {

content/shell/browser/shell_content_browser_client.h

@@ -112,8 +112,7 @@ class ShellContentBrowserClient : public ContentBrowserClient {
       network::mojom::CertVerifierCreationParams* cert_verifier_creation_params)
       override;
   std::vector<base::FilePath> GetNetworkContextsParentDirectory() override;
-  void BindBrowserControlInterface(
-      mojo::GenericPendingReceiver receiver) override;
+  void BindBrowserControlInterface(mojo::ScopedMessagePipeHandle pipe) override;
 
   ShellBrowserContext* browser_context();
   ShellBrowserContext* off_the_record_browser_context();