[OfflinePages] Fixes RequestCoordinator bug not clearing state on timeout

components/offline_pages/background/request_coordinator.cc

Index: components/offline_pages/background/request_coordinator.cc
diff --git a/components/offline_pages/background/request_coordinator.cc b/components/offline_pages/background/request_coordinator.cc
index 098024afd8436c03a6875d2c1f49a780973de2b2..023710520e02f459e80459103f191bdc37aebc0d 100644
--- a/components/offline_pages/background/request_coordinator.cc
+++ b/components/offline_pages/background/request_coordinator.cc
@@ -241,13 +241,15 @@ void RequestCoordinator::StopPrerendering(Offliner::RequestStatus stop_status) {
     DCHECK(active_request_.get());
     offliner_->Cancel();
 
-    // If we timed out, let the offliner done callback handle it.
-    if (stop_status == Offliner::RequestStatus::REQUEST_COORDINATOR_TIMED_OUT)
-      return;
-
-    // Otherwise, this attempt never really had a chance to run, mark it
-    // aborted.
-    AbortRequestAttempt(*active_request_.get());
+    if (stop_status == Offliner::RequestStatus::REQUEST_COORDINATOR_TIMED_OUT) {
+      // Consider watchdog timeout a completed attempt.
+      SavePageRequest request(*active_request_.get());
+      UpdateRequestForCompletedAttempt(request,
+                                       Offliner::REQUEST_COORDINATOR_TIMED_OUT);
+    } else {
+      // Otherwise consider this stop an aborted attempt.
+      UpdateRequestForAbortedAttempt(*active_request_.get());
+    }
   }
 
   // Stopping offliner means it will not call callback so set last status.
@@ -302,7 +304,8 @@ bool RequestCoordinator::CancelActiveRequestIfItMatches(
   return false;
 }
 
-void RequestCoordinator::AbortRequestAttempt(const SavePageRequest& request) {
+void RequestCoordinator::UpdateRequestForAbortedAttempt(
+    const SavePageRequest& request) {
   if (request.started_attempt_count() >= policy_->GetMaxStartedTries()) {
     const BackgroundSavePageResult result(
         BackgroundSavePageResult::START_COUNT_EXCEEDED);
@@ -506,7 +509,13 @@ void RequestCoordinator::StopProcessing(
 }
 
 void RequestCoordinator::HandleWatchdogTimeout() {
-  StopProcessing(Offliner::REQUEST_COORDINATOR_TIMED_OUT);
+  Offliner::RequestStatus watchdog_status =
+      Offliner::REQUEST_COORDINATOR_TIMED_OUT;
+  StopPrerendering(watchdog_status);
+  if (ShouldTryNextRequest(watchdog_status))

[Pete Williamson, 2016/11/11 00:24:04]

Why call ShouldTryNextRequest() here?  In this case, the answer is always yes
(because timed out returns true).  It might be better to just call
TryNextRequest.

[dougarnett, 2016/11/11 16:36:45]

Yeah, almost did. The idea here was for the policy to be in the ShouldTry method
instead of sprinkled around (one place to look/change, no contradictions). 

+    TryNextRequest();
+  else
+    scheduler_callback_.Run(true);
 }
 
 // Returns true if the caller should expect a callback, false otherwise. For
@@ -741,16 +750,25 @@ void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
   RecordOfflinerResultUMA(request.client_id(), request.creation_time(),
                           last_offlining_status_);
   watchdog_timer_.Stop();
-
   is_busy_ = false;
   active_request_.reset(nullptr);
 
+  UpdateRequestForCompletedAttempt(request, status);
+  if (ShouldTryNextRequest(status))
+    TryNextRequest();
+  else
+    scheduler_callback_.Run(true);
+}
+
+void RequestCoordinator::UpdateRequestForCompletedAttempt(
+    const SavePageRequest& request,
+    Offliner::RequestStatus status) {
   if (status == Offliner::RequestStatus::FOREGROUND_CANCELED ||
       status == Offliner::RequestStatus::PRERENDERING_CANCELED) {
     // Update the request for the canceled attempt.
     // TODO(dougarnett): See if we can conclusively identify other attempt
     // aborted cases to treat this way (eg, for Render Process Killed).
-    AbortRequestAttempt(request);
+    UpdateRequestForAbortedAttempt(request);
   } else if (status == Offliner::RequestStatus::SAVED) {
     // Remove the request from the queue if it succeeded.
     RemoveAttemptedRequest(request, BackgroundSavePageResult::SUCCESS);
@@ -777,29 +795,27 @@ void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
                    weak_ptr_factory_.GetWeakPtr(), request.request_id(),
                    request.client_id()));
   }
+}
 
-  // Determine whether we might try another request in this
-  // processing window based on how the previous request completed.
-  switch (status) {
+bool RequestCoordinator::ShouldTryNextRequest(
+    Offliner::RequestStatus previous_request_status) {
+  switch (previous_request_status) {
     case Offliner::RequestStatus::SAVED:
     case Offliner::RequestStatus::SAVE_FAILED:
     case Offliner::RequestStatus::REQUEST_COORDINATOR_CANCELED:
     case Offliner::RequestStatus::REQUEST_COORDINATOR_TIMED_OUT:
     case Offliner::RequestStatus::PRERENDERING_FAILED_NO_RETRY:
-      // Consider processing another request in this service window.
-      TryNextRequest();
-      break;
+      return true;
     case Offliner::RequestStatus::FOREGROUND_CANCELED:
     case Offliner::RequestStatus::PRERENDERING_CANCELED:
     case Offliner::RequestStatus::PRERENDERING_FAILED:
       // No further processing in this service window.
-      // Let the scheduler know we are done processing.
-      scheduler_callback_.Run(true);
-      break;
+      return false;
     default:
       // Make explicit choice about new status codes that actually reach here.
       // Their default is no further processing in this service window.
       NOTREACHED();
+      return false;
   }
 }