Bug 107577 - Prevent race condition during Worker shutdown
Summary: Prevent race condition during Worker shutdown
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-22 12:06 PST by Joshua Bell
Modified: 2013-01-22 15:47 PST (History)
5 users (show)

See Also:


Attachments
Patch (6.01 KB, patch)
2013-01-22 12:14 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (5.30 KB, patch)
2013-01-22 14:50 PST, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2013-01-22 12:06:31 PST
Prevent race condition during Worker shutdown
Comment 1 Joshua Bell 2013-01-22 12:14:46 PST
Created attachment 184031 [details]
Patch
Comment 2 Darin Adler 2013-01-22 14:31:38 PST
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.
Comment 3 Joshua Bell 2013-01-22 14:46:17 PST
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.
Comment 4 Joshua Bell 2013-01-22 14:50:00 PST
Created attachment 184052 [details]
Patch
Comment 5 Joshua Bell 2013-01-22 15:35:46 PST
r+ anyone?
Comment 6 WebKit Review Bot 2013-01-22 15:47:14 PST
Comment on attachment 184052 [details]
Patch

Clearing flags on attachment: 184052

Committed r140483: <http://trac.webkit.org/changeset/140483>
Comment 7 WebKit Review Bot 2013-01-22 15:47:17 PST
All reviewed patches have been landed.  Closing bug.