[OfflinePages] Fix Coordinator issue with lagging NetworkChangeNotifier

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"
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/rand_util.h"
 #include "base/sys_info.h"
 #include "base/time/time.h"
 #include "components/offline_pages/background/offliner_factory.h"
 #include "components/offline_pages/background/offliner_policy.h"
 #include "components/offline_pages/background/save_page_request.h"
 #include "components/offline_pages/client_policy_controller.h"
 #include "components/offline_pages/offline_page_feature.h"
 #include "components/offline_pages/offline_page_item.h"
 #include "components/offline_pages/offline_page_model.h"
 
 namespace offline_pages {
 
 namespace {
 const bool kUserRequest = true;
+const bool kStartOfProcessing = true;
 const int kMinDurationSeconds = 1;
 const int kMaxDurationSeconds = 7 * 24 * 60 * 60;  // 7 days
 const int kDurationBuckets = 50;
 const int kDisabledTaskRecheckSeconds = 5;
 
 // TODO(dougarnett): Move to util location and share with model impl.
 std::string AddHistogramSuffix(const ClientId& client_id,
                                const char* histogram_name) {
   if (client_id.name_space.empty()) {
     NOTREACHED();
@@ skipping 325 matching lines @@
 
   // Record the network quality when this request is made.
   if (network_quality_estimator_) {
     UMA_HISTOGRAM_ENUMERATION(
         "OfflinePages.Background.EffectiveConnectionType.RemoveRequests",
         network_quality_estimator_->GetEffectiveConnectionType(),
         net::EFFECTIVE_CONNECTION_TYPE_LAST);
   }
 
   if (canceled)
-    TryNextRequest();
+    TryNextRequest(!kStartOfProcessing);
 }
 
 void RequestCoordinator::PauseRequests(
     const std::vector<int64_t>& request_ids) {
   bool canceled = CancelActiveRequestIfItMatches(request_ids);
   queue_->ChangeRequestsState(
       request_ids, SavePageRequest::RequestState::PAUSED,
       base::Bind(&RequestCoordinator::UpdateMultipleRequestsCallback,
                  weak_ptr_factory_.GetWeakPtr()));
 
   // Record the network quality when this request is made.
   if (network_quality_estimator_) {
     UMA_HISTOGRAM_ENUMERATION(
         "OfflinePages.Background.EffectiveConnectionType.PauseRequests",
         network_quality_estimator_->GetEffectiveConnectionType(),
         net::EFFECTIVE_CONNECTION_TYPE_LAST);
   }
 
   if (canceled)
-    TryNextRequest();
+    TryNextRequest(!kStartOfProcessing);
 }
 
 void RequestCoordinator::ResumeRequests(
     const std::vector<int64_t>& request_ids) {
   queue_->ChangeRequestsState(
       request_ids, SavePageRequest::RequestState::AVAILABLE,
       base::Bind(&RequestCoordinator::UpdateMultipleRequestsCallback,
                  weak_ptr_factory_.GetWeakPtr()));
 
   // Record the network quality when this request is made.
@@ skipping 59 matching lines @@
   }
 
   if (available_user_request)
     StartImmediatelyIfConnected();
 }
 
 // When we successfully remove a request that completed successfully, move on to
 // the next request.
 void RequestCoordinator::CompletedRequestCallback(
     const MultipleItemStatuses& status) {
-  TryNextRequest();
+  TryNextRequest(!kStartOfProcessing);
 }
 
 void RequestCoordinator::HandleRemovedRequestsAndCallback(
     const RemoveRequestsCallback& callback,
     RequestNotifier::BackgroundSavePageResult status,
     std::unique_ptr<UpdateRequestsResult> result) {
   // TODO(dougarnett): Define status code for user/api cancel and use here
   // to determine whether to record cancel time UMA.
   for (const auto& request : result->updated_items)
     RecordCancelTimeUMA(request);
@@ skipping 21 matching lines @@
   StopPrerendering(stop_status);
 
   // Let the scheduler know we are done processing.
   scheduler_callback_.Run(true);
 }
 
 void RequestCoordinator::HandleWatchdogTimeout() {
   Offliner::RequestStatus watchdog_status =
       Offliner::REQUEST_COORDINATOR_TIMED_OUT;
   StopPrerendering(watchdog_status);
-  TryNextRequest();
+  TryNextRequest(!kStartOfProcessing);
 }
 
 // Returns true if the caller should expect a callback, false otherwise. For
 // instance, this would return false if a request is already in progress.
 bool RequestCoordinator::StartProcessing(
     const DeviceConditions& device_conditions,
     const base::Callback<void(bool)>& callback) {
   DVLOG(2) << "Scheduled " << __func__;
   return StartProcessingInternal(ProcessingWindowState::SCHEDULED_WINDOW,
                                  device_conditions, callback);
 }
 
 bool RequestCoordinator::StartProcessingInternal(
     const ProcessingWindowState processing_state,
     const DeviceConditions& device_conditions,
     const base::Callback<void(bool)>& callback) {
   current_conditions_.reset(new DeviceConditions(device_conditions));
   if (is_starting_ || is_busy_)
     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();
+  TryNextRequest(kStartOfProcessing);
 
   return true;
 }
 
 void RequestCoordinator::StartImmediatelyIfConnected() {
   OfflinerImmediateStartStatus immediate_start_status = TryImmediateStart();
   UMA_HISTOGRAM_ENUMERATION(
       "OfflinePages.Background.ImmediateStartStatus", immediate_start_status,
       RequestCoordinator::OfflinerImmediateStartStatus::STATUS_COUNT);
 }
@@ skipping 23 matching lines @@
   // TODO(dougarnett): Obtain actual battery conditions (from Android/Java).
 
   DeviceConditions device_conditions(false, 0, GetConnectionType());
   if (StartProcessingInternal(ProcessingWindowState::IMMEDIATE_WINDOW,
                               device_conditions, immediate_schedule_callback_))
     return OfflinerImmediateStartStatus::STARTED;
   else
     return OfflinerImmediateStartStatus::NOT_ACCEPTED;
 }
 
-void RequestCoordinator::TryNextRequest() {
+void RequestCoordinator::TryNextRequest(bool is_start_of_processing) {
   is_starting_ = true;
   base::TimeDelta processing_time_budget;
   if (processing_state_ == ProcessingWindowState::SCHEDULED_WINDOW) {
     processing_time_budget = base::TimeDelta::FromSeconds(
         policy_->GetProcessingTimeBudgetWhenBackgroundScheduledInSeconds());
   } else {
     DCHECK(processing_state_ == ProcessingWindowState::IMMEDIATE_WINDOW);
     processing_time_budget = base::TimeDelta::FromSeconds(
         policy_->GetProcessingTimeBudgetForImmediateLoadInSeconds());
   }
 
+  // Determine connection type. If just starting processing, the best source is
+  // from the current device conditions (they are fresh and motivated starting
+  // processing whereas NetworkChangeNotifier may lag reality).
+  net::NetworkChangeNotifier::ConnectionType connection_type;
+  if (is_start_of_processing)
+    connection_type = current_conditions_->GetNetConnectionType();
+  else
+    connection_type = GetConnectionType();
+
   // 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 (GetConnectionType() ==
+  if (connection_type ==
           net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE ||
       (base::Time::Now() - operation_start_time_) > processing_time_budget) {
     is_starting_ = false;
 
     // 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__;
 
     return;
@@ skipping 138 matching lines @@
                                      request.request_id());
   last_offlining_status_ = status;
   RecordOfflinerResultUMA(request.client_id(), request.creation_time(),
                           last_offlining_status_);
   watchdog_timer_.Stop();
   is_busy_ = false;
   active_request_.reset(nullptr);
 
   UpdateRequestForCompletedAttempt(request, status);
   if (ShouldTryNextRequest(status))
-    TryNextRequest();
+    TryNextRequest(!kStartOfProcessing);
   else
     scheduler_callback_.Run(true);
 }
 
 void RequestCoordinator::UpdateRequestForCompletedAttempt(
     const SavePageRequest& request,
     Offliner::RequestStatus status) {
   if (status == Offliner::RequestStatus::FOREGROUND_CANCELED ||
       status == Offliner::RequestStatus::PRERENDERING_CANCELED) {
     // Update the request for the canceled attempt.
@@ skipping 121 matching lines @@
 
 ClientPolicyController* RequestCoordinator::GetPolicyController() {
   return policy_controller_.get();
 }
 
 void RequestCoordinator::Shutdown() {
   network_quality_estimator_ = nullptr;
 }
 
 }  // namespace offline_pages