[scheduler] Fix bug in task queue throttler

third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.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 "platform/scheduler/renderer/task_queue_throttler.h"
 
 #include <cstdint>
 
 #include "base/format_macros.h"
 #include "base/logging.h"
@@ skipping 314 matching lines @@
   if (!enabled) {
     task_queue->SetQueueEnabled(false);
     MaybeSchedulePumpQueue(FROM_HERE, tick_clock_->NowTicks(), task_queue,
                            base::nullopt);
   }
 }
 
 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();
       task_queue->SetQueueEnabled(false);
 
       if (!task_queue->IsEmpty()) {
         if (task_queue->HasPendingImmediateWork()) {
           OnTimeDomainHasImmediateWork(task_queue);
         } else {
           OnTimeDomainHasDelayedWork(task_queue);
@@ skipping 22 matching lines @@
       task_queue->RemoveFence();
       task_queue->SetQueueEnabled(enabled);
     }
 
     TRACE_EVENT1(tracing_category_, "TaskQueueThrottler_TaskQueueUntrottled",
                  "task_queue", 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;
   return find_it->second.IsThrottled();
 }
 
 void TaskQueueThrottler::UnregisterTaskQueue(TaskQueue* task_queue) {
   LazyNow lazy_now(tick_clock_);
   auto find_it = queue_details_.find(task_queue);
 
@@ skipping 242 matching lines @@
 
     queue->SetQueueEnabled(false);
     queue->SetTimeDomain(time_domain_.get());
     MaybeSchedulePumpQueue(FROM_HERE, lazy_now.Now(), queue,
                            GetNextAllowedRunTime(lazy_now.Now(), queue));
   }
 }
 
 }  // namespace scheduler
 }  // namespace blink