Bug 30920 – DOM Storage's condition variable needs to handle spurious wakeups

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

DOM Storage's condition variable needs to handle spurious wakeups.

[Jeremy Orlow]

Created attachment 42127 [details]
Patch v1

[Darin Adler]

Comment on attachment 42127 [details]
Patch v1

Is there any way to test this?

r=me

[Darin Fisher (:fishd, Google)]

Comment on attachment 42127 [details]
Patch v1

> Index: WebCore/ChangeLog
...
> +        Reviewed by NOBODY (OOPS!).
> +
> +        DOM Storage's condition variable needs to handle spurious wakeups
> +        https://bugs.webkit.org/show_bug.cgi?id=30920
> +
> +        DOM Storage's condition variable needs to handle spurious wakeups.

^^^ repeating the summary?


> Index: WebCore/storage/LocalStorageThread.cpp
> ===================================================================
> --- WebCore/storage/LocalStorageThread.cpp	(revision 50240)
> +++ WebCore/storage/LocalStorageThread.cpp	(working copy)
> @@ -112,7 +112,8 @@ void LocalStorageThread::terminate()
>  
>      m_queue.append(LocalStorageTask::createTerminate(this));
>  
> -    m_terminateCondition.wait(m_terminateLock);
> +    while (!m_queue.isKilled())
> +        m_terminateCondition.wait(m_terminateLock);

nit: I think a more explicit 'bool m_terminated' member would be a bit nicer,
purely so that one doesn't have to read the MessageQueue class to understand
what is going on here.

Also, I don't think that createTerminate needs to happen while m_terminateLock
is held.

[Jeremy Orlow]

(In reply to comment #2)
> (From update of attachment 42127 [details])
> Is there any way to test this?

I really wish there was, but I haven't been able to find a way to reliably reproduce the problem--inside or out of a layout test.  Maybe if we had some sort of unit test framework I could induce it.  Some day, I'd like to investigate how we could incorporate one into WebKit, but I'm pretty sure the answer today is no.  :-(

[Jeremy Orlow]

Created attachment 42134 [details]
Patch v1

[Jeremy Orlow]

Committed r50309: <http://trac.webkit.org/changeset/50309>