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");

[hidehiko, 2015/10/29 16:29:47]

nit: 4 indent.

[Luis Héctor Chávez, 2015/10/29 23:43:53]

Done.

+
+}  // namespace
+
+namespace arc {
+
+ArcBridgeService::ArcBridgeService(
+    const scoped_refptr<base::SequencedTaskRunner>& file_task_runner)
+  : ipc_thread_("ARC bridge listener"),

[hidehiko, 2015/10/29 16:29:47]

nit: 4 indent.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constructor_I...

[Luis Héctor Chávez, 2015/10/29 23:43:52]

Done.

+    origin_task_runner_(base::ThreadTaskRunnerHandle::Get()),
+    file_task_runner_(file_task_runner),
+    observer_list_(new base::ObserverListThreadSafe<Observer>()),
+    available_(false),
+    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_)
+    return;
+  SocketConnect(base::FilePath(kArcBridgeSocketPath));
+}
+
+void ArcBridgeService::Shutdown() {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  if (state_ == ArcBridgeService::STOPPED ||
+      state_ == ArcBridgeService::STOPPING) {
+    LOG(ERROR) << "Shutdown() called when ARC is not running";
+    return;
+  }
+  SetState(ArcBridgeService::STOPPING);
+  ipc_channel_->Close();

[Shuhei Takahashi, 2015/10/29 00:13:08]

ipc_channel_ can be null when state_ == CONNECTING.

[Luis Héctor Chávez, 2015/10/29 23:43:52]

Done.

+  ipc_channel_.reset();
+  base::PostTaskAndReplyWithResult(

[hidehiko, 2015/10/29 16:29:47]

In general, shutdown step is the reverse order of the set-up steps.
In this case, setting up is 1) creating a socket and then 2) register the DBus,
so IMHO, it's better to 1) unregister DBus first then, 2) deleting a socket.
WDYT?

[Luis Héctor Chávez, 2015/10/29 23:43:53]

SGTM, done.

+      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);
+}
+
+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_ && state_ != ArcBridgeService::STOPPED &&
+      state_ != ArcBridgeService::STOPPING)
+    Shutdown();

[Shuhei Takahashi, 2015/10/29 00:13:08]

I'm puzzled by asymmetry here that SetEnabled(false) calls Shutdown() but
SetEnabled(true) does not call HandleStartup().

Do you expect HandleStartup() is soon called by the same caller as
SetEnabled(true)? Then I think you can stop exposing HandleStartup() and
Shutdown(), and just provide SetEnabled() only.

[Luis Héctor Chávez, 2015/10/29 23:43:53]

HandleStartup() is called upon profile startup, whereas SetEnabled() can be
called later and with different values. Both functions are needed.

Eventually I will also start the connection when SetEnabled(true) is called
after HandleStartup() but I'm waiting to have more functionality to do so, like
the policy decision, but that will come later.

+  observer_list_->Notify(FROM_HERE,
+                         &Observer::OnEnabledChanged,
+                         enabled_);
+}
+
+bool ArcBridgeService::RegisterInputDevice(
+    const std::string& name, const std::string& device_type,
+    base::ScopedFD fd) {
+  // ipc_channel_->Send() is thread-safe, so there is no thread check.
+  if (state_ != ArcBridgeService::READY) {

[Shuhei Takahashi, 2015/10/29 00:13:08]

If this function is called from arbitrary threads, I think we need to protect
state_ and ipc_channel_ with a lock so ipc_channel_ is not nullified after we
check state_.

[Luis Héctor Chávez, 2015/10/29 23:43:53]

Ugh, yes. I was trying to avoid locks, but it's not going to be possible :/ To
avoid having to also protect |state_| (which should be protected by the
thread-checking code already), removing the READY requirement and relying only
on the channel being non-null.

+    LOG(ERROR) << "Called RegisterInputDevice when the service is not ready";
+    return false;
+  }
+  return ipc_channel_->Send(new ArcInstanceMsg_RegisterInputDevice(
+        name, device_type, base::FileDescriptor(fd.Pass())));
+}
+
+void ArcBridgeService::SocketConnect(const base::FilePath& socket_path) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  if (state_ != ArcBridgeService::STOPPED) {
+    LOG(ERROR) << "SocketConnect() called when instance is not stopped";
+    return;
+  }
+  SetState(ArcBridgeService::CONNECTING);
+  base::PostTaskAndReplyWithResult(
+      file_task_runner_.get(),
+      FROM_HERE,
+      base::Bind(&base::CreateDirectory, socket_path.DirName()),
+      base::Bind(&ArcBridgeService::SocketConnectAfterEnsureParentDirectory,
+                 weak_factory_.GetWeakPtr(),
+                 socket_path));
+}
+
+void ArcBridgeService::SocketConnectAfterEnsureParentDirectory(
+    const base::FilePath& socket_path, bool directory_present) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  if (state_ != ArcBridgeService::CONNECTING) {
+    LOG(ERROR) << "Shutdown requested while connecting";
+    return;
+  }
+  if (!directory_present) {
+    LOG(ERROR) << "Error creating directory for " << socket_path.value();
+    Shutdown();
+    return;
+  }
+
+  if (!Connect(IPC::ChannelHandle(socket_path.value()),
+               IPC::Channel::MODE_OPEN_NAMED_SERVER)) {
+    LOG(ERROR) << "Error connecting to " << socket_path.value();
+    Shutdown();
+    return;
+  }
+
+  base::PostTaskAndReplyWithResult(
+      file_task_runner_.get(),
+      FROM_HERE,
+      // TODO(lhchavez): Tighten the security around the socket by tying it to
+      // the user the instance will run as.
+      base::Bind(&base::SetPosixFilePermissions, socket_path, 0777),
+      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 (state_ != ArcBridgeService::CONNECTED) {
+    LOG(ERROR) << "Shutdown requested while connecting";
+    return;
+  }
+
+  if (!socket_permissions_success) {
+    LOG(ERROR) << "Error setting socket permissions for "
+               << socket_path.value();
+    Shutdown();
+    return;
+  }
+
+  // This will fail if the ArcInstanceService is not running on Chrome OS.
+  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());
+  // Testing code can have the state in STOPPED since it does not go through

[hidehiko, 2015/10/29 16:29:47]

How about DCHECK(state_ == CONNECTING);?
For testing, could you check my comment in the previous reply?

[Luis Héctor Chávez, 2015/10/29 23:43:53]

Keeping the current code as discussed offline.

+  // the whole connection flow.
+  if (state_ != ArcBridgeService::CONNECTING &&
+      state_ != ArcBridgeService::STOPPED) {
+    LOG(ERROR) << "Shutdown requested while connecting";
+    return false;
+  }
+
+  ipc_channel_ = IPC::ChannelProxy::Create(handle, mode, this,
+                                           ipc_thread_.task_runner().get());
+  if (!ipc_channel_)
+    return false;
+  SetState(ArcBridgeService::CONNECTED);

[hidehiko, 2015/10/29 16:29:47]

Let's move this to SocketConnectAfterEnsureParentDirectory.

[Luis Héctor Chávez, 2015/10/29 23:43:53]

Keeping the current code as discussed offline.

+  return true;
+}
+
+void ArcBridgeService::OnInstanceReady() {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  if (state_ != ArcBridgeService::CONNECTED) {
+    LOG(ERROR) << "Shutdown requested while connecting";
+    return;
+  }
+  SetState(ArcBridgeService::READY);
+}
+
+void ArcBridgeService::SetState(State state) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  if (state_ == state)
+    return;
+  state_ = state;
+  observer_list_->Notify(FROM_HERE,
+                         &Observer::OnStateChanged,
+                         state_);
+}
+
+void ArcBridgeService::FinishShutdownAfterSocketDeleted(bool socket_deleted) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  // STOPPING is the only valid state for this function.
+  CHECK(state_ == ArcBridgeService::STOPPING);
+  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(
+    chromeos::DBusMethodCallStatus status) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  if (state_ != ArcBridgeService::CONNECTED) {
+    LOG(ERROR) << "Shutdown requested while connecting";
+    return;
+  }
+  if (status != chromeos::DBUS_METHOD_CALL_SUCCESS) {
+    LOG(ERROR) << "ARC instance unable to start. Shutting down the bridge";
+    Shutdown();
+    return;
+  }
+  SetState(ArcBridgeService::STARTING);
+}
+
+void ArcBridgeService::OnInstanceStopped(
+    chromeos::DBusMethodCallStatus status) {
+  DCHECK(origin_task_runner_->RunsTasksOnCurrentThread());
+  // STOPPING is the only valid state for this function.
+  CHECK(state_ == ArcBridgeService::STOPPING);
+  SetState(ArcBridgeService::STOPPED);
+}
+
+}  // namespace arc