arc-bridge: Add the ARC Bridge Service

components/arc/arc_bridge_service.cc

Index: components/arc/arc_bridge_service.cc
diff --git a/components/arc/arc_bridge_service.cc b/components/arc/arc_bridge_service.cc
new file mode 100644
index 0000000000000000000000000000000000000000..855b92d79c415dfee596159d6e286ef0953baf94
--- /dev/null
+++ b/components/arc/arc_bridge_service.cc
@@ -0,0 +1,227 @@
+// Copyright 2015 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 "components/arc/arc_bridge_service.h"
+
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/prefs/pref_registry_simple.h"
+#include "base/prefs/pref_service.h"
+#include "base/task_runner_util.h"
+#include "base/thread_task_runner_handle.h"
+#include "chromeos/arc/bridge/common/arc_host_messages.h"
+#include "chromeos/arc/bridge/common/arc_instance_messages.h"
+#include "chromeos/dbus/arc_instance_client.h"
+#include "chromeos/dbus/dbus_method_call_status.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "components/arc/arc_pref_names.h"
+#include "ipc/ipc_channel.h"
+
+namespace {
+
+const char kArcBridgeSocketPath[] = "/home/chronos/ArcBridge/bridge.sock";

[satorux1, 2015/10/28 05:45:22]

Please use const FilePath::CharType and FILE_PATH_LITERAL to follow Chrome's
common practice

[Luis Héctor Chávez, 2015/10/28 17:19:14]

Done.

+
+bool EnsureParentDirectoryPresent(const std::string& socket_path) {

[satorux1, 2015/10/28 05:45:22]

how about taking base::FilePath? 

The caller should pass base::FilePath(kArcBridgeSocketPath)

also function comment is missing. please check other places too

[Luis Héctor Chávez, 2015/10/28 17:19:14]

Done.

[hidehiko, 2015/10/28 17:48:45]

Or, in this case, we can inline the code in the caller?
E.g.) at L67:

// Before SocketConnect(), ensure its parent directory exists.
base::PostTaskAndReplyWithResult(
  file_task_runner_.get(),
  FROM_HERE,
  base::Bind(&base::CreateDirectory,
             base::FilePath(kArcBridgeSocketPath).DirName()),
  base::Bind(&ArcBridgeService::SocketConnect, ...));

[Luis Héctor Chávez, 2015/10/28 21:06:32]

Yep, that's cleaner. Done.

+  base::FilePath path(socket_path);
+  return base::CreateDirectory(path.DirName());
+}
+
+bool SocketPermissions(const std::string& socket_path) {

[satorux1, 2015/10/28 05:45:22]

Maybe SetSocketPermissions? std::string -> base::FilePath.

[Luis Héctor Chávez, 2015/10/28 17:19:14]

Done.

[hidehiko, 2015/10/28 17:48:45]

Or, does base::SetPosixFilePermissions work?

[Luis Héctor Chávez, 2015/10/28 21:06:32]

Done.

+  // TODO(lhchavez): Tighten the security around the socket by tying it to the
+  // user the instance will run as.
+  return HANDLE_EINTR(chmod(socket_path.c_str(), 0777)) == 0;
+}
+
+}  // namespace
+
+namespace arc {
+
+ArcBridgeService::ArcBridgeService(
+    const scoped_refptr<base::SequencedTaskRunner>& file_task_runner)
+  : ipc_thread_("ARC bridge listener"),
+    origin_task_runner_(base::ThreadTaskRunnerHandle::Get()),
+    file_task_runner_(file_task_runner),
+    state_(ArcBridgeService::DISABLED) {
+  ipc_thread_.StartWithOptions(
+      base::Thread::Options(base::MessageLoop::TYPE_IO, 0));
+}
+
+ArcBridgeService::~ArcBridgeService() {}
+
+// static
+void ArcBridgeService::RegisterPrefs(PrefRegistrySimple* registry) {
+  registry->RegisterBooleanPref(prefs::kArcEnabled, false);
+}
+
+// static
+bool ArcBridgeService::GetEnabledPref(PrefService* pref_service) {
+  // TODO(lhchavez): Once this is user-configurable, use the real pref.
+  return true;
+}
+
+void ArcBridgeService::HandleStartup() {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  if (state_ != ArcBridgeService::STOPPED)

[hidehiko, 2015/10/28 17:48:45]

Is this expected case?
If yes, it's good to comment when this happens, IMO.
If not, CHECK family (or at least LOG) would be your friend.

Also, as this does not update the state_, IPC channel could be created twice or
more,
if this is called multiple-times quickly.

[Luis Héctor Chávez, 2015/10/28 21:06:32]

Done

+    return;
+  base::PostTaskAndReplyWithResult(

[hidehiko, 2015/10/28 17:48:45]

Just a suggestion.
1) It seems like directory creation is a part of SocketConnect, rather than a
part of HandleStartup()?
2) How about making a naming rule for async functions?

E.g.;
Here:
SocketConnect(base::FilePath(kArcBridgeSocketPath));

:

void SocketConnect(const base::FilePath& socket_path) {
  base::PostTaskAndReplyWithResult(
     :
     base::Bind(base::CreateDirectory, socket_path.Dir());
     base::Bind(SocketConnectAfterEnsureParentDirectory, ..., socket_path));
}


void SocketConnectAfterEnsureParentDirectory(const base::FilePath& socket_path,
                                             bool
is_ensure_parent_directory_succeeded) {
   :
   if (!ConnectIPCChannel(...)) {
     :
   }

   base::PostTaskAndReplyWithResult(
     :
     base::Bind(SetSocketPermission, ...),
     base::Bind(SocketConnectAfterSetSocketPermission, ...));
}

  :

And so on...
Some more notes: it'd be better to use |socket_path| or kArcBridgeSocketPath
consistently.
Current code seems to use in a mixed way.

[Luis Héctor Chávez, 2015/10/28 21:06:32]

Done.

+      file_task_runner_.get(),
+      FROM_HERE,
+      base::Bind(&EnsureParentDirectoryPresent, kArcBridgeSocketPath),
+      base::Bind(&ArcBridgeService::SocketConnect,
+                 base::Unretained(this), kArcBridgeSocketPath));

[satorux1, 2015/10/28 05:45:22]

How about using WeakPtrFactory and remove use of Unretained() from this class?
As mentioned elsewhere, Unretained() should generally be avoided, and Chrome's
common practice is to use WeakPtr for the 2nd callback for PostTaskAndRely,
like:

[Luis Héctor Chávez, 2015/10/28 17:19:14]

All other KeyedServices I saw used Unretained(), but done.

+}
+
+void ArcBridgeService::Shutdown() {

[hidehiko, 2015/10/28 17:48:45]

Looks to have a race issue?

If this is called while HandleStartup() but not completed,
then this instance would get into a something weird state.

[Luis Héctor Chávez, 2015/10/28 21:06:32]

All state transitions are now guarded.

+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  if (state_ != ArcBridgeService::CONNECTED &&
+      state_ != ArcBridgeService::STARTING &&
+      state_ != ArcBridgeService::READY)

[hidehiko, 2015/10/28 17:48:45]

Maybe logging?

[Luis Héctor Chávez, 2015/10/28 21:06:32]

Done.

+    return;
+  ipc_channel_->Close();
+  base::PostTaskAndReplyWithResult(
+      file_task_runner_.get(),
+      FROM_HERE,
+      base::Bind(&base::DeleteFile, base::FilePath(kArcBridgeSocketPath),
+                 false),
+      base::Bind(&ArcBridgeService::FinishShutdownAfterSocketDeleted,
+                 base::Unretained(this)));
+}
+
+void ArcBridgeService::AddObserver(Observer* observer) {
+  observer_list_.AddObserver(observer);
+}
+
+void ArcBridgeService::RemoveObserver(Observer* observer) {
+  observer_list_.RemoveObserver(observer);
+}
+
+void ArcBridgeService::SetEnabled(bool enabled) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+
+  if ((!enabled && state_ == ArcBridgeService::DISABLED) ||
+      (enabled && state_ != ArcBridgeService::DISABLED))
+    return;
+  if (state_ != ArcBridgeService::DISABLED) {
+    Shutdown();
+    NotifyStateChanged(ArcBridgeService::DISABLED);
+    return;
+  }
+  NotifyStateChanged(ArcBridgeService::STOPPED);
+}
+
+bool ArcBridgeService::RegisterInputDevice(
+    const std::string& name, const std::string& device_type,
+    base::ScopedFD fd) {

[satorux1, 2015/10/28 05:45:22]

on which thread should this be called? Add a DCHECK ?

[Luis Héctor Chávez, 2015/10/28 17:19:14]

It doesn't matter, IPC::ChannelProxy has a thread-safe Send.

[hidehiko, 2015/10/28 17:48:45]

FYI: I believe this runs on the browser thread. Assuming that, ScopedFD would be
somehow danger, because it would try to close fd in dtor on the thread
(BrowserThread), where the FD operation is not allowed.

[Luis Héctor Chávez, 2015/10/28 21:06:32]

This can run on any thread (you're the second to mention this, so it deserves a
code comment), so the caller must ensure it can safely pass the ScopedFD in the
first place. Also, since we are using Pass(), it will not even try to close it.

+  return ipc_channel_->Send(new ArcInstanceMsg_RegisterInputDevice(
+        name, device_type, base::FileDescriptor(fd.Pass())));
+}
+
+void ArcBridgeService::SocketConnect(const std::string& socket_path,
+                                     bool directory_creation_success) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+
+  if (!directory_creation_success) {
+    LOG(ERROR) << "Error creating directory for " << socket_path;
+    NotifyStateChanged(ArcBridgeService::ERROR);
+    return;
+  }
+
+  if (!Connect(IPC::ChannelHandle(socket_path),
+               IPC::Channel::MODE_OPEN_NAMED_SERVER)) {
+    LOG(ERROR) << "Error connecting to " << socket_path;
+    NotifyStateChanged(ArcBridgeService::ERROR);
+    return;
+  }
+
+  base::PostTaskAndReplyWithResult(
+      file_task_runner_.get(),
+      FROM_HERE,
+      base::Bind(&SocketPermissions, socket_path),
+      base::Bind(&ArcBridgeService::FinishConnectAfterSocketPermissions,
+                 base::Unretained(this), socket_path));
+}
+
+void ArcBridgeService::FinishConnectAfterSocketPermissions(
+    const std::string& socket_path, bool socket_permissions_success) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+
+  if (!socket_permissions_success) {
+    LOG(ERROR) << "Error setting socket permissions for " << socket_path;
+    NotifyStateChanged(ArcBridgeService::ERROR);
+    return;
+  }
+
+  // This will fail if the ArcInstanceService is not running on Chrome OS.
+  // Use base::Unretained since this is a KeyedService.
+  chromeos::DBusThreadManager::Get()->GetArcInstanceClient()->StartInstance(
+      socket_path,
+      base::Bind(&ArcBridgeService::OnInstanceStarted, base::Unretained(this)));
+}
+
+
+bool ArcBridgeService::Connect(const IPC::ChannelHandle& handle,
+                               IPC::Channel::Mode mode) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+
+  ipc_channel_ = IPC::ChannelProxy::Create(handle, mode, this,
+                                           ipc_thread_.task_runner().get());
+  if (!ipc_channel_)
+    return false;
+  NotifyStateChanged(ArcBridgeService::CONNECTED);
+  return true;
+}
+
+void ArcBridgeService::OnInstanceReady() {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  NotifyStateChanged(ArcBridgeService::READY);
+}
+
+void ArcBridgeService::NotifyStateChanged(State state) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  if (state_ == state)
+    return;
+  state_ = state;
+  FOR_EACH_OBSERVER(Observer, observer_list_, OnStateChanged(state_));
+}
+
+void ArcBridgeService::FinishShutdownAfterSocketDeleted(bool socket_deleted) {
+  NotifyStateChanged(ArcBridgeService::STOPPED);
+  if (state_ != ArcBridgeService::STARTING && state_ != ArcBridgeService::READY)
+    return;
+  chromeos::DBusThreadManager::Get()->GetArcInstanceClient()->StopInstance(
+      base::Bind(&ArcBridgeService::OnInstanceStopped, base::Unretained(this)));
+}
+
+bool ArcBridgeService::OnMessageReceived(const IPC::Message& message) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  bool handled = true;
+
+  IPC_BEGIN_MESSAGE_MAP(ArcBridgeService, message)
+    IPC_MESSAGE_HANDLER(ArcInstanceHostMsg_InstanceReady, OnInstanceReady)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+
+  if (!handled)
+    LOG(ERROR) << "Invalid message with type = " << message.type();
+  return handled;
+}
+
+void ArcBridgeService::OnInstanceStarted(
+    chromeos::DBusMethodCallStatus status) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  if (status != chromeos::DBUS_METHOD_CALL_SUCCESS) {
+    LOG(ERROR) << "ARC instance unable to start. Shutting down the bridge";
+    Shutdown();
+    return;
+  }
+  NotifyStateChanged(ArcBridgeService::STARTING);
+}
+
+void ArcBridgeService::OnInstanceStopped(
+    chromeos::DBusMethodCallStatus status) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  NotifyStateChanged(ArcBridgeService::STOPPED);
+}
+
+}  // namespace arc