arc: ARC loading progress should not be shown when started from OOBE.

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 4858e187b06495dc36b4f14436338ec23116974e..84175634079955981f0bddcbd63659da42b5743a 100644
--- a/chrome/browser/chromeos/arc/arc_session_manager.cc
+++ b/chrome/browser/chromeos/arc/arc_session_manager.cc
@@ -20,9 +20,11 @@
 #include "chrome/browser/chromeos/arc/arc_auth_notification.h"
 #include "chrome/browser/chromeos/arc/arc_optin_uma.h"
 #include "chrome/browser/chromeos/arc/arc_support_host.h"
-#include "chrome/browser/chromeos/arc/optin/arc_terms_of_service_negotiator.h"
+#include "chrome/browser/chromeos/arc/optin/arc_terms_of_service_default_negotiator.h"
+#include "chrome/browser/chromeos/arc/optin/arc_terms_of_service_oobe_negotiator.h"
 #include "chrome/browser/chromeos/arc/policy/arc_android_management_checker.h"
 #include "chrome/browser/chromeos/arc/policy/arc_policy_util.h"
+#include "chrome/browser/chromeos/login/ui/login_display_host.h"
 #include "chrome/browser/chromeos/login/user_flow.h"
 #include "chrome/browser/chromeos/login/users/chrome_user_manager.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
@@ -82,6 +84,20 @@ ash::ShelfDelegate* GetShelfDelegate() {
   return nullptr;
 }
 
+bool IsOobeOptInActive() {

[hidehiko, 2017/01/30 10:12:35]

Could you add a unittest for this method?
I think it's ok to expose it for testing via ArcSessionManager with comments.

[khmel, 2017/01/31 02:47:08]

Done.

+  // ARC OOBE OptIn is optional for now. Test if it exists and login host is
+  // active.
+  if (!user_manager::UserManager::Get()->IsCurrentUserNew())
+    return false;
+  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
+          chromeos::switches::kEnableArcOOBEOptIn))
+    return false;
+  chromeos::LoginDisplayHost* host = chromeos::LoginDisplayHost::default_host();

[hidehiko, 2017/01/30 10:12:35]

nit:

if (!chromeos::LoginDisplayHost::default_host())
  return false;
return true;

[khmel, 2017/01/31 02:47:08]

Done.

+  if (!host)
+    return false;
+  return true;
+}
+
 }  // namespace
 
 ArcSessionManager::ArcSessionManager(
@@ -652,13 +668,18 @@ void ArcSessionManager::OnOptInPreferenceChanged() {
   // 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 (profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)) {
-    support_host_->ShowArcLoading();
+    // Don't show UI for this progress if it was not shown.
+    if (support_host_->ui_page() != ArcSupportHost::UIPage::NO_PAGE)
+      support_host_->ShowArcLoading();
     StartArcAndroidManagementCheck();
     return;
   }
 
-  // Need user's explicit Terms Of Service agreement.
-  StartTermsOfServiceNegotiation();
+  // Need user's explicit Terms Of Service agreement. Prevent race condition

[hidehiko, 2017/01/30 10:12:35]

Could you elaborate the race condition more?
For what case/what scenario do you want to change this behavior?

[khmel, 2017/01/31 02:47:08]

This legacy problem actually. It is less visible in current implementation
because of user iteration. With this implementation, when we set arc.enabled on
OOBE show this is reproducible in 100%. We have code OnIsSyncingChanged which is
called when profile is synced. From this callback we call
OnOptInPreferenceChanged. syncing profile is async
method which happens after initializing arc session manager.

So following happens in case OOBE flow:
* Initialize ArcSessionManager::OnPrimary...
* Show Arc OOBE
* Set arc.enabled - OnOptInPreferenceChanged is called.
* ARC OOBE is shown to user.
* Profile is synced,  OnIsSyncingChanged called and OnOptInPreferenceChanged is
called from it. This is second call of OnOptInPreferenceChanged and without this
fix we call  StartTermsOfServiceNegotiation twice which is wrong.

Non-OOBE case.
1. Initialize ArcSessionManager::OnPrimary...
2. Profile is synced,  OnIsSyncingChanged called but OnOptInPreferenceChanged is
not called because arc.enabled is not set.
3. User activates Arc, let say via settings. arc.enabled is set and
OnOptInPreferenceChanged is called and  StartTermsOfServiceNegotiation is called
once only.

Next is variation of Non-OOBE case.
2a. User activates Arc before OnIsSyncingChanged. OnOptInPreferenceChanged is
called and we show OptIn.
3a. While Terms are shown profile is actually synced and OnIsSyncingChanged
called. Now we have similar problem with OOBE case.

Last case is quite rare but I personally saw it few times.

[hidehiko, 2017/01/31 13:55:30]

Thank you for detailed explanation! It's much clearer to me.
So, what we need actually is checking no-dup call of OnOptInPreferenceChanged.
At the moment, I'm fine with your current change for short term solution only
for OOBE, but we need to fix the problem from root cause.
Could you file a bug (with your explanation above) and add TODO(crbug.com/...)?

[khmel, 2017/01/31 15:25:22]

Done.

+  // when ARC can be enabled before profile is synced. In last case
+  // OnOptInPreferenceChanged is called twice.
+  if (state_ != State::SHOWING_TERMS_OF_SERVICE)
+    StartTermsOfServiceNegotiation();
 }
 
 void ArcSessionManager::ShutdownSession() {
@@ -824,10 +845,20 @@ void ArcSessionManager::StartTermsOfServiceNegotiation() {
   }
 
   SetState(State::SHOWING_TERMS_OF_SERVICE);
-  if (support_host_) {
+  if (!IsOobeOptInActive()) {

[hidehiko, 2017/01/30 10:12:35]

nit: Let's reduce the indent level.

if (IsOobeOptInActive()) {
  ...
} else if (support_host_) {
  ...
}

if (negotiator_) {
  ...
}

[khmel, 2017/01/31 02:47:08]

Done.

+    if (support_host_) {
+      VLOG(1) << "Use default negotiator.";
+      terms_of_service_negotiator_ =
+          base::MakeUnique<ArcTermsOfServiceDefaultNegotiator>(
+              profile_->GetPrefs(), support_host_.get());
+    }
+  } else {
+    VLOG(1) << "Use OOBE negotiator.";
     terms_of_service_negotiator_ =
-        base::MakeUnique<ArcTermsOfServiceNegotiator>(profile_->GetPrefs(),
-                                                      support_host_.get());
+        base::MakeUnique<ArcTermsOfServiceOobeNegotiator>();
+  }
+
+  if (terms_of_service_negotiator_) {
     terms_of_service_negotiator_->StartNegotiation(
         base::Bind(&ArcSessionManager::OnTermsOfServiceNegotiated,
                    weak_ptr_factory_.GetWeakPtr()));
@@ -848,7 +879,10 @@ void ArcSessionManager::OnTermsOfServiceNegotiated(bool accepted) {
   // Terms were accepted.
   profile_->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, true);
 
-  support_host_->ShowArcLoading();
+  // 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();
 }
 
@@ -856,7 +890,9 @@ 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::CHECKING_ANDROID_MANAGEMENT ||
+         (state_ == State::STOPPED &&
+          profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)));
   SetState(State::CHECKING_ANDROID_MANAGEMENT);
 
   android_management_checker_.reset(new ArcAndroidManagementChecker(