Fix ArcSessionManager state machine, part 1.

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 46 matching lines @@
 
 // The Android management check is disabled by default, it's used only for
 // testing.
 bool g_enable_check_android_management_for_testing = false;
 
 // Maximum amount of time we'll wait for ARC to finish booting up. Once this
 // timeout expires, keep ARC running in case the user wants to file feedback,
 // but present the UI to try again.
 constexpr base::TimeDelta kArcSignInTimeout = base::TimeDelta::FromMinutes(5);
 
+// Update UMA with user cancel only if error is not currently shown.

[Yusuke Sato, 2017/03/06 20:52:16]

Updates

[hidehiko, 2017/03/07 14:52:20]

Done.

+void MaybeUpdateOptInCancelUMA(const ArcSupportHost* support_host) {
+  if (!support_host ||
+      support_host->ui_page() == ArcSupportHost::UIPage::NO_PAGE ||
+      support_host->ui_page() == ArcSupportHost::UIPage::ERROR) {
+    return;
+  }
+
+  UpdateOptInCancelUMA(OptInCancelReason::USER_CANCEL);
+}
+
 }  // namespace
 
 ArcSessionManager::ArcSessionManager(
     std::unique_ptr<ArcSessionRunner> arc_session_runner)
     : arc_session_runner_(std::move(arc_session_runner)),
       attempt_user_exit_callback_(base::Bind(chrome::AttemptUserExit)),
       weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!g_arc_session_manager);
   g_arc_session_manager = this;
@@ skipping 426 matching lines @@
 
   if (state_ == State::NOT_INITIALIZED) {
     NOTREACHED();
     return;
   }
 
   // If ARC failed to boot normally, stop ARC. Similarly, if the current page is
   // LSO, closing the window should stop ARC since the user activity chooses to
   // not sign in. In any other case, ARC is booting normally and the instance
   // should not be stopped.
-  if ((state_ != State::SHOWING_TERMS_OF_SERVICE &&
+  if ((state_ != State::NEGOTIATING_TERMS_OF_SERVICE &&
        state_ != State::CHECKING_ANDROID_MANAGEMENT) &&
       (!support_host_ ||
        (support_host_->ui_page() != ArcSupportHost::UIPage::ERROR &&
         support_host_->ui_page() != ArcSupportHost::UIPage::LSO))) {
     return;
   }
 
-  // Update UMA with user cancel only if error is not currently shown.
-  if (support_host_ &&
-      support_host_->ui_page() != ArcSupportHost::UIPage::NO_PAGE &&
-      support_host_->ui_page() != ArcSupportHost::UIPage::ERROR) {
-    UpdateOptInCancelUMA(OptInCancelReason::USER_CANCEL);
-  }
-
+  MaybeUpdateOptInCancelUMA(support_host_.get());
   StopArc();
   SetArcPlayStoreEnabledForProfile(profile_, false);
 }
 
 void ArcSessionManager::RecordArcState() {
   // Only record Enabled state if ARC is allowed in the first place, so we do
   // not split the ARC population by devices that cannot run ARC.
   if (IsAllowed())
     UpdateEnabledStateUMA(enable_requested_);
 }
@@ skipping 23 matching lines @@
 
   if (state_ == State::REMOVING_DATA_DIR) {
     // Data removal request is in progress. Set flag to re-enable ARC once it
     // is finished.
     reenable_arc_ = true;
     return;
   }
 
   PrefService* const prefs = profile_->GetPrefs();
 
-  // 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.
-  // TODO(poromov): Move to more Kiosk dedicated set-up phase.
-  if (IsArcKioskMode())
-    prefs->SetBoolean(prefs::kArcTermsAccepted, true);
-
-  // Skip to show UI asking users to set up ARC OptIn preferences, if all of
-  // them are managed by the admin policy. Note that the ToS agreement is anyway
-  // not shown in the case of the managed ARC.
-  if (AreArcAllOptInPreferencesManaged())
-    prefs->SetBoolean(prefs::kArcTermsAccepted, true);
-
   // If it is marked that sign in has been successfully done, then directly
   // start ARC.
   // For testing, and for Kiosk mode, we also skip ToS negotiation procedure.
   // For backward compatibility, this check needs to be prior to the
   // kArcTermsAccepted check below.

[Yusuke Sato, 2017/03/06 20:52:16]

"below" is ambiguous now. Can you rephrase?

[hidehiko, 2017/03/07 14:52:20]

The comment looks obsolete, so fully rewrote.
To follow the order of comment (actually prod -> testing), reordered the
condition terms.

   if (prefs->GetBoolean(prefs::kArcSignedIn) ||
       IsArcOptInVerificationDisabled() || IsArcKioskMode()) {
     StartArc();
 
     // Skip Android management check for testing.
     // We also skip if Android management check for Kiosk mode,
     // because there are no managed human users for Kiosk exist.
     if (IsArcOptInVerificationDisabled() || IsArcKioskMode() ||
         (g_disable_ui_for_testing &&
          !g_enable_check_android_management_for_testing)) {
       return;
     }
 
     // Check Android management in parallel.

[Yusuke Sato, 2017/03/06 20:52:16]

Can you factor out L594-604 too? Since you have
ArcSessionManager::StartArcAndroidManagementCheck() for the non-background
check, having a function for the background one seems cleaner and easier to read
to me.

[Yusuke Sato, 2017/03/06 22:04:56]

Just noticed you're doing it in another CL already.

[hidehiko, 2017/03/07 14:52:20]

Yes, it's done. Thank you for review for the CL, too.

     // Note: Because the callback may be called in synchronous way (i.e. called
     // on the same stack), StartCheck() needs to be called *after* StartArc().
     // Otherwise, SetArcPlayStoreEnabledForProfile() which may be called in
     // OnBackgroundAndroidManagementChecked() could be ignored.
     android_management_checker_ = base::MakeUnique<ArcAndroidManagementChecker>(
         profile_, context_->token_service(), context_->account_id(),
         true /* retry_on_error */);
     android_management_checker_->StartCheck(
         base::Bind(&ArcSessionManager::OnBackgroundAndroidManagementChecked,
                    weak_ptr_factory_.GetWeakPtr()));
     return;
   }
 
-  // If it is marked that the Terms of service is accepted already,
-  // just skip the negotiation with user, and start Android management
-  // check directly.
-  // This happens, e.g., when;
-  // 1) User accepted the Terms of service on OOBE flow.
-  // 2) User accepted the Terms of service on Opt-in flow, but logged out
-  //   before ARC sign in procedure was done. Then, logs in again.
-  if (prefs->GetBoolean(prefs::kArcTermsAccepted)) {
-    // Don't show UI for this progress if it was not shown.
-    if (support_host_ &&
-        support_host_->ui_page() != ArcSupportHost::UIPage::NO_PAGE) {
-      support_host_->ShowArcLoading();
-    }
-    StartArcAndroidManagementCheck();
-    return;
-  }
-
   StartTermsOfServiceNegotiation();
 }
 
 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();
 }
 
 void ArcSessionManager::StartTermsOfServiceNegotiation() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  DCHECK(profile_);
   DCHECK(!terms_of_service_negotiator_);
 
+  // TODO(hidehiko): Remove this condition, when the state machine is fixed.
   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.
     if (support_host_) {
       support_host_->ShowError(
           ArcSupportHost::Error::SIGN_IN_SERVICE_UNAVAILABLE_ERROR, false);
     }
     return;
   }
 
-  SetState(State::SHOWING_TERMS_OF_SERVICE);
+  // TODO(hidehiko): DCHECK if |state_| is STOPPED, when the state machine
+  // is fixed.
+  SetState(State::NEGOTIATING_TERMS_OF_SERVICE);
+
+  if (!IsArcTermsOfServiceNegotiationNeeded()) {
+    // Moves to next state, Android management check, immediately, as if
+    // Terms of Service negotiation is done successfully.
+    StartArcAndroidManagementCheck();
+    return;
+  }
+
   if (IsOobeOptInActive()) {
     VLOG(1) << "Use OOBE negotiator.";
     terms_of_service_negotiator_ =
         base::MakeUnique<ArcTermsOfServiceOobeNegotiator>();
   } else if (support_host_) {
     VLOG(1) << "Use default negotiator.";
     terms_of_service_negotiator_ =
         base::MakeUnique<ArcTermsOfServiceDefaultNegotiator>(
             profile_->GetPrefs(), support_host_.get());
   }
 
   if (terms_of_service_negotiator_) {

[Yusuke Sato, 2017/03/06 20:52:16]

It was unclear to me when this variable can be a nullptr. Is this for dealing
with |g_disable_ui_for_testing|??? Can you add a code comment?

[hidehiko, 2017/03/07 14:52:20]

Yes, it's testing purpose.
Added comment with DCHECK.

     terms_of_service_negotiator_->StartNegotiation(
         base::Bind(&ArcSessionManager::OnTermsOfServiceNegotiated,
                    weak_ptr_factory_.GetWeakPtr()));
   }
 }
 
 void ArcSessionManager::OnTermsOfServiceNegotiated(bool accepted) {
+  DCHECK_EQ(state_, State::NEGOTIATING_TERMS_OF_SERVICE);
+  DCHECK(profile_);
   DCHECK(terms_of_service_negotiator_);
   terms_of_service_negotiator_.reset();
 
   if (!accepted) {
-    // To cancel, user needs to close the window. Note that clicking "Cancel"
-    // button effectively just closes the window.
-    CancelAuthCode();
+    // User does not accept the Terms of Service. Disable Google Play Store.
+    MaybeUpdateOptInCancelUMA(support_host_.get());
+    SetArcPlayStoreEnabledForProfile(profile_, false);
     return;
   }
 
   // Terms were accepted.
   profile_->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, true);
+  StartArcAndroidManagementCheck();
+}
 
-  // Don't show UI for this progress if it was not shown.
-  if (support_host_ &&
-      support_host_->ui_page() != ArcSupportHost::UIPage::NO_PAGE)
-    support_host_->ShowArcLoading();
-  StartArcAndroidManagementCheck();
+bool ArcSessionManager::IsArcTermsOfServiceNegotiationNeeded() const {
+  DCHECK(profile_);
+
+  // 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.
+  if (IsArcKioskMode()) {

[Yusuke Sato, 2017/03/06 20:52:16]

When is this check necessary? Because of the IsArcKioskMode() check at L582,
RequestEnableImpl() does not seem to call into this function at least.

[hidehiko, 2017/03/07 14:52:20]

Good catch!
So, I removed this from here, and added DCHECKs in StartToSNegotiation(). The
comment at L695- is moved to RequestEnableImpl()'s condition, with some
rephrasing.

+    VLOG(1) << "In Kiosk mode. Skip ARC Terms of Service negotiation.";
+    return false;
+  }
+
+  // Skip to show UI asking users to set up ARC OptIn preferences, if all of
+  // them are managed by the admin policy. Note that the ToS agreement is anyway
+  // not shown in the case of the managed ARC.
+  if (AreArcAllOptInPreferencesManaged()) {

[hidehiko, 2017/03/06 13:38:39]

Note: crrev.com/2731453003 moves the utility to c/b/c/arc/arc_util, which helps
to extract this function.

+    VLOG(1) << "All opt-in preferences are under managed. "
+            << "Skip ARC Terms of Service negotiation.";
+    return false;
+  }
+
+  // If it is marked that the Terms of service is accepted already,
+  // just skip the negotiation with user, and start Android management
+  // check directly.
+  // This happens, e.g., when a user accepted the Terms of service on Opt-in

[hidehiko, 2017/03/06 13:38:39]

Note: 1) of the original comment is obsolete. khmel@ redisigned the OOBE flow,
so for better adaption to the ASM state transition.

+  // flow, but logged out before ARC sign in procedure was done. Then, logs
+  // in again.
+  if (profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)) {
+    VLOG(1) << "The user already accepts ARC Terms of Service.";
+    return false;
+  }
+
+  return true;
 }
 
 bool ArcSessionManager::AreArcAllOptInPreferencesManaged() const {
   return IsArcPlayStoreEnabledPreferenceManagedForProfile(profile_) &&
          profile_->GetPrefs()->IsManagedPreference(
              prefs::kArcBackupRestoreEnabled) &&
          profile_->GetPrefs()->IsManagedPreference(
              prefs::kArcLocationServiceEnabled);
 }
 
 void ArcSessionManager::StartArcAndroidManagementCheck() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(arc_session_runner_->IsStopped());
-  DCHECK(state_ == State::SHOWING_TERMS_OF_SERVICE ||
-         state_ == State::CHECKING_ANDROID_MANAGEMENT ||
-         (state_ == State::STOPPED &&
-          profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)));
+  DCHECK(state_ == State::NEGOTIATING_TERMS_OF_SERVICE ||
+         state_ == State::CHECKING_ANDROID_MANAGEMENT);
   SetState(State::CHECKING_ANDROID_MANAGEMENT);
 
+  // Don't show UI for this progress if it was not shown.

[victorhsieh, 2017/03/06 23:11:52]

optional: I'm doing the same thing locally, and I wonder if it makes sense to
change the comment to "// Show UI only if opt-in is ongoing".  WDYT?

[hidehiko, 2017/03/07 14:52:20]

Rephrased for positive case, with its background.
Note that "opt-in is ongoing" is not the condition here, because even after OOBE
ToS negotiation, if management check fails, ARC support is shown with retry
button. On clicking the retry, we need to show loading. Hmm... complicated...

+  if (support_host_ &&
+      support_host_->ui_page() != ArcSupportHost::UIPage::NO_PAGE) {
+    support_host_->ShowArcLoading();
+  }
+
   android_management_checker_.reset(new ArcAndroidManagementChecker(

[Yusuke Sato, 2017/03/06 20:52:16]

MakeUnique? (L317 too)

[hidehiko, 2017/03/07 14:52:20]

Done in another CL. As for L317, I'm making another CL to extract the launcher
part. Let me replace it in the CL.

       profile_, context_->token_service(), context_->account_id(),
       false /* retry_on_error */));
   android_management_checker_->StartCheck(
       base::Bind(&ArcSessionManager::OnAndroidManagementChecked,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ArcSessionManager::OnAndroidManagementChecked(
     policy::AndroidManagementClient::Result result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ skipping 107 matching lines @@
     support_host_->ShowArcLoading();
     ShutdownSession();
     reenable_arc_ = true;
   } else if (state_ == State::ACTIVE) {
     // This case is handled in ArcAuthService.
     // Do nothing.
   } else {
     // Otherwise, we restart ARC. Note: this is the first boot case.
     // For second or later boot, either ERROR_WITH_FEEDBACK case or ACTIVE
     // case must hit.
-    support_host_->ShowArcLoading();
     StartArcAndroidManagementCheck();
   }
 }
 
 void ArcSessionManager::OnSendFeedbackClicked() {
   DCHECK(support_host_);
   chrome::OpenFeedbackDialog(nullptr);
 }
 
 void ArcSessionManager::SetArcSessionRunnerForTesting(
@@ skipping 14 matching lines @@
 
 std::ostream& operator<<(std::ostream& os,
                          const ArcSessionManager::State& state) {
 #define MAP_STATE(name)                \
   case ArcSessionManager::State::name: \
     return os << #name
 
   switch (state) {
     MAP_STATE(NOT_INITIALIZED);
     MAP_STATE(STOPPED);
-    MAP_STATE(SHOWING_TERMS_OF_SERVICE);
+    MAP_STATE(NEGOTIATING_TERMS_OF_SERVICE);
     MAP_STATE(CHECKING_ANDROID_MANAGEMENT);
     MAP_STATE(REMOVING_DATA_DIR);
     MAP_STATE(ACTIVE);
   }
 
 #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