Created attachment 262084 [details]
Comment on attachment 262084 [details]
Landed in http://trac.webkit.org/changeset/190324
I suspect that this caused flaky crashes, rdar://problem/22916304
Re-opened since this is blocked by bug 149671
(In reply to comment #4)
> I suspect that this caused flaky crashes, rdar://problem/22916304
Yeah, this needs to be rolled out, and I believe that I understand now why the patch is wrong.
The observation behind this patch is that the task cannot run again after runFunctionInParallel returns. That's true. But the way that ParallelHelperPool::helperThreadBody works, it will notify runFunctionInParallel that it's no longer going to run the task *before* it derefs the task. The sequence is:
1) get a task and ref it.
2) run the task.
3) notify task complection.
---> at this point, runFunctionInParallel can return.
4) deref the task.
---> at this point, the helper thread body will crash if runFunctionInParallel returned.
We could still find other ways to avoid allocating the task. But I think that it's not worth it if it will be complicated. The easiest mental model is to treat the task as a proper ref-counted object, in which case a stray RefPtr referencing the task after it cannot run anymore is fine. If we wanted to allow stack-allocation of the task, then we'd have to change our mental model to: it's illegal to have a RefPtr to the task after we have consensus that the task cannot run anymore. I don't want to have to think that hard about this code.