[mac] App shims

chrome/browser/web_applications/app_shim_host_controller.mm

Index: chrome/browser/web_applications/app_shim_host_controller.mm
diff --git a/chrome/browser/web_applications/app_shim_host_controller.mm b/chrome/browser/web_applications/app_shim_host_controller.mm
new file mode 100644
index 0000000000000000000000000000000000000000..199b3bdaacafe7c2d318f665c59316c69597dc1a
--- /dev/null
+++ b/chrome/browser/web_applications/app_shim_host_controller.mm
@@ -0,0 +1,229 @@
+// Copyright 2013 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.
+
+#import <Cocoa/Cocoa.h>
+
+#include "chrome/browser/web_applications/app_shim_host_controller.h"
+
+#include "base/bind.h"
+#include "base/files/file_path.h"
+#include "base/logging.h"
+#include "base/path_service.h"
+#include "base/threading/non_thread_safe.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/extensions/extension_host.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_system.h"
+#include "chrome/browser/extensions/shell_window_registry.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/ui/extensions/application_launch.h"
+#include "chrome/browser/ui/extensions/shell_window.h"
+#include "chrome/common/app_shim_messages.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/common/extensions/extension_constants.h"
+#include "chrome/common/mac/app_mode_common.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+#include "ipc/ipc_channel_proxy.h"
+#include "ipc/ipc_listener.h"
+#include "ipc/ipc_message.h"
+
+//-----------------------------------------------------------------------------
+
+// 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 content::NotificationObserver,
+                    public base::NonThreadSafe {
+ public:
+  explicit AppShimHost(const IPC::ChannelHandle& channel);
+  virtual ~AppShimHost();
+
+ private:
+  // IPC::Listener implementation.
+  virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
+
+  // The app shim process is requesting that an app be launched. Once it has
+  // done so the profile and app id are stored, and all future messages from
+  // the app shim relate to the app it launched. It is an error for the app
+  // shim to send multiple launch messages.
+  void OnLaunchApp(std::string profile, std::string app_id);
+
+  bool LaunchAppImpl(const std::string& app_id, const std::string& profile_dir);

[palmer, 2013/03/11 19:28:18]

The app_id and profile[_dir] arguments are in reverse order compared to
OnLaunchApp. Seems slightly confusing to me.

[jeremya, 2013/03/12 03:45:36]

Haha, oops. The names were only wrong in the declaration.

+
+  // When the app closes, a signal is sent (by way of closing the channel) to
+  // the app shim to tell it to quit.
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
+  // Closes the channel and destroys the AppShimHost.
+  void Close();
+
+  IPC::ChannelProxy* channel_;
+

[palmer, 2013/03/11 19:28:18]

NIT: Remove this blank line? Or is channel_ different enough from the other
members that it deserves its own stanza?

[jeremya, 2013/03/12 03:45:36]

Done.

+  std::string app_id_;
+  Profile* profile_;
+  content::NotificationRegistrar registrar_;
+};
+
+AppShimHost::AppShimHost(const IPC::ChannelHandle& handle)
+    : channel_(NULL), profile_(NULL) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  channel_ = new IPC::ChannelProxy(handle, IPC::Channel::MODE_SERVER, this,
+      BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO));
+}
+
+AppShimHost::~AppShimHost() {
+  DCHECK(CalledOnValidThread());
+  delete channel_;
+}
+
+bool AppShimHost::OnMessageReceived(const IPC::Message& message) {
+  DCHECK(CalledOnValidThread());
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(AppShimHost, message)
+    IPC_MESSAGE_HANDLER(AppShimHostMsg_LaunchApp, OnLaunchApp)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+
+  return handled;
+}
+
+void AppShimHost::OnLaunchApp(std::string profile_dir, std::string app_id) {
+  DCHECK(CalledOnValidThread());
+  if (!channel_)
+    return;
+  bool success = LaunchAppImpl(profile_dir, app_id);
+  channel_->Send(new AppShimMsg_LaunchApp_Done(success));
+}
+
+bool AppShimHost::LaunchAppImpl(const std::string& profile_dir,
+                                const std::string& app_id) {
+  DCHECK(CalledOnValidThread());
+  if (profile_) {
+    // Only one app launch message per channel.
+    return false;
+  }
+  if (profile_dir.length() == 0) {

[jeremy, 2013/03/11 09:32:04]

// profile_dir and app_id are received over IPC so we need to be extra defensive
here.

[palmer, 2013/03/11 19:28:18]

This isn't enough validation. This could be an arbitrary pathname, which is bad.
I propose that we validate that it is a basename *only*, and provide the parent
ourselves. Call basename(3) or (some equivalent from base/ ?) on profile_dir, to
enforce that it really is a basename and not a tricky pathname. You should also
enforce some lexical sanity on basename(profile_dir), so that it isn't something
like "...      evil|silly____))))::\\\\ASDFASDF    ".

    profile_dir = basename(profile_dir);
    if (!LexicallySane(profile_dir)) {
        // fail
    }
    true_profile_dir = user_data_dir + "/" + profile_dir;

LexicallySane should be something really simple, like a regexp match on
/^\w{8,20}$/.

That way this is no longer an arbitrary pathname IPC, but a tightly-defined
basename IPC.

Obviously, below on line 121 you do my last line of pseudocode, so you just need
to put basename and LexicallySane above 121. There might already be some
definition of LexicallySane for profile directories somewhere? I don't know.

[jeremya, 2013/03/12 03:45:36]

I see, I was hoping that ProfileManager::GetProfile would do that sanitisation,
but it doesn't seem to so far as I can tell.

- it also shouldn't be '..' or '.'
- ... how does anything ever manage to call this safely?

+    LOG(ERROR) << "Bad profile directory from app shim launch message.";
+    return false;
+  }
+  if (!extensions::Extension::IdIsValid(app_id)) {
+    LOG(ERROR) << "Bad app ID from app shim launch message.";
+    return false;
+  }
+  ProfileManager* profileManager = g_browser_process->profile_manager();
+  base::FilePath path(profile_dir);
+  path = profileManager->user_data_dir().Append(path);

[palmer, 2013/03/11 19:28:18]

Elsewhere, you use the expression PathService::Get(chrome::DIR_USER_DATA,
&user_data_dir). Is there a difference?

[jeremya, 2013/03/12 03:45:36]

Unless PathService::Get(chrome::DIR_USER_DATA, &user_data_dir) can vary within a
single session of Chrome, no (see BrowserProcessImpl::CreateProfileManager()).

+  profile_ = profileManager->GetProfile(path);
+  if (!profile_) {
+    LOG(ERROR) << "Unable to locate a suitable profile for profile directory '"
+               << profile_dir << "' while trying to load app with id '"
+               << app_id << "'.";
+    return false;
+  }
+  extensions::ExtensionSystem* extension_system =
+      extensions::ExtensionSystem::Get(profile_);
+  ExtensionServiceInterface* extension_service =
+      extension_system->extension_service();
+  const extensions::Extension* extension =
+      extension_service->GetExtensionById(
+          app_id, false);
+  if (!extension) {
+    LOG(ERROR) << "Shortcut attempted to launch nonexistent app with id '"
+               << app_id << "'.";
+    return false;
+  }
+  app_id_ = app_id;
+  chrome::AppLaunchParams params(profile_,
+                                 extension,
+                                 extension_misc::LAUNCH_NONE,
+                                 NEW_WINDOW);
+  chrome::OpenApplication(params);
+
+  registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,
+                 content::Source<Profile>(profile_));
+  return true;
+}
+
+void AppShimHost::Observe(int type,
+                          const content::NotificationSource& source,
+                          const content::NotificationDetails& details) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(content::Source<Profile>(source).ptr() == profile_);
+  switch (type) {
+    case chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED: {
+      extensions::ExtensionHost* extension_host =
+          content::Details<extensions::ExtensionHost>(details).ptr();
+      if (app_id_ == extension_host->extension_id())
+        Close();
+      break;
+    }
+    default:
+      NOTREACHED() << "Unexpected notification sent.";
+      break;
+  }
+}
+
+void AppShimHost::Close() {
+  DCHECK(CalledOnValidThread());
+  delete this;
+}
+
+//-----------------------------------------------------------------------------
+
+namespace {
+
+void CreateAppShimHost(const IPC::ChannelHandle& handle) {
+  new AppShimHost(handle);
+}
+
+}  // namespace
+
+AppShimHostController::AppShimHostController() : factory_(NULL) {
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&AppShimHostController::InitOnFileThread,
+          base::Unretained(this)));
+}
+
+AppShimHostController::~AppShimHostController() {
+  delete factory_;
+}
+
+void AppShimHostController::InitOnFileThread() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+  base::FilePath user_data_dir;
+  if (!PathService::Get(chrome::DIR_USER_DATA, &user_data_dir)) {
+    LOG(ERROR) << "Couldn't get user data directory while creating App Shim "
+               << "Host controller.";
+    return;
+  }
+  base::FilePath socket_path =
+      user_data_dir.Append(app_mode::kAppShimSocketName);
+  factory_ = new IPC::ChannelFactory(socket_path, this);
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&AppShimHostController::ListenOnIO, base::Unretained(this)));
+}
+
+void AppShimHostController::ListenOnIO() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  factory_->Listen();
+}
+
+void AppShimHostController::OnClientConnected(
+    const IPC::ChannelHandle& handle) {
+  // called on IO thread
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&CreateAppShimHost, handle));
+}
+
+void AppShimHostController::OnListenError() {
+  // TODO(jeremya): set a timeout and attempt to reconstruct the channel.
+}