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 133 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, StartArcDataRemoval()
+    // synchronously calls OnArcDataRemoved(), which causes unexpected

[Yusuke Sato, 2017/03/07 21:03:08]

This line seems outdated now. StartArcDataRemoval no longer synchronously calls
OnArcDataRemoved().

[hidehiko, 2017/03/09 09:25:51]

Done.

+    // 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();
+  StartArcDataRemoval();
 }
 
 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 123 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.
+  StartArcDataRemoval();
 }
 
 void ArcSessionManager::Shutdown() {
   enable_requested_ = false;
   ShutdownSession();
   if (support_host_) {
     support_host_->Close();
     support_host_->RemoveObserver(this);
     support_host_.reset();
   }
@@ skipping 184 matching lines @@
   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::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 StartArcDataRemoval(). 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())
+    StartArcDataRemoval();
 }
 
 void ArcSessionManager::StartTermsOfServiceNegotiation() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!terms_of_service_negotiator_);
 
   if (!arc_session_runner_->IsStopped()) {
     // If the user attempts to re-enable ARC while the ARC instance is still
     // running the user should not be able to continue until the ARC instance
     // has stopped.
@@ skipping 155 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::StartArcDataRemoval() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(profile_);
+  // Data removal cannot run, in parallel with ARC session.

[Luis Héctor Chávez, 2017/03/07 21:52:00]

nit: remove comma.

[hidehiko, 2017/03/09 09:25:51]

Done.

+  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));

[Yusuke Sato, 2017/03/07 21:03:08]

Thanks, the cb function is much easier to follow now.

BTW, this file has countless number of DCHECKs, but I'm wondering how many of
them are automatically evaluated by tests. Do you have any numbers of this?

[hidehiko, 2017/03/09 09:25:50]

Could you explain a bit more details about your question?
DCHECK causes fatal error on tests so passing tests means, at least in such
scenarios, DCHECKs pass properly.

+  if (success)
+    VLOG(1) << "ARC data removal successful";
+  else
+    LOG(ERROR) << "Request for ARC user data removal failed.";

[Yusuke Sato, 2017/03/07 21:03:08]

nit: What about adding 'See session_manager logs for more details.' too?

[hidehiko, 2017/03/09 09:25:51]

Done.

+  profile_->GetPrefs()->SetBoolean(prefs::kArcDataRemoveRequested, false);
+
+  // Data removal runs (regardless of whether it is successfully done or

[Luis Héctor Chávez, 2017/03/07 21:52:00]

This is not strictly true: there have been cases where session_manager becomes
unresponsive via D-Bus and will *not* remove data. See crbug.com/631640#c63 for
an instance where a similar thing happened.

But in order to know here what part of the call failed is complicated. Given
that the behavior is preserved, I'm fine with the code being left as-is, but the
wording of the comment does need to change.

(Or did I grossly misunderstood the comment and you only meant that the
OnArcDataRemoved *event* runs regardless of ...?)

[hidehiko, 2017/03/09 09:25:51]

I just wanted to say this is called regardless of data removal completed
successfully or failed. Rephrased.

+  // not), so notify observers.
+  for (auto& observer : observer_list_)
+    observer.OnArcDataRemoved();
+
+  MaybeReenableArc();
+}
+
+void ArcSessionManager::MaybeReenableArc() {

[Yusuke Sato, 2017/03/07 21:03:08]

nit: why did you move this down? shouldn't we preserve git blame logs whenever
possible?

..ah you followed the order in the header file. Then this is fine probably.

[hidehiko, 2017/03/09 09:25:50]

Yes, (and that's the order of the state transition).

+  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();
 }
 
@@ skipping 75 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