Force-disable MojoIpcz in remoting binaries

These binaries don't yet work with MojoIpcz enabled. This CL introduces
a way for Mojo consumers to override the MojoIpcz feature setting to
forcibly disable it, and this is used by all remoting binaries to
disable ipcz in preparation for MojoIpcz being enabled by default soon.

In addition, most remoting tests which rely on intraprocess isolated
connections have been adapted to use message pipes directly (i.e. no
separate invitation or connection step) since MojoIpcz will not support
self-connection.

Finally, these changes also impact some components_unittests,
requiring a small refactor of how Mojo is initialized in that
test suite as well as in some underlying Content test suite
helpers.

Bug: 1299283,1378803
Change-Id: I4c4af60e270b75040bb8da57e809a0d606748527
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4241455
Reviewed-by: Yuwei Huang <yuweih@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1105220}

chrome/browser/feed/rss_links_fetcher_unittest.cc

@@ -8,7 +8,6 @@
 #include "base/test/task_environment.h"
 #include "components/feed/core/v2/test/callback_receiver.h"
 #include "components/feed/mojom/rss_link_reader.mojom.h"
-#include "mojo/core/embedder/embedder.h"
 #include "mojo/public/cpp/bindings/receiver.h"
 #include "services/service_manager/public/cpp/interface_provider.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -66,7 +65,6 @@ class StubRssLinkReader : public mojom::RssLinkReader {
 class RssLinksFetcherUnitTest : public ::testing::Test {
  public:
   RssLinksFetcherUnitTest() = default;
-  void SetUp() override { mojo::core::Init(); }
 
  protected:
   base::test::TaskEnvironment task_env_;

components/named_mojo_ipc_server/BUILD.gn

@@ -74,6 +74,7 @@ source_set("unit_tests") {
     ":testing_mojom",
     "//base",
     "//base/test:test_support",
+    "//components/test:test_support",
     "//mojo/public/cpp/bindings",
     "//mojo/public/cpp/platform",
     "//mojo/public/cpp/system",

components/named_mojo_ipc_server/DEPS

@@ -1,4 +1,5 @@
 include_rules = [
+  "+components/test",
   "+mojo/public",
   "+mojo/core/embedder",
 ]

components/named_mojo_ipc_server/named_mojo_ipc_server.cc

@@ -136,21 +136,6 @@ void NamedMojoIpcServerBase::OnClientConnected(
     return;
   }
 
-  if (!options_.message_pipe_id.has_value()) {
-    // Create isolated connection.
-    auto connection = std::make_unique<mojo::IsolatedConnection>();
-    mojo::ScopedMessagePipeHandle message_pipe =
-        connection->Connect(std::move(endpoint));
-    mojo::ReceiverId receiver_id =
-        TrackMessagePipe(std::move(message_pipe), impl, peer_pid);
-    active_connections_[receiver_id] = std::move(connection);
-    return;
-  }
-
-  // Create non-isolated connection.
-  mojo::OutgoingInvitation invitation;
-  mojo::ScopedMessagePipeHandle message_pipe =
-      invitation.AttachMessagePipe(*options_.message_pipe_id);
 #if BUILDFLAG(IS_WIN)
   // Open process with minimum permissions since the client process might have
   // restricted its access with DACL.
@@ -168,6 +153,22 @@ void NamedMojoIpcServerBase::OnClientConnected(
     return;
   }
 #undef INVALID_PROCESS_LOG
+
+  if (!options_.message_pipe_id.has_value()) {
+    // Create isolated connection.
+    auto connection = std::make_unique<mojo::IsolatedConnection>();
+    mojo::ScopedMessagePipeHandle message_pipe =
+        connection->Connect(std::move(endpoint), std::move(peer_process));
+    mojo::ReceiverId receiver_id =
+        TrackMessagePipe(std::move(message_pipe), impl, peer_pid);
+    active_connections_[receiver_id] = std::move(connection);
+    return;
+  }
+
+  // Create non-isolated connection.
+  mojo::OutgoingInvitation invitation;
+  mojo::ScopedMessagePipeHandle message_pipe =
+      invitation.AttachMessagePipe(*options_.message_pipe_id);
   mojo::OutgoingInvitation::Send(std::move(invitation), peer_process.Handle(),
                                  std::move(endpoint));
   TrackMessagePipe(std::move(message_pipe), impl, peer_pid);

components/named_mojo_ipc_server/named_mojo_ipc_server_unittest.cc

@@ -29,6 +29,7 @@
 #include "components/named_mojo_ipc_server/named_mojo_ipc_server_client_util.h"
 #include "components/named_mojo_ipc_server/named_mojo_ipc_test_util.h"
 #include "components/named_mojo_ipc_server/testing.test-mojom.h"
+#include "components/test/test_switches.h"
 #include "mojo/core/embedder/scoped_ipc_support.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/receiver_set.h"
@@ -170,6 +171,10 @@ base::Process NamedMojoIpcServerTest::LaunchClientProcess(
   }
   if (GetParam()) {
     cmd_line.AppendSwitch(kClientProcessUseIsolatedConnectionSwitch);
+
+    // Make sure the new process is a broker, because isolated connections are
+    // only supported between two brokers.
+    cmd_line.AppendSwitch(switches::kInitializeMojoAsBroker);
   }
   return base::SpawnMultiProcessTestChild(kEchoClientName, cmd_line,
                                           /* options= */ {});

components/test/BUILD.gn

@@ -11,6 +11,8 @@ source_set("test_support") {
   sources = [
     "components_test_suite.cc",
     "components_test_suite.h",
+    "test_switches.cc",
+    "test_switches.h",
   ]
 
   deps = [

components/test/components_test_suite.cc

@@ -7,6 +7,7 @@
 #include <memory>
 #include <utility>
 
+#include "base/base_switches.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/functional/bind.h"
@@ -18,6 +19,7 @@
 #include "components/breadcrumbs/core/breadcrumb_manager.h"
 #include "components/breadcrumbs/core/crash_reporter_breadcrumb_observer.h"
 #include "components/content_settings/core/common/content_settings_pattern.h"
+#include "components/test/test_switches.h"
 #include "mojo/core/embedder/embedder.h"
 #include "services/network/public/cpp/features.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -53,8 +55,6 @@ class ComponentsTestSuite : public base::TestSuite {
   void Initialize() override {
     base::TestSuite::Initialize();
 
-    mojo::core::Init();
-
     // These schemes need to be added globally to pass tests of
     // autocomplete_input_unittest.cc and content_settings_pattern*
     // TODO(https://crbug.com/1047702): Move this scheme initialization into the
@@ -139,13 +139,27 @@ class ComponentsUnitTestEventListener : public testing::EmptyTestEventListener {
 }  // namespace
 
 base::RunTestSuiteCallback GetLaunchCallback(int argc, char** argv) {
+  auto components_test_suite =
+      std::make_unique<ComponentsTestSuite>(argc, argv);
+
+  // In the main test process, Mojo must be initialized as a broker. By
+  // default child processes are initialized as non-brokers, but tests may
+  // override this by passing kInitializeMojoAsBroker when launching children.
+  const auto& cmd = *base::CommandLine::ForCurrentProcess();
+  const bool is_test_child = cmd.HasSwitch(switches::kTestChildProcess);
+  const bool force_broker = mojo::core::IsMojoIpczEnabled() &&
+                            cmd.HasSwitch(switches::kInitializeMojoAsBroker);
+  const mojo::core::Configuration mojo_config{
+      .is_broker_process = !is_test_child || force_broker,
+  };
+
 #if !BUILDFLAG(IS_IOS)
   auto test_suite = std::make_unique<content::UnitTestTestSuite>(
-      new ComponentsTestSuite(argc, argv),
-      base::BindRepeating(
-          content::UnitTestTestSuite::CreateTestContentClients));
+      components_test_suite.release(),
+      base::BindRepeating(content::UnitTestTestSuite::CreateTestContentClients),
+      mojo_config);
 #else
-  auto test_suite = std::make_unique<ComponentsTestSuite>(argc, argv);
+  mojo::core::Init(mojo_config);
 #endif
 
   testing::TestEventListeners& listeners =
@@ -156,6 +170,7 @@ base::RunTestSuiteCallback GetLaunchCallback(int argc, char** argv) {
   return base::BindOnce(&content::UnitTestTestSuite::Run,
                         std::move(test_suite));
 #else
-  return base::BindOnce(&base::TestSuite::Run, std::move(test_suite));
+  return base::BindOnce(&base::TestSuite::Run,
+                        std::move(components_test_suite));
 #endif
 }

components/test/test_switches.cc

@@ -0,0 +1,13 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/test/test_switches.h"
+
+namespace switches {
+
+// Used by some tests to force Mojo broker initialization in a spawned child
+// process.
+extern const char kInitializeMojoAsBroker[] = "initialize-mojo-as-broker";
+
+}  // namespace switches

components/test/test_switches.h

@@ -0,0 +1,14 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_TEST_TEST_SWITCHES_H_
+#define COMPONENTS_TEST_TEST_SWITCHES_H_
+
+namespace switches {
+
+extern const char kInitializeMojoAsBroker[];
+
+}  // namespace switches
+
+#endif  // COMPONENTS_TEST_TEST_SWITCHES_H_

content/public/test/blink_test_environment.cc

@@ -17,6 +17,7 @@
 #include "content/public/test/content_test_suite_base.h"
 #include "content/public/test/test_content_client_initializer.h"
 #include "content/test/test_blink_web_unit_test_support.h"
+#include "mojo/core/embedder/embedder.h"
 #include "third_party/blink/public/platform/web_cache.h"
 #include "third_party/blink/public/platform/web_runtime_features.h"
 #include "third_party/blink/public/web/blink.h"
@@ -40,6 +41,10 @@ class TestEnvironment {
         &discardable_memory_allocator_);
     ContentTestSuiteBase::InitializeResourceBundle();
 
+    // TestBlinkWebUnitTestSupport construction needs Mojo to be initialized
+    // first.
+    mojo::core::Init(mojo::core::Configuration{.is_broker_process = true});
+
     // Depends on resource bundle initialization so has to happen after.
     blink_test_support_ = std::make_unique<TestBlinkWebUnitTestSupport>(
         TestBlinkWebUnitTestSupport::SchedulerType::kRealScheduler);

content/public/test/unittest_test_suite.cc

@@ -120,7 +120,8 @@ UnitTestTestSuite::CreateTestContentClients() {
 
 UnitTestTestSuite::UnitTestTestSuite(
     base::TestSuite* test_suite,
-    base::RepeatingCallback<std::unique_ptr<ContentClients>()> create_clients)
+    base::RepeatingCallback<std::unique_ptr<ContentClients>()> create_clients,
+    absl::optional<mojo::core::Configuration> child_mojo_config)
     : test_suite_(test_suite), create_clients_(create_clients) {
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
   std::string enabled =
@@ -138,11 +139,14 @@ UnitTestTestSuite::UnitTestTestSuite(
 
   scoped_feature_list_.InitFromCommandLine(enabled, disabled);
 
-  // Do this here even though TestBlinkWebUnitTestSupport calls it since a
-  // multi process unit test won't get to create TestBlinkWebUnitTestSupport.
-  // This is safe to call multiple times.
   mojo::core::InitFeatures();
-  InitializeMojo();
+  if (command_line->HasSwitch(switches::kTestChildProcess)) {
+    // Note that in the main test process, TestBlinkWebUnitTestSupport
+    // initializes Mojo; so we only do this in child processes.
+    mojo::core::Init(child_mojo_config.value_or(mojo::core::Configuration{}));
+  } else {
+    mojo::core::Init(mojo::core::Configuration{.is_broker_process = true});
+  }
 
   DCHECK(test_suite);
   test_host_resolver_ = std::make_unique<TestHostResolver>();

content/public/test/unittest_test_suite.h

@@ -10,6 +10,8 @@
 #include "base/functional/callback.h"
 #include "base/test/scoped_feature_list.h"
 #include "build/build_config.h"
+#include "mojo/core/embedder/configuration.h"
+#include "third_party/abseil-cpp/absl/types/optional.h"
 
 namespace base {
 class TestSuite;
@@ -47,9 +49,14 @@ class UnitTestTestSuite {
   static std::unique_ptr<UnitTestTestSuite::ContentClients>
   CreateTestContentClients();
 
-  UnitTestTestSuite(base::TestSuite* test_suite,
-                    base::RepeatingCallback<std::unique_ptr<ContentClients>()>
-                        create_clients);
+  // Constructs a new UnitTestTestSuite to wrap `test_suite`.
+  // `child_mojo_config`, if provided, is the Mojo configuration to use in any
+  // child process spawned by these tests.
+  UnitTestTestSuite(
+      base::TestSuite* test_suite,
+      base::RepeatingCallback<std::unique_ptr<ContentClients>()> create_clients,
+      absl::optional<mojo::core::Configuration> child_mojo_config =
+          absl::nullopt);
 
   UnitTestTestSuite(const UnitTestTestSuite&) = delete;
   UnitTestTestSuite& operator=(const UnitTestTestSuite&) = delete;

content/test/run_all_unittests.cc

@@ -21,8 +21,8 @@ int main(int argc, char** argv) {
 
   content::UnitTestTestSuite test_suite(
       new content::ContentTestSuite(argc, argv),
-      base::BindRepeating(
-          content::UnitTestTestSuite::CreateTestContentClients));
+      base::BindRepeating(content::UnitTestTestSuite::CreateTestContentClients),
+      /*child_mojo_config=*/absl::nullopt);
   return base::LaunchUnitTests(argc, argv,
                                base::BindOnce(&content::UnitTestTestSuite::Run,
                                               base::Unretained(&test_suite)));

content/test/test_blink_web_unit_test_support.cc

@@ -22,7 +22,6 @@
 #include "base/threading/platform_thread.h"
 #include "build/build_config.h"
 #include "cc/trees/layer_tree_settings.h"
-#include "content/app/mojo/mojo_init.h"
 #include "content/child/child_process.h"
 #include "media/base/media.h"
 #include "media/media_buildflags.h"
@@ -117,9 +116,6 @@ TestBlinkWebUnitTestSupport::TestBlinkWebUnitTestSupport(
     base::ThreadPoolInstance::Get()->StartWithDefaultParams();
   }
 
-  // Initialize mojo firstly to enable Blink initialization to use it.
-  InitializeMojo();
-
   // Set V8 flags.
   v8::V8::SetFlagsFromString(v8_flags.c_str(), v8_flags.size());
 

mojo/core/embedder/configuration.h

@@ -22,6 +22,13 @@ struct Configuration {
   // relevant in multiprocess environments.
   bool is_broker_process = false;
 
+  // Forcibly disables the Mojo's ipcz-based implementation. This is an
+  // alternative to manual feature override for applications which don't use
+  // base::FeatureList.
+  //
+  // TODO(https://crbug.com/1299283): Remove this once dependents are gone.
+  bool disable_ipcz = false;
+
   // If |true|, this process will always attempt to allocate shared memory
   // directly rather than synchronously delegating to a broker process where
   // applicable.

mojo/core/embedder/embedder.cc

@@ -85,6 +85,12 @@ void EnableMojoIpcz() {
 
 void Init(const Configuration& configuration) {
   internal::g_configuration = configuration;
+
+  if (configuration.disable_ipcz) {
+    // Allow the caller to override MojoIpcz even when enabled as a Feature.
+    g_mojo_ipcz_enabled.store(false, std::memory_order_release);
+  }
+
   if (IsMojoIpczEnabled()) {
     CHECK(InitializeIpczNodeForProcess({
         .is_broker = configuration.is_broker_process,

mojo/public/cpp/system/BUILD.gn

@@ -60,6 +60,7 @@ component("system") {
 
   public_deps = [
     "//base",
+    "//mojo/core/embedder",
     "//mojo/public/c/system",
     "//mojo/public/cpp/platform",
   ]

mojo/public/cpp/system/DEPS

@@ -0,0 +1,3 @@
+include_rules = [
+  "+mojo/core/embedder",
+]

mojo/public/cpp/system/isolated_connection.cc

@@ -4,6 +4,7 @@
 
 #include "mojo/public/cpp/system/isolated_connection.h"
 
+#include "mojo/core/embedder/embedder.h"
 #include "mojo/public/cpp/platform/platform_channel.h"
 #include "mojo/public/cpp/system/invitation.h"
 
@@ -21,15 +22,26 @@ IsolatedConnection::~IsolatedConnection() {
   // terminating isolated connections, but this is a decision made to minimize
   // the API surface dedicated to isolated connections in anticipation of the
   // concept being deprecated eventually.
-  PlatformChannel channel;
-  OutgoingInvitation::SendIsolated(channel.TakeLocalEndpoint(),
-                                   token_.ToString());
+  //
+  // This is not done with MojoIpcz enabled, because with MojoIpcz, isolated
+  // connections are not transient and can outlive this object.
+  if (!mojo::core::IsMojoIpczEnabled()) {
+    PlatformChannel channel;
+    OutgoingInvitation::SendIsolated(channel.TakeLocalEndpoint(),
+                                     token_.ToString());
+  }
 }
 
 ScopedMessagePipeHandle IsolatedConnection::Connect(
     PlatformChannelEndpoint endpoint) {
+  return Connect(std::move(endpoint), base::Process{});
+}
+
+ScopedMessagePipeHandle IsolatedConnection::Connect(
+    PlatformChannelEndpoint endpoint,
+    base::Process process) {
   return OutgoingInvitation::SendIsolated(std::move(endpoint),
-                                          token_.ToString());
+                                          token_.ToString(), process.Handle());
 }
 
 ScopedMessagePipeHandle IsolatedConnection::Connect(