Bug 126161

Summary:

Convert ThreadCondition in WebKit2 over to std::condition_variable

Product:

WebKit

Reporter:

Anders Carlsson <andersca>

Component:

New Bugs

Assignee:

Anders Carlsson <andersca>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, eflews.bot, gyuyoung.kim, ltilve+ews, philn, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none

Description Anders Carlsson 2013-12-23 07:56:56 PST

Convert ThreadCondition in WebKit2 over to std::condition_variable

Comment 1 Anders Carlsson 2013-12-23 08:02:14 PST

Created attachment 219915 [details]
Patch

Comment 2 EFL EWS Bot 2013-12-23 08:11:46 PST

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

Comment 3 Anders Carlsson 2013-12-23 09:06:40 PST

Created attachment 219916 [details]
Patch

Comment 4 EFL EWS Bot 2013-12-23 09:12:08 PST

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

Comment 5 WebKit Commit Bot 2013-12-23 10:33:28 PST

Comment on attachment 219916 [details]
Patch

Clearing flags on attachment: 219916

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

Comment 6 WebKit Commit Bot 2013-12-23 10:33:30 PST

All reviewed patches have been landed.  Closing bug.

Comment 7 Filip Pizlo 2015-08-12 21:37:28 PDT

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?

Comment 8 Anders Carlsson 2015-08-13 10:17:49 PDT

> 
> 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.