Commit 932fceaf authored by Etienne Pierre-doray's avatar Etienne Pierre-doray Committed by Commit Bot

[Jobs API]: Simplify JobHandle::Cancel

Current implementation delegates to Join(), but there's no need to bump
priority of a canceled task. This CL simply calls WillJoin().

There's a subtle race that was hidden by priority bump
(which takes ThreadGroup::lock_).
1- Thread B calls GetRemainingConcurrency(). state_ is not canceled.
2- Thread A calls JobHandle::Cancel() and frees job state.
3- Thread B calls GetMaxConcurrency() -> use after free
RemoveJobTaskSource() becomes necessary again to make sure the
task_source doesn't become dangling while still in the queue.
Once JobHandle::Cancel() returns, it's ok if we end up with dangling
references of job_task_source since it's already canceled.

Change-Id: I6cf831b64fc75e555e32ae9b399342d64ed3f7d1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2369259
Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
Reviewed-by: default avatarFrançois Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#801497}
parent bacf0ac1
......@@ -97,12 +97,20 @@ 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;
}
void JobHandle::Cancel() {
task_source_->Cancel();
Join();
bool must_run = task_source_->WillJoin();
DCHECK(!must_run);
// Remove |task_source_| from the ThreadPool to prevent access to
// |max_concurrency_callback| after Join().
task_source_->delegate()->RemoveJobTaskSource(task_source_);
task_source_ = nullptr;
}
void JobHandle::CancelAndDetach() {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment