[Jobs API]: Fix use-after-free.

It's possible for a thread to re-enqueue a JobTaskSource right before
the job is canceled/joined and will sill end up in the queue (the main thread
may try to remove it before it's added again).
The fix is simply to avoid calling user callback if the job has the cancel
bit set. In addition, a job that's joined needs to be marked as canceled.

Bug: 1120686
Change-Id: I1ca9458a22eda71c400cfb6f0ef42efc85e38e86
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2369435Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#800730}

base/task/post_job.cc

@@ -97,9 +97,6 @@ void JobHandle::Join() {
   bool must_run = task_source_->WillJoin();
   while (must_run)
     must_run = task_source_->RunJoinTask();
-  // Remove |task_source_| from the ThreadPool to prevent access to
-  // |max_concurrency_callback| after Join().
-  task_source_->delegate()->RemoveJobTaskSource(task_source_);
   task_source_ = nullptr;
 }
 

base/task/thread_pool/job_task_source.cc

@@ -194,19 +194,14 @@ bool JobTaskSource::WaitForParticipationOpportunity() {
   DCHECK_EQ(state.worker_count(), 1U);
   DCHECK(state.is_canceled() || max_concurrency == 0U);
   state_.DecrementWorkerCount();
+  // Prevent subsequent accesses to user callbacks.
+  state_.Cancel();
   return false;
 }
 
 TaskSource::RunStatus JobTaskSource::WillRunTask() {
   CheckedAutoLock auto_lock(worker_lock_);
-
   auto state_before_add = state_.Load();
-  const size_t max_concurrency =
-      GetMaxConcurrency(state_before_add.worker_count());
-  if (!state_before_add.is_canceled() &&
-      state_before_add.worker_count() < max_concurrency) {
-    state_before_add = state_.IncrementWorkerCount();
-  }
 
   // Don't allow this worker to run the task if either:
   //   A) |state_| was canceled.
@@ -215,6 +210,11 @@ TaskSource::RunStatus JobTaskSource::WillRunTask() {
   // Case A:
   if (state_before_add.is_canceled())
     return RunStatus::kDisallowed;
+
+  const size_t max_concurrency =
+      GetMaxConcurrency(state_before_add.worker_count());
+  if (state_before_add.worker_count() < max_concurrency)
+    state_before_add = state_.IncrementWorkerCount();
   const size_t worker_count_before_add = state_before_add.worker_count();
   // Case B) or C):
   if (worker_count_before_add >= max_concurrency)
@@ -230,9 +230,11 @@ size_t JobTaskSource::GetRemainingConcurrency() const {
   // It is safe to read |state_| without a lock since this variable is atomic,
   // and no other state is synchronized with GetRemainingConcurrency().
   const auto state = TS_UNCHECKED_READ(state_).Load();
+  if (state.is_canceled())
+    return 0;
   const size_t max_concurrency = GetMaxConcurrency(state.worker_count());
   // Avoid underflows.
-  if (state.is_canceled() || state.worker_count() > max_concurrency)
+  if (state.worker_count() > max_concurrency)
     return 0;
   return max_concurrency - state.worker_count();
 }