[Jobs API]: Fix gin job lifetime.

Previously, job_task lifetime in gin was bound to handle. This was fine
until we exposed CancelAndDetach, where worker_task are allowed to run
beyond handle lifetime. job_task lifetime must thus be
extended to match worker_task.
This CL also adds a gin test.
Drive by: Fix IsRunning->IsValid calls in tests.


Change-Id: I1f67d9c759e5b1a6fbc98de909a194df2001f087
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2566724Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833296}

gin/v8_platform.cc

@@ -304,8 +304,7 @@ class JobDelegateImpl : public v8::JobDelegate {
 
 class JobHandleImpl : public v8::JobHandle {
  public:
-  JobHandleImpl(base::JobHandle handle, std::unique_ptr<v8::JobTask> job_task)
-      : handle_(std::move(handle)), job_task_(std::move(job_task)) {}
+  explicit JobHandleImpl(base::JobHandle handle) : handle_(std::move(handle)) {}
   ~JobHandleImpl() override = default;
 
   JobHandleImpl(const JobHandleImpl&) = delete;
@@ -342,7 +341,6 @@ class JobHandleImpl : public v8::JobHandle {
   }
 
   base::JobHandle handle_;
-  std::unique_ptr<v8::JobTask> job_task_;
 };
 
 }  // namespace
@@ -532,22 +530,25 @@ std::unique_ptr<v8::JobHandle> V8Platform::PostJob(
       task_traits = kBlockingTaskTraits;
       break;
   }
+  // Ownership of |job_task| is assumed by |worker_task|, while
+  // |max_concurrency_callback| uses an unretained pointer.
+  auto* job_task_ptr = job_task.get();
   auto handle =
       base::PostJob(FROM_HERE, task_traits,
                     base::BindRepeating(
-                        [](v8::JobTask* job_task, base::JobDelegate* delegate) {
+                        [](const std::unique_ptr<v8::JobTask>& job_task,
+                           base::JobDelegate* delegate) {
                           JobDelegateImpl delegate_impl(delegate);
                           job_task->Run(&delegate_impl);
                         },
-                        base::Unretained(job_task.get())),
+                        std::move(job_task)),
                     base::BindRepeating(
                         [](v8::JobTask* job_task, size_t worker_count) {
                           return job_task->GetMaxConcurrency(worker_count);
                         },
-                        base::Unretained(job_task.get())));
+                        base::Unretained(job_task_ptr)));
 
-  return std::make_unique<JobHandleImpl>(std::move(handle),
-                                         std::move(job_task));
+  return std::make_unique<JobHandleImpl>(std::move(handle));
 }
 
 bool V8Platform::IdleTasksEnabled(v8::Isolate* isolate) {

gin/v8_platform_unittest.cc

@@ -6,7 +6,9 @@
 
 #include <atomic>
 
+#include "base/barrier_closure.h"
 #include "base/test/task_environment.h"
+#include "base/test/test_waitable_event.h"
 #include "base/trace_event/trace_event.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -84,10 +86,65 @@ TEST(V8PlatformTest, PostJobSimple) {
   auto handle =
       V8Platform::Get()->PostJob(v8::TaskPriority::kUserVisible,
                                  std::make_unique<Task>(&num_tasks_to_run));
-  EXPECT_TRUE(handle->IsRunning());
+  EXPECT_TRUE(handle->IsValid());
   handle->Join();
-  EXPECT_FALSE(handle->IsRunning());
+  EXPECT_FALSE(handle->IsValid());
   DCHECK_EQ(num_tasks_to_run, 0U);
 }
 
+// Tests that JobTask's lifetime is extended beyond job handle, until no
+// references are left; and is gracefully destroyed.
+TEST(V8PlatformTest, PostJobLifetime) {
+  std::atomic_size_t num_tasks_to_run(4);
+
+  base::TestWaitableEvent threads_running;
+  base::TestWaitableEvent threads_continue;
+  base::RepeatingClosure threads_running_barrier = base::BarrierClosure(
+      num_tasks_to_run,
+      BindOnce(&base::TestWaitableEvent::Signal, Unretained(&threads_running)));
+
+  class Task : public v8::JobTask {
+   public:
+    explicit Task(std::atomic_size_t* num_tasks_to_run,
+                  base::RepeatingClosure threads_running_barrier,
+                  base::TestWaitableEvent* threads_continue)
+        : num_tasks_to_run_(num_tasks_to_run),
+          threads_running_barrier_(std::move(threads_running_barrier)),
+          threads_continue_(threads_continue) {}
+    ~Task() override {
+      // This should only be destroyed once all workers returned.
+      EXPECT_EQ(*num_tasks_to_run_, 0U);
+    }
+
+    void Run(v8::JobDelegate* delegate) override {
+      threads_running_barrier_.Run();
+      threads_continue_->Wait();
+      --(*num_tasks_to_run_);
+    }
+
+    size_t GetMaxConcurrency(size_t /* worker_count*/) const override {
+      return *num_tasks_to_run_;
+    }
+
+    std::atomic_size_t* num_tasks_to_run_;
+    base::RepeatingClosure threads_running_barrier_;
+    base::TestWaitableEvent* threads_continue_;
+  };
+
+  base::test::TaskEnvironment task_environment;
+
+  auto handle = V8Platform::Get()->PostJob(
+      v8::TaskPriority::kUserVisible,
+      std::make_unique<Task>(&num_tasks_to_run,
+                             std::move(threads_running_barrier),
+                             &threads_continue));
+  EXPECT_TRUE(handle->IsValid());
+  threads_running.Wait();
+  handle->CancelAndDetach();
+  handle.reset();
+
+  // Release workers and let the job get destroyed.
+  threads_continue.Signal();
+}
+
 }  // namespace gin