Adds a prerenderer watchdog timer.

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"
 #include "base/logging.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/request_picker.h"
 #include "components/offline_pages/background/save_page_request.h"
 #include "components/offline_pages/offline_page_item.h"
 
 namespace offline_pages {
 
 namespace {
 // TODO(dougarnett/petewil): Move to Policy object. Also consider lower minimum
 // battery percentage once there is some processing time limits in place.
 const Scheduler::TriggerConditions kUserRequestTriggerConditions(
     false /* require_power_connected */,
     50 /* minimum_battery_percentage */,
     false /* require_unmetered_network */);
+
+// Timeout is 3 minutes based on the size of Marshmallow doze mode
+// maintenance window.  TODO(petewil): Find the optimal timeout based

[dougarnett, 2016/06/30 19:16:48]

maybe line break before TODO

[dewittj, 2016/06/30 20:38:33]

Please make a bug specifically to find this number out, and reference it here.

[Pete Williamson, 2016/07/01 17:16:54]

Done.

[Pete Williamson, 2016/07/01 17:16:55]

Done.

+// on data for 2G connections and common EM websites.

[dougarnett, 2016/06/30 19:16:48]

another TODO might be to move this value into Policy object

[Pete Williamson, 2016/07/01 17:16:55]

Done.

+long OFFLINER_TIMEOUT_SECONDS = 180;

[dewittj, 2016/06/30 20:38:33]

If this is exactly the marshmallow timeout, then the timer might not actually
fire when you want it to.  Perhaps use something slightly shorter like 180 - 30?

[Pete Williamson, 2016/07/01 17:16:54]

Done.

+
 }  // 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_canceled_(false),
+      offliner_timeout_(OFFLINER_TIMEOUT_SECONDS),
       offliner_(nullptr),
       policy_(std::move(policy)),
       factory_(std::move(factory)),
       queue_(std::move(queue)),
       scheduler_(std::move(scheduler)),
       last_offlining_status_(Offliner::RequestStatus::UNKNOWN),
       weak_ptr_factory_(this) {
   DCHECK(policy_ != nullptr);
   picker_.reset(new RequestPicker(queue_.get()));
 }
@@ skipping 25 matching lines @@
 
   // Inform the scheduler that we have an outstanding task.
   // TODO(petewil): Determine trigger conditions from policy.
   scheduler_->Schedule(GetTriggerConditionsForUserRequest());
 }
 
 // Called in response to updating a request in the request queue.
 void RequestCoordinator::UpdateRequestCallback(
     RequestQueue::UpdateRequestResult result) {}
 
 void RequestCoordinator::StopProcessing() {

[dewittj, 2016/06/30 20:38:33]

as far as I can tell, this is only called in tests.  Is that true? If not, how
does it interact with RequestQueueEpty()?  Should that method call
StopProcessing?

[Pete Williamson, 2016/07/01 17:16:54]

The primary purpose of this function is to cancel a render in progress.

This is called by the watchdog timer (line 161 in this file in this version of
the patch).  It is also a top level API to the request coordinator so that we
can stop processing if we realize later that the user has picked up their phone
and started interacting with it, and would like their CPU back.  (DougArnett has
a design doc for that he just sent out this week).

RequestQueueEmpty and this method both call scheduler_callback_.Run() to return
control to the scheduler, so we don't really need to have one call the other. 
If you think it makes the code more readable, I could make a new function to
return control to the scheduler that both call.

[dewittj, 2016/07/01 17:27:48]

Ah right, turns out code search ignores new additions in codereview! :)
The reason I ask is that RequestQueueEmpty and this function do slightly
different things but the end result should be the same.  For example,
scheduler_->Unschedule() is not called here.  Should it be?

[Pete Williamson, 2016/07/01 17:56:44]

I presume that StopProcessing is being called because the user wants to use the
device.  In that case, we still want to do pre-rendering, we just want to do it
another time, so we don't call unschedule() to remove the pending schedule
appointment

On the other hand, if the request queue is empty, there is just no work to be
done, so we remove the safety schedule appointment so we don't get scheduled
again.

This is explained in the design docs, but may not be well explained in the code,
let me know there is anything specific you think I could to do explain it better
in the comments.

   is_canceled_ = true;
   if (offliner_ && is_busy_)
     offliner_->Cancel();
+
+  // Return control to the scheduler when there is no more to do.

[dougarnett, 2016/06/30 19:16:48]

Think this comment needs some rewording. Perhaps
// Let scheduler know we are now done with processing.

[Pete Williamson, 2016/07/01 17:16:55]

Done (changed here and in RequestQueueEmpty)

+  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) {
   if (is_busy_) return false;
 
   is_canceled_ = false;
@@ skipping 42 matching lines @@
     return;
   }
 
   DCHECK(!is_busy_);
   is_busy_ = true;
 
   // Start the load and save process in the offliner (Async).
   offliner_->LoadAndSave(request,
                          base::Bind(&RequestCoordinator::OfflinerDoneCallback,
                                     weak_ptr_factory_.GetWeakPtr()));
+
+  // Start a watchdog timer to catch pre-renders running too long
+  watchdog_timer_.Start(
+      FROM_HERE, base::TimeDelta::FromSeconds(offliner_timeout_), this,
+      &RequestCoordinator::StopProcessing);
 }
 
 void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
                                               Offliner::RequestStatus status) {
   DVLOG(2) << "offliner finished, saved: "
            << (status == Offliner::RequestStatus::SAVED) << ", status: "
            << (int) status << ", " << __FUNCTION__;
   event_logger_.RecordSavePageRequestUpdated(
       request.client_id().name_space,
       "Saved",
       request.request_id());
   last_offlining_status_ = status;
+  watchdog_timer_.Stop();
 
   is_busy_ = false;
 
   // If the request succeeded, remove it from the Queue and maybe schedule
   // another one.
   if (status == Offliner::RequestStatus::SAVED) {
     queue_->RemoveRequest(request.request_id(),
                           base::Bind(&RequestCoordinator::UpdateRequestCallback,
                                      weak_ptr_factory_.GetWeakPtr()));
 
     // TODO(petewil): Check time budget. Return to the scheduler if we are out.
 
     // Start another request if we have time.
     TryNextRequest();
   }
 }
 
 const Scheduler::TriggerConditions&
 RequestCoordinator::GetTriggerConditionsForUserRequest() {
   return kUserRequestTriggerConditions;
 }
 
 void RequestCoordinator::GetOffliner() {
   if (!offliner_) {
     offliner_ = factory_->GetOffliner(policy_.get());
   }
 }
 
 }  // namespace offline_pages