Move ArcSessionRunner from ArcBridgeService to ArcSessionManager.

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 22 matching lines @@
 #include "chrome/browser/ui/app_list/arc/arc_app_utils.h"
 #include "chrome/browser/ui/ash/multi_user/multi_user_util.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/grit/generated_resources.h"
 #include "chromeos/chromeos_switches.h"
 #include "chromeos/cryptohome/cryptohome_parameters.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/session_manager_client.h"
 #include "components/arc/arc_bridge_service.h"
+#include "components/arc/arc_session_runner.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
 #include "components/sync_preferences/pref_service_syncable.h"
 #include "components/user_manager/user.h"
 #include "content/public/browser/browser_thread.h"
 #include "extensions/browser/extension_prefs.h"
 
 namespace arc {
 
 namespace {
@@ skipping 21 matching lines @@
     return g_shelf_delegate_for_testing;
   if (ash::WmShell::HasInstance()) {
     DCHECK(ash::WmShell::Get()->shelf_delegate());
     return ash::WmShell::Get()->shelf_delegate();
   }
   return nullptr;
 }
 
 }  // namespace
 
-ArcSessionManager::ArcSessionManager(ArcBridgeService* bridge_service)
-    : ArcService(bridge_service),
+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;
-
-  arc_bridge_service()->AddObserver(this);
+  arc_session_runner_->AddObserver(this);
 }
 
 ArcSessionManager::~ArcSessionManager() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   Shutdown();
-  arc_bridge_service()->RemoveObserver(this);
+  arc_session_runner_->RemoveObserver(this);
 
   DCHECK_EQ(this, g_arc_session_manager);
   g_arc_session_manager = nullptr;
 }
 
 // static
 ArcSessionManager* ArcSessionManager::Get() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   return g_arc_session_manager;
 }
@@ skipping 76 matching lines @@
   }
 
   return true;
 }
 
 // static
 bool ArcSessionManager::IsArcKioskMode() {
   return user_manager::UserManager::Get()->IsLoggedInAsArcKioskApp();
 }
 
+void ArcSessionManager::OnSessionReady() {
+  for (auto& observer : arc_session_observer_list_)
+    observer.OnSessionReady();
+}
+
 void ArcSessionManager::OnSessionStopped(StopReason reason) {
   // 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.
-    DCHECK(arc_bridge_service()->stopped());
+    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 : arc_session_observer_list_)
+    observer.OnSessionStopped(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_bridge_service()->stopped()) {
-    // Just set a flag. On bridge stopped, this will be re-called,
+  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;
   }
 
   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()));
@@ skipping 13 matching lines @@
   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, OnBridgeStopped() should trigger
+  // "on managed lost" case. In that case, OnSessionStopped() should trigger
   // the RemoveArcData(), then this.
   if (!reenable_arc_ || !IsArcEnabled())
     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";
   EnableArc();
 }
 
 void ArcSessionManager::OnProvisioningFinished(ProvisioningResult result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  // Due asynchronous nature of stopping Arc bridge, OnProvisioningFinished may
-  // arrive after setting the |State::STOPPED| state and |State::Active| is not
-  // guaranty set here. prefs::kArcDataRemoveRequested is also can be active
-  // for now.
+  // Due asynchronous nature of stopping ARC instance, OnProvisioningFinished

[Luis Héctor Chávez, 2016/12/22 18:29:56]

nit: the ARC instance

[hidehiko, 2016/12/22 19:04:12]

Done.

+  // may arrive after setting the |State::STOPPED| state and |State::Active|
+  // is not guaranty set here. prefs::kArcDataRemoveRequested is also can be

[Luis Héctor Chávez, 2016/12/22 18:29:56]

nit: s/guaranty/guaranteed to be/;s/is also can be/also can be/

[hidehiko, 2016/12/22 19:04:12]

Done.

+  // active for now.
 
   if (provisioning_reported_) {
     // We don't expect ProvisioningResult::SUCCESS is reported twice or reported
     // after an error.
     DCHECK_NE(result, ProvisioningResult::SUCCESS);
     // TODO (khmel): Consider changing LOG to NOTREACHED once we guaranty that
     // no double message can happen in production.
     LOG(WARNING) << " Provisioning result was already reported. Ignoring "
                  << " additional result " << static_cast<int>(result) << ".";
     return;
@@ skipping 73 matching lines @@
       break;
     default:
       error = ArcSupportHost::Error::SIGN_IN_UNKNOWN_ERROR;
       break;
   }
 
   if (result == ProvisioningResult::ARC_STOPPED ||
       result == ProvisioningResult::CHROME_SERVER_COMMUNICATION_ERROR) {
     if (profile_->GetPrefs()->HasPrefPath(prefs::kArcSignedIn))
       profile_->GetPrefs()->SetBoolean(prefs::kArcSignedIn, false);
-    ShutdownBridge();
+    ShutdownSession();
     if (support_host_)
       support_host_->ShowError(error, false);
     return;
   }
 
   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) {
     RemoveArcData();
   }
 
-  // We'll delay shutting down the bridge in this case to allow people to send
-  // feedback.
+  // 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;
 }
 
 bool ArcSessionManager::IsAllowed() const {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ skipping 84 matching lines @@
       profile_->IsNewProfile() &&
       !profile_->GetPrefs()->HasPrefPath(prefs::kArcEnabled)) {
     ArcAuthNotification::Show(profile_);
   }
 }
 
 void ArcSessionManager::Shutdown() {
   if (!g_disable_ui_for_testing)
     ArcAuthNotification::Hide();
 
-  ShutdownBridge();
+  ShutdownSession();
   if (support_host_) {
     support_host_->Close();
     support_host_->RemoveObserver(this);
     support_host_.reset();
   }
   if (profile_) {
     sync_preferences::PrefServiceSyncable* pref_service_syncable =
         PrefServiceSyncableFromProfile(profile_);
     pref_service_syncable->RemoveObserver(this);
     pref_service_syncable->RemoveSyncedPrefObserver(prefs::kArcEnabled, this);
@@ skipping 21 matching lines @@
         shelf_delegate->UnpinAppWithID(ArcSupportHost::kHostAppId);
     }
   }
 }
 
 void ArcSessionManager::StopArc() {
   if (state_ != State::STOPPED) {
     profile_->GetPrefs()->SetBoolean(prefs::kArcSignedIn, false);
     profile_->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, false);
   }
-  ShutdownBridge();
+  ShutdownSession();
   if (support_host_)
     support_host_->Close();
 }
 
 void ArcSessionManager::OnOptInPreferenceChanged() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(profile_);
 
   // TODO(dspaid): Move code from OnSyncedPrefChanged into this method.
   OnSyncedPrefChanged(prefs::kArcEnabled, IsArcManaged());
@@ skipping 78 matching lines @@
   if (profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)) {
     support_host_->ShowArcLoading();
     StartArcAndroidManagementCheck();
     return;
   }
 
   // Need user's explicit Terms Of Service agreement.
   StartTermsOfServiceNegotiation();
 }
 
-void ArcSessionManager::ShutdownBridge() {
+void ArcSessionManager::ShutdownSession() {
   arc_sign_in_timer_.Stop();
   playstore_launcher_.reset();
   terms_of_service_negotiator_.reset();
   android_management_checker_.reset();
-  arc_bridge_service()->RequestStop();
+  arc_session_runner_->RequestStop();
+  // TODO(hidehiko): Actual ARC instance stop will be delayed, and until
+  // it's completed, ARC instance is still running. Do not set the state
+  // STOPPED immediately here.

[Luis Héctor Chávez, 2016/12/22 18:29:56]

just curious: is the plan to delete IsSessionStopped() and have
IsSessionRunning() be the only source of truth?

If that's the case, maybe it's better to change all the checks in
chrome/browser/chromeos/arc/arc_session_manager_unittest.cc to only use
IsSessionRunning() to avoid future changes there. (only if the weird nature of
both flags not being exactly opposite of each other doesn't interact badly with
the test, but given that they all used ready() I *think* it should be fine).

[hidehiko, 2016/12/22 19:04:12]

TBD. Currently, IsSessionStopped()/Running() is just proxy from
ArcSessionRunner. IMHO, whether Actually ARC is running or not looks too details
for this layer, and maybe it's better to avoid exposing them for encapsulation.
However, for now, it looks not yet clear enough to me whether it is technically
doable. So, let's address it separately.

As for testing code, indeed the original code looked to assume "STOPPED ==
!RUNNING", but it is not true, as you know. So, I'd like to keep the current
code to follow the current semantics, for now.

WDYT?

   if (state_ != State::NOT_INITIALIZED && state_ != State::REMOVING_DATA_DIR)
     SetState(State::STOPPED);
   for (auto& observer : observer_list_)
     observer.OnArcBridgeShutdown();
 }
 
 void ArcSessionManager::AddObserver(Observer* observer) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   observer_list_.AddObserver(observer);
 }
 
 void ArcSessionManager::RemoveObserver(Observer* observer) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   observer_list_.RemoveObserver(observer);
 }
 
+void ArcSessionManager::AddSessionObserver(ArcSessionObserver* observer) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  arc_session_observer_list_.AddObserver(observer);
+}
+
+void ArcSessionManager::RemoveSessionObserver(ArcSessionObserver* observer) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  arc_session_observer_list_.RemoveObserver(observer);
+}
+
+bool ArcSessionManager::IsSessionRunning() const {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  return arc_session_runner_->IsRunning();
+}
+
+bool ArcSessionManager::IsSessionStopped() const {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  return arc_session_runner_->IsStopped();
+}
+
 // This is the special method to support enterprise mojo API.
 // TODO(hidehiko): Remove this.
 void ArcSessionManager::StopAndEnableArc() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  DCHECK(!arc_bridge_service()->stopped());
+  DCHECK(!arc_session_runner_->IsStopped());
   reenable_arc_ = true;
   StopArc();
 }
 
 void ArcSessionManager::StartArc() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   // Arc must be started only if no pending data removal request exists.
   DCHECK(!profile_->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
 
   provisioning_reported_ = false;
 
-  arc_bridge_service()->RequestStart();
+  arc_session_runner_->RequestStart();
   SetState(State::ACTIVE);
 }
 
 void ArcSessionManager::OnArcSignInTimeout() {
   LOG(ERROR) << "Timed out waiting for first sign in.";
   OnProvisioningFinished(ProvisioningResult::OVERALL_SIGN_IN_TIMEOUT);
 }
 
 void ArcSessionManager::CancelAuthCode() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ skipping 66 matching lines @@
   // 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(IsArcEnabled());
 }
 
 void ArcSessionManager::StartTermsOfServiceNegotiation() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(!terms_of_service_negotiator_);
 
-  if (!arc_bridge_service()->stopped()) {
-    // If the user attempts to re-enable ARC while the bridge is still running
-    // the user should not be able to continue until the bridge has stopped.
+  if (!arc_session_runner_->IsStopped()) {
+    // If the user attempts to re-enable ARC while ARC instance is still

[Luis Héctor Chávez, 2016/12/22 18:29:56]

nit: the ARC instance. same below.

[hidehiko, 2016/12/22 19:04:12]

Done.

+    // running the user should not be able to continue until 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_) {
     terms_of_service_negotiator_ =
@@ skipping 18 matching lines @@
 
   // Terms were accepted.
   profile_->GetPrefs()->SetBoolean(prefs::kArcTermsAccepted, true);
 
   support_host_->ShowArcLoading();
   StartArcAndroidManagementCheck();
 }
 
 void ArcSessionManager::StartArcAndroidManagementCheck() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  DCHECK(arc_bridge_service()->stopped());
+  DCHECK(arc_session_runner_->IsStopped());
   DCHECK(state_ == State::SHOWING_TERMS_OF_SERVICE ||
          state_ == State::CHECKING_ANDROID_MANAGEMENT);
   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()));
 }
 
 void ArcSessionManager::OnAndroidManagementChecked(
     policy::AndroidManagementClient::Result result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK_EQ(state_, State::CHECKING_ANDROID_MANAGEMENT);
 
   switch (result) {
     case policy::AndroidManagementClient::Result::UNMANAGED:
       VLOG(1) << "Starting ARC for first sign in.";
       sign_in_time_ = base::Time::Now();
       arc_sign_in_timer_.Start(
           FROM_HERE, kArcSignInTimeout,
           base::Bind(&ArcSessionManager::OnArcSignInTimeout,
                      weak_ptr_factory_.GetWeakPtr()));
       StartArc();
       break;
     case policy::AndroidManagementClient::Result::MANAGED:
-      ShutdownBridge();
+      ShutdownSession();
       if (support_host_) {
         support_host_->ShowError(
             ArcSupportHost::Error::ANDROID_MANAGEMENT_REQUIRED_ERROR, false);
       }
       UpdateOptInCancelUMA(OptInCancelReason::ANDROID_MANAGEMENT_REQUIRED);
       break;
     case policy::AndroidManagementClient::Result::ERROR:
-      ShutdownBridge();
+      ShutdownSession();
       if (support_host_) {
         support_host_->ShowError(
             ArcSupportHost::Error::SERVER_COMMUNICATION_ERROR, false);
       }
       UpdateOptInCancelUMA(OptInCancelReason::NETWORK_ERROR);
       break;
   }
 }
 
 void ArcSessionManager::OnBackgroundAndroidManagementChecked(
@@ skipping 36 matching lines @@
 
   UpdateOptInActionUMA(OptInActionType::RETRY);
 
   // TODO(hidehiko): Simplify the retry logic.
   if (terms_of_service_negotiator_) {
     // Currently Terms of service is shown. ArcTermsOfServiceNegotiator should
     // handle this.
   } else if (!profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)) {
     StartTermsOfServiceNegotiation();
   } else if (support_host_->ui_page() == ArcSupportHost::UIPage::ERROR &&
-             !arc_bridge_service()->stopped()) {
+             !arc_session_runner_->IsStopped()) {
     // ERROR_WITH_FEEDBACK is set in OnSignInFailed(). In the case, stopping
     // ARC was postponed to contain its internal state into the report.
     // Here, on retry, stop it, then restart.
     DCHECK_EQ(State::ACTIVE, state_);
     support_host_->ShowArcLoading();
-    ShutdownBridge();
+    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(
+    std::unique_ptr<ArcSessionRunner> arc_session_runner) {
+  DCHECK(arc_session_runner);
+  DCHECK(arc_session_runner_);
+  DCHECK(arc_session_runner_->IsStopped());
+  arc_session_runner_->RemoveObserver(this);
+  arc_session_runner_ = std::move(arc_session_runner);
+  arc_session_runner_->AddObserver(this);
+}
+
 void ArcSessionManager::SetAttemptUserExitCallbackForTesting(
     const base::Closure& callback) {
   DCHECK(!callback.is_null());
   attempt_user_exit_callback_ = callback;
 }
 
 std::ostream& operator<<(std::ostream& os,
                          const ArcSessionManager::State& state) {
   switch (state) {
     case ArcSessionManager::State::NOT_INITIALIZED:
@@ skipping 10 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