Fix a bug with throttling and timer queue suspension

components/scheduler/renderer/throttling_helper.cc

 // Copyright 2015 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/scheduler/renderer/throttling_helper.h"
 
 #include "base/logging.h"
 #include "components/scheduler/base/real_time_domain.h"
 #include "components/scheduler/child/scheduler_tqm_delegate.h"
 #include "components/scheduler/renderer/renderer_scheduler_impl.h"
@@ skipping 25 matching lines @@
   // before unregistering the time domain.
   for (const TaskQueueMap::value_type& map_entry : throttled_queues_) {
     TaskQueue* task_queue = map_entry.first;
     task_queue->SetTimeDomain(renderer_scheduler_->real_time_domain());
     task_queue->SetPumpPolicy(TaskQueue::PumpPolicy::AUTO);
   }
 
   renderer_scheduler_->UnregisterTimeDomain(time_domain_.get());
 }
 
+void ThrottlingHelper::SetQueueEnabled(TaskQueue* task_queue, bool enabled) {
+  TaskQueueMap::iterator find_it = throttled_queues_.find(task_queue);
+
+  if (find_it == throttled_queues_.end()) {
+    task_queue->SetQueueEnabled(enabled);
+    return;
+  }
+
+  find_it->second.enabled = enabled;
+
+  if (!enabled)

[Sami, 2016/05/31 18:11:04]

Maybe add a comment why we don't want to enable the queue directly here? (it
could defeat throttling).

[alex clarke (OOO till 29th), 2016/06/02 13:51:05]

Done.

+    task_queue->SetQueueEnabled(false);
+}
+
 void ThrottlingHelper::IncreaseThrottleRefCount(TaskQueue* task_queue) {
   DCHECK_NE(task_queue, task_runner_.get());
 
   std::pair<TaskQueueMap::iterator, bool> insert_result =
-      throttled_queues_.insert(std::make_pair(task_queue, 1));
+      throttled_queues_.insert(std::make_pair(
+          task_queue, Metadata(1, task_queue->IsQueueEnabled())));
 
   if (insert_result.second) {
     // The insert was succesful so we need to throttle the queue.
     task_queue->SetTimeDomain(time_domain_.get());
     task_queue->SetPumpPolicy(TaskQueue::PumpPolicy::MANUAL);
     task_queue->SetQueueEnabled(false);
 
     if (!task_queue->IsEmpty()) {
       if (task_queue->HasPendingImmediateWork()) {
         OnTimeDomainHasImmediateWork();
       } else {
         OnTimeDomainHasDelayedWork();
       }
     }
   } else {
     // An entry already existed in the map so we need to increment the refcount.
-    insert_result.first->second++;
+    insert_result.first->second.throttling_ref_count++;
   }
 }
 
 void ThrottlingHelper::DecreaseThrottleRefCount(TaskQueue* task_queue) {
   TaskQueueMap::iterator iter = throttled_queues_.find(task_queue);
 
-  if (iter != throttled_queues_.end() && --iter->second == 0) {
+  if (iter != throttled_queues_.end() &&
+      --iter->second.throttling_ref_count == 0) {
+    bool enabled = iter->second.enabled;
     // The refcount has become zero, we need to unthrottle the queue.
     throttled_queues_.erase(iter);
 
     task_queue->SetTimeDomain(renderer_scheduler_->real_time_domain());
     task_queue->SetPumpPolicy(TaskQueue::PumpPolicy::AUTO);
-    task_queue->SetQueueEnabled(true);
+    task_queue->SetQueueEnabled(enabled);
   }
 }
 
 void ThrottlingHelper::UnregisterTaskQueue(TaskQueue* task_queue) {
   throttled_queues_.erase(task_queue);
 }
 
 void ThrottlingHelper::OnTimeDomainHasImmediateWork() {
   // Forward to the main thread if called from another thread.
   if (!task_runner_->RunsTasksOnCurrentThread()) {
@@ skipping 22 matching lines @@
   TRACE_EVENT0(tracing_category_, "ThrottlingHelper::PumpThrottledTasks");
   pending_pump_throttled_tasks_runtime_ = base::TimeTicks();
 
   base::TimeTicks now = tick_clock_->NowTicks();
   time_domain_->AdvanceTo(now);
   for (const TaskQueueMap::value_type& map_entry : throttled_queues_) {
     TaskQueue* task_queue = map_entry.first;
     if (task_queue->IsEmpty())
       continue;
 
-    task_queue->SetQueueEnabled(true);
+    task_queue->SetQueueEnabled(map_entry.second.enabled);

[Sami, 2016/05/31 18:11:04]

Mind adding a DCHECK here that makes sure the enabled status is what we expect?

[alex clarke (OOO till 29th), 2016/06/02 13:51:05]

This in principle is a good idea, but due to the way
ThrottlingHelper::SetQueueEnabled operates it's a bit complex.  I think we can
get away without doing it.

     task_queue->PumpQueue(false);
   }
   // Make sure NextScheduledRunTime gives us an up-to date result.
   time_domain_->ClearExpiredWakeups();
 
   base::TimeTicks next_scheduled_delayed_task;
   // Maybe schedule a call to ThrottlingHelper::PumpThrottledTasks if there is
   // a pending delayed task. NOTE posting a non-delayed task in the future will
   // result in ThrottlingHelper::OnTimeDomainHasImmediateWork being called.
   if (time_domain_->NextScheduledRunTime(&next_scheduled_delayed_task)) {
@@ skipping 29 matching lines @@
 
   base::TimeDelta delay = pending_pump_throttled_tasks_runtime_ - now;
   TRACE_EVENT1(tracing_category_,
                "ThrottlingHelper::MaybeSchedulePumpThrottledTasksLocked",
                "delay_till_next_pump_ms", delay.InMilliseconds());
   task_runner_->PostDelayedTask(
       from_here, suspend_timers_when_backgrounded_closure_.callback(), delay);
 }
 
 }  // namespace scheduler