arc: Fix re-try does not work on provisioning flow.

BUG=b/118447711
TEST=Manually + unit-test

Change-Id: If307a9f64bf8a6e8937bc4b17683ef621155c786
Reviewed-on: https://chromium-review.googlesource.com/c/1300690Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Commit-Queue: Yury Khmel <khmel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604134}

chrome/browser/chromeos/arc/arc_session_manager.cc

@@ -858,8 +858,17 @@ void ArcSessionManager::OnTermsOfServiceNegotiated(bool accepted) {
 
 void ArcSessionManager::StartAndroidManagementCheck() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  // State::STOPPED appears here in following scenario.
+  // Initial provisioning finished with state
+  // ProvisioningResult::ArcStop or
+  // ProvisioningResult::CHROME_SERVER_COMMUNICATION_ERROR.
+  // At this moment |prefs::kArcTermsAccepted| is set to true, once user
+  // confirmed ToS prior to provisioning flow. Once user presses "Try Again"
+  // button, OnRetryClicked calls this immediately.
   DCHECK(state_ == State::NEGOTIATING_TERMS_OF_SERVICE ||
-         state_ == State::CHECKING_ANDROID_MANAGEMENT)
+         state_ == State::CHECKING_ANDROID_MANAGEMENT ||
+         state_ == State::STOPPED)
       << state_;
   state_ = State::CHECKING_ANDROID_MANAGEMENT;
 
@@ -1071,32 +1080,39 @@ void ArcSessionManager::OnWindowClosed() {
 }
 
 void ArcSessionManager::OnRetryClicked() {
-  DCHECK(support_host_);
-  DCHECK_EQ(support_host_->ui_page(), ArcSupportHost::UIPage::ERROR);
+  DCHECK(!g_ui_enabled || support_host_);
+  DCHECK(!g_ui_enabled ||
+         support_host_->ui_page() == ArcSupportHost::UIPage::ERROR);
   DCHECK(!terms_of_service_negotiator_);
-  DCHECK(!support_host_->HasAuthDelegate());
+  DCHECK(!g_ui_enabled || !support_host_->HasAuthDelegate());
 
   UpdateOptInActionUMA(OptInActionType::RETRY);
 
-  if (!profile_->GetPrefs()->GetBoolean(prefs::kArcTermsAccepted)) {
-    // This can currently happen when an error page is shown when re-opt-in
-    // right after opt-out (this is a bug as it should not show an error).  When
-    // the user click the retry button on this error page, we may start terms of
-    // service negotiation instead of recreating the instance.
-    // TODO(hidehiko): consider removing this case after fixing the bug.
-    MaybeStartTermsOfServiceNegotiation();
-  } else if (state_ == State::ACTIVE) {
+  if (state_ == State::ACTIVE) {
     // 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.
-    support_host_->ShowArcLoading();
-    ShutdownSession();
+    if (support_host_)
+      support_host_->ShowArcLoading();
+    // In unit tests ShutdownSession may be executed inline and OnSessionStopped
+    // is called before |reenable_arc_| is set.
     reenable_arc_ = true;
+    ShutdownSession();
   } 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.
-    StartAndroidManagementCheck();
+    // Otherwise, we start ARC once it is stopped now. Usually ARC container is
+    // left active after provisioning failure but in case
+    // ProvisioningResult::ARC_STOPPED and
+    // ProvisioningResult::CHROME_SERVER_COMMUNICATION_ERROR failures container
+    // is stopped.
+    // At this point ToS is already accepted and
+    // IsArcTermsOfServiceNegotiationNeeded returns true or ToS needs not to be
+    // shown at all. However there is an exception when this does not happen in
+    // case an error page is shown when re-opt-in right after opt-out (this is a
+    // bug as it should not show an error). When the user click the retry
+    // button on this error page, we may start ToS negotiation instead of
+    // recreating the instance.
+    // TODO(hidehiko): consider removing this case after fixing the bug.
+    MaybeStartTermsOfServiceNegotiation();
   }
 }
 

chrome/browser/chromeos/arc/arc_session_manager_unittest.cc

@@ -1276,6 +1276,169 @@ TEST_P(ArcSessionOobeOptInNegotiatorTest, OobeTermsViewDestroyed) {
   }
 }
 
+struct ArcSessionRetryTestParam {
+  enum class Negotiation {
+    // Negotiation is required for provisioning.
+    REQUIRED,
+    // Negotiation is not required and not shown for provisioning.
+    SKIPPED,
+  };
+
+  Negotiation negotiation;
+
+  // Provisioning error to test.
+  ProvisioningResult error;
+
+  // Whether ARC++ container is alive on error.
+  bool container_alive;
+
+  // Whether data is removed on error.
+  bool data_removed;
+};
+
+constexpr ArcSessionRetryTestParam kRetryTestCases[] = {
+    {ArcSessionRetryTestParam::Negotiation::REQUIRED,
+     ProvisioningResult::UNKNOWN_ERROR, true, true},
+    {ArcSessionRetryTestParam::Negotiation::REQUIRED,
+     ProvisioningResult::GMS_NETWORK_ERROR, true, false},
+    {ArcSessionRetryTestParam::Negotiation::REQUIRED,
+     ProvisioningResult::GMS_SERVICE_UNAVAILABLE, true, false},
+    {ArcSessionRetryTestParam::Negotiation::REQUIRED,
+     ProvisioningResult::GMS_BAD_AUTHENTICATION, true, false},
+    {ArcSessionRetryTestParam::Negotiation::REQUIRED,
+     ProvisioningResult::DEVICE_CHECK_IN_FAILED, true, false},
+    {ArcSessionRetryTestParam::Negotiation::SKIPPED,
+     ProvisioningResult::CLOUD_PROVISION_FLOW_FAILED, true, true},
+    {ArcSessionRetryTestParam::Negotiation::REQUIRED,
+     ProvisioningResult::MOJO_VERSION_MISMATCH, true, false},
+    {ArcSessionRetryTestParam::Negotiation::REQUIRED,
+     ProvisioningResult::MOJO_CALL_TIMEOUT, true, false},
+    {ArcSessionRetryTestParam::Negotiation::REQUIRED,
+     ProvisioningResult::DEVICE_CHECK_IN_TIMEOUT, true, false},
+    {ArcSessionRetryTestParam::Negotiation::REQUIRED,
+     ProvisioningResult::DEVICE_CHECK_IN_INTERNAL_ERROR, true, false},
+    {ArcSessionRetryTestParam::Negotiation::REQUIRED,
+     ProvisioningResult::GMS_SIGN_IN_FAILED, true, false},
+    {ArcSessionRetryTestParam::Negotiation::REQUIRED,
+     ProvisioningResult::GMS_SIGN_IN_TIMEOUT, true, false},
+    {ArcSessionRetryTestParam::Negotiation::REQUIRED,
+     ProvisioningResult::GMS_SIGN_IN_INTERNAL_ERROR, true, false},
+    {ArcSessionRetryTestParam::Negotiation::SKIPPED,
+     ProvisioningResult::CLOUD_PROVISION_FLOW_TIMEOUT, true, true},
+    {ArcSessionRetryTestParam::Negotiation::SKIPPED,
+     ProvisioningResult::CLOUD_PROVISION_FLOW_INTERNAL_ERROR, true, true},
+    {ArcSessionRetryTestParam::Negotiation::REQUIRED,
+     ProvisioningResult::ARC_STOPPED, false, false},
+    {ArcSessionRetryTestParam::Negotiation::REQUIRED,
+     ProvisioningResult::OVERALL_SIGN_IN_TIMEOUT, true, true},
+    {ArcSessionRetryTestParam::Negotiation::REQUIRED,
+     ProvisioningResult::CHROME_SERVER_COMMUNICATION_ERROR, false, false},
+    {ArcSessionRetryTestParam::Negotiation::REQUIRED,
+     ProvisioningResult::NO_NETWORK_CONNECTION, true, false},
+};
+
+class ArcSessionRetryTest
+    : public ArcSessionManagerTest,
+      public testing::WithParamInterface<ArcSessionRetryTestParam> {
+ public:
+  ArcSessionRetryTest() = default;
+
+  void SetUp() override {
+    ArcSessionManagerTest::SetUp();
+
+    GetFakeUserManager()->set_current_user_new(true);
+
+    // Make negotiation not needed by switching to managed flow with other
+    // preferences under the policy, similar to google.com provisioning case.
+    if (GetParam().negotiation ==
+        ArcSessionRetryTestParam::Negotiation::SKIPPED) {
+      policy::ProfilePolicyConnector* const connector =
+          policy::ProfilePolicyConnectorFactory::GetForBrowserContext(
+              profile());
+      connector->OverrideIsManagedForTesting(true);
+
+      profile()->GetTestingPrefService()->SetManagedPref(
+          prefs::kArcEnabled, std::make_unique<base::Value>(true));
+      // Set all prefs as managed to simulate google.com account provisioning.
+      profile()->GetTestingPrefService()->SetManagedPref(
+          prefs::kArcBackupRestoreEnabled,
+          std::make_unique<base::Value>(false));
+      profile()->GetTestingPrefService()->SetManagedPref(
+          prefs::kArcLocationServiceEnabled,
+          std::make_unique<base::Value>(false));
+      EXPECT_FALSE(arc::IsArcTermsOfServiceNegotiationNeeded(profile()));
+    }
+  }
+
+  void TearDown() override {
+    // Correctly stop service.
+    arc_session_manager()->Shutdown();
+    ArcSessionManagerTest::TearDown();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ArcSessionRetryTest);
+};
+
+INSTANTIATE_TEST_CASE_P(,
+                        ArcSessionRetryTest,
+                        ::testing::ValuesIn(kRetryTestCases));
+
+// Verifies that Android container behaves as expected.* This checks:
+//   * Whether ARC++ container alive or not on error.
+//   * Whether Android data is removed or not on error.
+//   * ARC++ Container is restared on retry.
+TEST_P(ArcSessionRetryTest, ContainerRestarted) {
+  arc_session_manager()->SetProfile(profile());
+  arc_session_manager()->Initialize();
+  arc_session_manager()->RequestEnable();
+
+  if (GetParam().negotiation ==
+      ArcSessionRetryTestParam::Negotiation::REQUIRED) {
+    EXPECT_EQ(ArcSessionManager::State::NEGOTIATING_TERMS_OF_SERVICE,
+              arc_session_manager()->state());
+    arc_session_manager()->OnTermsOfServiceNegotiatedForTesting(true);
+  }
+
+  EXPECT_EQ(ArcSessionManager::State::CHECKING_ANDROID_MANAGEMENT,
+            arc_session_manager()->state());
+  arc_session_manager()->StartArcForTesting();
+  EXPECT_EQ(ArcSessionManager::State::ACTIVE, arc_session_manager()->state());
+
+  arc_session_manager()->OnProvisioningFinished(GetParam().error);
+
+  // In case of permanent error data removal request is scheduled.
+  EXPECT_EQ(GetParam().data_removed,
+            profile()->GetPrefs()->GetBoolean(prefs::kArcDataRemoveRequested));
+
+  if (GetParam().container_alive) {
+    // We don't stop ARC due to let user submit user feedback with alive Android
+    // container.
+    EXPECT_EQ(ArcSessionManager::State::ACTIVE, arc_session_manager()->state());
+  } else {
+    // Container is stopped automatically on this error.
+    EXPECT_EQ(ArcSessionManager::State::STOPPED,
+              arc_session_manager()->state());
+  }
+
+  arc_session_manager()->OnRetryClicked();
+
+  if (GetParam().data_removed) {
+    // Check state goes from REMOVING_DATA_DIR to CHECKING_ANDROID_MANAGEMENT
+    EXPECT_TRUE(WaitForDataRemoved(
+        ArcSessionManager::State::CHECKING_ANDROID_MANAGEMENT));
+  }
+
+  arc_session_manager()->StartArcForTesting();
+  EXPECT_EQ(ArcSessionManager::State::ACTIVE, arc_session_manager()->state());
+
+  // Successful retry keeps ARC++ container running.
+  arc_session_manager()->OnProvisioningFinished(ProvisioningResult::SUCCESS);
+  EXPECT_EQ(ArcSessionManager::State::ACTIVE, arc_session_manager()->state());
+
+  arc_session_manager()->Shutdown();
+}
+
 }  // namespace
 
 }  // namespace arc