arc-bridge: Add the ARC Bridge Service

components/arc/arc_bridge_service.cc

+// 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 base::FilePath::CharType kArcBridgeSocketPath[] =
+  FILE_PATH_LITERAL("/home/chronos/ArcBridge/bridge.sock");
+
+// Ensures the parent directory of |socket_path| is present. Returns true if it
+// was created or if it was already present.
+bool EnsureParentDirectoryPresent(const base::FilePath& socket_path) {
+  base::FilePath path(socket_path);
+  return base::CreateDirectory(path.DirName());
+}
+
+// Changes the permissions on the socket so that the ARC instance can use it.
+bool SetSocketPermissions(const base::FilePath& socket_path) {
+  // TODO(lhchavez): Tighten the security around the socket by tying it to the

[Shuhei Takahashi, 2015/10/28 21:13:57]

Could you file a bug so we don't forget this?

[Luis Héctor Chávez, 2015/10/28 23:12:20]

It's part of the same bug.

+  // user the instance will run as.
+  return HANDLE_EINTR(chmod(socket_path.value().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),
+    available_(false),

[Shuhei Takahashi, 2015/10/28 21:13:57]

Hmm,I don't see any code setting available_ to true.

[Luis Héctor Chávez, 2015/10/28 23:12:20]

It's not there yet, but I want to have it available now.

+    enabled_(false),
+    state_(ArcBridgeService::STOPPED),
+    weak_factory_(this) {
+  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 (!enabled_ || state_ != ArcBridgeService::STOPPED)
+    return;
+  base::PostTaskAndReplyWithResult(
+      file_task_runner_.get(),
+      FROM_HERE,
+      base::Bind(&EnsureParentDirectoryPresent,
+                 base::FilePath(kArcBridgeSocketPath)),
+      base::Bind(&ArcBridgeService::SocketConnect,
+                 weak_factory_.GetWeakPtr(),
+                 base::FilePath(kArcBridgeSocketPath)));
+}
+
+void ArcBridgeService::Shutdown() {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  if (state_ != ArcBridgeService::CONNECTED &&
+      state_ != ArcBridgeService::STARTING &&
+      state_ != ArcBridgeService::READY)
+    return;
+  ipc_channel_->Close();

[Shuhei Takahashi, 2015/10/28 21:13:57]

Could you set ipc_channel_ to null?

[Luis Héctor Chávez, 2015/10/28 23:12:20]

Done.

+  base::PostTaskAndReplyWithResult(
+      file_task_runner_.get(),
+      FROM_HERE,
+      base::Bind(&base::DeleteFile, base::FilePath(kArcBridgeSocketPath),
+                 false),
+      base::Bind(&ArcBridgeService::FinishShutdownAfterSocketDeleted,
+                 weak_factory_.GetWeakPtr()));
+}
+
+void ArcBridgeService::AddObserver(Observer* observer) {
+  observer_list_.AddObserver(observer);

[Shuhei Takahashi, 2015/10/28 21:13:57]

ObserverList is not thread-safe. Could you insert a thread assertion here, or if
the list can be accessed from multiple threads, please use
ObserverListThreadSafe.

[Luis Héctor Chávez, 2015/10/28 23:12:20]

Done.

+}
+
+void ArcBridgeService::RemoveObserver(Observer* observer) {
+  observer_list_.RemoveObserver(observer);
+}
+
+void ArcBridgeService::SetEnabled(bool enabled) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+
+  if (enabled_ == enabled)
+    return;
+  enabled_ = enabled;
+  if (!enabled_)
+    Shutdown();

[Shuhei Takahashi, 2015/10/28 21:13:57]

Shutdown() will fail when state_ is either STOPPED or ERROR.

Could you clarify when SetEnabled() is called? I'm not sure the lifetime of
ArcBridgeService. Does it outlive multiple sessions and SetEnabled() called
multiple times? Or is SetEnabled() called at most once when the session started?

[Luis Héctor Chávez, 2015/10/28 23:12:20]

Good catch. Done.

The lifetime of ArcBridgeService is tied to the session. SetEnabled can be
called multiple times since the preference can be changed.

+  FOR_EACH_OBSERVER(Observer, observer_list_, OnEnabledChanged(enabled_));
+}
+
+bool ArcBridgeService::RegisterInputDevice(
+    const std::string& name, const std::string& device_type,
+    base::ScopedFD fd) {
+  return ipc_channel_->Send(new ArcInstanceMsg_RegisterInputDevice(

[Shuhei Takahashi, 2015/10/28 21:13:57]

Could you check state_? In some cases ipc_channel_ can be null.

[Luis Héctor Chávez, 2015/10/28 23:12:20]

Done.

+        name, device_type, base::FileDescriptor(fd.Pass())));
+}
+
+void ArcBridgeService::SocketConnect(const base::FilePath& socket_path,
+                                     bool directory_creation_success) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+
+  if (!directory_creation_success) {
+    LOG(ERROR) << "Error creating directory for " << socket_path.value();
+    NotifyStateChanged(ArcBridgeService::ERROR);
+    return;
+  }
+
+  if (!Connect(IPC::ChannelHandle(socket_path.value()),
+               IPC::Channel::MODE_OPEN_NAMED_SERVER)) {
+    LOG(ERROR) << "Error connecting to " << socket_path.value();
+    NotifyStateChanged(ArcBridgeService::ERROR);
+    return;
+  }
+
+  base::PostTaskAndReplyWithResult(
+      file_task_runner_.get(),
+      FROM_HERE,
+      base::Bind(&SetSocketPermissions, socket_path),
+      base::Bind(&ArcBridgeService::FinishConnectAfterSetSocketPermissions,
+                 weak_factory_.GetWeakPtr(), socket_path));
+}
+
+void ArcBridgeService::FinishConnectAfterSetSocketPermissions(
+    const base::FilePath& socket_path, bool socket_permissions_success) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+
+  if (!socket_permissions_success) {
+    LOG(ERROR) << "Error setting socket permissions for "
+               << socket_path.value();
+    NotifyStateChanged(ArcBridgeService::ERROR);
+    return;
+  }
+
+  // This will fail if the ArcInstanceService is not running on Chrome OS.
+  // Use base::Unretained since this is a KeyedService.

[Shuhei Takahashi, 2015/10/28 21:13:57]

Unretained seems not used here?

[Luis Héctor Chávez, 2015/10/28 23:12:20]

Missed that one. Done.

+  chromeos::DBusThreadManager::Get()->GetArcInstanceClient()->StartInstance(
+      socket_path.value(),
+      base::Bind(&ArcBridgeService::OnInstanceStarted,
+                 weak_factory_.GetWeakPtr()));
+}
+
+
+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);

[Shuhei Takahashi, 2015/10/28 21:13:57]

It is confusing that state is changed only when IPC::ChannelProxy::Create has
succeeded. Please move this line inside SocketConnect().

[Luis Héctor Chávez, 2015/10/28 23:12:20]

Connect is also used from tests, so it needs to stay here.

+  return true;
+}
+
+void ArcBridgeService::OnInstanceReady() {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  NotifyStateChanged(ArcBridgeService::READY);
+}
+
+void ArcBridgeService::NotifyStateChanged(State state) {

[Shuhei Takahashi, 2015/10/28 21:13:57]

This function does set the current state, not only notifying observers. Could
you name this SetState() or something?

[Luis Héctor Chávez, 2015/10/28 23:12:20]

Done.

+  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) {
+  if (state_ != ArcBridgeService::STARTING && state_ != ArcBridgeService::READY)
+    return;

[Shuhei Takahashi, 2015/10/28 21:13:57]

In this code path, state_ is not reset to STOPPED.

[Luis Héctor Chávez, 2015/10/28 23:12:20]

It still needs to wait for the DBus message to return. Otherwise a different
message to start can be delivered while this is in-flight and get in a weird
state. This code was also changed in the next revision.

+  chromeos::DBusThreadManager::Get()->GetArcInstanceClient()->StopInstance(
+      base::Bind(&ArcBridgeService::OnInstanceStopped,
+                 weak_factory_.GetWeakPtr()));
+}
+
+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(

[Shuhei Takahashi, 2015/10/28 21:13:57]

Could you swap the position of OnInstanceStarted() and OnInstanceReady()? The
definition order looks strange.

[Luis Héctor Chávez, 2015/10/28 23:12:20]

I don't want to reorder any further until it's ready to commit to avoid
polluting the diffs any more.

+    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