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

chrome/browser/chromeos/arc/arc_session_manager.cc

Index: chrome/browser/chromeos/arc/arc_session_manager.cc
diff --git a/chrome/browser/chromeos/arc/arc_session_manager.cc b/chrome/browser/chromeos/arc/arc_session_manager.cc
index 21effca88e1741d7f087ae3c547c954e774f4945..612cc4fcb2a854f2439ba0c640867a94c21e4ee1 100644
--- a/chrome/browser/chromeos/arc/arc_session_manager.cc
+++ b/chrome/browser/chromeos/arc/arc_session_manager.cc
@@ -110,6 +110,7 @@ void ArcSessionManager::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   // TODO(dspaid): Implement a mechanism to allow this to sync on first boot
   // only.
+  registry->RegisterBooleanPref(prefs::kArcDataRemoveRequested, false);
   registry->RegisterBooleanPref(prefs::kArcEnabled, false);
   registry->RegisterBooleanPref(prefs::kArcSignedIn, false);
   registry->RegisterBooleanPref(prefs::kArcTermsAccepted, false);
@@ -188,7 +189,7 @@ void ArcSessionManager::OnBridgeStopped(ArcBridgeService::StopReason reason) {
     OnProvisioningFinished(ProvisioningResult::ARC_STOPPED);
   }
 
-  if (clear_required_) {
+  if (profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)) {
     // This should be always true, but just in case as this is looked at
     // inside RemoveArcData() at first.
     DCHECK(arc_bridge_service()->stopped());
@@ -205,13 +206,20 @@ void ArcSessionManager::OnBridgeStopped(ArcBridgeService::StopReason reason) {
 }
 
 void ArcSessionManager::RemoveArcData() {
+  // Ignore redundant data removal request.
+  if (data_removal_in_progress_)

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

So this is state management. Could you add a new State instead of independent
bool flag?

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

Good idea, done.

+    return;
+
+  // OnArcDataRemoved resets this flag.
+  profile_->GetPrefs()->SetBoolean(prefs::kArcDataRemoveRequested, true);
+
   if (!arc_bridge_service()->stopped()) {
     // Just set a flag. On bridge stopped, this will be re-called,
     // then session manager should remove the data.
-    clear_required_ = true;
     return;
   }
-  clear_required_ = false;
+
+  data_removal_in_progress_ = true;
   chromeos::DBusThreadManager::Get()->GetSessionManagerClient()->RemoveArcData(
       cryptohome::Identification(
           multi_user_util::GetAccountIdFromProfile(profile_)),
@@ -222,12 +230,19 @@ void ArcSessionManager::RemoveArcData() {
 void ArcSessionManager::OnArcDataRemoved(bool success) {
   LOG_IF(ERROR, !success) << "Required ARC user data wipe failed.";
 
+  data_removal_in_progress_ = false;
+
+  // In case data removal completed between shutdown and deleting this manager.
+  // It happens at least in browser tests.
+  if (IsAllowed())

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

Hmm, ok. So this check is only for browser_test, right?
Indeed, in some test, Shutdown() is directly called, but in real usage, it is
only from dtor. (Note: it is also in OnPrimaryUserProfilePrepared(), but it
looks something redundant).

Then, could you return immediately at the top of this function with commenting
it is only for browser test and the code should be cleaned later?

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

Done.

+    profile_->GetPrefs()->SetBoolean(prefs::kArcDataRemoveRequested, false);
+
   // Here check if |reenable_arc_| is marked or not.
   // The only case this happens should be in the special case for enterprise
   // "on managed lost" case. In that case, OnBridgeStopped() should trigger
   // the RemoveArcData(), then this.
   // TODO(hidehiko): Restructure the code.
-  if (!reenable_arc_)
+  if (!reenable_arc_ || !IsArcEnabled())
     return;
 
   // Restart ARC anyway. Let the enterprise reporting instance decide whether
@@ -261,6 +276,9 @@ void ArcSessionManager::OnProvisioningFinished(ProvisioningResult result) {
     if (support_host_)
       support_host_->Close();
 
+    // No data removal request is expected on Arc boot.

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

It is still unclear to me why this is guaranteed.

# And, actually, consecutive 
ArcEnterpriseReportingService::ReportManagementState(MANAGED_DO_LOST) and
ArcAuthService::OnSignInComplete() invocation looks to hit the case, at least.

Could you comment what guarantees the condition?

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

ReportManagementState(MANAGED_DO_LOST) calls 2 functions:
RemoveArcData and 
StopAndEnableArc

Theoretically OnProvisioningFinished(ProvisioningResult::SUCCESS)  may happen
due async nature of bridge stopping. However we already have check
DCHECK_EQ(state_, State::ACTIVE) at the beginning of this function; That means
this check is also incorrect because StopAndEnableArc sets state to STOP, right?
From this they are both incorrect or correct.

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

Oh, good catch! So, the DCHECK above is wrong...
Could you remove both DCHECK with comments?

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

Done.

+    DCHECK(!profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
+
     if (profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn))
       return;
 
@@ -483,6 +501,8 @@ void ArcSessionManager::OnOptInPreferenceChanged() {
 
   if (!arc_enabled) {
     StopArc();
+    // Reset any pending request to re-enable Arc.
+    reenable_arc_ = false;
     RemoveArcData();
     return;
   }
@@ -493,6 +513,14 @@ void ArcSessionManager::OnOptInPreferenceChanged() {
   if (support_host_)
     support_host_->SetArcManaged(IsArcManaged());
 
+  // Don't start ARC if there is a pending request to remove the data. Restart

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

I still think this is a part of OnPrimaryUserProfilePrepared, because this
should happen only when user logs in at ChromeOS.
Is there any other cases we need to take a look? Then, could you comment?

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

I refactored this code based on state we introduced and moving analyzing this
flag to OnPrimaryUserProfilePrepared

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

Nice!

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

Acknowledged.

+  // ARC once data removal finishes.
+  if (profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)) {
+    reenable_arc_ = true;
+    RemoveArcData();
+    return;
+  }
+
   // In case UI is disabled we assume that ARC is opted-in. For ARC Kiosk we
   // skip ToS because it is very likely that near the device there will be
   // no one who is eligible to accept them. We skip if Android management check
@@ -565,6 +593,10 @@ void ArcSessionManager::StopAndEnableArc() {
 
 void ArcSessionManager::StartArc() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  // Arc must be started only if no pending data removal request exists.
+  DCHECK(!profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
+
   arc_bridge_service()->RequestStart();
   SetState(State::ACTIVE);
 }