[Offline Pages] Close race condition of multiple StartProcessing callers.

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 <utility>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 61 matching lines @@
 // is nothing for it to do.
 void EmptySchedulerCallback(bool started) {}
 
 }  // namespace
 
 RequestCoordinator::RequestCoordinator(std::unique_ptr<OfflinerPolicy> policy,
                                        std::unique_ptr<OfflinerFactory> factory,
                                        std::unique_ptr<RequestQueue> queue,
                                        std::unique_ptr<Scheduler> scheduler)
     : is_busy_(false),
+      is_starting_(false),
       is_stopped_(false),
       use_test_connection_type_(false),
       test_connection_type_(),
       offliner_(nullptr),
       policy_(std::move(policy)),
       factory_(std::move(factory)),
       queue_(std::move(queue)),
       scheduler_(std::move(scheduler)),
       active_request_(nullptr),
       last_offlining_status_(Offliner::RequestStatus::UNKNOWN),
@@ skipping 246 matching lines @@
   // Let the scheduler know we are done processing.
   scheduler_callback_.Run(true);
 }
 
 // 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) {
   current_conditions_.reset(new DeviceConditions(device_conditions));
-  if (is_busy_) return false;
+  if (is_starting_ || is_busy_)

[Pete Williamson, 2016/09/06 22:56:44]

Please add a bug to refactor to a single state variable.

[dougarnett, 2016/09/06 23:39:10]

Done. https://bugs.chromium.org/p/chromium/issues/detail?id=644519

+    return false;

[Pete Williamson, 2016/09/06 22:56:44]

(as we discussed in person) BackgroundOfflinerTask.java is not checking the
return value, we should do that.

[dougarnett, 2016/09/06 23:39:10]

Done.

+  is_starting_ = true;
 
   // Mark the time at which we started processing so we can check our time
   // budget.
   operation_start_time_ = base::Time::Now();
 
   is_stopped_ = false;
   scheduler_callback_ = callback;
 
   TryNextRequest();
 
@@ skipping 19 matching lines @@
 void RequestCoordinator::TryNextRequest() {
   // If there is no time left in the budget, return to the scheduler.
   // We do not remove the pending 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 (base::Time::Now() - operation_start_time_ >
       base::TimeDelta::FromSeconds(
           policy_->GetBackgroundProcessingTimeBudgetSeconds())) {
     // Let the scheduler know we are done processing.
     scheduler_callback_.Run(true);
 

[Pete Williamson, 2016/09/06 22:56:44]

is_starting_ = false

[dougarnett, 2016/09/06 23:39:10]

Done. Thanks!

     return;
   }
 
   // Choose a request to process that meets the available conditions.
   // This is an async call, and returns right away.
   picker_->ChooseNextRequest(base::Bind(&RequestCoordinator::RequestPicked,
                                         weak_ptr_factory_.GetWeakPtr()),
                              base::Bind(&RequestCoordinator::RequestNotPicked,
                                         weak_ptr_factory_.GetWeakPtr()),
                              current_conditions_.get());
 }
 
 // Called by the request picker when a request has been picked.
 void RequestCoordinator::RequestPicked(const SavePageRequest& request) {
-  // Send the request on to the offliner.
-  SendRequestToOffliner(request);
+  is_starting_ = false;
+
+  // Make sure we were not stopped while picking.
+  if (!is_stopped_) {
+    // Send the request on to the offliner.
+    SendRequestToOffliner(request);
+  }
 }
 
 void RequestCoordinator::RequestNotPicked(
     bool non_user_requested_tasks_remaining) {
+  is_starting_ = false;
+
   // Clear the outstanding "safety" task in the scheduler.
   scheduler_->Unschedule();
 
   if (non_user_requested_tasks_remaining)
     scheduler_->Schedule(GetTriggerConditions(!kUserRequest));
   // Let the scheduler know we are done processing.
   scheduler_callback_.Run(true);
 }
 
 void RequestCoordinator::SendRequestToOffliner(const SavePageRequest& request) {
@@ skipping 141 matching lines @@
   FOR_EACH_OBSERVER(Observer, observers_, OnChanged(request));
 }
 
 void RequestCoordinator::GetOffliner() {
   if (!offliner_) {
     offliner_ = factory_->GetOffliner(policy_.get());
   }
 }
 
 }  // namespace offline_pages