Commit 245b8e62 authored by mrowe@apple.com's avatar mrowe@apple.com

<rdar://problem/10177824> IconDatabase’s use of ThreadCondition leads to...

<rdar://problem/10177824> IconDatabase’s use of ThreadCondition leads to assertion failures in the face of spurious wakeups

It's possible for ThreadCondition::wait to return spuriously without the condition having been signaled.
When that happens we should immediately return to waiting rather than doing our normal work, as some of that
work relies on wakeSyncThread having been called to signal the condition.

Reviewed by Sam Weinig.

* loader/icon/IconDatabase.cpp:
(WebCore::IconDatabase::IconDatabase):
(WebCore::IconDatabase::wakeSyncThread): Note that we have work for the sync thread to do.
(WebCore::IconDatabase::syncThreadMainLoop): If we were woken with no work to do, immediately
go back to waiting on the condition variable. Otherwise, reset m_syncThreadHasWorkToDo and then
do that work. We also switch to moving m_disabledSuddenTerminationForSyncThread immediately in to
our local shouldReenableSuddenTermination variable since it can be updated by other threads while
we don't hold the lock. This makes it inappropriate to make assumptions about its value after dropping
and reacquiring the lock.
* loader/icon/IconDatabase.h:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@95929 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent f566950d
2011-09-25 Mark Rowe <mrowe@apple.com>
<rdar://problem/10177824> IconDatabase’s use of ThreadCondition leads to assertion failures in the face of spurious wakeups
It's possible for ThreadCondition::wait to return spuriously without the condition having been signaled.
When that happens we should immediately return to waiting rather than doing our normal work, as some of that
work relies on wakeSyncThread having been called to signal the condition.
Reviewed by Sam Weinig.
* loader/icon/IconDatabase.cpp:
(WebCore::IconDatabase::IconDatabase):
(WebCore::IconDatabase::wakeSyncThread): Note that we have work for the sync thread to do.
(WebCore::IconDatabase::syncThreadMainLoop): If we were woken with no work to do, immediately
go back to waiting on the condition variable. Otherwise, reset m_syncThreadHasWorkToDo and then
do that work. We also switch to moving m_disabledSuddenTerminationForSyncThread immediately in to
our local shouldReenableSuddenTermination variable since it can be updated by other threads while
we don't hold the lock. This makes it inappropriate to make assumptions about its value after dropping
and reacquiring the lock.
* loader/icon/IconDatabase.h:
2011-09-25 Dan Bernstein <mitz@apple.com> 2011-09-25 Dan Bernstein <mitz@apple.com>
<rdar://problem/10156263> ASSERT in WebCore::FrameView::scheduleRelayoutOfSubtree <rdar://problem/10156263> ASSERT in WebCore::FrameView::scheduleRelayoutOfSubtree
...@@ -767,6 +767,7 @@ IconDatabase::IconDatabase() ...@@ -767,6 +767,7 @@ IconDatabase::IconDatabase()
, m_threadTerminationRequested(false) , m_threadTerminationRequested(false)
, m_removeIconsRequested(false) , m_removeIconsRequested(false)
, m_iconURLImportComplete(false) , m_iconURLImportComplete(false)
, m_syncThreadHasWorkToDo(false)
, m_disabledSuddenTerminationForSyncThread(false) , m_disabledSuddenTerminationForSyncThread(false)
, m_initialPruningComplete(false) , m_initialPruningComplete(false)
, m_client(defaultClient()) , m_client(defaultClient())
...@@ -818,6 +819,7 @@ void IconDatabase::wakeSyncThread() ...@@ -818,6 +819,7 @@ void IconDatabase::wakeSyncThread()
disableSuddenTermination(); disableSuddenTermination();
} }
m_syncThreadHasWorkToDo = true;
m_syncCondition.signal(); m_syncCondition.signal();
} }
...@@ -1350,10 +1352,11 @@ void* IconDatabase::syncThreadMainLoop() ...@@ -1350,10 +1352,11 @@ void* IconDatabase::syncThreadMainLoop()
{ {
ASSERT_ICON_SYNC_THREAD(); ASSERT_ICON_SYNC_THREAD();
bool shouldReenableSuddenTermination = false;
m_syncLock.lock(); m_syncLock.lock();
bool shouldReenableSuddenTermination = m_disabledSuddenTerminationForSyncThread;
m_disabledSuddenTerminationForSyncThread = false;
// It's possible thread termination is requested before the main loop even starts - in that case, just skip straight to cleanup // It's possible thread termination is requested before the main loop even starts - in that case, just skip straight to cleanup
while (!m_threadTerminationRequested) { while (!m_threadTerminationRequested) {
m_syncLock.unlock(); m_syncLock.unlock();
...@@ -1427,14 +1430,17 @@ void* IconDatabase::syncThreadMainLoop() ...@@ -1427,14 +1430,17 @@ void* IconDatabase::syncThreadMainLoop()
// The following is balanced by the call to disableSuddenTermination in the // The following is balanced by the call to disableSuddenTermination in the
// wakeSyncThread function. Any time we wait on the condition, we also have // wakeSyncThread function. Any time we wait on the condition, we also have
// to enableSuddenTermation, after doing the next batch of work. // to enableSuddenTermation, after doing the next batch of work.
ASSERT(m_disabledSuddenTerminationForSyncThread);
enableSuddenTermination(); enableSuddenTermination();
m_disabledSuddenTerminationForSyncThread = false;
} }
m_syncCondition.wait(m_syncLock); while (!m_syncThreadHasWorkToDo)
m_syncCondition.wait(m_syncLock);
m_syncThreadHasWorkToDo = false;
ASSERT(m_disabledSuddenTerminationForSyncThread);
shouldReenableSuddenTermination = true; shouldReenableSuddenTermination = true;
m_disabledSuddenTerminationForSyncThread = false;
} }
m_syncLock.unlock(); m_syncLock.unlock();
...@@ -1446,8 +1452,9 @@ void* IconDatabase::syncThreadMainLoop() ...@@ -1446,8 +1452,9 @@ void* IconDatabase::syncThreadMainLoop()
// The following is balanced by the call to disableSuddenTermination in the // The following is balanced by the call to disableSuddenTermination in the
// wakeSyncThread function. Any time we wait on the condition, we also have // wakeSyncThread function. Any time we wait on the condition, we also have
// to enableSuddenTermation, after doing the next batch of work. // to enableSuddenTermation, after doing the next batch of work.
ASSERT(m_disabledSuddenTerminationForSyncThread);
enableSuddenTermination(); enableSuddenTermination();
MutexLocker locker(m_syncLock);
m_disabledSuddenTerminationForSyncThread = false; m_disabledSuddenTerminationForSyncThread = false;
} }
......
...@@ -144,7 +144,7 @@ private: ...@@ -144,7 +144,7 @@ private:
bool m_isEnabled; bool m_isEnabled;
bool m_privateBrowsingEnabled; bool m_privateBrowsingEnabled;
mutable Mutex m_syncLock; mutable Mutex m_syncLock;
ThreadCondition m_syncCondition; ThreadCondition m_syncCondition;
String m_databaseDirectory; String m_databaseDirectory;
...@@ -154,6 +154,7 @@ private: ...@@ -154,6 +154,7 @@ private:
bool m_threadTerminationRequested; bool m_threadTerminationRequested;
bool m_removeIconsRequested; bool m_removeIconsRequested;
bool m_iconURLImportComplete; bool m_iconURLImportComplete;
bool m_syncThreadHasWorkToDo;
bool m_disabledSuddenTerminationForSyncThread; bool m_disabledSuddenTerminationForSyncThread;
Mutex m_urlAndIconLock; Mutex m_urlAndIconLock;
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment