[OfflinePages] Fixes RequestCoordinator bug not clearing state on timeout

components/offline_pages/background/request_coordinator.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 "components/offline_pages/background/request_coordinator.h"
 
 #include <limits>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 223 matching lines @@
     RequestQueue::GetRequestsResult result,
     std::vector<std::unique_ptr<SavePageRequest>> requests) {
   callback.Run(std::move(requests));
 }
 
 void RequestCoordinator::StopPrerendering(Offliner::RequestStatus stop_status) {
   if (offliner_ && is_busy_) {
     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.
   last_offlining_status_ = stop_status;
 
   if (active_request_) {
     event_logger_.RecordOfflinerResult(active_request_->client_id().name_space,
                                        last_offlining_status_,
                                        active_request_->request_id());
     RecordOfflinerResultUMA(active_request_->client_id(),
@@ skipping 34 matching lines @@
                                        active_request_->request_id())) {
       StopPrerendering(Offliner::RequestStatus::REQUEST_COORDINATOR_CANCELED);
       active_request_.reset(nullptr);
       return true;
     }
   }
 
   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);
     event_logger_.RecordDroppedSavePageRequest(request.client_id().name_space,
                                                result, request.request_id());
     RemoveAttemptedRequest(request, result);
   } else {
     queue_->MarkAttemptAborted(
         request.request_id(),
         base::Bind(&RequestCoordinator::MarkAttemptAbortedDone,
@@ skipping 183 matching lines @@
 void RequestCoordinator::StopProcessing(
     Offliner::RequestStatus stop_status) {
   processing_state_ = ProcessingWindowState::STOPPED;
   StopPrerendering(stop_status);
 
   // Let the scheduler know we are done processing.
   scheduler_callback_.Run(true);
 }
 
 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
 // instance, this would return false if a request is already in progress.
 bool RequestCoordinator::StartProcessing(
     const DeviceConditions& device_conditions,
     const base::Callback<void(bool)>& callback) {
   return StartProcessingInternal(ProcessingWindowState::SCHEDULED_WINDOW,
                                  device_conditions, callback);
 }
@@ skipping 214 matching lines @@
            << (status == Offliner::RequestStatus::SAVED)
            << ", status: " << static_cast<int>(status) << ", " << __func__;
   DCHECK_NE(status, Offliner::RequestStatus::UNKNOWN);
   DCHECK_NE(status, Offliner::RequestStatus::LOADED);
   event_logger_.RecordOfflinerResult(request.client_id().name_space, status,
                                      request.request_id());
   last_offlining_status_ = status;
   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);
   } else if (status == Offliner::RequestStatus::PRERENDERING_FAILED_NO_RETRY) {
     RemoveAttemptedRequest(request,
                            BackgroundSavePageResult::PRERENDER_FAILURE);
   } else if (request.completed_attempt_count() + 1 >=
              policy_->GetMaxCompletedTries()) {
     // Remove from the request queue if we exceeded max retries. The +1
     // represents the request that just completed. Since we call
     // MarkAttemptCompleted within the if branches, the completed_attempt_count
     // has not yet been updated when we are checking the if condition.
     const BackgroundSavePageResult result(
         BackgroundSavePageResult::RETRY_COUNT_EXCEEDED);
     event_logger_.RecordDroppedSavePageRequest(request.client_id().name_space,
                                                result, request.request_id());
     RemoveAttemptedRequest(request, result);
   } else {
     // If we failed, but are not over the limit, update the request in the
     // queue.
     queue_->MarkAttemptCompleted(
         request.request_id(),
         base::Bind(&RequestCoordinator::MarkAttemptCompletedDoneCallback,
                    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;
   }
 }
 
 void RequestCoordinator::EnableForOffliner(int64_t request_id) {
   // Since the recent tab helper might call multiple times, ignore subsequent
   // calls for a particular request_id.
   if (disabled_requests_.find(request_id) == disabled_requests_.end())
     return;
   disabled_requests_.erase(request_id);
   // If we are not busy, start processing right away.
@@ skipping 59 matching lines @@
 
 ClientPolicyController* RequestCoordinator::GetPolicyController() {
   return policy_controller_.get();
 }
 
 void RequestCoordinator::Shutdown() {
   network_quality_estimator_ = nullptr;
 }
 
 }  // namespace offline_pages