Revert "Run PrioritizedTaskRunner tasks with the same priority in posting order"

This reverts commit 413675e9.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 564933 as the
culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtLzQxMzY3NWU5YjY2N2UwN2FlODRhMTE1MjY4YWJlZGZkYjc0MzA3NDAM

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.memory/Linux%20ChromiumOS%20MSan%20Tests/7480

Sample Failed Step: browser_tests

Original change's description:
> Run PrioritizedTaskRunner tasks with the same priority in posting order
> 
> What:
> If two tasks have the same priority, they should run in posting order.
> 
> A second thing this CL does is fix a race in the unittests. I meant to block
> the task runner before queueing up requests but failed to. Fixed here.
> 
> Why:
> This can prevent hypothetical task starvation. It's also necessary for the
> SimpleCachePrioritization control group to ensure that we maintain old
> behavior.
> 
> Bug: 850085
> Change-Id: I1f259296604bf04ea33ecbee9ad3180f064c4266
> Reviewed-on: https://chromium-review.googlesource.com/1088759
> Commit-Queue: Josh Karlin <jkarlin@chromium.org>
> Reviewed-by: Maks Orlovich <morlovich@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#564933}

Change-Id: Id4930090c0026aefa9d92bb8871e809a3fcfa124
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 850085
Reviewed-on: https://chromium-review.googlesource.com/1089892
Cr-Commit-Position: refs/heads/master@{#565096}

net/base/prioritized_task_runner.cc

@@ -15,13 +15,11 @@ namespace net {
 PrioritizedTaskRunner::Job::Job(const base::Location& from_here,
                                 base::OnceClosure task,
                                 base::OnceClosure reply,
-                                uint32_t priority,
-                                uint32_t task_count)
+                                uint32_t priority)
     : from_here(from_here),
       task(std::move(task)),
       reply(std::move(reply)),
-      priority(priority),
-      task_count(task_count) {}
+      priority(priority) {}
 
 PrioritizedTaskRunner::Job::~Job() = default;
 PrioritizedTaskRunner::Job::Job(Job&& other) = default;
@@ -36,8 +34,7 @@ void PrioritizedTaskRunner::PostTaskAndReply(const base::Location& from_here,
                                              base::OnceClosure task,
                                              base::OnceClosure reply,
                                              uint32_t priority) {
-  Job job(from_here, std::move(task), std::move(reply), priority,
-          task_count_++);
+  Job job(from_here, std::move(task), std::move(reply), priority);
   {
     base::AutoLock lock(job_heap_lock_);
     job_heap_.push_back(std::move(job));

net/base/prioritized_task_runner.h

@@ -48,7 +48,7 @@ class NET_EXPORT_PRIVATE PrioritizedTaskRunner
   // Similar to TaskRunner::PostTaskAndReply, except that the task runs at
   // |priority|. Priority 0 is the highest priority and will run before other
   // priority values. Multiple tasks with the same |priority| value are run in
-  // order of posting.
+  // arbitrary order.
   void PostTaskAndReply(const base::Location& from_here,
                         base::OnceClosure task,
                         base::OnceClosure reply,
@@ -80,8 +80,7 @@ class NET_EXPORT_PRIVATE PrioritizedTaskRunner
     Job(const base::Location& from_here,
         base::OnceClosure task,
         base::OnceClosure reply,
-        uint32_t priority,
-        uint32_t task_count);
+        uint32_t priority);
     ~Job();
 
     Job(Job&& other);
@@ -91,7 +90,6 @@ class NET_EXPORT_PRIVATE PrioritizedTaskRunner
     base::OnceClosure task;
     base::OnceClosure reply;
     uint32_t priority;
-    uint32_t task_count;
     scoped_refptr<base::TaskRunner> reply_task_runner =
         base::ThreadTaskRunnerHandle::Get();
 
@@ -101,8 +99,6 @@ class NET_EXPORT_PRIVATE PrioritizedTaskRunner
 
   struct JobComparer {
     bool operator()(const Job& left, const Job& right) {
-      if (left.priority == right.priority)
-        return left.task_count > right.task_count;
       return left.priority > right.priority;
     }
   };
@@ -124,11 +120,6 @@ class NET_EXPORT_PRIVATE PrioritizedTaskRunner
   // Accessed on Job::reply_task_runner.
   scoped_refptr<base::TaskRunner> task_runner_;
 
-  // Used to preserve order of jobs of equal priority. This can overflow and
-  // cause periodic priority inversion. This should be infrequent enough to be
-  // of negligible impact.
-  uint32_t task_count_;
-
   DISALLOW_COPY_AND_ASSIGN(PrioritizedTaskRunner);
 };
 

net/base/prioritized_task_runner_unittest.cc

@@ -4,22 +4,18 @@
 
 #include "net/base/prioritized_task_runner.h"
 
-#include <algorithm>
 #include <limits>
 #include <vector>
 
 #include "base/bind_helpers.h"
 #include "base/location.h"
-#include "base/rand_util.h"
 #include "base/run_loop.h"
 #include "base/sequenced_task_runner.h"
-#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/synchronization/lock.h"
-#include "base/synchronization/waitable_event.h"
 #include "base/task_scheduler/post_task.h"
 #include "base/test/scoped_task_environment.h"
-#include "base/threading/thread_restrictions.h"
+
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
@@ -59,25 +55,11 @@ class PrioritizedTaskRunnerTest : public testing::Test {
     return out;
   }
 
-  void BlockTaskRunner(base::TaskRunner* task_runner) {
-    waitable_event_.Reset();
-
-    auto wait_function = [](base::WaitableEvent* waitable_event) {
-      base::ScopedAllowBaseSyncPrimitivesForTesting sync;
-      waitable_event->Wait();
-    };
-    task_runner->PostTask(FROM_HERE,
-                          base::BindOnce(wait_function, &waitable_event_));
-  }
-
-  void ReleaseTaskRunner() { waitable_event_.Signal(); }
-
  protected:
   base::test::ScopedTaskEnvironment scoped_task_environment_;
 
   std::vector<std::string> callback_names_;
   base::Lock callback_names_lock_;
-  base::WaitableEvent waitable_event_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(PrioritizedTaskRunnerTest);
@@ -133,7 +115,6 @@ TEST_F(PrioritizedTaskRunnerTest, PostTaskAndReplyTestPriority) {
   auto prioritized_task_runner =
       base::MakeRefCounted<PrioritizedTaskRunner>(task_runner);
 
-  BlockTaskRunner(task_runner.get());
   prioritized_task_runner->PostTaskAndReply(
       FROM_HERE,
       base::BindOnce(&PrioritizedTaskRunnerTest::PushName,
@@ -157,7 +138,6 @@ TEST_F(PrioritizedTaskRunnerTest, PostTaskAndReplyTestPriority) {
       base::BindOnce(&PrioritizedTaskRunnerTest::PushName,
                      base::Unretained(this), "Reply7"),
       7);
-  ReleaseTaskRunner();
 
   // Run the TaskRunner and all of the tasks and replies should have run, in
   // priority order.
@@ -175,7 +155,6 @@ TEST_F(PrioritizedTaskRunnerTest, PriorityOverflow) {
 
   const uint32_t kMaxPriority = std::numeric_limits<uint32_t>::max();
 
-  BlockTaskRunner(task_runner.get());
   prioritized_task_runner->PostTaskAndReply(
       FROM_HERE,
       base::BindOnce(&PrioritizedTaskRunnerTest::PushName,
@@ -199,7 +178,6 @@ TEST_F(PrioritizedTaskRunnerTest, PriorityOverflow) {
       base::BindOnce(&PrioritizedTaskRunnerTest::PushName,
                      base::Unretained(this), "ReplyMaxPlus1"),
       kMaxPriority + 1);
-  ReleaseTaskRunner();
 
   // Run the TaskRunner and all of the tasks and replies should have run, in
   // priority order.
@@ -236,7 +214,6 @@ TEST_F(PrioritizedTaskRunnerTest, PostTaskAndReplyWithResultTestPriority) {
   auto prioritized_task_runner =
       base::MakeRefCounted<PrioritizedTaskRunner>(task_runner);
 
-  BlockTaskRunner(task_runner.get());
   prioritized_task_runner->PostTaskAndReplyWithResult(
       FROM_HERE,
       base::BindOnce(&PrioritizedTaskRunnerTest::PushNameWithResult,
@@ -260,7 +237,6 @@ TEST_F(PrioritizedTaskRunnerTest, PostTaskAndReplyWithResultTestPriority) {
       base::BindOnce(&PrioritizedTaskRunnerTest::PushName,
                      base::Unretained(this)),
       3);
-  ReleaseTaskRunner();
 
   // Run the TaskRunner and both the Task and Reply should run.
   scoped_task_environment_.RunUntilIdle();
@@ -269,45 +245,5 @@ TEST_F(PrioritizedTaskRunnerTest, PostTaskAndReplyWithResultTestPriority) {
             ReplyOrder());
 }
 
-TEST_F(PrioritizedTaskRunnerTest, OrderSamePriorityByPostOrder) {
-  auto task_runner =
-      base::CreateSequencedTaskRunnerWithTraits(base::TaskTraits());
-  auto prioritized_task_runner =
-      base::MakeRefCounted<PrioritizedTaskRunner>(task_runner);
-
-  std::vector<int> expected;
-
-  // Create 1000 tasks with random priorities between 1 and 3. Those that have
-  // the same priorities should run in posting order.
-  BlockTaskRunner(task_runner.get());
-  for (int i = 0; i < 1000; i++) {
-    int priority = base::RandInt(0, 2);
-    int id = (priority * 1000) + i;
-
-    expected.push_back(id);
-    prioritized_task_runner->PostTaskAndReply(
-        FROM_HERE,
-        base::BindOnce(&PrioritizedTaskRunnerTest::PushName,
-                       base::Unretained(this), base::IntToString(id)),
-        base::BindOnce(base::DoNothing::Once()), priority);
-  }
-  ReleaseTaskRunner();
-
-  // This is the order the tasks should run on the queue.
-  std::sort(expected.begin(), expected.end());
-
-  scoped_task_environment_.RunUntilIdle();
-
-  // This is the order that the tasks ran on the queue.
-  std::vector<int> results;
-  for (const std::string& result : callback_names_) {
-    int result_id;
-    EXPECT_TRUE(base::StringToInt(result, &result_id));
-    results.push_back(result_id);
-  }
-
-  EXPECT_EQ(expected, results);
-}
-
 }  // namespace
 }  // namespace net