Added retry policy

chrome/browser/chromeos/login/enrollment/enrollment_screen.cc

 // Copyright (c) 2012 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/login/enrollment/enrollment_screen.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/command_line.h"
@@ skipping 46 matching lines @@
 namespace chromeos {
 
 // static
 EnrollmentScreen* EnrollmentScreen::Get(ScreenManager* manager) {
   return static_cast<EnrollmentScreen*>(
       manager->GetScreen(WizardController::kEnrollmentScreenName));
 }
 
 EnrollmentScreen::EnrollmentScreen(BaseScreenDelegate* base_screen_delegate,
                                    EnrollmentScreenActor* actor)
-    : BaseScreen(base_screen_delegate),
-      actor_(actor),
-      weak_ptr_factory_(this) {}
+    : BaseScreen(base_screen_delegate), actor_(actor), weak_ptr_factory_(this) {

[joth, 2016/11/23 22:24:02]

as a general guide use initializer list when you can. so here:

, retry_backoff_(&retry_policy) {
  policy_.num_errors_to_ignore = 0; 
  ... // etc
}

[kumarniranjan, 2016/11/24 18:12:46]

Done.

+  policy_ = new net::BackoffEntry::Policy();
+  policy_->num_errors_to_ignore = 0;
+  policy_->initial_delay_ms = 4000;
+  policy_->multiply_factor = 2.0;

[joth, 2016/11/23 22:24:02]

given we're using this to paper over the TPM init, I'm inclined to have it not
multiply quite so quickly. suggest:
factor = 1.5 

[kumarniranjan, 2016/11/24 18:12:46]

Done.

+  policy_->jitter_factor = 0.1;
+  policy_->maximum_backoff_ms = -1;

[joth, 2016/11/23 22:24:03]

Lets cap the maximum delay to 8 minutes

[kumarniranjan, 2016/11/24 18:12:46]

Done.

+  policy_->entry_lifetime_ms = -1;
+  backoff_ = new net::BackoffEntry(policy_);
+}
 
 EnrollmentScreen::~EnrollmentScreen() {
   DCHECK(!enrollment_helper_ || g_browser_process->IsShuttingDown());
 }
 
 void EnrollmentScreen::SetParameters(
     const policy::EnrollmentConfig& enrollment_config,
     pairing_chromeos::ControllerPairingController* shark_controller) {
   enrollment_config_ = enrollment_config;
   switch (enrollment_config_.auth_mechanism) {
@@ skipping 111 matching lines @@
   // TODO(rsorokin): Move ShowAdJoin after STEP_REGISTRATION
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           chromeos::switches::kEnableAd)) {
     actor_->ShowAdJoin();
   } else {
     OnAdJoined("");
   }
 }
 
 void EnrollmentScreen::OnRetry() {
   Show();

[joth, 2016/11/23 22:24:02]

is it safe to call this multiple times? (just wondering if we can have multiple
errors or user clicking "retry" resulting in multiple calls to this)
Maybe you can make a unit test to cover this scenario 

[kumarniranjan, 2016/11/24 18:12:46]

The current UI without my changes allows for this to be called multiple times. I
tried doing it manually, and could not observe any ill effects. However, I agree
that a test is needed for this.

 }
 
 void EnrollmentScreen::OnCancel() {
   if (AdvanceToNextAuth()) {
     Show();
     return;
   }
 
   UMA(policy::kMetricEnrollmentCancelled);
   if (elapsed_timer_)
@@ skipping 34 matching lines @@
   LOG(WARNING) << "Enrollment error occured: status=" << status.status()
                << " http status=" << status.http_status()
                << " DM status=" << status.client_status();
   RecordEnrollmentErrorMetrics();
   // If the DM server does not have a device pre-provisioned for attestation-
   // based enrollment and we have a fallback authentication, show it.
   if (status.status() == policy::EnrollmentStatus::STATUS_REGISTRATION_FAILED &&
       status.client_status() == policy::DM_STATUS_SERVICE_DEVICE_NOT_FOUND &&
       current_auth_ == AUTH_ATTESTATION && AdvanceToNextAuth())
     Show();
-  else
+  else {
     actor_->ShowEnrollmentStatus(status);
+    if (policy::DeviceCloudPolicyManagerChromeOS::
+            GetZeroTouchEnrollmentMode() ==
+        policy::ZeroTouchEnrollmentMode::HANDS_OFF) {
+      backoff_->InformOfRequest(false);
+      timer_.Start(FROM_HERE, backoff_->GetTimeUntilRelease(),
+                   base::Bind(&EnrollmentScreen::OnRetry,

[joth, 2016/11/23 22:24:02]

I don't have a strong preference, but it maybe clearer to make a new "DoRetry"
method that we call from here and from "OnRetry" as the latter is (AIUI?)
intended as a UI button click handler so it's misleading to call it from the
timer

It'd be also nice to log when the retry is happening and why (user button press
vs timer)

[kumarniranjan, 2016/11/24 18:12:46]

I agree. I ended up calling the method "ProcessRetry" instead of "DoRetry"

+                              weak_ptr_factory_.GetWeakPtr()));
+    }
+  }
 }
 
 void EnrollmentScreen::OnOtherError(
     EnterpriseEnrollmentHelper::OtherError error) {

[joth, 2016/11/23 22:24:02]

Wonder if we should retry here too?

https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/enrollment...

The 2 errors look pretty fatal (code bug and device attempting to enroll to
wrong domain) and "should" be impossible in rialto. otoh it'd probably not be
harmful to retry anyway. (Maybe the domain mismatched can be fixed in the CPanel
and the retry would eventually succeed)

[kumarniranjan, 2016/11/24 18:12:46]

Considering we are in hands-off mode, it can't hurt to retry here, especially
since the user can't do anything. I will go ahead and add that logic here.

   RecordEnrollmentErrorMetrics();
   actor_->ShowOtherError(error);
 }
 
 void EnrollmentScreen::OnDeviceEnrolled(const std::string& additional_token) {
   // TODO(pbond): remove this LOG once http://crbug.com/586961 is fixed.
   LOG(WARNING) << "Device is successfully enrolled.";
   if (!additional_token.empty())
     SendEnrollmentAuthToken(additional_token);
 
@@ skipping 52 matching lines @@
   std::string location = policy ? policy->annotated_location() : std::string();
   actor_->ShowAttributePromptScreen(asset_id, location);
 }
 
 void EnrollmentScreen::SendEnrollmentAuthToken(const std::string& token) {
   DCHECK(shark_controller_);
   shark_controller_->OnAuthenticationDone(enrolling_user_domain_, token);
 }
 
 void EnrollmentScreen::ShowEnrollmentStatusOnSuccess() {
+  backoff_->InformOfRequest(true);
   if (elapsed_timer_)
     UMA_ENROLLMENT_TIME(kMetricEnrollmentTimeSuccess, elapsed_timer_);
   actor_->ShowEnrollmentStatus(policy::EnrollmentStatus::ForStatus(
       policy::EnrollmentStatus::STATUS_SUCCESS));
 }
 
 void EnrollmentScreen::UMA(policy::MetricEnrollment sample) {
   EnrollmentUMA(sample, config_.mode);
 }
 
 void EnrollmentScreen::ShowSigninScreen() {
   actor_->Show();
   actor_->ShowSigninScreen();
 }
 
 void EnrollmentScreen::RecordEnrollmentErrorMetrics() {
   enrollment_failed_once_ = true;
   //  TODO(drcrash): Maybe create multiple metrics (http://crbug.com/640313)?
   if (elapsed_timer_)
     UMA_ENROLLMENT_TIME(kMetricEnrollmentTimeFailure, elapsed_timer_);
 }
 
 }  // namespace chromeos