Bug 165962 – REGRESSION: HipChat and Mail sometimes hang beneath JSC::Heap::lastChanceToFinalize()

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

There is an inherent race when waiting on the condition variable and the timeout fires.  The waitFor() can timeout just before getting a notification from another thread.

AutomaticThread::start() uses a condition variable to wait for work from another thread.  The other thread waits for the machine thread to respond to the notification, but instead it goes to sleep since it doesn't see the notification.

[Michael Saboff]

Created attachment 297335 [details]
Patch

[Filip Pizlo]

Comment on attachment 297335 [details]
Patch

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

> Source/WTF/wtf/AutomaticThread.cpp:50
> +    m_condition.notifyOne();

This is sort of wrong since it makes notifyOne mean notifyTwo.

Programs that use notifyOne often rely on the bounded nature of the wake-up.  I think that one solution is to simply move this call to the bottom of the method: only m_condition.notifyOne() if we did not find any threads to wake up ourselves.

> Source/WTF/wtf/AutomaticThread.cpp:212
> +                        if (!awokenByNotify && m_isWaiting) {
> +                            m_isWaiting = false;

Do we still need awokenByNotify?

[Michael Saboff]

(In reply to comment #2)
> Comment on attachment 297335 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297335&action=review
> 
> > Source/WTF/wtf/AutomaticThread.cpp:50
> > +    m_condition.notifyOne();
> 
> This is sort of wrong since it makes notifyOne mean notifyTwo.
> 
> Programs that use notifyOne often rely on the bounded nature of the wake-up.
> I think that one solution is to simply move this call to the bottom of the
> method: only m_condition.notifyOne() if we did not find any threads to wake
> up ourselves.

That makes sense.  I'll make that change.

> > Source/WTF/wtf/AutomaticThread.cpp:212
> > +                        if (!awokenByNotify && m_isWaiting) {
> > +                            m_isWaiting = false;
> 
> Do we still need awokenByNotify?

No.  I'll remove it.

[Michael Saboff]

Created attachment 297339 [details]
Addressed Phil's comments and fixed m_condition ASSERTs

[Michael Saboff]

Committed r209938: <http://trac.webkit.org/changeset/209938>

[Geoffrey Garen]

<rdar://problem/29229439>