Bug 126161 – Convert ThreadCondition in WebKit2 over to std::condition_variable

• Component: New Bugs
• Status: RESOLVED FIXED

[Anders Carlsson]

Convert ThreadCondition in WebKit2 over to std::condition_variable

[Anders Carlsson]

Created attachment 219915 [details]
Patch

[EFL EWS Bot]

Comment on attachment 219915 [details]
Patch

Attachment 219915 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/45768344

[Anders Carlsson]

Created attachment 219916 [details]
Patch

[EFL EWS Bot]

Comment on attachment 219916 [details]
Patch

Attachment 219916 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/49998225

[WebKit Commit Bot]

Comment on attachment 219916 [details]
Patch

Clearing flags on attachment: 219916

Committed r161002: <http://trac.webkit.org/changeset/161002>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

Comment on attachment 219916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=219916&action=review

> Source/WebKit2/Platform/CoreIPC/Connection.cpp:423
> -        if (!m_waitForMessageCondition.timedWait(m_waitForMessageMutex, absoluteTime)) {
> +        // FIXME: It would be better if Connection::waitForMessage took an std::chrono::milliseconds instead of a double.
> +        std::chrono::milliseconds timeoutInMilliseconds(static_cast<std::chrono::milliseconds::rep>(timeout * 1000));
> +        if (m_waitForMessageCondition.wait_for(lock, timeoutInMilliseconds) == std::cv_status::timeout) {

This looks like a regression: previously, if there was a spurious return from timedWait() that happened just before the timeout and caused us to loop around, the subsequent timedWait() would wake up after the remainder of the original timeout.

After the change, if timedWait() returns spuriously and we loop around, we will wait the full timeout.  I.e. spurious returns from wait can cause us to never time out.

Was this an intentional change in behavior?

[Anders Carlsson]

> 
> This looks like a regression: previously, if there was a spurious return
> from timedWait() that happened just before the timeout and caused us to loop
> around, the subsequent timedWait() would wake up after the remainder of the
> original timeout.
> 
> After the change, if timedWait() returns spuriously and we loop around, we
> will wait the full timeout.  I.e. spurious returns from wait can cause us to
> never time out.
> 
> Was this an intentional change in behavior?

No, good catch.