Bug 68783

Summary: IconDatabase’s use of ThreadCondition leads to assertion failures in the face of spurious wakeups
Product: WebKit Reporter: Mark Rowe (bdash) <mrowe>
Component: WebKit Misc.Assignee: Mark Rowe (bdash) <mrowe>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, beidson, dino, mitz, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch v1
none
Updated patch sam: review+

Description Mark Rowe (bdash) 2011-09-25 16:23:07 PDT
The code in IconDatabase::syncThreadMainLoop assumes that if the call to ThreadCondition::wait returns then it must have been due to the call to ThreadCondition::signal.  However, spurious wakeups (<http://en.wikipedia.org/wiki/Spurious_wakeup>) can lead to wait returning without the condition being signaled. When this happens we’ll hit the following assertion in a debug build:

            ASSERT(m_disabledSuddenTerminationForSyncThread);

In a release build we’ll get all sorts of console spew due to the resulting unbalanced call to enableSuddenTermination.

<rdar://problem/10177824>
Comment 1 Mark Rowe (bdash) 2011-09-25 16:28:24 PDT
Created attachment 108616 [details]
Patch v1
Comment 2 WebKit Review Bot 2011-09-25 16:29:38 PDT
Attachment 108616 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Mark Rowe (bdash) 2011-09-25 17:02:42 PDT
This patch isn’t quite right. If wakeSyncThread is called on the main thread while the sync thread is executing the body of the sync thread loop (e.g., when the lock has been dropped) then m_disabledSuddenTerminationForSyncThread and m_syncThreadHasWorkToDo will be set, but the block of code immediately before we call ThreadCondition::wait will reset m_disabledSuddenTerminationForSyncThread to false, causing us to then fail the assertion immediately after the call to ThreadCondition::wait.
Comment 4 Mark Rowe (bdash) 2011-09-25 17:49:47 PDT
Created attachment 108618 [details]
Updated patch
Comment 5 WebKit Review Bot 2011-09-25 17:52:39 PDT
Attachment 108618 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Mark Rowe (bdash) 2011-09-25 18:39:19 PDT
Landed in r95929.
Comment 7 Adam Roben (:aroben) 2011-09-26 08:36:46 PDT
This is causing assertion failures for me on launch:

ASSERTION FAILED: m_disabledSuddenTerminationForSyncThread
/Users/aroben/dev/WebKit/OpenSource/Source/WebCore/loader/icon/IconDatabase.cpp(1441) : void *WebCore::IconDatabase::syncThreadMainLoop()
1   WebCore::IconDatabase::syncThreadMainLoop()
2   WebCore::IconDatabase::iconDatabaseSyncThread()
3   WebCore::IconDatabase::iconDatabaseSyncThreadStart(void*)
4   _ZN3WTFL16threadEntryPointEPv
5   _pthread_start
6   thread_start

It looks like wakeSyncThread is being called before the sync thread has even started. This causes shouldReenableSuddenTermination to be set to true and m_disabledSuddenTerminationForSyncThread to be set to false at the start of syncThreadMainLoop.
Comment 8 Mark Rowe (bdash) 2011-09-26 09:41:39 PDT
Filed bug 68809 to follow up on that.