[Offline Pages] Improve RequestCoordinator state tracking.

components/offline_pages/core/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/core/background/request_coordinator.h"
 
 #include <limits>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 179 matching lines @@
 }
 
 RequestCoordinator::RequestCoordinator(
     std::unique_ptr<OfflinerPolicy> policy,
     std::unique_ptr<Offliner> offliner,
     std::unique_ptr<RequestQueue> queue,
     std::unique_ptr<Scheduler> scheduler,
     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)),
       policy_(std::move(policy)),
       queue_(std::move(queue)),
       scheduler_(std::move(scheduler)),
       policy_controller_(new ClientPolicyController()),
       network_quality_estimator_(network_quality_estimator),
       active_request_(nullptr),
       last_offlining_status_(Offliner::RequestStatus::UNKNOWN),
@@ skipping 68 matching lines @@
 void RequestCoordinator::GetQueuedRequestsCallback(
     const GetRequestsCallback& callback,
     GetRequestsResult result,
     std::vector<std::unique_ptr<SavePageRequest>> requests) {
   callback.Run(std::move(requests));
 }
 
 void RequestCoordinator::StopPrerendering(
     const Offliner::CancelCallback& final_callback,
     Offliner::RequestStatus stop_status) {
-  if (offliner_ && is_busy_) {
+  if (offliner_ && state_ == RequestCoordinatorState::OFFLINING) {
     DCHECK(active_request_.get());
     offliner_->Cancel(base::Bind(
         &RequestCoordinator::HandleCancelUpdateStatusCallback,
         weak_ptr_factory_.GetWeakPtr(), final_callback, stop_status));
     return;
   }
 
   UpdateStatusForCancel(stop_status);
   int64_t request_id =
       active_request_.get() ? active_request_->request_id() : 0LL;
@@ skipping 247 matching lines @@
   // 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;
+    state_ = RequestCoordinatorState::IDLE;
     active_request_.reset();
   }
 }
 
 void RequestCoordinator::ResetActiveRequestCallback(int64_t offline_id) {
   active_request_.reset();
 }
 
 void RequestCoordinator::StartSchedulerCallback(int64_t offline_id) {
   scheduler_callback_.Run(true);
@@ skipping 48 matching lines @@
       "OfflinePages.Background.ImmediateStartStatus", immediate_start_status,
       RequestCoordinator::OfflinerImmediateStartStatus::STATUS_COUNT);
   return immediate_start_status == OfflinerImmediateStartStatus::STARTED;
 }
 
 // The current_conditions_ must be set sometime before calling
 // StartProcessingInternal on all calling code paths.
 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;
 
   // Mark the time at which we started processing so we can check our time
   // budget.
   operation_start_time_ = base::Time::Now();
 
   TryNextRequest(kStartOfProcessing);
 
   return true;
 }
 
 void RequestCoordinator::StartImmediatelyIfConnected() {
   StartImmediateProcessing(internal_start_processing_callback_);
 }
 
 RequestCoordinator::OfflinerImmediateStartStatus
 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.
   if (is_low_end_device_ &&
       !offline_pages::IsOfflinePagesSvelteConcurrentLoadingEnabled()) {
     DVLOG(2) << "low end device, returning";
     // Let the scheduler know we are done processing and failed due to svelte.
     callback.Run(false);
     return OfflinerImmediateStartStatus::NOT_STARTED_ON_SVELTE;
   }
@@ skipping 34 matching lines @@
   // If we have already set the connection type for testing, don't get it from
   // android, but use what the test already set up.
   if (use_test_device_conditions_)
     return;
 
   current_conditions_ = base::MakeUnique<DeviceConditions>(
       scheduler_->GetCurrentDeviceConditions());
 }
 
 void RequestCoordinator::TryNextRequest(bool is_start_of_processing) {
-  is_starting_ = true;
+  state_ = RequestCoordinatorState::PICKING;
 
   // If this is the first call, the device conditions are current, no need to
   // update them.
   // TODO(petewil): Now that we can get conditions any time, consider getting
   // them now instead of passing them in earlier when we start scheduled
   // processing.
   if (!is_start_of_processing) {
     // Get current device conditions from the Java side across the bridge.
     // NetworkChangeNotifier will not have the right conditions if chromium is
     // in the background in android, so prefer to always get the conditions via
@@ skipping 14 matching lines @@
   net::NetworkChangeNotifier::ConnectionType connection_type =
       current_conditions_->GetNetConnectionType();
 
   // If there is no network or no time left in the budget, return to the
   // scheduler. We do not remove the pending scheduler task that was set
   // up earlier in case we run out of time, so the background scheduler
   // will return to us at the next opportunity to run background tasks.
   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.
     if (processing_state_ == ProcessingWindowState::IMMEDIATE_WINDOW)
       RequestConnectedEventForStarting();
 
     // Let the scheduler know we are done processing.
     // TODO: Make sure the scheduler callback is valid before running it.
     scheduler_callback_.Run(true);
     DVLOG(2) << " out of time, giving up. " << __func__;
@@ skipping 15 matching lines @@
   // TODO(petewil): Verify current_conditions has a good value on all calling
   // paths.  It is really more of a "last known conditions" than "current
   // conditions".  Consider having a call to Java to check the current
   // conditions.
 }
 
 // Called by the request picker when a request has been picked.
 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.
+  // If we were stopped while picking or offliner got killed then reset the
+  // state of request coordinator, otherwise send the request to offliner.
+  if (processing_state_ == ProcessingWindowState::STOPPED || !offliner_) {
+    if (!offliner_) {

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

Let's remove this if !offliner check from both if statements, and the TODO.  We
were originally concerned that the offliner pointer could be null if the
offliner crashed, but that doesn't happen (it just points at garbage), which
makes this check useless.  That will make this if statement easier to read.

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

Done.

+      // 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.";
+    }
+    state_ = RequestCoordinatorState::IDLE;
+  } else {
     SendRequestToOffliner(request);

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

Since we aren't checking the return value anymore, should we put the code back
to what it used to be?

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

Done.

   }
 
   // Schedule a queue cleanup if needed.
   if (cleanup_needed)
     queue_->CleanupRequestQueue();
 }
 
 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();
 
   // If disabled tasks remain, post a new safety task for 5 sec from now.
   if (disabled_requests_.size() > 0) {
     scheduler_->BackupSchedule(GetTriggerConditions(kUserRequest),
                                kDisabledTaskRecheckSeconds);
   } else if (non_user_requested_tasks_remaining) {
     // If we don't have any of those, check for non-user-requested tasks.
@@ skipping 48 matching lines @@
       UMA_HISTOGRAM_COUNTS_1000(
           "OfflinePages.Background.ImmediateStart.AvailableRequestCount",
           available_requests);
       UMA_HISTOGRAM_COUNTS_1000(
           "OfflinePages.Background.ImmediateStart.UnavailableRequestCount",
           total_requests - available_requests);
     }
   }
 }
 
-void RequestCoordinator::SendRequestToOffliner(const SavePageRequest& request) {
-  // Check that offlining didn't get cancelled while performing some async
-  // steps.
-  if (processing_state_ == ProcessingWindowState::STOPPED)
-    return;
-
-  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;
-  }
-
-  DCHECK(!is_busy_);
-  is_busy_ = true;
-
+bool RequestCoordinator::SendRequestToOffliner(const SavePageRequest& request) {
+  DCHECK(state_ == RequestCoordinatorState::OFFLINING);
   // Record start time if this is first attempt.
   if (request.started_attempt_count() == 0) {
     RecordStartTimeUMA(request);
   }
 
   // Mark attempt started in the database and start offliner when completed.
   queue_->MarkAttemptStarted(
       request.request_id(),
       base::Bind(&RequestCoordinator::StartOffliner,
                  weak_ptr_factory_.GetWeakPtr(), request.request_id(),
                  request.client_id().name_space));
+  return true;
 }
 
 void RequestCoordinator::StartOffliner(
     int64_t request_id,
     const std::string& client_namespace,
     std::unique_ptr<UpdateRequestsResult> update_result) {
   if (update_result->store_state != StoreState::LOADED ||
       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 =
         update_result->store_state != StoreState::LOADED
             ? UpdateRequestResult::STORE_FAILURE
             : UpdateRequestResult::REQUEST_DOES_NOT_EXIST;
     event_logger_.RecordUpdateRequestFailed(client_namespace, request_result);
     return;
   }
 
@@ skipping 19 matching lines @@
           policy_->GetSinglePageTimeLimitForImmediateLoadInSeconds());
     }
 
     // Inform observer of active request.
     NotifyChanged(*active_request_.get());
 
     // Start a watchdog timer to catch pre-renders running too long
     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);
 
     // We need to undo the MarkAttemptStarted that brought us to this
     // method since we didn't success in starting after all.
     MarkAttemptAborted(request_id, client_namespace);
   }
 }
 
 void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
                                               Offliner::RequestStatus status) {
   DVLOG(2) << "offliner finished, saved: "
            << (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;
+  state_ = RequestCoordinatorState::IDLE;
   active_request_.reset(nullptr);
 
   UpdateRequestForCompletedAttempt(request, status);
   if (ShouldTryNextRequest(status))
     TryNextRequest(!kStartOfProcessing);
   else
     scheduler_callback_.Run(true);
 }
 
 void RequestCoordinator::OfflinerProgressCallback(
@@ skipping 145 matching lines @@
 
 ClientPolicyController* RequestCoordinator::GetPolicyController() {
   return policy_controller_.get();
 }
 
 void RequestCoordinator::Shutdown() {
   network_quality_estimator_ = nullptr;
 }
 
 }  // namespace offline_pages