Remove explicit singletonness of ArcBridgeService part 4.

This CL gets rid of using ArcBridgeService::Get() in
arc_robot_auto_code_fetcher_browsertest, by getting rid of
depending of ArcAuthService.

Test for AttemptUserExit is moved to arc_session_manager_unittest.
For that purpose, fixed username_hash for ARC Kiosk fake user.

BUG=657687
BUG=b/31079732
TEST=Ran bots.

Review-Url: https://codereview.chromium.org/2553193002
Cr-Commit-Position: refs/heads/master@{#437811}

chrome/browser/chromeos/arc/arc_session_manager.cc

@@ -81,7 +81,9 @@ ash::ShelfDelegate* GetShelfDelegate() {
 }  // namespace
 
 ArcSessionManager::ArcSessionManager(ArcBridgeService* bridge_service)
-    : ArcService(bridge_service), weak_ptr_factory_(this) {
+    : ArcService(bridge_service),
+      attempt_user_exit_callback_(base::Bind(chrome::AttemptUserExit)),
+      weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!g_arc_session_manager);
   g_arc_session_manager = this;
@@ -291,7 +293,7 @@ void ArcSessionManager::OnProvisioningFinished(ProvisioningResult result) {
       VLOG(1) << "Robot account auth code fetching error";
       // Log out the user. All the cleanup will be done in Shutdown() method.
       // The callback is not called because auth code is empty.
-      chrome::AttemptUserExit();
+      attempt_user_exit_callback_.Run();
       return;
     }
 
@@ -898,6 +900,12 @@ void ArcSessionManager::OnSendFeedbackClicked() {
   chrome::OpenFeedbackDialog(nullptr);
 }
 
+void ArcSessionManager::SetAttemptUserExitCallbackForTesting(
+    const base::Closure& callback) {
+  DCHECK(!callback.is_null());
+  attempt_user_exit_callback_ = callback;
+}
+
 std::ostream& operator<<(std::ostream& os,
                          const ArcSessionManager::State& state) {
   switch (state) {

chrome/browser/chromeos/arc/arc_session_manager.h

@@ -206,6 +206,8 @@ class ArcSessionManager : public ArcService,
 
   void OnProvisioningFinished(ProvisioningResult result);
 
+  void SetAttemptUserExitCallbackForTesting(const base::Closure& callback);
+
  private:
   // Negotiates the terms of service to user.
   void StartTermsOfServiceNegotiation();
@@ -254,6 +256,7 @@ class ArcSessionManager : public ArcService,
   std::unique_ptr<ArcAndroidManagementChecker> android_management_checker_;
 
   base::Time sign_in_time_;
+  base::Closure attempt_user_exit_callback_;
 
   base::WeakPtrFactory<ArcSessionManager> weak_ptr_factory_;
 

chrome/browser/chromeos/arc/arc_session_manager_unittest.cc

@@ -48,12 +48,12 @@
 
 namespace arc {
 
-class ArcSessionManagerTest : public testing::Test {
+class ArcSessionManagerTestBase : public testing::Test {
  public:
-  ArcSessionManagerTest()
+  ArcSessionManagerTestBase()
       : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
-        user_manager_enabler_(new chromeos::FakeChromeUserManager) {}
-  ~ArcSessionManagerTest() override = default;
+        user_manager_enabler_(new chromeos::FakeChromeUserManager()) {}
+  ~ArcSessionManagerTestBase() override = default;
 
   void SetUp() override {
     chromeos::DBusThreadManager::GetSetterForTesting()->SetSessionManagerClient(
@@ -67,6 +67,7 @@ class ArcSessionManagerTest : public testing::Test {
 
     EXPECT_TRUE(temp_dir_.CreateUniqueTempDir());
     TestingProfile::Builder profile_builder;
+    profile_builder.SetProfileName("user@gmail.com");
     profile_builder.SetPath(temp_dir_.GetPath().AppendASCII("TestArcProfile"));
 
     profile_ = profile_builder.Build();
@@ -81,11 +82,6 @@ class ArcSessionManagerTest : public testing::Test {
     // Check initial conditions.
     EXPECT_TRUE(bridge_service()->stopped());
 
-    const AccountId account_id(
-        AccountId::FromUserEmailGaiaId("user@gmail.com", "1234567890"));
-    GetFakeUserManager()->AddUser(account_id);
-    GetFakeUserManager()->LoginUser(account_id);
-
     chromeos::WallpaperManager::Initialize();
   }
 
@@ -127,11 +123,10 @@ class ArcSessionManagerTest : public testing::Test {
   void StartPreferenceSyncing() const {
     PrefServiceSyncableFromProfile(profile_.get())
         ->GetSyncableService(syncer::PREFERENCES)
-        ->MergeDataAndStartSyncing(syncer::PREFERENCES, syncer::SyncDataList(),
-                                   std::unique_ptr<syncer::SyncChangeProcessor>(
-                                       new syncer::FakeSyncChangeProcessor),
-                                   std::unique_ptr<syncer::SyncErrorFactory>(
-                                       new syncer::SyncErrorFactoryMock()));
+        ->MergeDataAndStartSyncing(
+            syncer::PREFERENCES, syncer::SyncDataList(),
+            base::MakeUnique<syncer::FakeSyncChangeProcessor>(),
+            base::MakeUnique<syncer::SyncErrorFactoryMock>());
   }
 
   content::TestBrowserThreadBundle thread_bundle_;
@@ -141,6 +136,23 @@ class ArcSessionManagerTest : public testing::Test {
   chromeos::ScopedUserManagerEnabler user_manager_enabler_;
   base::ScopedTempDir temp_dir_;
 
+  DISALLOW_COPY_AND_ASSIGN(ArcSessionManagerTestBase);
+};
+
+class ArcSessionManagerTest : public ArcSessionManagerTestBase {
+ public:
+  ArcSessionManagerTest() = default;
+
+  void SetUp() override {
+    ArcSessionManagerTestBase::SetUp();
+
+    const AccountId account_id(AccountId::FromUserEmailGaiaId(
+        profile()->GetProfileUserName(), "1234567890"));
+    GetFakeUserManager()->AddUser(account_id);
+    GetFakeUserManager()->LoginUser(account_id);
+  }
+
+ private:
   DISALLOW_COPY_AND_ASSIGN(ArcSessionManagerTest);
 };
 
@@ -485,4 +497,38 @@ TEST_F(ArcSessionManagerTest, IgnoreSecondErrorReporting) {
   arc_session_manager()->Shutdown();
 }
 
+class ArcSessionManagerKioskTest : public ArcSessionManagerTestBase {
+ public:
+  ArcSessionManagerKioskTest() = default;
+
+  void SetUp() override {
+    ArcSessionManagerTestBase::SetUp();
+    const AccountId account_id(
+        AccountId::FromUserEmail(profile()->GetProfileUserName()));
+    GetFakeUserManager()->AddArcKioskAppUser(account_id);
+    GetFakeUserManager()->LoginUser(account_id);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ArcSessionManagerKioskTest);
+};
+
+TEST_F(ArcSessionManagerKioskTest, AuthFailure) {
+  profile()->GetPrefs()->SetBoolean(prefs::kArcEnabled, true);
+  arc_session_manager()->OnPrimaryUserProfilePrepared(profile());
+  EXPECT_EQ(ArcSessionManager::State::ACTIVE, arc_session_manager()->state());
+
+  // Replace chrome::AttemptUserExit() for testing.
+  // At the end of test, leave the dangling pointer |terminated|,
+  // assuming the callback is invoked exactly once in OnProvisioningFinished()
+  // and not invoked then, including TearDown().
+  bool terminated = false;
+  arc_session_manager()->SetAttemptUserExitCallbackForTesting(
+      base::Bind([](bool* terminated) { *terminated = true; }, &terminated));
+
+  arc_session_manager()->OnProvisioningFinished(
+      ProvisioningResult::CHROME_SERVER_COMMUNICATION_ERROR);
+  EXPECT_TRUE(terminated);
+}
+
 }  // namespace arc

chrome/browser/chromeos/arc/auth/arc_robot_auth_code_fetcher_browsertest.cc

@@ -12,15 +12,13 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/arc/arc_auth_service.h"
 #include "chrome/browser/chromeos/arc/arc_session_manager.h"
+#include "chrome/browser/chromeos/arc/auth/arc_robot_auth_code_fetcher.h"
 #include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
 #include "chrome/browser/chromeos/login/users/scoped_user_manager_enabler.h"
 #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
 #include "chrome/browser/policy/cloud/test_request_interceptor.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chromeos/chromeos_switches.h"
-#include "components/arc/arc_bridge_service.h"
-#include "components/arc/arc_service_manager.h"
-#include "components/arc/common/auth.mojom.h"
 #include "components/policy/core/common/cloud/device_management_service.h"
 #include "components/policy/core/common/cloud/mock_cloud_policy_client.h"
 #include "components/policy/core/common/cloud/user_cloud_policy_manager.h"
@@ -66,16 +64,6 @@ net::URLRequestJob* ResponseJob(net::URLRequest* request,
                                     response_data, true);
 }
 
-class FakeAuthInstance : public arc::mojom::AuthInstance {
- public:
-  void Init(arc::mojom::AuthHostPtr host_ptr) override {}
-  void OnAccountInfoReady(arc::mojom::AccountInfoPtr account_info) override {
-    ASSERT_FALSE(callback.is_null());
-    callback.Run(account_info);
-  }
-  base::Callback<void(const arc::mojom::AccountInfoPtr&)> callback;
-};
-
 }  // namespace
 
 class ArcRobotAuthCodeFetcherBrowserTest : public InProcessBrowserTest {
@@ -93,12 +81,13 @@ class ArcRobotAuthCodeFetcherBrowserTest : public InProcessBrowserTest {
   }
 
   void SetUpOnMainThread() override {
-    interceptor_.reset(new policy::TestRequestInterceptor(
+    interceptor_ = base::MakeUnique<policy::TestRequestInterceptor>(
         "localhost", content::BrowserThread::GetTaskRunnerForThread(
-                         content::BrowserThread::IO)));
+                         content::BrowserThread::IO));
 
-    user_manager_enabler_.reset(new chromeos::ScopedUserManagerEnabler(
-        new chromeos::FakeChromeUserManager));
+    user_manager_enabler_ =
+        base::MakeUnique<chromeos::ScopedUserManagerEnabler>(
+            new chromeos::FakeChromeUserManager());
 
     const AccountId account_id(AccountId::FromUserEmail(kFakeUserName));
     GetFakeUserManager()->AddArcKioskAppUser(account_id);
@@ -118,14 +107,9 @@ class ArcRobotAuthCodeFetcherBrowserTest : public InProcessBrowserTest {
             cloud_policy_manager->core()->client());
     cloud_policy_client->SetDMToken("fake-dm-token");
     cloud_policy_client->client_id_ = "client-id";
-
-    ArcBridgeService::Get()->auth()->SetInstance(&auth_instance_);
   }
 
   void TearDownOnMainThread() override {
-    ArcBridgeService::Get()->auth()->SetInstance(nullptr);
-    ArcSessionManager::Get()->Shutdown();
-    ArcServiceManager::Get()->Shutdown();
     user_manager_enabler_.reset();
 
     // Verify that all the expected requests were handled.
@@ -138,10 +122,26 @@ class ArcRobotAuthCodeFetcherBrowserTest : public InProcessBrowserTest {
         user_manager::UserManager::Get());
   }
 
-  std::unique_ptr<policy::TestRequestInterceptor> interceptor_;
-  FakeAuthInstance auth_instance_;
+  policy::TestRequestInterceptor* interceptor() { return interceptor_.get(); }
+
+  static void FetchAuthCode(ArcRobotAuthCodeFetcher* fetcher,
+                            std::string* output_auth_code) {
+    base::RunLoop run_loop;
+    fetcher->Fetch(base::Bind(
+        [](std::string* output_auth_code, base::RunLoop* run_loop,
+           const std::string& auth_code) {
+          *output_auth_code = auth_code;
+          run_loop->Quit();
+        },
+        output_auth_code, &run_loop));
+    // Because the Fetch() operation needs to interact with other threads,
+    // RunUntilIdle() won't work here. Instead, use Run() and Quit() explicitly
+    // in the callback.
+    run_loop.Run();
+  }
 
  private:
+  std::unique_ptr<policy::TestRequestInterceptor> interceptor_;
   std::unique_ptr<chromeos::ScopedUserManagerEnabler> user_manager_enabler_;
 
   DISALLOW_COPY_AND_ASSIGN(ArcRobotAuthCodeFetcherBrowserTest);
@@ -149,33 +149,27 @@ class ArcRobotAuthCodeFetcherBrowserTest : public InProcessBrowserTest {
 
 IN_PROC_BROWSER_TEST_F(ArcRobotAuthCodeFetcherBrowserTest,
                        RequestAccountInfoSuccess) {
-  interceptor_->PushJobCallback(base::Bind(&ResponseJob));
-
-  auth_instance_.callback =
-      base::Bind([](const mojom::AccountInfoPtr& account_info) {
-        EXPECT_EQ(kFakeAuthCode, account_info->auth_code.value());
-        EXPECT_EQ(mojom::ChromeAccountType::ROBOT_ACCOUNT,
-                  account_info->account_type);
-        EXPECT_FALSE(account_info->is_managed);
-      });
-
-  ArcAuthService::GetForTest()->RequestAccountInfo();
-  base::RunLoop().RunUntilIdle();
+  interceptor()->PushJobCallback(base::Bind(&ResponseJob));
+
+  std::string auth_code;
+  auto robot_fetcher = base::MakeUnique<ArcRobotAuthCodeFetcher>();
+  FetchAuthCode(robot_fetcher.get(), &auth_code);
+  EXPECT_EQ(kFakeAuthCode, auth_code);
 }
 
 IN_PROC_BROWSER_TEST_F(ArcRobotAuthCodeFetcherBrowserTest,
                        RequestAccountInfoError) {
-  interceptor_->PushJobCallback(
+  interceptor()->PushJobCallback(
       policy::TestRequestInterceptor::BadRequestJob());
 
-  auth_instance_.callback =
-      base::Bind([](const mojom::AccountInfoPtr&) { FAIL(); });
+  // We expect auth_code is empty in this case. So initialize with non-empty
+  // value.
+  std::string auth_code = "NOT-YET-FETCHED";
+  auto robot_fetcher = base::MakeUnique<ArcRobotAuthCodeFetcher>();
+  FetchAuthCode(robot_fetcher.get(), &auth_code);
 
-  ArcSessionManager::Get()->StartArc();
-  ArcAuthService::GetForTest()->RequestAccountInfo();
-  // This MessageLoop will be stopped by AttemptUserExit(), that is called as
-  // a result of error of auth code fetching.
-  base::RunLoop().Run();
+  // Use EXPECT_EQ for better logging in case of failure.
+  EXPECT_EQ(std::string(), auth_code);
 }
 
 }  // namespace arc

chrome/browser/chromeos/login/users/fake_chrome_user_manager.cc

@@ -4,6 +4,8 @@
 
 #include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
 
+#include <set>
+
 #include "base/command_line.h"
 #include "base/memory/ptr_util.h"
 #include "base/sys_info.h"
@@ -94,6 +96,8 @@ user_manager::User* FakeChromeUserManager::AddArcKioskAppUser(
     const AccountId& account_id) {
   user_manager::User* user =
       user_manager::User::CreateArcKioskAppUser(account_id);
+  user->set_username_hash(ProfileHelper::GetUserIdHashByUserIdForTesting(
+      account_id.GetUserEmail()));
   users_.push_back(user);
   return user;
 }