Added retry policy

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

Index: chrome/browser/chromeos/login/enrollment/enrollment_screen.cc
diff --git a/chrome/browser/chromeos/login/enrollment/enrollment_screen.cc b/chrome/browser/chromeos/login/enrollment/enrollment_screen.cc
index b752d4e61daf06870246a90520e41c1071b32fdf..e64ada44c8c263a49032b8736c57b9397106e18e 100644
--- a/chrome/browser/chromeos/login/enrollment/enrollment_screen.cc
+++ b/chrome/browser/chromeos/login/enrollment/enrollment_screen.cc
@@ -66,7 +66,19 @@ EnrollmentScreen::EnrollmentScreen(BaseScreenDelegate* base_screen_delegate,
                                    EnrollmentScreenActor* actor)
     : BaseScreen(base_screen_delegate),
       actor_(actor),
-      weak_ptr_factory_(this) {}
+      retry_backoff_(&retry_policy_),

[xiyuan, 2016/11/28 22:16:34]

Can we use a unique_ptr and create |retry_backoff_| after policy is filled up,
similar to [1] ?

[1]
https://cs.chromium.org/chromium/src/chromeos/network/portal_detector/network...

[kumarniranjan, 2016/12/05 22:04:53]

Done.

+      num_retries_(0),

[xiyuan, 2016/11/28 22:16:34]

nit: move the initializer to header

https://chromium-cpp.appspot.com/, check out "Non-Static Class Member
Initializers" row.

[kumarniranjan, 2016/12/05 22:04:52]

Done.

+      weak_ptr_factory_(this) {
+  // It is okay to initialize the fields of retry_policy_ after constructing
+  // retry_backoff_ with a pointer to this struct.
+  retry_policy_.num_errors_to_ignore = 0;
+  retry_policy_.initial_delay_ms = 4000;
+  retry_policy_.multiply_factor = 1.5;
+  retry_policy_.jitter_factor = 0.1;
+  retry_policy_.maximum_backoff_ms = 480000;

[The one and only Dr. Crash, 2016/11/30 23:30:08]

You set a maximum backoff but you never call ShouldRejectRequest() so what
effect does that backoff time have? Is that the maximum interval between two
attempts at the very worst? Also maybe interesting to see how different your
policy is from, say the one for chromoting:
https://cs.chromium.org/chromium/src/remoting/host/chromoting_host.cc?rcl=148...
 

How did you come up w/ the different numbers?

[kumarniranjan, 2016/12/05 22:04:52]

Jonathan wanted the maximum interval between retry attempts to be 8 min (480000
ms). He also wanted the multiplication factor to be 1.5. I came up with the 4 s
initial delay. It works well when I try it on my device. If you think I should
change these numbers, please let me know.

[joth__google, 2016/12/05 22:59:26]

I gave a bit of reasoning in https://codereview.chromium.org/2526973002/#msg4
but basically:
- x2 multiple is reasonable for network requests but for local operatoin I like
the retry not to back quiet so quick as we expect it to normally be done within
the initial delay, and if it wasn't then waiting a further 2x initial delay is a
high penalty.

- 8mins max retry interval is kind of arbitrary, but if the failure is due to
network reception (likely case) then it's actually cheap to keep the retry bound
relatively low (effectively each retry is a fail fast no-op) and connecting to a
network (when it comes in range) may take ~5mins so a retry  rate in this
vicinity feels about right.
If we let it back out to taking hours (or days) between retries, they become
worthless.

+  retry_policy_.entry_lifetime_ms = -1;
+  retry_policy_.always_use_initial_delay = true;
+}
 
 EnrollmentScreen::~EnrollmentScreen() {
   DCHECK(!enrollment_helper_ || g_browser_process->IsShuttingDown());
@@ -198,6 +210,22 @@ void EnrollmentScreen::OnLoginDone(const std::string& user,
 }
 
 void EnrollmentScreen::OnRetry() {
+  num_retries_++;

[xiyuan, 2016/11/28 22:16:34]

nit: ++num_retries_;

[The one and only Dr. Crash, 2016/11/30 23:09:56]

Yes! (Sorry, I am a big fan of pre-increment/decrement).

[kumarniranjan, 2016/12/05 22:04:52]

Done.

+  LOG(WARNING) << "Enrollment retry " << num_retries_
+               << " initiated by user button press.";
+  ProcessRetry();

[xiyuan, 2016/11/28 22:16:34]

retry_timer_.Stop() in case the timer is running?

[kumarniranjan, 2016/12/05 22:04:52]

Good catch! We don't want an automatic retry to happen while we are processing
the user's retry request.

+}
+
+void EnrollmentScreen::AutomaticRetry() {

[xiyuan, 2016/11/28 22:16:34]

nit: AutomaticRetry -> StartRetryTimer

+  retry_backoff_.InformOfRequest(false);
+  retry_timer_.Start(FROM_HERE, retry_backoff_.GetTimeUntilRelease(),
+                     base::Bind(&EnrollmentScreen::ProcessRetry,
+                                weak_ptr_factory_.GetWeakPtr()));
+  num_retries_++;

[xiyuan, 2016/11/28 22:16:34]

nit: ditto

[kumarniranjan, 2016/12/05 22:04:52]

Done.

+  LOG(WARNING) << "Enrollment retry " << num_retries_ << " initiated by timer.";

[xiyuan, 2016/11/28 22:16:34]

What is the purpose of |num_retries_| other than output in the log? Should we
have an upper limit of the retry?

[kumarniranjan, 2016/12/05 22:04:52]

It gives us the capability to set an upper limit on the number of retries in the
future. This number will probably be quite large. Since there is no UI and the
user's only recourse in the event of the problem is to call tech support, it
makes sense to retry every 8 minutes for multiple days. The num_retries_
variable is also very useful for testing.

+}
+
+void EnrollmentScreen::ProcessRetry() {
   Show();
 }
 
@@ -251,16 +279,24 @@ void EnrollmentScreen::OnEnrollmentError(policy::EnrollmentStatus status) {
   // 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())
+      current_auth_ == AUTH_ATTESTATION && AdvanceToNextAuth()) {
     Show();
-  else
+  } else {
     actor_->ShowEnrollmentStatus(status);
+    if (policy::DeviceCloudPolicyManagerChromeOS::
+            GetZeroTouchEnrollmentMode() ==
+        policy::ZeroTouchEnrollmentMode::HANDS_OFF)

[xiyuan, 2016/11/28 22:16:34]

nit: wrap the condition into a helper function (e.g. ShouldAutoRetryOnError)
since it is used in two places.

+      AutomaticRetry();

[xiyuan, 2016/11/28 22:16:34]

nit: wrap with {} since condition takes more than 1 line

[The one and only Dr. Crash, 2016/11/30 23:09:56]

Probably unnecessary once a helper method is available.

+  }
 }
 
 void EnrollmentScreen::OnOtherError(
     EnterpriseEnrollmentHelper::OtherError error) {
   RecordEnrollmentErrorMetrics();
   actor_->ShowOtherError(error);
+  if (policy::DeviceCloudPolicyManagerChromeOS::GetZeroTouchEnrollmentMode() ==
+      policy::ZeroTouchEnrollmentMode::HANDS_OFF)
+    AutomaticRetry();

[xiyuan, 2016/11/28 22:16:33]

nit: wrap with {}

[The one and only Dr. Crash, 2016/11/30 23:09:56]

Probably unnecessary once a helper method is available.

 }
 
 void EnrollmentScreen::OnDeviceEnrolled(const std::string& additional_token) {
@@ -331,6 +367,7 @@ void EnrollmentScreen::SendEnrollmentAuthToken(const std::string& token) {
 }
 
 void EnrollmentScreen::ShowEnrollmentStatusOnSuccess() {
+  retry_backoff_.InformOfRequest(true);
   if (elapsed_timer_)
     UMA_ENROLLMENT_TIME(kMetricEnrollmentTimeSuccess, elapsed_timer_);
   actor_->ShowEnrollmentStatus(policy::EnrollmentStatus::ForStatus(