[Offline Pages] Improve RequestCoordinator state tracking.

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 8060d6af4ab0678672c381b76ee821aabf821870..ebc3db1ce147b9db4e28c1f823c0fb47e86c6d0b 100644
--- a/components/offline_pages/core/background/request_coordinator.cc
+++ b/components/offline_pages/core/background/request_coordinator.cc
@@ -197,8 +197,7 @@ RequestCoordinator::RequestCoordinator(
     net::NetworkQualityEstimator::NetworkQualityProvider*
         network_quality_estimator)
     : is_low_end_device_(base::SysInfo::IsLowEndDevice()),
-      is_busy_(false),
-      is_starting_(false),
+      state_(RequestCoordinatorState::IDLE),
       processing_state_(ProcessingWindowState::STOPPED),
       use_test_device_conditions_(false),
       offliner_(std::move(offliner)),
@@ -287,7 +286,7 @@ void RequestCoordinator::GetQueuedRequestsCallback(
 void RequestCoordinator::StopPrerendering(
     const Offliner::CancelCallback& final_callback,
     Offliner::RequestStatus stop_status) {
-  if (offliner_ && is_busy_) {
+  if (offliner_ && state_ == RequestCoordinatorState::OFFLINING) {

[Pete Williamson, 2017/03/28 00:35:13]

What should we do if the state is STARTING?  We probably don't want to go on
with the request.  Maybe we should check in RequestPicked() to see if a cancel
happened while we were picking, and if so, to not then proceed to try the
request.

[romax, 2017/03/28 01:09:56]

I think this is guarded by ProcessingWindowState::STOPPED in StopProcessing().
The processing_state_ is checked in RequestPicked.
Are you worried about StopPrerendering()?

[Pete Williamson, 2017/03/28 17:12:43]

My concern is that we might have a call to StopPrerendering come in when the
request picker is active.  

StopPrerendering can be called from several places.  It can be called if the
user cancels a request, or from a call to StopProcessing (which can itself be
called externally since we expose the API), or from a call to the watchdog
timeout.  Calling from the watchdog timeout should only happen if the state is
offlining, so that is not an issue.

It could be that StopProcessing() is called after the processing_state_ check in
PickRequestTask.  While most of the code in request coordinator is single
threaded, the PickRequestTask does happen on a request queue.  That means it
might interleave unpredictably, or we might have a worker thread someday.

ProcessingState is really intended for us to know if we are doing background or
foreground rendering, not to be a guard if we are active, this state variable
should be the code guard for whether the renderer is active.

[romax, 2017/03/29 03:02:09]

Done.

     DCHECK(active_request_.get());
     offliner_->Cancel(base::Bind(
         &RequestCoordinator::HandleCancelUpdateStatusCallback,
@@ -555,7 +554,7 @@ void RequestCoordinator::UpdateStatusForCancel(
     RecordOfflinerResultUMA(active_request_->client_id(),
                             active_request_->creation_time(),
                             last_offlining_status_);
-    is_busy_ = false;
+    state_ = RequestCoordinatorState::IDLE;
     active_request_.reset();
   }
 }
@@ -624,7 +623,7 @@ bool RequestCoordinator::StartImmediateProcessing(
 bool RequestCoordinator::StartProcessingInternal(
     const ProcessingWindowState processing_state,
     const base::Callback<void(bool)>& callback) {
-  if (is_starting_ || is_busy_)
+  if (state_ != RequestCoordinatorState::IDLE)
     return false;
   processing_state_ = processing_state;
   scheduler_callback_ = callback;
@@ -647,7 +646,7 @@ RequestCoordinator::TryImmediateStart(
     const base::Callback<void(bool)>& callback) {
   DVLOG(2) << "Immediate " << __func__;
   // Make sure not already busy processing.
-  if (is_busy_)
+  if (state_ == RequestCoordinatorState::OFFLINING)
     return OfflinerImmediateStartStatus::BUSY;
 
   // Make sure we are not on svelte device to start immediately.
@@ -702,7 +701,7 @@ void RequestCoordinator::UpdateCurrentConditionsFromAndroid() {
 }
 
 void RequestCoordinator::TryNextRequest(bool is_start_of_processing) {
-  is_starting_ = true;
+  state_ = RequestCoordinatorState::STARTING;
 
   // If this is the first call, the device conditions are current, no need to
   // update them.
@@ -737,7 +736,7 @@ void RequestCoordinator::TryNextRequest(bool is_start_of_processing) {
   if (connection_type ==
           net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE ||
       (base::Time::Now() - operation_start_time_) > processing_time_budget) {
-    is_starting_ = false;
+    state_ = RequestCoordinatorState::IDLE;
 
     // If we were doing immediate processing, try to start it again
     // when we get connected.
@@ -773,12 +772,15 @@ void RequestCoordinator::TryNextRequest(bool is_start_of_processing) {
 void RequestCoordinator::RequestPicked(const SavePageRequest& request,
                                        bool cleanup_needed) {
   DVLOG(2) << request.url() << " " << __func__;
-  is_starting_ = false;
+  state_ = RequestCoordinatorState::OFFLINING;
 
   // Make sure we were not stopped while picking.
   if (processing_state_ != ProcessingWindowState::STOPPED) {
     // Send the request on to the offliner.
-    SendRequestToOffliner(request);
+    if (!SendRequestToOffliner(request))

[Pete Williamson, 2017/03/28 00:35:13]

Why check the return value, and then do the same thing either way?  Maybe the
else clause should be dropped.

[romax, 2017/03/28 01:09:56]

Seems like you mismatched the braces? :)
I've pulled a little bit more logic out of SendRequestToOffliner and now it
should look better. Also in order to keep the TODO there I kept the if check
inside the case which we should reset the state.

+      state_ = RequestCoordinatorState::IDLE;
+  } else {
+    state_ = RequestCoordinatorState::IDLE;
   }
 
   // Schedule a queue cleanup if needed.
@@ -790,7 +792,7 @@ void RequestCoordinator::RequestNotPicked(
     bool non_user_requested_tasks_remaining,
     bool cleanup_needed) {
   DVLOG(2) << __func__;
-  is_starting_ = false;
+  state_ = RequestCoordinatorState::IDLE;
 
   // Clear the outstanding "safety" task in the scheduler.
   scheduler_->Unschedule();
@@ -859,22 +861,21 @@ void RequestCoordinator::RequestCounts(bool is_start_of_processing,
   }
 }
 
-void RequestCoordinator::SendRequestToOffliner(const SavePageRequest& request) {
+bool RequestCoordinator::SendRequestToOffliner(const SavePageRequest& request) {
+  DCHECK(state_ == RequestCoordinatorState::OFFLINING);
+
   // Check that offlining didn't get cancelled while performing some async
   // steps.
   if (processing_state_ == ProcessingWindowState::STOPPED)
-    return;
+    return false;
 
   if (!offliner_) {
     // TODO(chili,petewil): We should have UMA here to track frequency of this,
     // if it happens at all.
     DVLOG(0) << "Offliner crashed. Cannot background offline page.";
-    return;
+    return false;
   }
 
-  DCHECK(!is_busy_);
-  is_busy_ = true;
-
   // Record start time if this is first attempt.
   if (request.started_attempt_count() == 0) {
     RecordStartTimeUMA(request);
@@ -886,6 +887,7 @@ void RequestCoordinator::SendRequestToOffliner(const SavePageRequest& request) {
       base::Bind(&RequestCoordinator::StartOffliner,
                  weak_ptr_factory_.GetWeakPtr(), request.request_id(),
                  request.client_id().name_space));
+  return true;
 }
 
 void RequestCoordinator::StartOffliner(
@@ -896,7 +898,7 @@ void RequestCoordinator::StartOffliner(
       update_result->item_statuses.size() != 1 ||
       update_result->item_statuses.at(0).first != request_id ||
       update_result->item_statuses.at(0).second != ItemActionStatus::SUCCESS) {
-    is_busy_ = false;
+    state_ = RequestCoordinatorState::IDLE;
     StopProcessing(Offliner::QUEUE_UPDATE_FAILED);
     DVLOG(1) << "Failed to mark attempt started: " << request_id;
     UpdateRequestResult request_result =
@@ -936,7 +938,7 @@ void RequestCoordinator::StartOffliner(
     watchdog_timer_.Start(FROM_HERE, timeout, this,
                           &RequestCoordinator::HandleWatchdogTimeout);
   } else {
-    is_busy_ = false;
+    state_ = RequestCoordinatorState::IDLE;
     DVLOG(0) << "Unable to start LoadAndSave";
     StopProcessing(Offliner::LOADING_NOT_ACCEPTED);
 
@@ -959,7 +961,7 @@ void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
   RecordOfflinerResultUMA(request.client_id(), request.creation_time(),
                           last_offlining_status_);
   watchdog_timer_.Stop();
-  is_busy_ = false;
+  state_ = RequestCoordinatorState::IDLE;
   active_request_.reset(nullptr);
 
   UpdateRequestForCompletedAttempt(request, status);