[scheduler] Fix bug in task queue throttler

third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc

Index: third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
diff --git a/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc b/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
index 803cf4e82cba882c257e953857ff71bed594826c..58d5697300d1a88b47be07d36e7b964fd0da62f9 100644
--- a/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
+++ b/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc
@@ -332,13 +332,10 @@ void TaskQueueThrottler::SetQueueEnabled(TaskQueue* task_queue, bool enabled) {
 void TaskQueueThrottler::IncreaseThrottleRefCount(TaskQueue* task_queue) {
   DCHECK_NE(task_queue, task_runner_.get());
 
-  std::pair<TaskQueueMap::iterator, bool> insert_result =
-      queue_details_.insert(std::make_pair(task_queue, Metadata()));
+  std::pair<TaskQueueMap::iterator, bool> insert_result = queue_details_.insert(
+      std::make_pair(task_queue, Metadata(0, task_queue->IsQueueEnabled())));

[Sami, 2016/11/22 19:27:35]

0 /* ref_count */  (I had to look it up)

[altimin, 2016/11/23 11:11:29]

Done.

 
   if (!insert_result.first->second.IsThrottled()) {
-    // The insert was successful so we need to throttle the queue.
-    insert_result.first->second.enabled = task_queue->IsQueueEnabled();
-
     if (allow_throttling_) {
       task_queue->SetTimeDomain(time_domain_.get());
       task_queue->RemoveFence();
@@ -381,6 +378,9 @@ void TaskQueueThrottler::DecreaseThrottleRefCount(TaskQueue* task_queue) {
 }
 
 bool TaskQueueThrottler::IsThrottled(TaskQueue* task_queue) const {
+  if (!allow_throttling_)

[Sami, 2016/11/22 19:27:35]

Is this a necessary change? To me this query says "is queue X managed by the
throttler?" Whether or not throttling is enabled is orthogonal to that.

Maybe we should rename this method and change the call site(s) that depend on
this distinction also check whether throttling is enabled?

[altimin, 2016/11/23 11:11:29]

Yes, this is the source of the bug.

I think that comment in header is very clear: "Returns true if the |task_queue|
is throttled". That means that if throttling is disabled, this method should
return false.

[Sami, 2016/11/23 11:52:06]

I think this is still pretty confusing. Based on those comments if I call 
IncreaseThrottleRefCount(), I would expect IsThrottled() to return true but
that's not guaranteed at all.

How about this: We rename Metadata::IsThrottled() to CanThrottle() or something
like that and change the inc/decref comments to say they are actually making the
queue *eligible* for throttling -- contingent on the overall throttling state.
WDYT?

Alternatively we could rename this IsThrottled() to CanThrottle() too and make
the callers check both bits. I think I prefer the other model though.

[altimin, 2016/11/23 14:29:17]

It is very confusing indeed! I took another look and found more bugs originating
from that. I decided to remove Metadata::IsThrottled and rely on
TaskQueueThrottler::IsThrottler in the majority of cases or just use
Metadata.throttling_ref_count in a few places where it's really needed.

+    return false;
+
   auto find_it = queue_details_.find(task_queue);
   if (find_it == queue_details_.end())
     return false;