[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) {
+      // Since not a timeout, consider that the current attempt never had a
+      // chance to complete, mark it aborted.
+      AbortRequestAttempt(active_request_.get());

[Pete Williamson, 2016/11/10 01:03:30]

When we get a timeout like this, we should get the OfflinerDoneCallback, which
should be the one to mark is_busy_ to false.  Is that callback not happening
somehow?  If it is not happening, that's a problem it is what calls
TryNextRequest()

[dougarnett, 2016/11/10 16:38:38]

Offliner::Cancel contract is to not call the callback:
https://cs.chromium.org/chromium/src/components/offline_pages/background/offl...
so don't see how the early return from here ever worked.

I will look at breaking out common "completion" logic from OfflinerDoneCallback
to call from HandleWatchdogTimeout and see if that seems viable.

[Pete Williamson, 2016/11/10 19:01:29]

OK, in that case, it is my misunderstanding of the contract.  So, we need to
look at what OfflinerDoneCallback does, and do the relevant parts .  Doing it
from HandleWatchdogTimeout seems like a good idea.  We need to at least clear
the is_busy_ flag and active_request_, and call TryNextRequest().  We may also
need to track some UMA.  We should remove the request if it is over the retry
limit, and call MarkCompleted() if it is not over the retry limit.

There may be an opportunity to refactor some common tasks with
OfflinerDoneCallback and HandleWatchdogTimeout into a single function.

+    }
   }
 
   // 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 608 matching lines @@
 
 ClientPolicyController* RequestCoordinator::GetPolicyController() {
   return policy_controller_.get();
 }
 
 void RequestCoordinator::Shutdown() {
   network_quality_estimator_ = nullptr;
 }
 
 }  // namespace offline_pages