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..68a22081a69af563c422ac064f96674b3836666c 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,15 @@ void ArcSessionManager::OnBridgeStopped(ArcBridgeService::StopReason reason) {
 }
 
 void ArcSessionManager::RemoveArcData() {
+  // 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;
+
   chromeos::DBusThreadManager::Get()->GetSessionManagerClient()->RemoveArcData(
       cryptohome::Identification(
           multi_user_util::GetAccountIdFromProfile(profile_)),
@@ -222,6 +225,8 @@ void ArcSessionManager::RemoveArcData() {
 void ArcSessionManager::OnArcDataRemoved(bool success) {
   LOG_IF(ERROR, !success) << "Required ARC user data wipe failed.";
 
+  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
@@ -261,6 +266,9 @@ void ArcSessionManager::OnProvisioningFinished(ProvisioningResult result) {
     if (support_host_)
       support_host_->Close();
 
+    // Make sure request to remove data folder is off.

[hidehiko, 2016/12/01 00:58:02]

Better to comment an example case that this actually needs to be.

[khmel, 2016/12/01 03:12:20]

I don't see any use case. Probably we can change it to 
DHCECK(!profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));

[hidehiko, 2016/12/01 16:15:03]

After some more investigation I found a case that may hit this:
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/enterprise/a...

IIUC, there is no guarantee about the invocation timing of the mojo call.
If no guarantee, please remove this. Otherwise the flag will be unintentionally
removed.
If there is, could you replace by DCHECK and comment the reason why this should
be always false?

[khmel, 2016/12/01 17:18:59]

Replaced by DCHECK

+    profile_->GetPrefs()->SetBoolean(prefs::kArcDataRemoveRequested, false);
+
     if (profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn))
       return;
 
@@ -565,6 +573,15 @@ void ArcSessionManager::StopAndEnableArc() {
 
 void ArcSessionManager::StartArc() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  // Don't start ARC if there is a pending request to remove the data. Restart
+  // ARC once data removal finishes.
+  if (profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)) {

[hidehiko, 2016/12/01 00:58:02]

Should this be in OnPrimaryUserProfilePrepared() ?

[khmel, 2016/12/01 03:12:21]

I think this is more reliable point to handle this case. Assume following test
case:
Profile is inited and arc.enabled is off 
In this case we force removing data.
While data is been removing arc.enabled is set to true. As result StartArc may
be called while data removal is in progress.
Having it here guaranties that Arc won't be started until invalidated data is
removed.

[hidehiko, 2016/12/01 16:15:03]

Good example scenario. I think, in that case, we should not call StartArc(). It
is because;
- It actually calls RemoveArcData() twice, which is not what we want here.
- Additionally, Not-starting the ARC by StartArc() is confusing.

Also, could you add the unittest scenario for that, too?

[khmel, 2016/12/01 17:18:59]

I moved this check to upper level OnOptInPreferenceChanged. Also added runtime
flag to prevent double removal request

+    reenable_arc_ = true;

[hidehiko, 2016/12/01 00:58:02]

This has an edge case:

Remove request is set in perf.
System rebooted.
StartArc() triggers.
RemoveArcData() triggers to remove the data.
* While removing, ARC is disabled.
Then, OnRemoveArcData() overrides the state?

[khmel, 2016/12/01 03:12:20]

Good catch. I think in this case we can reset reenable_arc_ to make sure that
Arc won't be re-enabled.
WDYT?

[hidehiko, 2016/12/01 16:15:03]

IsAllowed() check looks not the one we need here?
We actually need to check if the ARC is still enabled there.

Also, L230 (in new PS) looks not the right place to check, because, e.g., anyway
the flag needs to be reset.

[khmel, 2016/12/01 17:18:59]

IsAllowed is required to safely reset request. It checks for profile_ attached
(Shutdown resets profile).
Also added check IsArcEnabled below.

+    RemoveArcData();
+    return;
+  }
+
   arc_bridge_service()->RequestStart();
   SetState(State::ACTIVE);
 }