Changeset 95929 in webkit

Timestamp:

Sep 25, 2011 6:40:04 PM (13 years ago)

Author:

mrowe@apple.com

Message:

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

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• loader/icon/IconDatabase.cpp (modified) (5 diffs)
• loader/icon/IconDatabase.h (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/loader/icon/IconDatabase.cpp

@@ -768 +768 @@
     , m_removeIconsRequested(false)
     , m_iconURLImportComplete(false)
+    , m_syncThreadHasWorkToDo(false)
     , m_disabledSuddenTerminationForSyncThread(false)
     , m_initialPruningComplete(false)
@@ -819 +820 @@
     }
 
+    m_syncThreadHasWorkToDo = true;
     m_syncCondition.signal();
 }
@@ -1351 +1353 @@
     ASSERT_ICON_SYNC_THREAD();
 
-    bool shouldReenableSuddenTermination = false;
-
     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
@@ -1428 +1431 @@
             // wakeSyncThread function. Any time we wait on the condition, we also have
             // to enableSuddenTermation, after doing the next batch of work.
-            ASSERT(m_disabledSuddenTerminationForSyncThread);
             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;
+        m_disabledSuddenTerminationForSyncThread = false;
     }
 
@@ -1447 +1453 @@
         // wakeSyncThread function. Any time we wait on the condition, we also have
         // to enableSuddenTermation, after doing the next batch of work.
-        ASSERT(m_disabledSuddenTerminationForSyncThread);
         enableSuddenTermination();
+
+        MutexLocker locker(m_syncLock);
         m_disabledSuddenTerminationForSyncThread = false;
     }

trunk/Source/WebCore/loader/icon/IconDatabase.h

@@ -145 +145 @@
     bool m_isEnabled;
     bool m_privateBrowsingEnabled;
-    
+
     mutable Mutex m_syncLock;
     ThreadCondition m_syncCondition;
@@ -155 +155 @@
     bool m_removeIconsRequested;
     bool m_iconURLImportComplete;
+    bool m_syncThreadHasWorkToDo;
     bool m_disabledSuddenTerminationForSyncThread;