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

chrome/browser/chromeos/arc/arc_session_manager.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 "chrome/browser/chromeos/arc/arc_session_manager.h"
 
 #include <utility>
 
 #include "ash/common/shelf/shelf_delegate.h"
 #include "ash/common/wm_shell.h"
@@ skipping 92 matching lines @@
 ArcSessionManager* ArcSessionManager::Get() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   return g_arc_session_manager;
 }
 
 // static
 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);
   registry->RegisterBooleanPref(prefs::kArcBackupRestoreEnabled, true);
   registry->RegisterBooleanPref(prefs::kArcLocationServiceEnabled, true);
 }
 
 // static
 void ArcSessionManager::DisableUIForTesting() {
   g_disable_ui_for_testing = true;
@@ skipping 58 matching lines @@
 bool ArcSessionManager::IsArcKioskMode() {
   return user_manager::UserManager::Get()->IsLoggedInAsArcKioskApp();
 }
 
 void ArcSessionManager::OnBridgeStopped(ArcBridgeService::StopReason reason) {
   // TODO(crbug.com/625923): Use |reason| to report more detailed errors.
   if (arc_sign_in_timer_.IsRunning()) {
     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());
     RemoveArcData();
   } else {
     // To support special "Stop and enable ARC" procedure for enterprise,
     // here call OnArcDataRemoved(true) as if the data removal is successfully
     // done.
     // TODO(hidehiko): Restructure the code. crbug.com/665316
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(&ArcSessionManager::OnArcDataRemoved,
                               weak_ptr_factory_.GetWeakPtr(), true));
   }
 }
 
 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_)),
       base::Bind(&ArcSessionManager::OnArcDataRemoved,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 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
   // the RemoveArcData(), then this.
   // TODO(hidehiko): Restructure the code.
   if (!reenable_arc_)
     return;
 
   // Restart ARC anyway. Let the enterprise reporting instance decide whether
   // the ARC user data wipe is still required or not.
@@ skipping 19 matching lines @@
     UpdateProvisioningResultUMA(result,
                                 policy_util::IsAccountManaged(profile_));
     if (result != ProvisioningResult::SUCCESS)
       UpdateOptInCancelUMA(OptInCancelReason::CLOUD_PROVISION_FLOW_FAIL);
   }
 
   if (result == ProvisioningResult::SUCCESS) {
     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;
 
     profile_->GetPrefs()->SetBoolean(prefs::kArcSignedIn, true);
     // Don't show Play Store app for ARC Kiosk because the only one UI in kiosk
     // mode must be the kiosk app and device is not needed for opt-in.
     if (!IsOptInVerificationDisabled() && !IsArcKioskMode()) {
       playstore_launcher_.reset(
           new ArcAppLauncher(profile_, kPlayStoreAppId, true));
     }
@@ skipping 284 matching lines @@
 // TODO(hidehiko): Remove this.
 void ArcSessionManager::StopAndEnableArc() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!arc_bridge_service()->stopped());
   reenable_arc_ = true;
   StopArc();
 }
 
 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);
 }
 
 void ArcSessionManager::OnArcSignInTimeout() {
   LOG(ERROR) << "Timed out waiting for first sign in.";
   OnProvisioningFinished(ProvisioningResult::OVERALL_SIGN_IN_TIMEOUT);
 }
 
 void ArcSessionManager::CancelAuthCode() {
@@ skipping 248 matching lines @@
       return os << "ACTIVE";
   }
 
   // Some compiler reports an error even if all values of an enum-class are
   // covered indivisually in a switch statement.
   NOTREACHED();
   return os;
 }
 
 }  // namespace arc