arc: Make request to remove Android's data folder persistent.

chrome/browser/chromeos/arc/arc_session_manager_unittest.cc

 // Copyright 2016 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 <memory>
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
 #include "base/run_loop.h"
 #include "chrome/browser/chromeos/arc/arc_optin_uma.h"
 #include "chrome/browser/chromeos/arc/arc_session_manager.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/login/users/wallpaper/wallpaper_manager.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/prefs/pref_service_syncable_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_list_prefs.h"
 #include "chrome/browser/ui/ash/multi_user/multi_user_util.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_profile.h"
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/fake_session_manager_client.h"
 #include "components/arc/arc_bridge_service.h"
 #include "components/arc/test/fake_arc_bridge_service.h"
 #include "components/prefs/pref_service.h"
 #include "components/signin/core/account_id/account_id.h"
 #include "components/sync/model/fake_sync_change_processor.h"
 #include "components/sync/model/sync_error_factory_mock.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "components/user_manager/user_manager.h"
 #include "components/user_manager/user_names.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "google_apis/gaia/gaia_constants.h"
 #include "google_apis/gaia/gaia_urls.h"
 #include "net/http/http_status_code.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace arc {
 
 class ArcSessionManagerTest : public testing::Test {
  public:
   ArcSessionManagerTest()
       : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
         user_manager_enabler_(new chromeos::FakeChromeUserManager) {}
   ~ArcSessionManagerTest() override = default;
 
   void SetUp() override {
+    chromeos::DBusThreadManager::GetSetterForTesting()->SetSessionManagerClient(
+        base::WrapUnique(new chromeos::FakeSessionManagerClient));

[hidehiko, 2016/12/06 05:05:30]

nit: base::MakeUnique<chromeos::FakeSessionManagerClient>()

http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Prefer-...

[khmel, 2016/12/06 18:16:12]

Done.

+
     chromeos::DBusThreadManager::Initialize();
 
     base::CommandLine::ForCurrentProcess()->AppendSwitch(
         chromeos::switches::kEnableArc);
     ArcSessionManager::DisableUIForTesting();
 
     EXPECT_TRUE(temp_dir_.CreateUniqueTempDir());
     TestingProfile::Builder profile_builder;
     profile_builder.SetPath(temp_dir_.GetPath().AppendASCII("TestArcProfile"));
 
@@ skipping 57 matching lines @@
   ASSERT_EQ(ArcSessionManager::State::NOT_INITIALIZED,
             arc_session_manager()->state());
 
   PrefService* const pref = profile()->GetPrefs();
   ASSERT_FALSE(pref->GetBoolean(prefs::kArcEnabled));
 
   arc_session_manager()->OnPrimaryUserProfilePrepared(profile());
   ASSERT_EQ(ArcSessionManager::State::STOPPED, arc_session_manager()->state());
 
   pref->SetBoolean(prefs::kArcEnabled, true);
+  base::RunLoop().RunUntilIdle();
   ASSERT_EQ(ArcSessionManager::State::SHOWING_TERMS_OF_SERVICE,
             arc_session_manager()->state());
 
   pref->SetBoolean(prefs::kArcEnabled, false);
   ASSERT_EQ(ArcSessionManager::State::STOPPED, arc_session_manager()->state());
 
   // Correctly stop service.
   arc_session_manager()->Shutdown();
 }
 
@@ skipping 46 matching lines @@
   ASSERT_FALSE(bridge_service()->ready());
   ASSERT_EQ(ArcSessionManager::State::NOT_INITIALIZED,
             arc_session_manager()->state());
 
   arc_session_manager()->OnPrimaryUserProfilePrepared(profile());
 
   // By default ARC is not enabled.
   ASSERT_EQ(ArcSessionManager::State::STOPPED, arc_session_manager()->state());
 
   profile()->GetPrefs()->SetBoolean(prefs::kArcEnabled, true);
+  base::RunLoop().RunUntilIdle();
 
   // Setting profile and pref initiates a code fetching process.
   ASSERT_EQ(ArcSessionManager::State::SHOWING_TERMS_OF_SERVICE,
             arc_session_manager()->state());
 
   // TODO(hidehiko): Verify state transition from SHOWING_TERMS_OF_SERVICE ->
   // CHECKING_ANDROID_MANAGEMENT, when we extract ArcSessionManager.
   arc_session_manager()->StartArc();
 
   ASSERT_EQ(ArcSessionManager::State::ACTIVE, arc_session_manager()->state());
@@ skipping 20 matching lines @@
 
   // Correctly stop service.
   arc_session_manager()->Shutdown();
 }
 
 TEST_F(ArcSessionManagerTest, CancelFetchingDisablesArc) {
   PrefService* const pref = profile()->GetPrefs();
 
   arc_session_manager()->OnPrimaryUserProfilePrepared(profile());
   pref->SetBoolean(prefs::kArcEnabled, true);
+  base::RunLoop().RunUntilIdle();
+
   ASSERT_EQ(ArcSessionManager::State::SHOWING_TERMS_OF_SERVICE,
             arc_session_manager()->state());
 
   arc_session_manager()->CancelAuthCode();
   ASSERT_EQ(ArcSessionManager::State::STOPPED, arc_session_manager()->state());
   ASSERT_FALSE(pref->GetBoolean(prefs::kArcEnabled));
 
   // Correctly stop service.
   arc_session_manager()->Shutdown();
 }
 
 TEST_F(ArcSessionManagerTest, CloseUIKeepsArcEnabled) {
   PrefService* const pref = profile()->GetPrefs();
 
   arc_session_manager()->OnPrimaryUserProfilePrepared(profile());
   pref->SetBoolean(prefs::kArcEnabled, true);
+  base::RunLoop().RunUntilIdle();
 
   arc_session_manager()->StartArc();
 
   ASSERT_EQ(ArcSessionManager::State::ACTIVE, arc_session_manager()->state());
 
   arc_session_manager()->CancelAuthCode();
   ASSERT_EQ(ArcSessionManager::State::ACTIVE, arc_session_manager()->state());
   ASSERT_TRUE(pref->GetBoolean(prefs::kArcEnabled));
 
   // Correctly stop service.
@@ skipping 103 matching lines @@
   GetFakeUserManager()->SwitchActiveUser(account_id);
   second_profile->GetPrefs()->SetBoolean(prefs::kArcEnabled, true);
 
   // Check that non-primary user can't use Arc.
   EXPECT_FALSE(chromeos::ProfileHelper::IsPrimaryProfile(second_profile.get()));
   EXPECT_FALSE(ArcAppListPrefs::Get(second_profile.get()));
 
   arc_session_manager()->Shutdown();
 }
 
+TEST_F(ArcSessionManagerTest, RemoveDataFolder) {
+  profile()->GetPrefs()->SetBoolean(prefs::kArcEnabled, false);
+  // Starting session manager with prefs::kArcEnabled off automatically removes
+  // Android's data folder.
+  arc_session_manager()->OnPrimaryUserProfilePrepared(profile());
+  EXPECT_TRUE(
+      profile()->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
+  EXPECT_EQ(ArcSessionManager::State::STOPPED, arc_session_manager()->state());
+  // Enable ARC. Data is removed asyncronously. At this moment session manager
+  // still should be stopped and data removal request is active.
+  profile()->GetPrefs()->SetBoolean(prefs::kArcEnabled, true);
+  EXPECT_TRUE(
+      profile()->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
+  EXPECT_EQ(ArcSessionManager::State::STOPPED, arc_session_manager()->state());
+  // Wait until data is removed.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(
+      profile()->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
+  EXPECT_EQ(ArcSessionManager::State::SHOWING_TERMS_OF_SERVICE,
+            arc_session_manager()->state());
+  arc_session_manager()->StartArc();
+  EXPECT_EQ(ArcSessionManager::State::ACTIVE, arc_session_manager()->state());
+
+  // Now request to remove data and stop session manager.
+  ArcSessionManager::Get()->RemoveArcData();

[hidehiko, 2016/12/02 08:54:55]

s/ArcSessionManager::Get()/arc_session_manager()/

[khmel, 2016/12/06 03:23:57]

Done.

+  ASSERT_TRUE(
+      profile()->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
+  EXPECT_EQ(ArcSessionManager::State::ACTIVE, arc_session_manager()->state());
+  ArcSessionManager::Get()->Shutdown();

[hidehiko, 2016/12/02 08:54:55]

Ditto.

[khmel, 2016/12/06 03:23:57]

Done.

+  base::RunLoop().RunUntilIdle();
+  // Request should persist.
+  ASSERT_TRUE(
+      profile()->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
+
+  // Emulate next sign-in. Data should be removed first and ARC started after.

[hidehiko, 2016/12/02 08:54:55]

To emulate re-sign-in, shouldn't we re-instantiate the arc_session_manager()?

[khmel, 2016/12/06 03:23:57]

Hmm, probably I did not get you. Second and next
OnPrimaryUserProfilePrepared(profile()) is very common pattern in this file and
used in many tests here.

[hidehiko, 2016/12/06 05:05:30]

Oh, sorry for my poor explanation.
I was wondering if re-creating the instance can help to remove the check only
for testing at arc_session_manager:L236.
If not, I'm fine to leave this as is in this CL.

Note that, TBH, it's better to recreate ArcSessionManager instance (in any test
to emulate re-sign-in) to make emulation more closer to the ARC's real behavior,
but it looks out of focus of this CL, I think. Let's address it later.

[khmel, 2016/12/06 18:16:12]

I did similar what you suggested in some other tests where ArcAppListPrefs also
included into the testing and recreating managers is required. In this tests we 
can test this component more independently so this is not strictly required.
Also this test does not affect check you mentioned above because we implemented
wait cycles to test that state switches correctly. 

[hidehiko, 2016/12/06 18:32:09]

My understanding is, the check in the callback is for testing situation where
the Shutdown() is called, but the instance is not yet deleted and the message
loop is running (like L416).

Because the callback is bound with ArcSessionManager's weak_ptr, if we can
delete the instance after Shutdown always, the if check should not be there, as
callback should never be called then.

Anyway, I'm fine to leave it for now, and let's address it later.

+  arc_session_manager()->OnPrimaryUserProfilePrepared(profile());
+  EXPECT_TRUE(
+      profile()->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
+  EXPECT_EQ(ArcSessionManager::State::STOPPED, arc_session_manager()->state());
+  base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(
+      profile()->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
+  EXPECT_EQ(ArcSessionManager::State::SHOWING_TERMS_OF_SERVICE,
+            arc_session_manager()->state());
+  arc_session_manager()->StartArc();
+  EXPECT_EQ(ArcSessionManager::State::ACTIVE, arc_session_manager()->state());
+  ArcSessionManager::Get()->Shutdown();

[hidehiko, 2016/12/02 08:54:55]

Ditto.

[khmel, 2016/12/06 03:23:57]

Done.

+}
+
 }  // namespace arc