Add back an error handling to AppShimHostManager if the acceptor fails to start.

The old UnixDomainSocketAcceptor had an error callout on its delegate,
which was removed when switching to the MachBootstrapAcceptor in
3ddc5f07. Since it appears that there
are still errors that occur in the field, add this functionality to the
MachBootstrapAcceptor.

Bug: 984896, 272577
Change-Id: I2c009611c93b098ccbec7f43c633b57b722360f9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1741613
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#685171}

chrome/app_shim/app_shim_controller.h

@@ -15,6 +15,10 @@
 #include "mojo/public/cpp/platform/named_platform_channel.h"
 #include "mojo/public/cpp/system/isolated_connection.h"
 
+namespace apps {
+class MachBootstrapAcceptorTest;
+}
+
 @class AppShimDelegate;
 
 // The AppShimController is responsible for launching and maintaining the
@@ -47,6 +51,7 @@ class AppShimController : public chrome::mojom::AppShim {
 
  private:
   friend class TestShimClient;
+  friend class apps::MachBootstrapAcceptorTest;
 
   // Create a channel from the Mojo |endpoint| and send a LaunchApp message.
   void CreateChannelAndSendLaunchApp(mojo::PlatformChannelEndpoint endpoint);

chrome/browser/apps/app_shim/DEPS

@@ -2,4 +2,7 @@ specific_include_rules = {
   "app_shim_host_manager_browsertest_mac.mm": [
     "+chrome/app_shim/app_shim_controller.h",
   ],
+  "mach_bootstrap_acceptor_unittest.mm": [
+    "+chrome/app_shim/app_shim_controller.h",
+  ],
 }

chrome/browser/apps/app_shim/app_shim_host_manager_mac.h

@@ -55,6 +55,7 @@ class AppShimHostManager : public apps::MachBootstrapAcceptor::Delegate,
   // MachBootstrapAcceptor::Delegate:
   void OnClientConnected(mojo::PlatformChannelEndpoint endpoint,
                          base::ProcessId peer_pid) override;
+  void OnServerChannelCreateError() override;
 
   // The |acceptor_| must be created on a thread which allows blocking I/O.
   void InitOnBackgroundThread();

chrome/browser/apps/app_shim/app_shim_host_manager_mac.mm

@@ -98,3 +98,11 @@ void AppShimHostManager::OnClientConnected(
           base::BindOnce(&AppShimHostBootstrap::CreateForChannelAndPeerID,
                          std::move(endpoint), peer_pid));
 }
+
+void AppShimHostManager::OnServerChannelCreateError() {
+  // TODO(https://crbug.com/272577): Set a timeout and attempt to reconstruct
+  // the channel. Until cases where the error could occur are better known,
+  // just reset the acceptor to allow failure to be communicated via the test
+  // API.
+  mach_acceptor_.reset();
+}

chrome/browser/apps/app_shim/mach_bootstrap_acceptor.cc

@@ -37,6 +37,11 @@ void MachBootstrapAcceptor::Start() {
   options.server_name = server_name_;
   mojo::NamedPlatformChannel channel(options);
   endpoint_ = channel.TakeServerEndpoint();
+  if (!endpoint_.is_valid()) {
+    delegate_->OnServerChannelCreateError();
+    return;
+  }
+
   dispatch_source_ = std::make_unique<base::DispatchSourceMach>(
       server_name_.c_str(), port(), ^{
         HandleRequest();

chrome/browser/apps/app_shim/mach_bootstrap_acceptor.h

@@ -29,6 +29,9 @@ class MachBootstrapAcceptor {
     // Mach port it provided in |endpoint|.
     virtual void OnClientConnected(mojo::PlatformChannelEndpoint endpoint,
                                    base::ProcessId peer_pid) = 0;
+
+    // Called when there is an error creating the server channel.
+    virtual void OnServerChannelCreateError() = 0;
   };
 
   // Initializes the server by specifying the |name_fragment|, which will be
@@ -47,6 +50,8 @@ class MachBootstrapAcceptor {
   void Stop();
 
  private:
+  friend class MachBootstrapAcceptorTest;
+
   // Called by |dispatch_source_| when a Mach message is ready to be received
   // on |endpoint_|.
   void HandleRequest();

chrome/browser/apps/app_shim/mach_bootstrap_acceptor_unittest.mm

+// Copyright 2019 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/apps/app_shim/mach_bootstrap_acceptor.h"
+
+#include "base/process/process_handle.h"
+#include "base/run_loop.h"
+#include "base/test/scoped_task_environment.h"
+#include "chrome/app_shim/app_shim_controller.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace apps {
+
+class MachBootstrapAcceptorTest : public testing::Test {
+ public:
+  // Friend accessors:
+  static mojo::PlatformChannelEndpoint ConnectToBrowser(
+      const mojo::NamedPlatformChannel::ServerName& server_name) {
+    return AppShimController::ConnectToBrowser(server_name);
+  }
+
+  static mach_port_t GetAcceptorPort(MachBootstrapAcceptor* acceptor) {
+    return acceptor->port();
+  }
+
+  static const mojo::NamedPlatformChannel::ServerName& GetAcceptorServerName(
+      MachBootstrapAcceptor* acceptor) {
+    return acceptor->server_name_;
+  }
+
+ private:
+  base::test::ScopedTaskEnvironment task_environment_;
+};
+
+class TestMachBootstrapAcceptorDelegate
+    : public MachBootstrapAcceptor::Delegate {
+ public:
+  explicit TestMachBootstrapAcceptorDelegate(base::OnceClosure quit_closure)
+      : quit_closure_(std::move(quit_closure)) {}
+
+  void OnClientConnected(mojo::PlatformChannelEndpoint endpoint,
+                         base::ProcessId pid) override {
+    endpoint_ = std::move(endpoint);
+    pid_ = pid;
+    std::move(quit_closure_).Run();
+  }
+
+  void OnServerChannelCreateError() override {
+    error_ = true;
+    std::move(quit_closure_).Run();
+  }
+
+  base::ProcessId pid() const { return pid_; }
+  const mojo::PlatformChannelEndpoint& endpoint() const { return endpoint_; }
+  bool error() const { return error_; }
+
+ private:
+  base::ProcessId pid_ = base::kNullProcessId;
+  mojo::PlatformChannelEndpoint endpoint_;
+
+  bool error_ = false;
+
+  base::OnceClosure quit_closure_;
+};
+
+TEST_F(MachBootstrapAcceptorTest, SingleRequest) {
+  base::RunLoop run_loop;
+  TestMachBootstrapAcceptorDelegate delegate(run_loop.QuitClosure());
+  MachBootstrapAcceptor acceptor("simplereq", &delegate);
+  acceptor.Start();
+
+  EXPECT_TRUE(GetAcceptorPort(&acceptor) != MACH_PORT_NULL);
+
+  mojo::PlatformChannelEndpoint endpoint =
+      ConnectToBrowser(GetAcceptorServerName(&acceptor));
+
+  run_loop.Run();
+
+  EXPECT_FALSE(delegate.error());
+  EXPECT_EQ(base::GetCurrentProcId(), delegate.pid());
+  // In the same process, the send and receive rights are known by the same
+  // Mach port name.
+  EXPECT_EQ(endpoint.platform_handle().GetMachReceiveRight().get(),
+            delegate.endpoint().platform_handle().GetMachSendRight().get());
+}
+
+TEST_F(MachBootstrapAcceptorTest, FailToRegister) {
+  base::RunLoop run_loop;
+  TestMachBootstrapAcceptorDelegate delegate(run_loop.QuitClosure());
+  MachBootstrapAcceptor acceptor("failtoreg", &delegate);
+
+  mojo::NamedPlatformChannel::ServerName server_name =
+      GetAcceptorServerName(&acceptor);
+
+  // Squat on the server name in the bootstrap server.
+  mojo::NamedPlatformChannel::Options server_options;
+  server_options.server_name = server_name;
+  mojo::NamedPlatformChannel server_endpoint =
+      mojo::NamedPlatformChannel(server_options);
+
+  // The acceptor will fail to start and reports an error.
+  acceptor.Start();
+
+  run_loop.Run();
+
+  EXPECT_TRUE(delegate.error());
+}
+
+}  // namespace apps

chrome/test/BUILD.gn

@@ -2780,6 +2780,7 @@ test("unit_tests") {
     "../browser/android/webapk/webapk_web_manifest_checker_unittest.cc",
     "../browser/android/webapps/add_to_homescreen_data_fetcher_unittest.cc",
     "../browser/app_controller_mac_unittest.mm",
+    "../browser/apps/app_shim/mach_bootstrap_acceptor_unittest.mm",
     "../browser/apps/user_type_filter_unittest.cc",
     "../browser/autocomplete/chrome_autocomplete_provider_client_unittest.cc",
     "../browser/autocomplete/chrome_autocomplete_scheme_classifier_unittest.cc",