Start arc data removal directly from Chrome

Chrome had to proxy through session_manager to start the arc-remove-data
Upstart job, but now it calls it directly.

BUG=b:115779632
TEST=Disable ARC++ and ensure arc data removal occurs

Change-Id: I0f30570c665efc85538b9de4bcd63bb158b37417
Reviewed-on: https://chromium-review.googlesource.com/c/1311124Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Commit-Queue: Christopher Morin <cmtm@google.com>
Cr-Commit-Position: refs/heads/master@{#604724}

chromeos/dbus/fake_session_manager_client.cc

@@ -589,13 +589,6 @@ void FakeSessionManagerClient::GetArcStartTime(
       arc_available_ ? base::make_optional(arc_start_time_) : base::nullopt);
 }
 
-void FakeSessionManagerClient::RemoveArcData(
-    const cryptohome::AccountIdentifier& cryptohome_id,
-    VoidDBusMethodCallback callback) {
-  if (!callback.is_null())
-    PostReply(FROM_HERE, std::move(callback), arc_available_);
-}
-
 void FakeSessionManagerClient::NotifyArcInstanceStopped(
     login_manager::ArcContainerStopReason reason,
     const std::string& container_instance_id) {

chromeos/dbus/fake_session_manager_client.h

@@ -109,8 +109,6 @@ class FakeSessionManagerClient : public SessionManagerClient {
   void EmitArcBooted(const cryptohome::AccountIdentifier& cryptohome_id,
                      VoidDBusMethodCallback callback) override;
   void GetArcStartTime(DBusMethodCallback<base::TimeTicks> callback) override;
-  void RemoveArcData(const cryptohome::AccountIdentifier& cryptohome_id,
-                     VoidDBusMethodCallback callback) override;
 
   // Notifies observers as if ArcInstanceStopped signal is received.
   void NotifyArcInstanceStopped(login_manager::ArcContainerStopReason,

chromeos/dbus/session_manager_client.cc

@@ -474,18 +474,6 @@ class SessionManagerClientImpl : public SessionManagerClient {
                        weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }
 
-  void RemoveArcData(const cryptohome::AccountIdentifier& cryptohome_id,
-                     VoidDBusMethodCallback callback) override {
-    dbus::MethodCall method_call(login_manager::kSessionManagerInterface,
-                                 login_manager::kSessionManagerRemoveArcData);
-    dbus::MessageWriter writer(&method_call);
-    writer.AppendString(cryptohome_id.account_id());
-    session_manager_proxy_->CallMethod(
-        &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        base::BindOnce(&SessionManagerClientImpl::OnVoidMethod,
-                       weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
-  }
-
  protected:
   void Init(dbus::Bus* bus) override {
     session_manager_proxy_ = bus->GetObjectProxy(

chromeos/dbus/session_manager_client.h

@@ -365,13 +365,6 @@ class CHROMEOS_EXPORT SessionManagerClient : public DBusClient {
   virtual void GetArcStartTime(
       DBusMethodCallback<base::TimeTicks> callback) = 0;
 
-  // Asynchronously removes all ARC user data for the user whose cryptohome is
-  // located by |cryptohome_id|. Upon completion, invokes |callback| with the
-  // result; true on success, false on failure (either session manager failed
-  // to remove user data or session manager can not be reached).
-  virtual void RemoveArcData(const cryptohome::AccountIdentifier& cryptohome_id,
-                             VoidDBusMethodCallback callback) = 0;
-
   // Creates the instance.
   static SessionManagerClient* Create(DBusClientImplementationType type);
 

components/arc/arc_data_remover.cc

@@ -10,11 +10,15 @@
 #include "base/callback_helpers.h"
 #include "base/logging.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
-#include "chromeos/dbus/session_manager_client.h"
+#include "chromeos/dbus/upstart_client.h"
 #include "components/arc/arc_prefs.h"
 
 namespace arc {
 
+// The conversion of upstart job names to dbus object paths is undocumented. See
+// function nih_dbus_path in libnih for the implementation.
+constexpr char kArcRemoveDataUpstartJob[] = "arc_2dremove_2ddata";
+
 ArcDataRemover::ArcDataRemover(PrefService* prefs,
                                const cryptohome::Identification& cryptohome_id)
     : cryptohome_id_(cryptohome_id), weak_factory_(this) {
@@ -42,11 +46,13 @@ void ArcDataRemover::Run(RunCallback callback) {
   }
 
   VLOG(1) << "Starting ARC data removal";
-  auto* session_manager_client =
-      chromeos::DBusThreadManager::Get()->GetSessionManagerClient();
-  DCHECK(session_manager_client);
-  session_manager_client->RemoveArcData(
-      cryptohome::CreateAccountIdentifierFromIdentification(cryptohome_id_),
+  auto* upstart_client = chromeos::DBusThreadManager::Get()->GetUpstartClient();
+  DCHECK(upstart_client);
+  const std::string account_id =
+      cryptohome::CreateAccountIdentifierFromIdentification(cryptohome_id_)
+          .account_id();
+  upstart_client->StartJob(
+      kArcRemoveDataUpstartJob, {"CHROMEOS_USER=" + account_id},
       base::AdaptCallbackForRepeating(
           base::BindOnce(&ArcDataRemover::OnDataRemoved,
                          weak_factory_.GetWeakPtr(), std::move(callback))));
@@ -59,7 +65,7 @@ void ArcDataRemover::OnDataRemoved(RunCallback callback, bool success) {
     VLOG(1) << "ARC data removal successful";
   } else {
     LOG(ERROR) << "Request for ARC user data removal failed. "
-               << "See session_manager logs for more details.";
+               << "See upstart logs for more details.";
   }
   pref_.SetValue(false);
 

components/arc/arc_data_remover_unittest.cc

@@ -9,9 +9,10 @@
 #include "base/bind.h"
 #include "base/run_loop.h"
 #include "base/test/scoped_task_environment.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "chromeos/cryptohome/cryptohome_parameters.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
-#include "chromeos/dbus/fake_session_manager_client.h"
+#include "chromeos/dbus/fake_upstart_client.h"
 #include "components/account_id/account_id.h"
 #include "components/arc/arc_prefs.h"
 #include "components/prefs/testing_pref_service.h"
@@ -20,13 +21,33 @@
 namespace arc {
 namespace {
 
+class TestUpstartClient : public chromeos::FakeUpstartClient {
+ public:
+  TestUpstartClient() = default;
+  ~TestUpstartClient() override = default;
+
+  void StartJob(const std::string& job,
+                const std::vector<std::string>& upstart_env,
+                chromeos::VoidDBusMethodCallback callback) override {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(std::move(callback), arc_available_));
+  }
+
+  void set_arc_available(bool arc_available) { arc_available_ = arc_available; }
+
+ private:
+  bool arc_available_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(TestUpstartClient);
+};
+
 class ArcDataRemoverTest : public testing::Test {
  public:
   ArcDataRemoverTest() = default;
 
   void SetUp() override {
-    chromeos::DBusThreadManager::GetSetterForTesting()->SetSessionManagerClient(
-        std::make_unique<chromeos::FakeSessionManagerClient>());
+    chromeos::DBusThreadManager::GetSetterForTesting()->SetUpstartClient(
+        std::make_unique<TestUpstartClient>());
     chromeos::DBusThreadManager::Initialize();
 
     prefs::RegisterProfilePrefs(prefs_.registry());
@@ -40,9 +61,9 @@ class ArcDataRemoverTest : public testing::Test {
     return cryptohome_id_;
   }
 
-  chromeos::FakeSessionManagerClient* session_manager_client() {
-    return static_cast<chromeos::FakeSessionManagerClient*>(
-        chromeos::DBusThreadManager::Get()->GetSessionManagerClient());
+  TestUpstartClient* upstart_client() {
+    return static_cast<TestUpstartClient*>(
+        chromeos::DBusThreadManager::Get()->GetUpstartClient());
   }
 
  private:
@@ -68,7 +89,7 @@ TEST_F(ArcDataRemoverTest, NotScheduled) {
 }
 
 TEST_F(ArcDataRemoverTest, Success) {
-  session_manager_client()->set_arc_available(true);
+  upstart_client()->set_arc_available(true);
 
   ArcDataRemover data_remover(prefs(), cryptohome_id());
   data_remover.Schedule();