Fix ArcSessionManager state machine, part 2.

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 144 matching lines @@
     // If ARC is being restarted, here do nothing, and just wait for its
     // next run.
     VLOG(1) << "ARC session is stopped, but being restarted: " << reason;
     return;
   }
 
   // TODO(crbug.com/625923): Use |reason| to report more detailed errors.
   if (arc_sign_in_timer_.IsRunning())
     OnProvisioningFinished(ProvisioningResult::ARC_STOPPED);
 
-  if (profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)) {
-    // This should be always true, but just in case as this is looked at
-    // inside RemoveArcData() at first.
-    VLOG(1) << "ARC had previously requested to remove user data.";
-    DCHECK(arc_session_runner_->IsStopped());
-    RemoveArcData();
-  } else {
-    // To support special "Stop and enable ARC" procedure for enterprise,
-    // here call MaybeReenableArc() asyncronously.
-    // TODO(hidehiko): Restructure the code. crbug.com/665316
+  for (auto& observer : observer_list_)
+    observer.OnArcSessionStopped(reason);
+
+  // Transition to the ARC data remove state.
+  if (!profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)) {
+    // TODO(crbug.com/665316): This is the workaround for the bug.
+    // If it is not necessary to remove the data, MaybeStartArcDataRemoval()
+    // synchronously calls MaybeReenableArc(), which causes unexpected
+    // ARC session stop. (Please see the bug for details).
+    SetState(State::REMOVING_DATA_DIR);
     base::ThreadTaskRunnerHandle::Get()->PostTask(
         FROM_HERE, base::Bind(&ArcSessionManager::MaybeReenableArc,
                               weak_ptr_factory_.GetWeakPtr()));
-  }
-
-  for (auto& observer : observer_list_)
-    observer.OnArcSessionStopped(reason);
-}
-
-void ArcSessionManager::RemoveArcData() {
-  // Ignore redundant data removal request.
-  if (state() == State::REMOVING_DATA_DIR)
-    return;
-
-  // OnArcDataRemoved resets this flag.
-  profile_->GetPrefs()->SetBoolean(prefs::kArcDataRemoveRequested, true);
-
-  if (!arc_session_runner_->IsStopped()) {
-    // Just set a flag. On session stopped, this will be re-called,
-    // then session manager should remove the data.
     return;
   }
 
-  VLOG(1) << "Starting ARC data removal";
-
-  // Remove Play user ID for Active Directory managed devices.
-  profile_->GetPrefs()->SetString(prefs::kArcActiveDirectoryPlayUserId,
-                                  std::string());
-
-  SetState(State::REMOVING_DATA_DIR);
-  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) {
-  if (success)
-    VLOG(1) << "ARC data removal successful";
-  else
-    LOG(ERROR) << "Request for ARC user data removal failed.";
-
-  // TODO(khmel): Browser tests may shutdown profile by itself. Update browser
-  // tests and remove this check.
-  if (state() == State::NOT_INITIALIZED)
-    return;
-
-  for (auto& observer : observer_list_)
-    observer.OnArcDataRemoved();
-
-  profile_->GetPrefs()->SetBoolean(prefs::kArcDataRemoveRequested, false);
-  DCHECK_EQ(state(), State::REMOVING_DATA_DIR);
-  SetState(State::STOPPED);
-
-  MaybeReenableArc();
-}
-
-void ArcSessionManager::MaybeReenableArc() {
-  // 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, OnSessionStopped() should trigger
-  // the RemoveArcData(), then this.
-  // TODO(hidehiko): It looks necessary to reset |reenable_arc_| regardless of
-  // |enable_requested_|. Fix it.
-  if (!reenable_arc_ || !enable_requested_)
-    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";
-  RequestEnableImpl();
+  MaybeStartArcDataRemoval();
 }
 
 void ArcSessionManager::OnProvisioningFinished(ProvisioningResult result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   // If the Mojo message to notify finishing the provisioning is already sent
   // from the container, it will be processed even after requesting to stop the
   // container. Ignore all |result|s arriving while ARC is disabled, in order to
   // avoid popping up an error message triggered below. This code intentionally
   // does not support the case of reenabling.
@@ skipping 126 matching lines @@
 
   if (result == ProvisioningResult::CLOUD_PROVISION_FLOW_FAILED ||
       result == ProvisioningResult::CLOUD_PROVISION_FLOW_TIMEOUT ||
       result == ProvisioningResult::CLOUD_PROVISION_FLOW_INTERNAL_ERROR ||
       // OVERALL_SIGN_IN_TIMEOUT might be an indication that ARC believes it is
       // fully setup, but Chrome does not.
       result == ProvisioningResult::OVERALL_SIGN_IN_TIMEOUT ||
       // Just to be safe, remove data if we don't know the cause.
       result == ProvisioningResult::UNKNOWN_ERROR) {
     VLOG(1) << "ARC provisioning failed permanently. Removing user data";
-    RemoveArcData();
+    RequestArcDataRemoval();
   }
 
   // We'll delay shutting down the ARC instance in this case to allow people
   // to send feedback.
   if (support_host_)
     support_host_->ShowError(error, true /* = show send feedback button */);
 }
 
 void ArcSessionManager::SetState(State state) {
   state_ = state;
@@ skipping 36 matching lines @@
   SetState(State::STOPPED);
 
   context_ = base::MakeUnique<ArcAuthContext>(profile_);
 
   if (!g_disable_ui_for_testing ||
       g_enable_check_android_management_for_testing) {
     ArcAndroidManagementChecker::StartClient();
   }
 
   // Chrome may be shut down before completing ARC data removal.
-  // In such a case, start removing the data now.
-  if (profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)) {
-    VLOG(1) << "ARC data removal requested in previous session.";
-    RemoveArcData();
-  }
+  // For such a case, start removing the data now, if necessary.
+  MaybeStartArcDataRemoval();
 }
 
 void ArcSessionManager::Shutdown() {
   enable_requested_ = false;
   ShutdownSession();
   if (support_host_) {
     support_host_->Close();
     support_host_->RemoveObserver(this);
     support_host_.reset();
   }
@@ skipping 133 matching lines @@
     // OnBackgroundAndroidManagementChecked() synchronously (or
     // asynchornously). In the callback, Google Play Store enabled preference
     // can be set to false if managed, and it triggers RequestDisable() via
     // ArcPlayStoreEnabledPreferenceHandler.
     // Thus, StartArc() should be called so that disabling should work even
     // if synchronous call case.
     StartBackgroundAndroidManagementCheck();
     return;
   }
 
-  StartTermsOfServiceNegotiation();
+  MaybeStartTermsOfServiceNegotiation();
 }
 
 void ArcSessionManager::RequestDisable() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(profile_);
 
   if (!enable_requested_) {
     VLOG(1) << "ARC is already disabled. Do nothing.";
     return;
   }
   enable_requested_ = false;
 
   // Reset any pending request to re-enable ARC.
-  VLOG(1) << "ARC opt-out. Removing user data.";
   reenable_arc_ = false;
   StopArc();
-  RemoveArcData();
+  VLOG(1) << "ARC opt-out. Removing user data.";
+  RequestArcDataRemoval();
 }
 
-void ArcSessionManager::StartTermsOfServiceNegotiation() {
+void ArcSessionManager::RequestArcDataRemoval() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(profile_);
+  // TODO(hidehiko): DCHECK the previous state. This is called for three cases;
+  // 1) Supporting managed user initial disabled case (Please see also
+  //    ArcPlayStoreEnabledPreferenceHandler::Start() for details).
+  // 2) Supporting enterprise triggered data removal.
+  // 3) One called in OnProvisioningFinished().
+  // 4) On request disabling.
+  // After the state machine is fixed, 2) should be replaced by
+  // RequestDisable() immediately followed by RequestEnable().
+  // 3) and 4) are internal state transition. So, as for public interface, 1)
+  // should be the only use case, and the |state_| should be limited to
+  // STOPPED, then.
+  // TODO(hidehiko): Think a way to get rid of 1), too.
+
+  // Just remember the request in persistent data. The actual removal
+  // is done via MaybeStartArcDataRemoval(). On completion (in
+  // OnArcDataRemoved()), this flag should be reset.
+  profile_->GetPrefs()->SetBoolean(prefs::kArcDataRemoveRequested, true);
+
+  // To support 1) case above, maybe start data removal.
+  if (state_ == State::STOPPED && arc_session_runner_->IsStopped())
+    MaybeStartArcDataRemoval();
+}
+
+void ArcSessionManager::MaybeStartTermsOfServiceNegotiation() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(profile_);
   DCHECK(!terms_of_service_negotiator_);
   // In Kiosk-mode, Terms of Service negotiation should be skipped.
   // See also RequestEnableImpl().
   DCHECK(!IsArcKioskMode());
   // If opt-in verification is disabled, Terms of Service negotiation should
   // be skipped, too. See also RequestEnableImpl().
   DCHECK(!IsArcOptInVerificationDisabled());
 
@@ skipping 203 matching lines @@
 void ArcSessionManager::StopArc() {
   if (state_ != State::STOPPED) {
     profile_->GetPrefs()->SetBoolean(prefs::kArcSignedIn, false);
     profile_->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, false);
   }
   ShutdownSession();
   if (support_host_)
     support_host_->Close();
 }
 
+void ArcSessionManager::MaybeStartArcDataRemoval() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(profile_);
+  // Data removal cannot run in parallel with ARC session.
+  DCHECK(arc_session_runner_->IsStopped());
+
+  // TODO(hidehiko): DCHECK the previous state, when the state machine is
+  // fixed.
+  SetState(State::REMOVING_DATA_DIR);
+
+  // TODO(hidehiko): Extract the implementation of data removal, so that
+  // shutdown can cancel the operation not to call OnArcDataRemoved callback.
+  if (!profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)) {
+    // ARC data removal is not requested. Just move to the next state.
+    MaybeReenableArc();
+    return;
+  }
+
+  VLOG(1) << "Starting ARC data removal";
+
+  // Remove Play user ID for Active Directory managed devices.
+  profile_->GetPrefs()->SetString(prefs::kArcActiveDirectoryPlayUserId,
+                                  std::string());
+
+  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) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  // TODO(khmel): Browser tests may shutdown profile by itself. Update browser
+  // tests and remove this check.
+  if (state() == State::NOT_INITIALIZED)
+    return;
+
+  DCHECK_EQ(state_, State::REMOVING_DATA_DIR);
+  DCHECK(profile_);
+  DCHECK(profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
+  if (success) {
+    VLOG(1) << "ARC data removal successful";
+  } else {
+    LOG(ERROR) << "Request for ARC user data removal failed. "
+               << "See session_manager logs for more details.";
+  }
+  profile_->GetPrefs()->SetBoolean(prefs::kArcDataRemoveRequested, false);
+
+  // Regardless of whether it is successfully done or not, notify observers.
+  for (auto& observer : observer_list_)
+    observer.OnArcDataRemoved();
+
+  MaybeReenableArc();
+}
+
+void ArcSessionManager::MaybeReenableArc() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK_EQ(state_, State::REMOVING_DATA_DIR);
+  SetState(State::STOPPED);
+
+  // Here check if |reenable_arc_| is marked or not.
+  // TODO(hidehiko): Conceptually |reenable_arc_| should be always false
+  // if |enable_requested_| is false. Replace by DCHECK after state machine
+  // fix is done.
+  if (!reenable_arc_ || !enable_requested_) {
+    // Reset the flag, just in case. TODO(hidehiko): Remove this.
+    reenable_arc_ = false;
+    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";
+  RequestEnableImpl();
+}
+
 void ArcSessionManager::OnWindowClosed() {
   DCHECK(support_host_);
   if (terms_of_service_negotiator_) {
     // In this case, ArcTermsOfServiceNegotiator should handle the case.
     // Do nothing.
     return;
   }
   CancelAuthCode();
 }
 
 void ArcSessionManager::OnTermsAgreed(bool is_metrics_enabled,
                                       bool is_backup_and_restore_enabled,
                                       bool is_location_service_enabled) {
   DCHECK(support_host_);
   DCHECK(terms_of_service_negotiator_);
   // This should be handled in ArcTermsOfServiceNegotiator. Do nothing here.
 }
 
 void ArcSessionManager::OnRetryClicked() {
   DCHECK(support_host_);
 
   UpdateOptInActionUMA(OptInActionType::RETRY);
 
   // TODO(hidehiko): Simplify the retry logic.
   if (terms_of_service_negotiator_) {
     // Currently Terms of service is shown. ArcTermsOfServiceNegotiator should
     // handle this.
   } else if (!profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)) {
-    StartTermsOfServiceNegotiation();
+    MaybeStartTermsOfServiceNegotiation();
   } else if (support_host_->ui_page() == ArcSupportHost::UIPage::ERROR &&
              !arc_session_runner_->IsStopped()) {
     // ERROR_WITH_FEEDBACK is set in OnSignInFailed(). In the case, stopping
     // ARC was postponed to contain its internal state into the report.
     // Here, on retry, stop it, then restart.
     DCHECK_EQ(State::ACTIVE, state_);
     support_host_->ShowArcLoading();
     ShutdownSession();
     reenable_arc_ = true;
   } else if (state_ == State::ACTIVE) {
@@ skipping 45 matching lines @@
 
 #undef MAP_STATE
 
   // Some compilers report an error even if all values of an enum-class are
   // covered exhaustively in a switch statement.
   NOTREACHED() << "Invalid value " << static_cast<int>(state);
   return os;
 }
 
 }  // namespace arc