[Offline Pages] Turn Offliner::Cancel into an async operation to resolve conflicting assumptions

components/offline_pages/core/background/request_coordinator.cc

Index: components/offline_pages/core/background/request_coordinator.cc
diff --git a/components/offline_pages/core/background/request_coordinator.cc b/components/offline_pages/core/background/request_coordinator.cc
index 83a844b09803805444ca8d8cfd86901be7869443..099a40d0d5bcb1da4b2af27b4b5e6e7310192db8 100644
--- a/components/offline_pages/core/background/request_coordinator.cc
+++ b/components/offline_pages/core/background/request_coordinator.cc
@@ -244,35 +244,19 @@ void RequestCoordinator::GetQueuedRequestsCallback(
   callback.Run(std::move(requests));
 }
 
-void RequestCoordinator::StopPrerendering(Offliner::RequestStatus stop_status) {
+void RequestCoordinator::StopPrerendering(

[fgorski, 2017/02/27 18:07:25]

Extra thing to consider: Now that this function is asynchronous and execution
will be split, is there a chance someone calls it again, before it finishes?
Would that cause problems? Can we guard from such situation?

[chili, 2017/02/28 00:44:14]

I *don't* think calling it again before it finishes will cause any problems.
Additional cancels, as it currently stands, will replace the callback on the
offliner, and only the last callback will be called.

+    const Offliner::CancelCallback& next_callback,

[fgorski, 2017/02/27 18:07:25]

nit: cancel_callback.
Or something indicating that is the final callback for this chain.

[chili, 2017/02/28 00:44:14]

Done.

+    Offliner::RequestStatus stop_status) {
   if (offliner_ && is_busy_) {
     DCHECK(active_request_.get());
-    offliner_->Cancel();
-
-    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());
-    }
+    offliner_->Cancel(
+        base::Bind(&RequestCoordinator::HandleCancelStopStatusCallback,
+                   weak_ptr_factory_.GetWeakPtr(), next_callback, stop_status));
+    return;
   }
 
-  // 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(),
-                            active_request_->creation_time(),
-                            last_offlining_status_);
-    is_busy_ = false;
-    active_request_.reset();
-  }
+  HandleCancelRecordResultCallback(next_callback, stop_status,
+                                   active_request_->request_id());

[Pete Williamson, 2017/02/24 00:35:46]

Does HandleCancelRecordResultCallback() get called twice?  Once here, and once
from the HandleCancelStopStatusCallback?

If we do make a function, would RecordCancelResult() be better, since this isn't
a callback per-se, but a function that the callback calls?

We might be able to move the UMA stuff into a function that gets called here,
and the is_busy_ and active_request_ handling into
HandleCancelStopStatusCallback.

[chili, 2017/02/28 00:44:14]

It shouldn't get called twice:

We return on line 255, so if there's an offliner_ && is_busy_, it will call
HandleCancelStopStatusCallback, which will call HandleCancelRecordResultCallback

If there isn't an offliner_ or it's not busy, we will call
HandleCancelRecordResultCallback here directly.

 }
 
 void RequestCoordinator::GetRequestsForSchedulingCallback(
@@ -303,8 +287,10 @@ bool RequestCoordinator::CancelActiveRequestIfItMatches(
   if (active_request_ != nullptr) {
     if (request_ids.end() != std::find(request_ids.begin(), request_ids.end(),
                                        active_request_->request_id())) {
-      StopPrerendering(Offliner::RequestStatus::REQUEST_COORDINATOR_CANCELED);
-      active_request_.reset(nullptr);
+      StopPrerendering(
+          base::Bind(&RequestCoordinator::ResetActiveRequestCallback,
+                     weak_ptr_factory_.GetWeakPtr()),
+          Offliner::RequestStatus::REQUEST_COORDINATOR_CANCELED);
       return true;
     }
   }
@@ -486,6 +472,56 @@ void RequestCoordinator::HandleRemovedRequests(
     NotifyCompleted(request, status);
 }
 
+void RequestCoordinator::HandleCancelStopStatusCallback(
+    const Offliner::CancelCallback& nextCallback,
+    Offliner::RequestStatus stop_status,
+    int64_t offline_id) {
+  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());
+  }
+
+  HandleCancelRecordResultCallback(nextCallback, stop_status, offline_id);
+}
+
+void RequestCoordinator::HandleCancelRecordResultCallback(
+    const Offliner::CancelCallback& nextCallback,
+    Offliner::RequestStatus stop_status,
+    int64_t offline_id) {
+  // 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(),
+                            active_request_->creation_time(),
+                            last_offlining_status_);
+    is_busy_ = false;
+    active_request_.reset();
+  }
+
+  nextCallback.Run(offline_id);

[Pete Williamson, 2017/02/24 00:35:46]

This callback chaining is kind of painful maintenance wise, as it causes the
logic for each ongoing operation to be split up even more.

Maybe HandleCancelStopStatus() could become a function that is called by
StartSchedulerCallback or TryNextRequestCallback, and we could pass those
callbacks directly to cancel.

[chili, 2017/03/01 02:37:33]

Per offline discussion, I updated the chain so one of them is simply a function
rather than a callback part of a chain.

+}
+
+void RequestCoordinator::ResetActiveRequestCallback(int64_t offline_id) {
+  active_request_.reset(nullptr);
+}
+
+void RequestCoordinator::StartSchedulerCallback(int64_t offline_id) {
+  scheduler_callback_.Run(true);
+}
+
+void RequestCoordinator::TryNextRequestCallback(int64_t offline_id) {
+  TryNextRequest(!kStartOfProcessing);
+}
+
 void RequestCoordinator::ScheduleAsNeeded() {
   // Get all requests from queue (there is no filtering mechanism).
   queue_->GetRequests(
@@ -495,17 +531,17 @@ void RequestCoordinator::ScheduleAsNeeded() {
 
 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);
+  StopPrerendering(base::Bind(&RequestCoordinator::StartSchedulerCallback,
+                              weak_ptr_factory_.GetWeakPtr()),
+                   stop_status);
 }
 
 void RequestCoordinator::HandleWatchdogTimeout() {
   Offliner::RequestStatus watchdog_status =
       Offliner::REQUEST_COORDINATOR_TIMED_OUT;
-  StopPrerendering(watchdog_status);
-  TryNextRequest(!kStartOfProcessing);
+  StopPrerendering(base::Bind(&RequestCoordinator::TryNextRequestCallback,
+                              weak_ptr_factory_.GetWeakPtr()),
+                   watchdog_status);
 }
 
 // Returns true if the caller should expect a callback, false otherwise. For