Migrate macOS AppShim from IPC to mojo

Change IPC messages in app_shim_messages to a mojo interface.
- Remove the AppShimMsg_RequestUserAttention method
- Remove the warning that the interface needs versioning

Update how the connection between shim and browser is initialized
- The connection process is initiated in the shim process in the
  AppShimController::CreateChannelAndSendLaunchApp function
- Make the socket created be an AppShimHost interface (the interface
  used to send from the shim to the browser)
- Immediately enqueue a LaunchApp message to the browser process, and
  add an AppShim interface to the LaunchApp method, to allow the
  browser to call back to the shim process
- Update AppShimHost::ServeChannel to bind itself as a AppShimHost
  to the created socket
- Update AppShimHost::LaunchApp to initialize its AppShim pointer.
  Intentionally do not update this method to have a reply, to minimize
  functional changes. Rather, send the reply via the passed AppShim.

Update how channel errors are handled by adding an error callback
to the mojo bindings created in AppShimHost::LaunchApp and
AppShimController::CreateChannelAndSendLaunchApp.

Update unit tests to use the mojo interfaces. Change AppShimHostTest
to use a weak pointer to the AppShimHost, because, when its message
loop is allowed to run, AppShimHost will delete itself on Close.

Remove the comments about requiring that the interface between the
shim and the browser versioned.

Bug: 821651
Change-Id: Ibad84de4ec25d1121e4ef4d8075a20bb3abd8642
Reviewed-on: https://chromium-review.googlesource.com/1103844Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568286}

chrome/app_shim/BUILD.gn

@@ -17,6 +17,7 @@ source_set("app_shim") {
   deps = [
     "//chrome:strings",
     "//chrome/common",
+    "//chrome/common:mojo_bindings",
     "//ipc",
     "//mojo/edk",
     "//ui/base",

chrome/app_shim/chrome_main_app_mode_mac.mm

@@ -33,14 +33,12 @@
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/mac/app_mode_common.h"
+#include "chrome/common/mac/app_shim.mojom.h"
 #include "chrome/common/mac/app_shim_messages.h"
 #include "chrome/grit/generated_resources.h"
-#include "ipc/ipc_channel_mojo.h"
-#include "ipc/ipc_channel_proxy.h"
-#include "ipc/ipc_listener.h"
-#include "ipc/ipc_message.h"
 #include "mojo/edk/embedder/embedder.h"
 #include "mojo/edk/embedder/scoped_ipc_support.h"
+#include "mojo/public/cpp/bindings/binding.h"
 #include "mojo/public/cpp/platform/named_platform_channel.h"
 #include "mojo/public/cpp/system/isolated_connection.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -91,7 +89,7 @@ class AppShimController;
 
 // The AppShimController is responsible for communication with the main Chrome
 // process, and generally controls the lifetime of the app shim process.
-class AppShimController : public IPC::Listener {
+class AppShimController : public chrome::mojom::AppShim {
  public:
   AppShimController();
   ~AppShimController() override;
@@ -124,31 +122,21 @@ class AppShimController : public IPC::Listener {
                     const std::vector<base::FilePath>& files);
 
  private:
-  // IPC::Listener implemetation.
-  bool OnMessageReceived(const IPC::Message& message) override;
-  void OnChannelError() override;
+  void ChannelError(uint32_t custom_reason, const std::string& description);
 
-  // If Chrome failed to launch the app, |success| will be false and the app
-  // shim process should die.
-  void OnLaunchAppDone(apps::AppShimLaunchResult result);
-
-  // Hide this app.
-  void OnHide();
-
-  // Set this app to the unhidden state. Happens when an app window shows
-  // itself.
-  void OnUnhideWithoutActivation();
-
-  // Requests user attention.
-  void OnRequestUserAttention();
-  void OnSetUserAttention(apps::AppShimAttentionType attention_type);
+  // chrome::mojom::AppShim implementation.
+  void LaunchAppDone(apps::AppShimLaunchResult result) override;
+  void Hide() override;
+  void UnhideWithoutActivation() override;
+  void SetUserAttention(apps::AppShimAttentionType attention_type) override;
 
   // Terminates the app shim process.
   void Close();
 
   base::FilePath user_data_dir_;
   mojo::IsolatedConnection mojo_connection_;
-  std::unique_ptr<IPC::ChannelProxy> channel_;
+  mojo::Binding<chrome::mojom::AppShim> shim_binding_;
+  chrome::mojom::AppShimHostPtr host_;
   base::scoped_nsobject<AppShimDelegate> delegate_;
   bool launch_app_done_;
   bool ping_chrome_reply_received_;
@@ -158,7 +146,8 @@ class AppShimController : public IPC::Listener {
 };
 
 AppShimController::AppShimController()
-    : delegate_([[AppShimDelegate alloc] init]),
+    : shim_binding_(this),
+      delegate_([[AppShimDelegate alloc] init]),
       launch_app_done_(false),
       ping_chrome_reply_received_(false),
       attention_request_id_(0) {
@@ -219,14 +208,15 @@ void AppShimController::Init() {
 
 void AppShimController::CreateChannelAndSendLaunchApp(
     const base::FilePath& socket_path) {
-  channel_ = IPC::ChannelProxy::Create(
-      IPC::ChannelMojo::CreateClientFactory(
-          mojo_connection_.Connect(
-              mojo::NamedPlatformChannel::ConnectToServer(socket_path.value())),
-          g_io_thread->task_runner().get(),
-          base::ThreadTaskRunnerHandle::Get()),
-      this, g_io_thread->task_runner().get(),
-      base::ThreadTaskRunnerHandle::Get());
+  mojo::ScopedMessagePipeHandle message_pipe = mojo_connection_.Connect(
+      mojo::NamedPlatformChannel::ConnectToServer(socket_path.value()));
+  host_ = chrome::mojom::AppShimHostPtr(
+      chrome::mojom::AppShimHostPtrInfo(std::move(message_pipe), 0));
+
+  chrome::mojom::AppShimPtr app_shim_ptr;
+  shim_binding_.Bind(mojo::MakeRequest(&app_shim_ptr));
+  shim_binding_.set_connection_error_with_reason_handler(
+      base::BindOnce(&AppShimController::ChannelError, base::Unretained(this)));
 
   bool launched_by_chrome = base::CommandLine::ForCurrentProcess()->HasSwitch(
       app_mode::kLaunchedByChromeProcessId);
@@ -238,8 +228,8 @@ void AppShimController::CreateChannelAndSendLaunchApp(
   std::vector<base::FilePath> files;
   [delegate_ getFilesToOpenAtStartup:&files];
 
-  channel_->Send(new AppShimHostMsg_LaunchApp(
-      g_info->profile_dir, g_info->app_mode_id, launch_type, files));
+  host_->LaunchApp(std::move(app_shim_ptr), g_info->profile_dir,
+                   g_info->app_mode_id, launch_type, files);
 }
 
 void AppShimController::SetUpMenu() {
@@ -290,29 +280,17 @@ void AppShimController::SetUpMenu() {
 }
 
 void AppShimController::SendQuitApp() {
-  channel_->Send(new AppShimHostMsg_QuitApp);
+  host_->QuitApp();
 }
 
-bool AppShimController::OnMessageReceived(const IPC::Message& message) {
-  bool handled = true;
-  IPC_BEGIN_MESSAGE_MAP(AppShimController, message)
-    IPC_MESSAGE_HANDLER(AppShimMsg_LaunchApp_Done, OnLaunchAppDone)
-    IPC_MESSAGE_HANDLER(AppShimMsg_Hide, OnHide)
-    IPC_MESSAGE_HANDLER(AppShimMsg_UnhideWithoutActivation,
-                        OnUnhideWithoutActivation)
-    IPC_MESSAGE_HANDLER(AppShimMsg_RequestUserAttention, OnRequestUserAttention)
-    IPC_MESSAGE_HANDLER(AppShimMsg_SetUserAttention, OnSetUserAttention)
-    IPC_MESSAGE_UNHANDLED(handled = false)
-  IPC_END_MESSAGE_MAP()
-
-  return handled;
-}
-
-void AppShimController::OnChannelError() {
+void AppShimController::ChannelError(uint32_t custom_reason,
+                                     const std::string& description) {
+  LOG(ERROR) << "Channel error custom_reason:" << custom_reason
+             << " description: " << description;
   Close();
 }
 
-void AppShimController::OnLaunchAppDone(apps::AppShimLaunchResult result) {
+void AppShimController::LaunchAppDone(apps::AppShimLaunchResult result) {
   if (result != apps::APP_SHIM_LAUNCH_SUCCESS) {
     Close();
     return;
@@ -325,19 +303,15 @@ void AppShimController::OnLaunchAppDone(apps::AppShimLaunchResult result) {
   launch_app_done_ = true;
 }
 
-void AppShimController::OnHide() {
+void AppShimController::Hide() {
   [NSApp hide:nil];
 }
 
-void AppShimController::OnUnhideWithoutActivation() {
+void AppShimController::UnhideWithoutActivation() {
   [NSApp unhideWithoutActivation];
 }
 
-void AppShimController::OnRequestUserAttention() {
-  OnSetUserAttention(apps::APP_SHIM_ATTENTION_INFORMATIONAL);
-}
-
-void AppShimController::OnSetUserAttention(
+void AppShimController::SetUserAttention(
     apps::AppShimAttentionType attention_type) {
   switch (attention_type) {
     case apps::APP_SHIM_ATTENTION_CANCEL:
@@ -363,7 +337,7 @@ void AppShimController::Close() {
 bool AppShimController::SendFocusApp(apps::AppShimFocusType focus_type,
                                      const std::vector<base::FilePath>& files) {
   if (launch_app_done_) {
-    channel_->Send(new AppShimHostMsg_FocusApp(focus_type, files));
+    host_->FocusApp(focus_type, files);
     return true;
   }
 
@@ -371,7 +345,7 @@ bool AppShimController::SendFocusApp(apps::AppShimFocusType focus_type,
 }
 
 void AppShimController::SendSetAppHidden(bool hidden) {
-  channel_->Send(new AppShimHostMsg_SetAppHidden(hidden));
+  host_->SetAppHidden(hidden);
 }
 
 @implementation AppShimDelegate

chrome/browser/apps/app_shim/BUILD.gn

@@ -21,6 +21,7 @@ source_set("app_shim") {
   ]
 
   deps = [
+    "//chrome/common:mojo_bindings",
     "//content/public/browser",
     "//content/public/common",
     "//extensions/browser",

chrome/browser/apps/app_shim/app_shim_host_mac.cc

@@ -10,12 +10,10 @@
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "chrome/browser/apps/app_shim/app_shim_handler_mac.h"
-#include "chrome/common/mac/app_shim_messages.h"
 #include "content/public/browser/browser_thread.h"
-#include "ipc/ipc_channel_mojo.h"
-#include "ipc/ipc_channel_proxy.h"
 
-AppShimHost::AppShimHost() : initial_launch_finished_(false) {}
+AppShimHost::AppShimHost()
+    : host_binding_(this), initial_launch_finished_(false) {}
 
 AppShimHost::~AppShimHost() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
@@ -26,18 +24,16 @@ AppShimHost::~AppShimHost() {
 
 void AppShimHost::ServeChannel(mojo::PlatformChannelEndpoint endpoint) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-  DCHECK(!channel_.get());
-  channel_ = IPC::ChannelProxy::Create(
-      IPC::ChannelMojo::CreateServerFactory(
-          mojo_connection_.Connect(std::move(endpoint)),
-          content::BrowserThread::GetTaskRunnerForThread(
-              content::BrowserThread::IO)
-              .get(),
-          base::ThreadTaskRunnerHandle::Get()),
-      this,
-      content::BrowserThread::GetTaskRunnerForThread(content::BrowserThread::IO)
-          .get(),
-      base::ThreadTaskRunnerHandle::Get());
+  mojo::ScopedMessagePipeHandle message_pipe =
+      mojo_connection_.Connect(std::move(endpoint));
+  BindToRequest(chrome::mojom::AppShimHostRequest(std::move(message_pipe)));
+}
+
+void AppShimHost::BindToRequest(
+    chrome::mojom::AppShimHostRequest host_request) {
+  host_binding_.Bind(std::move(host_request));
+  host_binding_.set_connection_error_with_reason_handler(
+      base::BindOnce(&AppShimHost::ChannelError, base::Unretained(this)));
 }
 
 base::FilePath AppShimHost::GetProfilePath() const {
@@ -48,39 +44,25 @@ std::string AppShimHost::GetAppId() const {
   return app_id_;
 }
 
-bool AppShimHost::OnMessageReceived(const IPC::Message& message) {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-  bool handled = true;
-  IPC_BEGIN_MESSAGE_MAP(AppShimHost, message)
-    IPC_MESSAGE_HANDLER(AppShimHostMsg_LaunchApp, OnLaunchApp)
-    IPC_MESSAGE_HANDLER(AppShimHostMsg_FocusApp, OnFocus)
-    IPC_MESSAGE_HANDLER(AppShimHostMsg_SetAppHidden, OnSetHidden)
-    IPC_MESSAGE_HANDLER(AppShimHostMsg_QuitApp, OnQuit)
-    IPC_MESSAGE_UNHANDLED(handled = false)
-  IPC_END_MESSAGE_MAP()
-
-  return handled;
-}
-
-void AppShimHost::OnChannelError() {
+void AppShimHost::ChannelError(uint32_t custom_reason,
+                               const std::string& description) {
+  LOG(ERROR) << "Channel error custom_reason:" << custom_reason
+             << " description: " << description;
   Close();
 }
 
-bool AppShimHost::Send(IPC::Message* message) {
-  DCHECK(channel_.get());
-  return channel_->Send(message);
-}
-
-void AppShimHost::OnLaunchApp(const base::FilePath& profile_dir,
-                              const std::string& app_id,
-                              apps::AppShimLaunchType launch_type,
-                              const std::vector<base::FilePath>& files) {
+void AppShimHost::LaunchApp(chrome::mojom::AppShimPtr app_shim_ptr,
+                            const base::FilePath& profile_dir,
+                            const std::string& app_id,
+                            apps::AppShimLaunchType launch_type,
+                            const std::vector<base::FilePath>& files) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK(profile_path_.empty());
   // Only one app launch message per channel.
   if (!profile_path_.empty())
     return;
 
+  app_shim_ = std::move(app_shim_ptr);
   profile_path_ = profile_dir;
   app_id_ = app_id;
 
@@ -91,22 +73,22 @@ void AppShimHost::OnLaunchApp(const base::FilePath& profile_dir,
   // this only happens at shutdown, do nothing here.
 }
 
-void AppShimHost::OnFocus(apps::AppShimFocusType focus_type,
-                          const std::vector<base::FilePath>& files) {
+void AppShimHost::FocusApp(apps::AppShimFocusType focus_type,
+                           const std::vector<base::FilePath>& files) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   apps::AppShimHandler* handler = apps::AppShimHandler::GetForAppMode(app_id_);
   if (handler)
     handler->OnShimFocus(this, focus_type, files);
 }
 
-void AppShimHost::OnSetHidden(bool hidden) {
+void AppShimHost::SetAppHidden(bool hidden) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   apps::AppShimHandler* handler = apps::AppShimHandler::GetForAppMode(app_id_);
   if (handler)
     handler->OnShimSetHidden(this, hidden);
 }
 
-void AppShimHost::OnQuit() {
+void AppShimHost::QuitApp() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   apps::AppShimHandler* handler = apps::AppShimHandler::GetForAppMode(app_id_);
   if (handler)
@@ -115,7 +97,7 @@ void AppShimHost::OnQuit() {
 
 void AppShimHost::OnAppLaunchComplete(apps::AppShimLaunchResult result) {
   if (!initial_launch_finished_) {
-    Send(new AppShimMsg_LaunchApp_Done(result));
+    app_shim_->LaunchAppDone(result);
     initial_launch_finished_ = true;
   }
 }
@@ -125,15 +107,15 @@ void AppShimHost::OnAppClosed() {
 }
 
 void AppShimHost::OnAppHide() {
-  Send(new AppShimMsg_Hide);
+  app_shim_->Hide();
 }
 
 void AppShimHost::OnAppUnhideWithoutActivation() {
-  Send(new AppShimMsg_UnhideWithoutActivation);
+  app_shim_->UnhideWithoutActivation();
 }
 
 void AppShimHost::OnAppRequestUserAttention(apps::AppShimAttentionType type) {
-  Send(new AppShimMsg_SetUserAttention(type));
+  app_shim_->SetUserAttention(type);
 }
 
 void AppShimHost::Close() {

chrome/browser/apps/app_shim/app_shim_host_mac.h

@@ -12,58 +12,40 @@
 #include "base/files/file_path.h"
 #include "base/threading/thread_checker.h"
 #include "chrome/browser/apps/app_shim/app_shim_handler_mac.h"
-#include "ipc/ipc_listener.h"
-#include "ipc/ipc_sender.h"
+#include "chrome/common/mac/app_shim.mojom.h"
+#include "mojo/public/cpp/bindings/binding.h"
 #include "mojo/public/cpp/platform/platform_channel_endpoint.h"
 #include "mojo/public/cpp/system/isolated_connection.h"
 
-namespace IPC {
-class ChannelProxy;
-class Message;
-}  // namespace IPC
-
 // This is the counterpart to AppShimController in
 // chrome/app/chrome_main_app_mode_mac.mm. The AppShimHost owns itself, and is
 // destroyed when the app it corresponds to is closed or when the channel
 // connected to the app shim is closed.
-class AppShimHost : public IPC::Listener,
-                    public IPC::Sender,
+class AppShimHost : public chrome::mojom::AppShimHost,
                     public apps::AppShimHandler::Host {
  public:
   AppShimHost();
   ~AppShimHost() override;
 
-  // Creates a new server-side IPC channel at |endpoint|, which should contain a
-  // file descriptor of a channel created by an UnixDomainSocketAcceptor,
-  // and begins listening for messages on it.
+  // Creates a new server-side mojo channel at |endpoint|, which should contain
+  // a file descriptor of a channel created by an UnixDomainSocketAcceptor, and
+  // begins listening for messages on it.
   void ServeChannel(mojo::PlatformChannelEndpoint endpoint);
 
  protected:
-  // IPC::Listener implementation.
-  bool OnMessageReceived(const IPC::Message& message) override;
-  void OnChannelError() override;
-
-  // IPC::Sender implementation.
-  bool Send(IPC::Message* message) override;
-
- private:
-  // The app shim process is requesting to be associated with the given profile
-  // and app_id. Once the profile and app_id are stored, and all future
-  // messages from the app shim relate to this app. The app is launched
-  // immediately if |launch_now| is true.
-  void OnLaunchApp(const base::FilePath& profile_dir,
-                   const std::string& app_id,
-                   apps::AppShimLaunchType launch_type,
-                   const std::vector<base::FilePath>& files);
-
-  // Called when the app shim process notifies that the app was focused.
-  void OnFocus(apps::AppShimFocusType focus_type,
-               const std::vector<base::FilePath>& files);
-
-  void OnSetHidden(bool hidden);
-
-  // Called when the app shim process notifies that the app should quit.
-  void OnQuit();
+  void BindToRequest(chrome::mojom::AppShimHostRequest host_request);
+  void ChannelError(uint32_t custom_reason, const std::string& description);
+
+  // chrome::mojom::AppShimHost implementation.
+  void LaunchApp(chrome::mojom::AppShimPtr app_shim_ptr,
+                 const base::FilePath& profile_dir,
+                 const std::string& app_id,
+                 apps::AppShimLaunchType launch_type,
+                 const std::vector<base::FilePath>& files) override;
+  void FocusApp(apps::AppShimFocusType focus_type,
+                const std::vector<base::FilePath>& files) override;
+  void SetAppHidden(bool hidden) override;
+  void QuitApp() override;
 
   // apps::AppShimHandler::Host overrides:
   void OnAppLaunchComplete(apps::AppShimLaunchResult result) override;
@@ -78,12 +60,14 @@ class AppShimHost : public IPC::Listener,
   void Close();
 
   mojo::IsolatedConnection mojo_connection_;
-  std::unique_ptr<IPC::ChannelProxy> channel_;
+  chrome::mojom::AppShimPtr app_shim_;
+  mojo::Binding<chrome::mojom::AppShimHost> host_binding_;
   std::string app_id_;
   base::FilePath profile_path_;
   bool initial_launch_finished_;
 
   THREAD_CHECKER(thread_checker_);
+  DISALLOW_COPY_AND_ASSIGN(AppShimHost);
 };
 
 #endif  // CHROME_BROWSER_APPS_APP_SHIM_APP_SHIM_HOST_MAC_H_

chrome/browser/apps/app_shim/app_shim_host_mac_unittest.cc

@@ -10,42 +10,70 @@
 
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
+#include "base/test/test_simple_task_runner.h"
 #include "chrome/common/mac/app_shim_messages.h"
 #include "ipc/ipc_message.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
 
-class TestingAppShimHost : public AppShimHost {
+class TestingAppShim : public chrome::mojom::AppShim {
  public:
-  TestingAppShimHost() {}
-  ~TestingAppShimHost() override {}
-
-  bool ReceiveMessage(IPC::Message* message);
+  TestingAppShim() : app_shim_binding_(this) {}
+
+  chrome::mojom::AppShimPtr GetMojoPtr() {
+    chrome::mojom::AppShimPtr app_shim_ptr;
+    chrome::mojom::AppShimRequest app_shim_request =
+        mojo::MakeRequest(&app_shim_ptr);
+    app_shim_binding_.Bind(std::move(app_shim_request));
+    return app_shim_ptr;
+  }
 
-  const std::vector<std::unique_ptr<IPC::Message>>& sent_messages() {
-    return sent_messages_;
+  chrome::mojom::AppShimHostRequest GetHostRequest() {
+    return mojo::MakeRequest(&host_ptr_);
   }
 
- protected:
-  bool Send(IPC::Message* message) override;
+  apps::AppShimLaunchResult GetLaunchResult() const {
+    EXPECT_TRUE(received_launch_done_result_);
+    return launch_done_result_;
+  }
 
  private:
-  std::vector<std::unique_ptr<IPC::Message>> sent_messages_;
+  // chrome::mojom::AppShim implementation.
+  void LaunchAppDone(apps::AppShimLaunchResult result) override {
+    received_launch_done_result_ = true;
+    launch_done_result_ = result;
+  }
+  void Hide() override {}
+  void UnhideWithoutActivation() override {}
+  void SetUserAttention(apps::AppShimAttentionType attention_type) override {}
 
-  DISALLOW_COPY_AND_ASSIGN(TestingAppShimHost);
+  bool received_launch_done_result_ = false;
+  apps::AppShimLaunchResult launch_done_result_ = apps::APP_SHIM_LAUNCH_SUCCESS;
+
+  chrome::mojom::AppShimHostPtr host_ptr_;
+  mojo::Binding<chrome::mojom::AppShim> app_shim_binding_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestingAppShim);
 };
 
-bool TestingAppShimHost::ReceiveMessage(IPC::Message* message) {
-  bool handled = OnMessageReceived(*message);
-  delete message;
-  return handled;
-}
+class TestingAppShimHost : public AppShimHost {
+ public:
+  explicit TestingAppShimHost(chrome::mojom::AppShimHostRequest host_request)
+      : test_weak_factory_(this) {
+    // AppShimHost will bind to the request from ServeChannel. For testing
+    // purposes, have this request passed in at creation.
+    BindToRequest(std::move(host_request));
+  }
 
-bool TestingAppShimHost::Send(IPC::Message* message) {
-  sent_messages_.push_back(base::WrapUnique(message));
-  return true;
-}
+  base::WeakPtr<TestingAppShimHost> GetWeakPtr() {
+    return test_weak_factory_.GetWeakPtr();
+  }
+
+ private:
+  base::WeakPtrFactory<TestingAppShimHost> test_weak_factory_;
+  DISALLOW_COPY_AND_ASSIGN(TestingAppShimHost);
+};
 
 const char kTestAppId[] = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
 const char kTestProfileDir[] = "Profile 1";
@@ -53,35 +81,34 @@ const char kTestProfileDir[] = "Profile 1";
 class AppShimHostTest : public testing::Test,
                         public apps::AppShimHandler {
  public:
-  AppShimHostTest() : launch_result_(apps::APP_SHIM_LAUNCH_SUCCESS),
-                      launch_count_(0),
-                      launch_now_count_(0),
-                      close_count_(0),
-                      focus_count_(0),
-                      quit_count_(0) {}
+  AppShimHostTest()
+      : task_runner_(new base::TestSimpleTaskRunner),
+        task_runner_handle_(task_runner_) {}
+
+  ~AppShimHostTest() override {
+    if (host_)
+      delete host_.get();
+    DCHECK(!host_);
+  }
 
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner() {
+    return task_runner_;
+  }
   TestingAppShimHost* host() { return host_.get(); }
+  chrome::mojom::AppShimHost* GetMojoHost() { return host_.get(); }
 
   void LaunchApp(apps::AppShimLaunchType launch_type) {
-    EXPECT_TRUE(host()->ReceiveMessage(
-        new AppShimHostMsg_LaunchApp(base::FilePath(kTestProfileDir),
-                                     kTestAppId,
-                                     launch_type,
-                                     std::vector<base::FilePath>())));
+    GetMojoHost()->LaunchApp(shim_->GetMojoPtr(),
+                             base::FilePath(kTestProfileDir), kTestAppId,
+                             launch_type, std::vector<base::FilePath>());
   }
 
   apps::AppShimLaunchResult GetLaunchResult() {
-    EXPECT_EQ(1u, host()->sent_messages().size());
-    IPC::Message* message = host()->sent_messages()[0].get();
-    EXPECT_EQ(AppShimMsg_LaunchApp_Done::ID, message->type());
-    AppShimMsg_LaunchApp_Done::Param param;
-    AppShimMsg_LaunchApp_Done::Read(message, &param);
-    return std::get<0>(param);
+    task_runner_->RunUntilIdle();
+    return shim_->GetLaunchResult();
   }
 
-  void SimulateDisconnect() {
-    static_cast<IPC::Listener*>(host_.release())->OnChannelError();
-  }
+  void SimulateDisconnect() { shim_.reset(); }
 
  protected:
   void OnShimLaunch(Host* host,
@@ -105,20 +132,29 @@ class AppShimHostTest : public testing::Test,
 
   void OnShimQuit(Host* host) override { ++quit_count_; }
 
-  apps::AppShimLaunchResult launch_result_;
-  int launch_count_;
-  int launch_now_count_;
-  int close_count_;
-  int focus_count_;
-  int quit_count_;
+  apps::AppShimLaunchResult launch_result_ = apps::APP_SHIM_LAUNCH_SUCCESS;
+  int launch_count_ = 0;
+  int launch_now_count_ = 0;
+  int close_count_ = 0;
+  int focus_count_ = 0;
+  int quit_count_ = 0;
 
  private:
   void SetUp() override {
     testing::Test::SetUp();
-    host_.reset(new TestingAppShimHost());
+    shim_.reset(new TestingAppShim());
+    TestingAppShimHost* host = new TestingAppShimHost(shim_->GetHostRequest());
+    host_ = host->GetWeakPtr();
   }
 
-  std::unique_ptr<TestingAppShimHost> host_;
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
+  base::ThreadTaskRunnerHandle task_runner_handle_;
+
+  std::unique_ptr<TestingAppShim> shim_;
+
+  // AppShimHost will destroy itself in AppShimHost::Close, so use a weak
+  // pointer here to avoid lifetime issues.
+  base::WeakPtr<TestingAppShimHost> host_;
 
   DISALLOW_COPY_AND_ASSIGN(AppShimHostTest);
 };
@@ -142,16 +178,19 @@ TEST_F(AppShimHostTest, TestLaunchAppWithHandler) {
       ->OnAppLaunchComplete(apps::APP_SHIM_LAUNCH_APP_NOT_FOUND);
   EXPECT_EQ(apps::APP_SHIM_LAUNCH_SUCCESS, GetLaunchResult());
 
-  EXPECT_TRUE(host()->ReceiveMessage(
-      new AppShimHostMsg_FocusApp(apps::APP_SHIM_FOCUS_NORMAL,
-                                  std::vector<base::FilePath>())));
+  GetMojoHost()->FocusApp(apps::APP_SHIM_FOCUS_NORMAL,
+                          std::vector<base::FilePath>());
+  task_runner()->RunUntilIdle();
   EXPECT_EQ(1, focus_count_);
 
-  EXPECT_TRUE(host()->ReceiveMessage(new AppShimHostMsg_QuitApp()));
+  GetMojoHost()->QuitApp();
+  task_runner()->RunUntilIdle();
   EXPECT_EQ(1, quit_count_);
 
   SimulateDisconnect();
+  task_runner()->RunUntilIdle();
   EXPECT_EQ(1, close_count_);
+  EXPECT_EQ(nullptr, host());
   apps::AppShimHandler::RemoveHandler(kTestAppId);
 }
 

chrome/browser/apps/app_shim/app_shim_host_manager_browsertest_mac.mm

@@ -9,8 +9,8 @@
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/macros.h"
-#include "base/message_loop/message_loop.h"
 #include "base/path_service.h"
+#include "base/run_loop.h"
 #include "base/threading/thread_restrictions.h"
 #include "chrome/browser/apps/app_shim/app_shim_handler_mac.h"
 #include "chrome/browser/apps/app_shim/test/app_shim_host_manager_test_api_mac.h"
@@ -19,14 +19,12 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/mac/app_mode_common.h"
+#include "chrome/common/mac/app_shim.mojom.h"
 #include "chrome/common/mac/app_shim_messages.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "components/version_info/version_info.h"
 #include "content/public/test/test_utils.h"
-#include "ipc/ipc_channel_mojo.h"
-#include "ipc/ipc_channel_proxy.h"
-#include "ipc/ipc_listener.h"
-#include "ipc/ipc_message.h"
+#include "mojo/public/cpp/bindings/binding.h"
 #include "mojo/public/cpp/platform/named_platform_channel.h"
 #include "mojo/public/cpp/system/isolated_connection.h"
 
@@ -34,34 +32,36 @@ namespace {
 
 const char kTestAppMode[] = "test_app";
 
-// A test version of the AppShimController IPC client in chrome_main_app_mode.
-class TestShimClient : public IPC::Listener {
+// A test version of the AppShimController mojo client in chrome_main_app_mode.
+class TestShimClient : public chrome::mojom::AppShim {
  public:
   TestShimClient();
-  ~TestShimClient() override;
 
-  template <class T>
-  void Send(const T& message) {
-    channel_->Send(message);
+  chrome::mojom::AppShimPtr GetMojoPtr() {
+    chrome::mojom::AppShimPtr app_shim_ptr;
+    chrome::mojom::AppShimRequest app_shim_request =
+        mojo::MakeRequest(&app_shim_ptr);
+    shim_binding_.Bind(std::move(app_shim_request));
+    return app_shim_ptr;
   }
 
- private:
-  // IPC::Listener overrides:
-  bool OnMessageReceived(const IPC::Message& message) override;
-  void OnChannelError() override;
+  chrome::mojom::AppShimHostPtr& host() { return host_; }
+
+  // chrome::mojom::AppShim implementation (not used in testing, but can be).
+  void LaunchAppDone(apps::AppShimLaunchResult result) override {}
+  void Hide() override {}
+  void UnhideWithoutActivation() override {}
+  void SetUserAttention(apps::AppShimAttentionType attention_type) override {}
 
-  base::Thread io_thread_;
+ private:
   mojo::IsolatedConnection mojo_connection_;
-  std::unique_ptr<IPC::ChannelProxy> channel_;
+  mojo::Binding<chrome::mojom::AppShim> shim_binding_;
+  chrome::mojom::AppShimHostPtr host_;
 
   DISALLOW_COPY_AND_ASSIGN(TestShimClient);
 };
 
-TestShimClient::TestShimClient() : io_thread_("TestShimClientIO") {
-  base::Thread::Options io_thread_options;
-  io_thread_options.message_loop_type = base::MessageLoop::TYPE_IO;
-  io_thread_.StartWithOptions(io_thread_options);
-
+TestShimClient::TestShimClient() : shim_binding_(this) {
   base::FilePath user_data_dir;
   CHECK(base::PathService::Get(chrome::DIR_USER_DATA, &user_data_dir));
   base::FilePath symlink_path =
@@ -72,27 +72,10 @@ TestShimClient::TestShimClient() : io_thread_("TestShimClientIO") {
   CHECK(base::ReadSymbolicLink(symlink_path, &socket_path));
   app_mode::VerifySocketPermissions(socket_path);
 
-  channel_ = IPC::ChannelProxy::Create(
-      IPC::ChannelMojo::CreateClientFactory(
-          mojo_connection_.Connect(
-              mojo::NamedPlatformChannel::ConnectToServer(socket_path.value())),
-          io_thread_.task_runner().get(), base::ThreadTaskRunnerHandle::Get()),
-      this, io_thread_.task_runner().get(),
-      base::ThreadTaskRunnerHandle::Get());
-}
-
-TestShimClient::~TestShimClient() {
-  base::ScopedAllowBlockingForTesting allow_blocking;
-  io_thread_.Stop();
-}
-
-bool TestShimClient::OnMessageReceived(const IPC::Message& message) {
-  return true;
-}
-
-void TestShimClient::OnChannelError() {
-  // Client should not get any channel errors for the current set of tests.
-  PLOG(FATAL) << "ChannelError";
+  mojo::ScopedMessagePipeHandle message_pipe = mojo_connection_.Connect(
+      mojo::NamedPlatformChannel::ConnectToServer(socket_path.value()));
+  host_ = chrome::mojom::AppShimHostPtr(
+      chrome::mojom::AppShimHostPtrInfo(std::move(message_pipe), 0));
 }
 
 // Browser Test for AppShimHostManager to test IPC interactions across the
@@ -100,8 +83,7 @@ void TestShimClient::OnChannelError() {
 class AppShimHostManagerBrowserTest : public InProcessBrowserTest,
                                       public apps::AppShimHandler {
  public:
-  AppShimHostManagerBrowserTest();
-  ~AppShimHostManagerBrowserTest() override;
+  AppShimHostManagerBrowserTest() {}
 
  protected:
   // Wait for OnShimLaunch, then send a quit, and wait for the response. Used to
@@ -126,34 +108,25 @@ class AppShimHostManagerBrowserTest : public InProcessBrowserTest,
 
   std::unique_ptr<TestShimClient> test_client_;
   std::vector<base::FilePath> last_launch_files_;
-  apps::AppShimLaunchType last_launch_type_;
+  apps::AppShimLaunchType last_launch_type_ = apps::APP_SHIM_LAUNCH_NUM_TYPES;
 
-private:
-  scoped_refptr<content::MessageLoopRunner> runner_;
+ private:
+  std::unique_ptr<base::RunLoop> runner_;
 
-  int launch_count_;
-  int quit_count_;
+  int launch_count_ = 0;
+  int quit_count_ = 0;
 
   DISALLOW_COPY_AND_ASSIGN(AppShimHostManagerBrowserTest);
 };
 
-AppShimHostManagerBrowserTest::AppShimHostManagerBrowserTest()
-    : last_launch_type_(apps::APP_SHIM_LAUNCH_NUM_TYPES),
-      launch_count_(0),
-      quit_count_(0) {
-}
-
-AppShimHostManagerBrowserTest::~AppShimHostManagerBrowserTest() {
-}
-
 void AppShimHostManagerBrowserTest::RunAndExitGracefully() {
-  runner_ = new content::MessageLoopRunner();
+  runner_ = std::make_unique<base::RunLoop>();
   EXPECT_EQ(0, launch_count_);
   runner_->Run();  // Will stop in OnShimLaunch().
   EXPECT_EQ(1, launch_count_);
 
-  runner_ = new content::MessageLoopRunner();
-  test_client_->Send(new AppShimHostMsg_QuitApp);
+  runner_ = std::make_unique<base::RunLoop>();
+  test_client_->host()->QuitApp();
   EXPECT_EQ(0, quit_count_);
   runner_->Run();  // Will stop in OnShimQuit().
   EXPECT_EQ(1, quit_count_);
@@ -190,11 +163,9 @@ void AppShimHostManagerBrowserTest::OnShimQuit(
 // Test regular launch, which would ask Chrome to launch the app.
 IN_PROC_BROWSER_TEST_F(AppShimHostManagerBrowserTest, LaunchNormal) {
   test_client_.reset(new TestShimClient());
-  test_client_->Send(new AppShimHostMsg_LaunchApp(
-      browser()->profile()->GetPath(),
-      kTestAppMode,
-      apps::APP_SHIM_LAUNCH_NORMAL,
-      std::vector<base::FilePath>()));
+  test_client_->host()->LaunchApp(
+      test_client_->GetMojoPtr(), browser()->profile()->GetPath(), kTestAppMode,
+      apps::APP_SHIM_LAUNCH_NORMAL, std::vector<base::FilePath>());
 
   RunAndExitGracefully();
   EXPECT_EQ(apps::APP_SHIM_LAUNCH_NORMAL, last_launch_type_);
@@ -204,11 +175,9 @@ IN_PROC_BROWSER_TEST_F(AppShimHostManagerBrowserTest, LaunchNormal) {
 // Test register-only launch, used when Chrome has already launched the app.
 IN_PROC_BROWSER_TEST_F(AppShimHostManagerBrowserTest, LaunchRegisterOnly) {
   test_client_.reset(new TestShimClient());
-  test_client_->Send(new AppShimHostMsg_LaunchApp(
-      browser()->profile()->GetPath(),
-      kTestAppMode,
-      apps::APP_SHIM_LAUNCH_REGISTER_ONLY,
-      std::vector<base::FilePath>()));
+  test_client_->host()->LaunchApp(
+      test_client_->GetMojoPtr(), browser()->profile()->GetPath(), kTestAppMode,
+      apps::APP_SHIM_LAUNCH_REGISTER_ONLY, std::vector<base::FilePath>());
 
   RunAndExitGracefully();
   EXPECT_EQ(apps::APP_SHIM_LAUNCH_REGISTER_ONLY, last_launch_type_);

chrome/common/BUILD.gn

@@ -684,6 +684,10 @@ mojom("mojo_bindings") {
     sources += [ "sandbox_status_extension_android.mojom" ]
   }
 
+  if (is_mac) {
+    sources += [ "mac/app_shim.mojom" ]
+  }
+
   if (is_win) {
     sources += [ "conflicts/module_event_sink_win.mojom" ]
   }

chrome/common/mac/OWNERS

@@ -6,3 +6,7 @@ thakis@chromium.org
 
 per-file *_messages*.h=set noparent
 per-file *_messages*.h=file://ipc/SECURITY_OWNERS
+per-file *.mojom=set noparent
+per-file *.mojom=file://ipc/SECURITY_OWNERS
+per-file *.typemap=set noparent
+per-file *.typemap=file://ipc/SECURITY_OWNERS

chrome/common/mac/app_shim.mojom

+// Copyright 2018 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.
+
+module chrome.mojom;
+
+import "mojo/public/mojom/base/file_path.mojom";
+
+[Native]
+enum AppShimLaunchType;
+
+[Native]
+enum AppShimLaunchResult;
+
+[Native]
+enum AppShimFocusType;
+
+[Native]
+enum AppShimAttentionType;
+
+// Interface through which the browser communicates to a shim process.
+interface AppShim {
+  // Signals that a previous LaunchApp message has been processed, and lets the
+  // app shim process know whether it was registered successfully.
+  LaunchAppDone(AppShimLaunchResult launch_result);
+
+  // Instructs the shim to hide the app.
+  Hide();
+
+  // Instructs the shim to show the app.
+  UnhideWithoutActivation();
+
+  // Instructs the shim to request or cancel user attention.
+  SetUserAttention(AppShimAttentionType attention_type);
+};
+
+// Interface through which the a process communicates to the browser process.
+interface AppShimHost {
+  // Signals to the main Chrome process that a shim has started. The app shim
+  // process is requesting to be associated with the given profile and app_id.
+  // Once the profile and app_id are stored, and all future messages from the
+  // app shim relate to this app.
+  LaunchApp(AppShim app_shim,
+            mojo_base.mojom.FilePath profile_dir,
+            string app_mode_id,
+            AppShimLaunchType launch_type,
+            array<mojo_base.mojom.FilePath> files);
+
+  // Sent when the user has indicated a desire to focus the app, either by
+  // clicking on the app's icon in the dock or by selecting it with Cmd+Tab. In
+  // response, Chrome brings the app's windows to the foreground, or relaunches
+  // if the focus type indicates a reopen and there are no open windows.
+  FocusApp(AppShimFocusType focus_type,
+           array<mojo_base.mojom.FilePath> files);
+
+  // Sent when the app shim is hidden or unhidden.
+  SetAppHidden(bool hidden);
+
+  // Called when the app shim process notifies that the app should quit.
+  QuitApp();
+};
+

chrome/common/mac/app_shim.typemap

+# Copyright 2018 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.
+
+mojom = "//chrome/common/mac/app_shim.mojom"
+
+public_headers = [ "//chrome/common/mac/app_shim_launch.h" ]
+
+traits_headers = [ "//chrome/common/mac/app_shim_messages.h" ]
+
+type_mappings = [
+  "chrome.mojom.AppShimLaunchType=apps::AppShimLaunchType",
+  "chrome.mojom.AppShimLaunchResult=apps::AppShimLaunchResult",
+  "chrome.mojom.AppShimFocusType=apps::AppShimFocusType",
+  "chrome.mojom.AppShimAttentionType=apps::AppShimAttentionType",
+]

chrome/common/mac/app_shim_messages.h

@@ -25,53 +25,4 @@ IPC_ENUM_TRAITS_MAX_VALUE(apps::AppShimFocusType,
 IPC_ENUM_TRAITS_MAX_VALUE(apps::AppShimAttentionType,
                           apps::APP_SHIM_ATTENTION_NUM_TYPES - 1)
 
-// IMPORTANT: Since app shims could be running a newer framework version to the
-// currently running Chrome, any changes to these IPCs must maintain the same
-// order and format. I.e. Only add to the bottom, don't delete any.
-
-// Signals that a previous LaunchApp message has been processed, and lets the
-// app shim process know whether it was registered successfully.
-IPC_MESSAGE_CONTROL1(AppShimMsg_LaunchApp_Done,
-                     apps::AppShimLaunchResult /* launch result */)
-
-// Instructs the shim to hide the app.
-IPC_MESSAGE_CONTROL0(AppShimMsg_Hide)
-
-// Deprecated. Do not delete.
-IPC_MESSAGE_CONTROL0(AppShimMsg_RequestUserAttention)
-
-// Signals to the main Chrome process that a shim has started. Indicates the
-// profile and app_id that the shim should be associated with and whether to
-// launch the app immediately.
-IPC_MESSAGE_CONTROL4(AppShimHostMsg_LaunchApp,
-                     base::FilePath /* profile dir */,
-                     std::string /* app id */,
-                     apps::AppShimLaunchType /* launch type */,
-                     std::vector<base::FilePath> /* files */)
-
-// Sent when the user has indicated a desire to focus the app, either by
-// clicking on the app's icon in the dock or by selecting it with Cmd+Tab. In
-// response, Chrome brings the app's windows to the foreground, or relaunches
-// if the focus type indicates a reopen and there are no open windows.
-IPC_MESSAGE_CONTROL2(AppShimHostMsg_FocusApp,
-                     apps::AppShimFocusType /* focus type */,
-                     std::vector<base::FilePath> /* files */)
-
-// Sent when the app shim is hidden or unhidden.
-IPC_MESSAGE_CONTROL1(AppShimHostMsg_SetAppHidden,
-                     bool /* hidden */)
-
-// Sent when the shim process receives a request to terminate. Once all of the
-// app's windows have closed, and the extension is unloaded, the AppShimHost
-// closes the channel. The shim process then completes the terminate request
-// and exits.
-IPC_MESSAGE_CONTROL0(AppShimHostMsg_QuitApp)
-
-// Instructs the shim to request or cancel user attention.
-IPC_MESSAGE_CONTROL1(AppShimMsg_SetUserAttention,
-                     apps::AppShimAttentionType /* attention_type */)
-
-// Instructs the shim to show the app.
-IPC_MESSAGE_CONTROL0(AppShimMsg_UnhideWithoutActivation)
-
 #endif  // CHROME_COMMON_MAC_APP_SHIM_MESSAGES_H_

chrome/typemaps.gni

@@ -4,6 +4,7 @@
 
 typemaps = [
   "//chrome/common/browser_controls_state.typemap",
+  "//chrome/common/mac/app_shim.typemap",
   "//chrome/common/search.typemap",
   "//chrome/common/web_application_info_provider.typemap",
   "//chrome/services/file_util/public/mojom/safe_archive_analyzer.typemap",