Bug 149635

Summary: ParallelHelperPool::runFunctionInParallel() shouldn't allocate, and ParallelHelperPool.h shouldn't be included everywhere
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED INVALID    
Severity: Normal CC: benjamin, cmarcelo, commit-queue
Priority: P2    
Version: Other   
Hardware: All   
OS: All   
Bug Depends on: 149671    
Bug Blocks: 149432    
Attachments:
Description Flags
the patch saam: review+

Filip Pizlo
Reported 2015-09-29 11:29:55 PDT
Patch forthcoming.
Attachments
the patch (5.08 KB, patch)
2015-09-29 11:32 PDT, Filip Pizlo 		saam: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Filip Pizlo
Comment 1 2015-09-29 11:32:01 PDT
Created attachment 262084 [details] the patch
Saam Barati
Comment 2 2015-09-29 11:35:25 PDT
Comment on attachment 262084 [details] the patch r=me
Filip Pizlo
Comment 3 2015-09-29 13:26:21 PDT
Landed in http://trac.webkit.org/changeset/190324
Alexey Proskuryakov
Comment 4 2015-09-30 10:02:29 PDT
I suspect that this caused flaky crashes, rdar://problem/22916304
WebKit Commit Bot
Comment 5 2015-09-30 10:09:30 PDT
Re-opened since this is blocked by bug 149671
Filip Pizlo
Comment 6 2015-09-30 10:14:11 PDT
(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.
Filip Pizlo
Comment 7 2015-09-30 10:14:29 PDT
See https://bugs.webkit.org/show_bug.cgi?id=149635#c6.
Note You need to log in before you can comment on or make changes to this bug.