MojoIpcz: Enable by default for most platforms

This enables MojoIpcz by default for most platforms, with or without an
initialized FeatureList.

Not enabled on macOS since that's controlled by an ongoing Finch
experiment. Not enabled on Chrome OS since that's delayed until some
more work can be done on the Chrome OS side.

This also fixes several small issues around the tree which were
surfaced by the Mojo impl change:

- ipc_tests and chrome_cleaner_unittests properly configure
  broker/non-broker processes
- some blob storage tests pump tasks on teardown to avoid new leaks
- a now-invalid Mojo Java test has been deleted
- a global tracking table has added for internal ipcz API objects
  and MojoIpcz driver objects to avoid LSan detection of existing
  leaks in various test suites around the tree.
- stricter enforcement of platform handle serialization to
  avoid situations where non-optional platform handle fields
  were accepting null platform handles
- fixes to chrome_cleaner, and gfx tests, to address bad
  platform handle usage
- fix to TransferableSocket mojom to make the internal handle
  optional, since that's how it's used in practice.

Bug: 1299283,1415046
Change-Id: Ied45f4ac1c64753d204695f08852352d34aa367b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4240555
Reviewed-by: Ayu Ishii <ayui@chromium.org>
Reviewed-by: Joe Mason <joenotcharles@google.com>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/main@{#1105863}

chrome/chrome_cleaner/engines/target/engine_file_requests_proxy.cc

@@ -52,7 +52,11 @@ void SaveFindCloseCallback(uint32_t* out_result,
 void SaveOpenReadOnlyFileCallback(base::win::ScopedHandle* result_holder,
                                   base::WaitableEvent* async_call_done_event,
                                   mojo::PlatformHandle handle) {
-  *result_holder = handle.TakeHandle();
+  if (handle.is_valid_handle()) {
+    *result_holder = handle.TakeHandle();
+  } else {
+    *result_holder = base::win::ScopedHandle(INVALID_HANDLE_VALUE);
+  }
   async_call_done_event->Signal();
 }
 

chrome/chrome_cleaner/ipc/BUILD.gn

@@ -13,6 +13,7 @@ source_set("mojo_task_runner") {
 
   deps = [
     "//base",
+    "//chrome/chrome_cleaner/constants:common_strings",
     "//mojo/core/embedder",
     "//mojo/public/cpp/system",
   ]

chrome/chrome_cleaner/ipc/mojo_task_runner.cc

@@ -6,9 +6,11 @@
 
 #include <utility>
 
+#include "base/base_switches.h"
 #include "base/check.h"
 #include "base/command_line.h"
 #include "base/message_loop/message_pump_type.h"
+#include "chrome/chrome_cleaner/constants/chrome_cleaner_switches.h"
 #include "mojo/core/embedder/embedder.h"
 
 namespace chrome_cleaner {
@@ -17,7 +19,11 @@ namespace chrome_cleaner {
 scoped_refptr<MojoTaskRunner> MojoTaskRunner::Create() {
   // Ensures thread-safe and unique initialization of the mojo lib.
   [[maybe_unused]] static bool mojo_initialization = []() {  // Leaked.
-    mojo::core::Init();
+    const auto& cmd = *base::CommandLine::ForCurrentProcess();
+    mojo::core::Init(mojo::core::Configuration{
+        .is_broker_process = !cmd.HasSwitch(switches::kTestChildProcess) &&
+                             !cmd.HasSwitch(kSandboxedProcessIdSwitch),
+    });
     return true;
   }();
 

chrome/chrome_cleaner/mojom/engine_file_requests.mojom

@@ -37,10 +37,11 @@ interface EngineFileRequests {
   // ::FindClose.
   SandboxFindClose(FindHandle find_handle) => (uint32 result);
 
-  // Returns a read-only file handle for the given file. The only acceptable
-  // values for |dwFlagsAndAttributes| are FILE_FLAG_NO_BUFFERING,
+  // Returns a read-only file handle for the given file, or a null handle if the
+  // file could not be opened. The only acceptable values for
+  // |dwFlagsAndAttributes| are FILE_FLAG_NO_BUFFERING,
   // FILE_FLAG_SEQUENTIAL_SCAN, FILE_FLAG_RANDOM_ACCESS, and
   // FILE_FLAG_OPEN_REPARSE_POINT.
   SandboxOpenReadOnlyFile(FilePath file_name, uint32 dwFlagsAndAttributes) =>
-    (handle<platform> result);
+    (handle<platform>? result);
 };

ipc/run_all_unittests.cc

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/base_switches.h"
+#include "base/command_line.h"
 #include "base/functional/bind.h"
 #include "base/test/launcher/unit_test_launcher.h"
 #include "base/test/multiprocess_test.h"
@@ -23,7 +25,11 @@ int main(int argc, char** argv) {
 
   base::TestSuite test_suite(argc, argv);
   ipcz::test::RegisterMultinodeTests();
-  mojo::core::Init();
+
+  mojo::core::Init(mojo::core::Configuration{
+      .is_broker_process = !base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kTestChildProcess),
+  });
   base::TestIOThread test_io_thread(base::TestIOThread::kAutoStart);
   mojo::core::ScopedIPCSupport ipc_support(
       test_io_thread.task_runner(),

mojo/core/embedder/embedder.cc

@@ -38,7 +38,12 @@ namespace mojo::core {
 
 namespace {
 
+#if BUILDFLAG(IS_CHROMEOS) || BUILDFLAG(IS_MAC) || BUILDFLAG(IS_FUCHSIA)
 std::atomic<bool> g_mojo_ipcz_enabled{false};
+#else
+// Default to enabled even if InitFeatures() is never called.
+std::atomic<bool> g_mojo_ipcz_enabled{true};
+#endif
 
 }  // namespace
 
@@ -76,6 +81,8 @@ void InitFeatures() {
 
   if (base::FeatureList::IsEnabled(kMojoIpcz)) {
     EnableMojoIpcz();
+  } else {
+    g_mojo_ipcz_enabled.store(false, std::memory_order_release);
   }
 }
 

mojo/core/embedder/features.cc

@@ -35,7 +35,11 @@ BASE_FEATURE(kMojoAvoidRandomPipeId,
              "MojoAvoidRandomPipeId",
              base::FEATURE_ENABLED_BY_DEFAULT);
 
+#if BUILDFLAG(IS_CHROMEOS) || BUILDFLAG(IS_MAC) || BUILDFLAG(IS_FUCHSIA)
 BASE_FEATURE(kMojoIpcz, "MojoIpcz", base::FEATURE_DISABLED_BY_DEFAULT);
+#else
+BASE_FEATURE(kMojoIpcz, "MojoIpcz", base::FEATURE_ENABLED_BY_DEFAULT);
+#endif
 
 }  // namespace core
 }  // namespace mojo

mojo/public/cpp/bindings/lib/handle_serialization.h

@@ -84,8 +84,9 @@ struct Serializer<PlatformHandle, PlatformHandle> {
   static void Serialize(PlatformHandle& input,
                         Handle_Data* output,
                         Message* message) {
+    const bool input_was_valid = input.is_valid();
     ScopedHandle handle = WrapPlatformHandle(std::move(input));
-    DCHECK(handle.is_valid());
+    DCHECK_EQ(handle.is_valid(), input_was_valid);
     SerializeHandle(std::move(handle), *message, *output);
   }
 

mojo/public/cpp/system/platform_handle.cc

@@ -184,6 +184,10 @@ base::subtle::PlatformSharedMemoryRegion UnwrapPlatformSharedMemoryRegion(
 }
 
 ScopedHandle WrapPlatformHandle(PlatformHandle handle) {
+  if (!handle.is_valid()) {
+    return ScopedHandle();
+  }
+
   MojoPlatformHandle platform_handle;
   PlatformHandle::ToMojoPlatformHandle(std::move(handle), &platform_handle);
 
@@ -196,6 +200,10 @@ ScopedHandle WrapPlatformHandle(PlatformHandle handle) {
 }
 
 PlatformHandle UnwrapPlatformHandle(ScopedHandle handle) {
+  if (!handle.is_valid()) {
+    return PlatformHandle();
+  }
+
   MojoPlatformHandle platform_handle;
   platform_handle.struct_size = sizeof(platform_handle);
   MojoResult result = MojoUnwrapPlatformHandle(handle.release().value(),

mojo/public/java/system/javatests/src/org/chromium/mojo/system/impl/CoreImplTest.java

@@ -18,7 +18,6 @@ import org.chromium.mojo.MojoTestRule;
 import org.chromium.mojo.system.Core;
 import org.chromium.mojo.system.DataPipe;
 import org.chromium.mojo.system.Handle;
-import org.chromium.mojo.system.InvalidHandle;
 import org.chromium.mojo.system.MessagePipeHandle;
 import org.chromium.mojo.system.MojoException;
 import org.chromium.mojo.system.MojoResult;
@@ -408,28 +407,6 @@ public class CoreImplTest {
         checkSharing(newHandle, handle);
     }
 
-    /**
-     * Testing that invalid handle can be used with this implementation.
-     */
-    @Test
-    @SmallTest
-    public void testInvalidHandle() {
-        Core core = CoreImpl.getInstance();
-        Handle handle = InvalidHandle.INSTANCE;
-
-        // Checking sending an invalid handle. Should result in an ABORTED
-        // exception.
-        Pair<MessagePipeHandle, MessagePipeHandle> handles = core.createMessagePipe(null);
-        addHandlePairToClose(handles);
-        try {
-            handles.first.writeMessage(null, Collections.<Handle>singletonList(handle),
-                    MessagePipeHandle.WriteFlags.NONE);
-            Assert.fail();
-        } catch (MojoException e) {
-            Assert.assertEquals(MojoResult.ABORTED, e.getMojoResult());
-        }
-    }
-
     /**
      * Testing the pass method on message pipes.
      */

services/network/public/mojom/transferable_socket.mojom

@@ -16,5 +16,5 @@ struct TransferableSocket {
 [EnableIfNot=is_win]
 struct TransferableSocket {
   // On non-Windows platforms this can be the raw socket.
-  handle<platform> socket;
+  handle<platform>? socket;
 };

storage/browser/blob/blob_registry_impl_unittest.cc

@@ -110,6 +110,10 @@ class BlobRegistryImplTest : public testing::Test {
 
   void TearDown() override {
     mojo::SetDefaultProcessErrorHandler(base::NullCallback());
+
+    // Give pending tasks a chance to run since they may release Mojo bindings
+    // resources.
+    base::RunLoop().RunUntilIdle();
   }
 
   std::unique_ptr<BlobDataHandle> CreateBlobFromString(

third_party/ipcz/src/ipcz/api_object.cc

@@ -4,11 +4,72 @@
 
 #include "ipcz/api_object.h"
 
+#include <cstdint>
+
+#include "third_party/abseil-cpp/absl/container/flat_hash_set.h"
+#include "third_party/abseil-cpp/absl/synchronization/mutex.h"
+
 namespace ipcz {
 
-APIObject::APIObject(ObjectType type) : type_(type) {}
+namespace {
 
-APIObject::~APIObject() = default;
+#if defined(LEAK_SANITIZER)
+// When LSan is enabled, we keep all living API objects tracked within a global
+// hash set so that they're always reachable and never detected as leaks. This
+// is to work around the fact that Chromium has amassed hundreds of tests across
+// more than a dozen test suites which leak Mojo handles; and prior to MojoIpcz
+// the leaks were masked from LSan by Mojo's use of a global handle table.
+//
+// TODO(https://crbug.com/1415046): Remove this once all of the leaky tests are
+// fixed.
+class TrackedObjectSet {
+ public:
+  TrackedObjectSet() = default;
+  ~TrackedObjectSet() = default;
+
+  void Add(APIObject* object) {
+    absl::MutexLock lock(&mutex_);
+    tracked_objects_.insert(reinterpret_cast<uintptr_t>(object));
+  }
+
+  void Remove(APIObject* object) {
+    absl::MutexLock lock(&mutex_);
+    tracked_objects_.erase(reinterpret_cast<uintptr_t>(object));
+  }
+
+ private:
+  absl::Mutex mutex_;
+
+  // Use a uintptr_t to track since these aren't meant to be usable as pointers.
+  absl::flat_hash_set<uintptr_t> tracked_objects_ ABSL_GUARDED_BY(mutex_);
+};
+
+TrackedObjectSet& GetTrackedObjectSet() {
+  static auto* set = new TrackedObjectSet();
+  return *set;
+}
+
+void TrackObject(APIObject* object) {
+  GetTrackedObjectSet().Add(object);
+}
+
+void UntrackObject(APIObject* object) {
+  GetTrackedObjectSet().Remove(object);
+}
+#else
+void TrackObject(APIObject*) {}
+void UntrackObject(APIObject*) {}
+#endif
+
+}  // namespace
+
+APIObject::APIObject(ObjectType type) : type_(type) {
+  TrackObject(this);
+}
+
+APIObject::~APIObject() {
+  UntrackObject(this);
+}
 
 bool APIObject::CanSendFrom(Portal& sender) {
   return false;

ui/gfx/mojom/mojom_traits_unittest.cc

@@ -4,6 +4,8 @@
 
 #include <utility>
 
+#include "base/memory/platform_shared_memory_handle.h"
+#include "base/memory/unsafe_shared_memory_region.h"
 #include "base/test/task_environment.h"
 #include "build/build_config.h"
 #include "mojo/public/cpp/bindings/receiver_set.h"
@@ -36,6 +38,24 @@ gfx::AcceleratedWidget CastToAcceleratedWidget(int i) {
 #endif
 }
 
+// Used by the GpuMemoryBufferHandle test to produce a valid object handle to
+// embed in a NativePixmapPlane object, so that the test isn't sending an
+// invalid FD/vmo object where the mojom requires a valid one.
+#if BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_CHROMEOS)
+base::ScopedFD CreateValidLookingBufferHandle() {
+  return base::UnsafeSharedMemoryRegion::TakeHandleForSerialization(
+             base::UnsafeSharedMemoryRegion::Create(1024))
+      .PassPlatformHandle()
+      .fd;
+}
+#elif BUILDFLAG(IS_FUCHSIA)
+zx::vmo CreateValidLookingBufferHandle() {
+  return base::UnsafeSharedMemoryRegion::TakeHandleForSerialization(
+             base::UnsafeSharedMemoryRegion::Create(1024))
+      .PassPlatformHandle();
+}
+#endif
+
 class StructTraitsTest : public testing::Test, public mojom::TraitsTestService {
  public:
   StructTraitsTest() {}
@@ -183,10 +203,10 @@ TEST_F(StructTraitsTest, GpuMemoryBufferHandle) {
   handle2.stride = kStride;
 #if BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_CHROMEOS)
   const uint64_t kModifier = 2;
-  base::ScopedFD buffer_handle;
+  base::ScopedFD buffer_handle = CreateValidLookingBufferHandle();
   handle2.native_pixmap_handle.modifier = kModifier;
 #elif BUILDFLAG(IS_FUCHSIA)
-  zx::vmo buffer_handle;
+  zx::vmo buffer_handle = CreateValidLookingBufferHandle();
   zx::eventpair client_handle, service_handle;
   auto status = zx::eventpair::create(0, &client_handle, &service_handle);
   DCHECK_EQ(status, ZX_OK);