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

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"
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string16.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "chrome/browser/chromeos/arc/arc_auth_context.h"
 #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"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/policy/profile_policy_connector.h"
 #include "chrome/browser/policy/profile_policy_connector_factory.h"
 #include "chrome/browser/prefs/pref_service_syncable_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_launcher.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_utils.h"
@@ skipping 39 matching lines @@
 ash::ShelfDelegate* GetShelfDelegate() {
   if (g_shelf_delegate_for_testing)
     return g_shelf_delegate_for_testing;
   if (ash::WmShell::HasInstance()) {
     DCHECK(ash::WmShell::Get()->shelf_delegate());
     return ash::WmShell::Get()->shelf_delegate();
   }
   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(
     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 550 matching lines @@
   }
 
   // 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 (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() {
   arc_sign_in_timer_.Stop();
   playstore_launcher_.reset();
   terms_of_service_negotiator_.reset();
   android_management_checker_.reset();
   arc_session_runner_->RequestStop();
   // TODO(hidehiko): The ARC instance's stopping is asynchronous, so it might
   // still be running when we return from this function. Do not set the
@@ skipping 145 matching lines @@
     // 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);
-  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()));
   }
 }
 
 void ArcSessionManager::OnTermsOfServiceNegotiated(bool accepted) {
   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();
     return;
   }
 
   // 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();
 }
 
 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(
       profile_, context_->token_service(), context_->account_id(),
       false /* retry_on_error */));
   android_management_checker_->StartCheck(
       base::Bind(&ArcSessionManager::OnAndroidManagementChecked,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
@@ skipping 136 matching lines @@
       return os << "ACTIVE";
   }
 
   // Some compiler reports an error even if all values of an enum-class are
   // covered indivisually in a switch statement.
   NOTREACHED();
   return os;
 }
 
 }  // namespace arc