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() {
+  // 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_)),
       base::Bind(&ArcSessionManager::OnArcDataRemoved,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 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
   // the ARC user data wipe is still required or not.
   reenable_arc_ = false;
   VLOG(1) << "Reenable ARC";
   EnableArc();
 }
 
 void ArcSessionManager::OnProvisioningFinished(ProvisioningResult result) {
@@ skipping 13 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();
 
+    // 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;
 
     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 202 matching lines @@
 
   // TODO(dspaid): Move code from OnSyncedPrefChanged into this method.
   OnSyncedPrefChanged(prefs::kArcEnabled, IsArcManaged());
 
   const bool arc_enabled = IsArcEnabled();
   for (auto& observer : observer_list_)
     observer.OnOptInEnabled(arc_enabled);
 
   if (!arc_enabled) {
     StopArc();
+    // Reset any pending request to re-enable Arc.
+    reenable_arc_ = false;
     RemoveArcData();
     return;
   }
 
   if (state_ == State::ACTIVE)
     return;
 
   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
   // because there are no managed human users for Kiosk exist.
   if (IsOptInVerificationDisabled() || IsArcKioskMode()) {
     // Automatically accept terms in kiosk mode. This is not required for
     // IsOptInVerificationDisabled mode because in last case it may cause
     // a privacy issue on next run without this flag set.
     if (IsArcKioskMode())
       profile_->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, true);
@@ skipping 52 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);
+
+  // 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);
 }
 
 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