Prevent multiple pending UpdatePolicy tasks.

content/renderer/scheduler/renderer_scheduler_impl.cc

Index: content/renderer/scheduler/renderer_scheduler_impl.cc
diff --git a/content/renderer/scheduler/renderer_scheduler_impl.cc b/content/renderer/scheduler/renderer_scheduler_impl.cc
index 6f7821690393a3b8c61b9956f28deb523e766a6b..da26e8b64f555cfa4d2b9b6e90af76bcee7c4cbc 100644
--- a/content/renderer/scheduler/renderer_scheduler_impl.cc
+++ b/content/renderer/scheduler/renderer_scheduler_impl.cc
@@ -35,10 +35,14 @@ RendererSchedulerImpl::RendererSchedulerImpl(
       last_input_type_(blink::WebInputEvent::Undefined),
       input_stream_state_(InputStreamState::INACTIVE),
       policy_may_need_update_(&incoming_signals_lock_),
+      pending_delayed_policy_update_(false),
       weak_factory_(this) {
   weak_renderer_scheduler_ptr_ = weak_factory_.GetWeakPtr();
   update_policy_closure_ = base::Bind(&RendererSchedulerImpl::UpdatePolicy,
                                       weak_renderer_scheduler_ptr_);
+  delayed_update_policy_closure_ =
+      base::Bind(&RendererSchedulerImpl::DelayedPolicyUpdateTask,
+                 weak_renderer_scheduler_ptr_);
   end_idle_period_closure_.Reset(base::Bind(
       &RendererSchedulerImpl::EndIdlePeriod, weak_renderer_scheduler_ptr_));
   idle_task_runner_ = make_scoped_refptr(new SingleThreadIdleTaskRunner(
@@ -185,8 +189,7 @@ void RendererSchedulerImpl::UpdateForInputEvent(
   if (input_stream_state_ != new_input_stream_state) {
     // Update scheduler policy if we should start a new policy mode.
     input_stream_state_ = new_input_stream_state;
-    policy_may_need_update_.SetWhileLocked(true);
-    PostUpdatePolicyOnControlRunner(base::TimeDelta());
+    ScheduleUrgentPolicyUpdate(FROM_HERE);
   }
   last_input_receipt_time_on_compositor_ = Now();
   // Clear the last known input processing time so that we know an input event
@@ -208,7 +211,7 @@ void RendererSchedulerImpl::DidProcessInputEvent(
       begin_frame_time < last_input_receipt_time_on_compositor_)
     return;
   last_input_process_time_on_main_ = Now();
-  policy_may_need_update_.SetWhileLocked(true);
+  UpdatePolicyLocked();

[Sami, 2015/03/11 16:58:25]

Could you add a DCHECK that this function is only called on the main thread?

[alex clarke (OOO till 29th), 2015/03/12 13:41:31]

Done.

 }
 
 bool RendererSchedulerImpl::IsHighPriorityWorkAnticipated() {
@@ -268,25 +271,50 @@ void RendererSchedulerImpl::MaybeUpdatePolicy() {
   }
 }
 
-void RendererSchedulerImpl::PostUpdatePolicyOnControlRunner(
+void RendererSchedulerImpl::ScheduleUrgentPolicyUpdate(

[rmcilroy, 2015/03/11 17:15:07]

nit on naming - how about PostUpdatePolicyToMainThread or similar to identify
that this should only really be used when you are not guaranteed to be on the
main thread?

[alex clarke (OOO till 29th), 2015/03/12 13:41:31]

I like the OnMainThread, but it doesn't always post so I prefer
ScheduleUrgentPolicyUpdateOnMainThread.

+    const tracked_objects::Location& from_here) {
+  incoming_signals_lock_.AssertAcquired();

[Sami, 2015/03/11 16:58:25]

Maybe add a DCHECK() that this is never called on the main thread, because there
we should just do the update immediately.

[rmcilroy, 2015/03/11 17:15:07]

It feels like this would make it easy to introduce bugs where you accidentally
prevent posting a more recent policy update if there was already a pending
update policy task with a long deadline - I don't think there is any code in
UpdatePolicy which would prevent us from doing this.  As with my comment above,
I think it would be safer to use a CancellableCallback to ensure that we always
do the update at the most up-to-date time.

[alex clarke (OOO till 29th), 2015/03/12 13:41:31]

In principle this is a good idea, but it makes testing tricky. We've discussed a
bit of a testing refactor, lets fix this at the same time.

[alex clarke (OOO till 29th), 2015/03/12 13:41:31]

The de-duping for ScheduleDelayedPolicyUpdate is separate from
ScheduleUrgentPolicyUpdate.   So we don't have a problem if there's a
UpdatePolicy posted for 100ms time but the compositor thread wants to post an
UpdatePolicy with no delay.

Hopefully the DeadlineTaskRunner addresses your other concerns.

+  if (!policy_may_need_update_.IsSet()) {
+    policy_may_need_update_.SetWhileLocked(true);
+    control_task_runner_->PostTask(from_here, update_policy_closure_);
+  }
+}
+
+void RendererSchedulerImpl::ScheduleDelayedPolicyUpdate(
+    const tracked_objects::Location& from_here,
     base::TimeDelta delay) {
-  control_task_runner_->PostDelayedTask(
-      FROM_HERE, update_policy_closure_, delay);
+  incoming_signals_lock_.AssertAcquired();

[Sami, 2015/03/11 16:58:25]

I think this is probably flexible enough for now, but to provide some insurance
in the future, could we change pending_delayed_policy_update_ to a timestamp and
DCHECK here that we're not trying to schedule a policy update sooner than that?

[alex clarke (OOO till 29th), 2015/03/12 13:41:31]

I ended up adding a DeadlineTaskRunner that should be future proof.

+  if (!pending_delayed_policy_update_) {
+    pending_delayed_policy_update_ = true;
+    control_task_runner_->PostDelayedTask(
+        from_here, delayed_update_policy_closure_, delay);
+  }
+}
+
+void RendererSchedulerImpl::DelayedPolicyUpdateTask() {
+  base::AutoLock lock(incoming_signals_lock_);
+  pending_delayed_policy_update_ = false;
+  UpdatePolicyLocked();
 }
 
 void RendererSchedulerImpl::UpdatePolicy() {
+  base::AutoLock lock(incoming_signals_lock_);
+  UpdatePolicyLocked();
+}
+
+void RendererSchedulerImpl::UpdatePolicyLocked() {
   DCHECK(main_thread_checker_.CalledOnValidThread());
+  incoming_signals_lock_.AssertAcquired();
   if (!task_queue_manager_)
     return;
 
-  base::AutoLock lock(incoming_signals_lock_);
   base::TimeTicks now;
   policy_may_need_update_.SetWhileLocked(false);
 
   base::TimeDelta new_policy_duration;
   Policy new_policy = ComputeNewPolicy(&new_policy_duration);
   if (new_policy_duration > base::TimeDelta())
-    PostUpdatePolicyOnControlRunner(new_policy_duration);
+    ScheduleDelayedPolicyUpdate(FROM_HERE, new_policy_duration);
 
   if (new_policy == current_policy_)
     return;