| Summary: | Prevent race condition during Worker shutdown | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Joshua Bell <jsbell> | ||||||
| Component: | New Bugs | Assignee: | Joshua Bell <jsbell> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | benjamin, dimich, levin+threading, ojan.autocc, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Joshua Bell
2013-01-22 12:06:31 PST
Created attachment 184031 [details]
Patch
Comment on attachment 184031 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184031&action=review Patch looks fine, but there are a few stylistic issues worth fixing. review- to fix those No need to have me be the one who reviews next time around. > Source/WTF/ChangeLog:14 > + (MessageQueue): > + (WTF): > + (WTF::::appendAndKill): All three of these lines look bogus. Please fix them if the script generates something that looks like nonsense. Per-function comments are even better. > Source/WebCore/ChangeLog:19 > + (WorkerRunLoop): Please remove this bogus line. > Source/WebCore/workers/WorkerRunLoop.h:64 > + void postTask(PassOwnPtr<ScriptExecutionContext::Task>, bool terminate = false); > + void postTaskForMode(PassOwnPtr<ScriptExecutionContext::Task>, const String& mode, bool terminate = false); I’m not sure that a boolean or enum here is superior to a separate postTaskAndTerminate function. It seems that a separate function will work just fine: void postTaskAndTerminate(PassOwnPtr<ScriptExecutionContext::Task> task) { m_messageQueue.appendAndKill(Task::create(task, defaultMode().isolatedCopy())); } No reason to touch these existing functions at all. > Source/WebCore/workers/WorkerThread.cpp:275 > + const bool terminate = true; > + m_runLoop.postTask(WorkerThreadShutdownStartTask::create(), terminate); This named boolean argument idiom is not what we use for things like this in WebKit; it’s too easy for it to be wrong or get out of sync with the function we are calling. Instead we use enums, which is a perfect fit here. We can and should define an enum in WorkerRunLoop.h. Thanks, darin@. Seemingly contradictory advice... (In reply to comment #2) > No reason to touch these existing functions at all. vs. > We can and should define an enum in WorkerRunLoop.h. .. but I went with adding postTaskAndTerminate() since that seems the clearest. Created attachment 184052 [details]
Patch
r+ anyone? Comment on attachment 184052 [details] Patch Clearing flags on attachment: 184052 Committed r140483: <http://trac.webkit.org/changeset/140483> All reviewed patches have been landed. Closing bug. |