Fix ArcSessionManager state machine, part 2.

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 f82e67c428a728954a8bffa79a62274fdc35d1e6..517bea4f7d24fe1184acfbd684202dcd6f9932ce 100644
--- a/chrome/browser/chromeos/arc/arc_session_manager.cc
+++ b/chrome/browser/chromeos/arc/arc_session_manager.cc
@@ -147,74 +147,26 @@ void ArcSessionManager::OnSessionStopped(ArcStopReason reason,
   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
-    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.
+  // Transition to the ARC data remove state.

[Yusuke Sato, 2017/03/06 21:39:59]

nit: move down the comment to L164?

[hidehiko, 2017/03/07 18:51:02]

It is intentional. The following if-stmt is workaround, and considered as a part
of REMOVING_DATA_DIR, so set the 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
+    // ARC session stop. (Please see the bug for details).
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(&ArcSessionManager::MaybeReenableArc,
+                              weak_ptr_factory_.GetWeakPtr()));
     return;
   }
 
-  VLOG(1) << "Starting ARC data removal";
-  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();
+  StartArcDataRemoval();
 }
 
 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_)
@@ -367,7 +319,7 @@ void ArcSessionManager::OnProvisioningFinished(ProvisioningResult result) {
       // 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
@@ -424,11 +376,8 @@ void ArcSessionManager::SetProfile(Profile* profile) {
   }
 
   // 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() {
@@ -646,7 +595,33 @@ void ArcSessionManager::RequestDisable() {
   VLOG(1) << "ARC opt-out. Removing user data.";

[Yusuke Sato, 2017/03/06 21:39:59]

Maybe move this to L596 to follow the comment in the header more precisely? "A
log statement with the removal reason must be added prior to calling this."

[hidehiko, 2017/03/07 18:51:02]

Done.

   reenable_arc_ = false;
   StopArc();
-  RemoveArcData();
+  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() {
@@ -804,6 +779,61 @@ void ArcSessionManager::StopArc() {
     support_host_->Close();
 }
 
+void ArcSessionManager::StartArcDataRemoval() {

[Yusuke Sato, 2017/03/06 21:39:59]

This is more like MaybeStartArcDataRemoval() since this is mostly no-op when the
pref is not set.

[hidehiko, 2017/03/07 18:51:01]

I did this intentionally for consistency with
StartArcTermsOfServiceNegotiation(). Or, should I add Maybe prefix for both
cases?

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

+1 for renaming both. Your call though.

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

Ok, done.

+  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

[Yusuke Sato, 2017/03/06 21:39:59]

What's still broken?

[hidehiko, 2017/03/07 18:51:01]

OnSessionStopped() can be called regardless of state, unfortunately...

+  // 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 invoke the callback as if
+    // the removal is successfully done.
+    OnArcDataRemoved(true);

[Yusuke Sato, 2017/03/06 21:39:59]

Changing the state to REMOVING_DATA_DIR, calling the callback synchronously (but
skipping the observer_list_), then setting the state back to STOPPED
synchronously, seems a bit difficult to read. Can't we move this up (to L787)
and just do

 if (!the_pref_set) {
   MaybeReenableArc();
   return;
 }

?

[hidehiko, 2017/03/07 18:51:01]

Changed to direct MaybeReenableArc() call.

As for state, I still would like to keep it as is in this CL.
As I wrote TODO, I'm planning to extract the data removal class. Then,
kArcDataRemoveRequested is encapsulated in the class. Thus;

ASM::StartArcDataRemoval() {
  SetState(REMOVING_DATA_DIR);
  data_deleter->MaybeStartArcDataRemoval(
    Bind(this.WeakPtr(), OnArcDataRemoved));
}

ASM::OnArcDataRemoved() {
  // Current MaybeReenableArc(). Function name is TBD.
}

class DataDeleter {  // TBD: name.
 public:
  void RequestArcDataRemoval();
  void MaybeStartArcDataRemoval(callback) {
    if (!pref_has_flag) {
      return callback.Run();
    }
    ... DBus Call ...
  }

  void OnDataRemoved() {
    ... // almost same.
    callback.Run();
  }
};

WDYT?

+    return;
+  }
+
+  VLOG(1) << "Starting ARC data removal";
+  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_);
+
+  if (profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested)) {

[Yusuke Sato, 2017/03/06 21:39:59]

Then can we remove this IF? Not notifying observer_list_ in OnArcDateRemoved
depending on the pref seems confusing to me.

[hidehiko, 2017/03/07 18:51:01]

Done.

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

[Yusuke Sato, 2017/03/06 21:39:59]

This is sort of a fatal error, right? It's a bit sad that only boolean is
returned from session_manager and Chrome can't know the exact reason why it
failed. Hopefully /var/log/message has enough error messages from
session_manager to debug?

(just saying, no fix needed.)

[hidehiko, 2017/03/07 18:51:01]

Acknowledged.

+    profile_->GetPrefs()->SetBoolean(prefs::kArcDataRemoveRequested, false);
+
+    // Data removal runs (regardless of whether it is successfully done or
+    // not), so notify observers.
+    for (auto& observer : observer_list_)
+      observer.OnArcDataRemoved();
+  }
+
+  SetState(State::STOPPED);
+  MaybeReenableArc();
+}
+
 void ArcSessionManager::OnWindowClosed() {
   DCHECK(support_host_);
   if (terms_of_service_negotiator_) {