Remove testing-only method and field from Cancelable

Implement similar functionality in the unit test which used this field.
One test gets slightly weaker by this.

R=mstarzinger@chromium.org

Bug: v8:8238
Change-Id: I0b047ff54f08a4549a2f78af30e21296bb1ee63f
Reviewed-on: https://chromium-review.googlesource.com/c/1327042
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57403}

src/cancelable-task.h

@@ -109,20 +109,13 @@ class V8_EXPORT_PRIVATE Cancelable {
   bool TryRun(Status* previous = nullptr) {
     return CompareExchangeStatus(kWaiting, kRunning, previous);
   }
-  intptr_t CancelAttempts() { return cancel_counter_; }
 
  private:
   friend class CancelableTaskManager;
 
   // Use {CancelableTaskManager} to abort a task that has not yet been
   // executed.
-  bool Cancel() {
-    if (CompareExchangeStatus(kWaiting, kCanceled)) {
-      return true;
-    }
-    cancel_counter_++;
-    return false;
-  }
+  bool Cancel() { return CompareExchangeStatus(kWaiting, kCanceled); }
 
   bool CompareExchangeStatus(Status expected, Status desired,
                              Status* previous = nullptr) {
@@ -137,11 +130,6 @@ class V8_EXPORT_PRIVATE Cancelable {
   std::atomic<Status> status_{kWaiting};
   const CancelableTaskManager::Id id_;
 
-  // The counter is incremented for failing tries to cancel a task. This can be
-  // used by the task itself as an indication how often external entities tried
-  // to abort it.
-  std::atomic<intptr_t> cancel_counter_{0};
-
   DISALLOW_COPY_AND_ASSIGN(Cancelable);
 };
 

test/unittests/cancelable-tasks-unittest.cc

@@ -5,9 +5,9 @@
 #include "src/base/atomicops.h"
 #include "src/base/platform/platform.h"
 #include "src/cancelable-task.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-
 namespace v8 {
 namespace internal {
 
@@ -15,41 +15,21 @@ namespace {
 
 using ResultType = std::atomic<CancelableTaskManager::Id>;
 
+class CancelableTaskManagerTest;
+
 class TestTask : public Task, public Cancelable {
  public:
-  enum Mode { kDoNothing, kWaitTillCanceledAgain, kCheckNotRun };
+  enum Mode { kDoNothing, kWaitTillCancelTriggered, kCheckNotRun };
 
-  TestTask(CancelableTaskManager* manager, ResultType* result, Mode mode)
-      : Cancelable(manager), result_(result), mode_(mode) {}
+  TestTask(CancelableTaskManagerTest* test, ResultType* result, Mode mode);
 
-  // Task overrides.
-  void Run() final {
-    if (TryRun()) {
-      RunInternal();
-    }
-  }
+  // Task override.
+  void Run() final;
 
  private:
-  void RunInternal() {
-    result_->store(id());
-
-    switch (mode_) {
-      case kWaitTillCanceledAgain:
-        // Simple busy wait until the main thread tried to cancel.
-        while (CancelAttempts() == 0) {
-        }
-        break;
-      case kCheckNotRun:
-        // Check that we never execute {RunInternal}.
-        EXPECT_TRUE(false);
-        break;
-      default:
-        break;
-    }
-  }
-
   ResultType* const result_;
   const Mode mode_;
+  CancelableTaskManagerTest* const test_;
 };
 
 class SequentialRunner {
@@ -94,24 +74,61 @@ class CancelableTaskManagerTest : public ::testing::Test {
 
   std::unique_ptr<TestTask> NewTask(
       ResultType* result, TestTask::Mode mode = TestTask::kDoNothing) {
-    return base::make_unique<TestTask>(&manager_, result, mode);
+    return base::make_unique<TestTask>(this, result, mode);
+  }
+
+  void CancelAndWait() {
+    cancel_triggered_.store(true);
+    manager_.CancelAndWait();
   }
 
+  TryAbortResult TryAbortAll() {
+    cancel_triggered_.store(true);
+    return manager_.TryAbortAll();
+  }
+
+  bool cancel_triggered() const { return cancel_triggered_.load(); }
+
  private:
   CancelableTaskManager manager_;
+  std::atomic<bool> cancel_triggered_{false};
 };
 
+TestTask::TestTask(CancelableTaskManagerTest* test, ResultType* result,
+                   Mode mode)
+    : Cancelable(test->manager()), result_(result), mode_(mode), test_(test) {}
+
+void TestTask::Run() {
+  if (!TryRun()) return;
+
+  result_->store(id());
+
+  switch (mode_) {
+    case kWaitTillCancelTriggered:
+      // Simple busy wait until the main thread tried to cancel.
+      while (!test_->cancel_triggered()) {
+      }
+      break;
+    case kCheckNotRun:
+      // Check that we never execute {RunInternal}.
+      EXPECT_TRUE(false);
+      break;
+    default:
+      break;
+  }
+}
+
 }  // namespace
 
 TEST_F(CancelableTaskManagerTest, EmptyCancelableTaskManager) {
-  manager()->CancelAndWait();
+  CancelAndWait();
 }
 
 TEST_F(CancelableTaskManagerTest, SequentialCancelAndWait) {
   ResultType result1{0};
   SequentialRunner runner1(NewTask(&result1, TestTask::kCheckNotRun));
   EXPECT_EQ(0u, result1);
-  manager()->CancelAndWait();
+  CancelAndWait();
   EXPECT_EQ(0u, result1);
   runner1.Run();
   EXPECT_EQ(0u, result1);
@@ -133,7 +150,7 @@ TEST_F(CancelableTaskManagerTest, SequentialMultipleTasks) {
   runner2.Run();
   EXPECT_EQ(2u, result2);
 
-  manager()->CancelAndWait();
+  CancelAndWait();
   EXPECT_EQ(TryAbortResult::kTaskRemoved, manager()->TryAbort(1));
   EXPECT_EQ(TryAbortResult::kTaskRemoved, manager()->TryAbort(2));
 }
@@ -141,14 +158,14 @@ TEST_F(CancelableTaskManagerTest, SequentialMultipleTasks) {
 TEST_F(CancelableTaskManagerTest, ThreadedMultipleTasksStarted) {
   ResultType result1{0};
   ResultType result2{0};
-  ThreadedRunner runner1(NewTask(&result1, TestTask::kWaitTillCanceledAgain));
-  ThreadedRunner runner2(NewTask(&result2, TestTask::kWaitTillCanceledAgain));
+  ThreadedRunner runner1(NewTask(&result1, TestTask::kWaitTillCancelTriggered));
+  ThreadedRunner runner2(NewTask(&result2, TestTask::kWaitTillCancelTriggered));
   runner1.Start();
   runner2.Start();
   // Busy wait on result to make sure both tasks are done.
   while (result1.load() == 0 || result2.load() == 0) {
   }
-  manager()->CancelAndWait();
+  CancelAndWait();
   runner1.Join();
   runner2.Join();
   EXPECT_EQ(1u, result1);
@@ -160,7 +177,7 @@ TEST_F(CancelableTaskManagerTest, ThreadedMultipleTasksNotRun) {
   ResultType result2{0};
   ThreadedRunner runner1(NewTask(&result1, TestTask::kCheckNotRun));
   ThreadedRunner runner2(NewTask(&result2, TestTask::kCheckNotRun));
-  manager()->CancelAndWait();
+  CancelAndWait();
   // Tasks are canceled, hence the runner will bail out and not update result.
   runner1.Start();
   runner2.Start();
@@ -178,7 +195,7 @@ TEST_F(CancelableTaskManagerTest, RemoveBeforeCancelAndWait) {
   EXPECT_EQ(TryAbortResult::kTaskAborted, manager()->TryAbort(id));
   runner1.Start();
   runner1.Join();
-  manager()->CancelAndWait();
+  CancelAndWait();
   EXPECT_EQ(0u, result1);
 }
 
@@ -189,7 +206,7 @@ TEST_F(CancelableTaskManagerTest, RemoveAfterCancelAndWait) {
   EXPECT_EQ(1u, id);
   runner1.Start();
   runner1.Join();
-  manager()->CancelAndWait();
+  CancelAndWait();
   EXPECT_EQ(TryAbortResult::kTaskRemoved, manager()->TryAbort(id));
   EXPECT_EQ(1u, result1);
 }
@@ -197,13 +214,13 @@ TEST_F(CancelableTaskManagerTest, RemoveAfterCancelAndWait) {
 TEST_F(CancelableTaskManagerTest, RemoveUnmanagedId) {
   EXPECT_EQ(TryAbortResult::kTaskRemoved, manager()->TryAbort(1));
   EXPECT_EQ(TryAbortResult::kTaskRemoved, manager()->TryAbort(2));
-  manager()->CancelAndWait();
+  CancelAndWait();
   EXPECT_EQ(TryAbortResult::kTaskRemoved, manager()->TryAbort(1));
   EXPECT_EQ(TryAbortResult::kTaskRemoved, manager()->TryAbort(3));
 }
 
 TEST_F(CancelableTaskManagerTest, EmptyTryAbortAll) {
-  EXPECT_EQ(TryAbortResult::kTaskRemoved, manager()->TryAbortAll());
+  EXPECT_EQ(TryAbortResult::kTaskRemoved, TryAbortAll());
 }
 
 TEST_F(CancelableTaskManagerTest, ThreadedMultipleTasksNotRunTryAbortAll) {
@@ -211,7 +228,7 @@ TEST_F(CancelableTaskManagerTest, ThreadedMultipleTasksNotRunTryAbortAll) {
   ResultType result2{0};
   ThreadedRunner runner1(NewTask(&result1, TestTask::kCheckNotRun));
   ThreadedRunner runner2(NewTask(&result2, TestTask::kCheckNotRun));
-  EXPECT_EQ(TryAbortResult::kTaskAborted, manager()->TryAbortAll());
+  EXPECT_EQ(TryAbortResult::kTaskAborted, TryAbortAll());
   // Tasks are canceled, hence the runner will bail out and not update result.
   runner1.Start();
   runner2.Start();
@@ -224,13 +241,18 @@ TEST_F(CancelableTaskManagerTest, ThreadedMultipleTasksNotRunTryAbortAll) {
 TEST_F(CancelableTaskManagerTest, ThreadedMultipleTasksStartedTryAbortAll) {
   ResultType result1{0};
   ResultType result2{0};
-  ThreadedRunner runner1(NewTask(&result1, TestTask::kWaitTillCanceledAgain));
-  ThreadedRunner runner2(NewTask(&result2, TestTask::kWaitTillCanceledAgain));
+  ThreadedRunner runner1(NewTask(&result1, TestTask::kWaitTillCancelTriggered));
+  ThreadedRunner runner2(NewTask(&result2, TestTask::kWaitTillCancelTriggered));
   runner1.Start();
   // Busy wait on result to make sure task1 is done.
   while (result1.load() == 0) {
   }
-  EXPECT_EQ(TryAbortResult::kTaskRunning, manager()->TryAbortAll());
+  // If the task saw that we triggered the cancel and finished *before* the
+  // actual cancel happened, we get {kTaskAborted}. Otherwise, we get
+  // {kTaskRunning}.
+  EXPECT_THAT(TryAbortAll(),
+              testing::AnyOf(testing::Eq(TryAbortResult::kTaskAborted),
+                             testing::Eq(TryAbortResult::kTaskRunning)));
   runner2.Start();
   runner1.Join();
   runner2.Join();