[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 93e127d3e59d00414187cfcac3176ef5b2eae8f1..7807e953a24994b60cec008464af2d831168d159 100644
--- a/components/offline_pages/background/request_coordinator.cc
+++ b/components/offline_pages/background/request_coordinator.cc
@@ -241,13 +241,11 @@ 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) {
+      // 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.