Changeset 120954 in webkit

Timestamp:

Jun 21, 2012 11:54:21 AM (12 years ago)

Author:

beidson@apple.com

Message:

<rdar://problem/11718988> and ​https://bugs.webkit.org/show_bug.cgi?id=89673
showModalDialog fix creates risk of never returning from RunLoop::performWork, potentially blocking other event sources

In case handling a function on the queue places additional functions on the queue, we should
limit the number of functions each invocation of performWork() performs so it can return and
other event sources have a chance to spin.

The showModalDialog fix in question is ​http://trac.webkit.org/changeset/120879

Reviewed by Darin Adler and Anders Carlson.

• platform/RunLoop.cpp:

(WebCore::RunLoop::performWork): If there are only N functions in the queue when performWork is called,

only handle up to N functions before returning. Any additional functions will be handled the next time
the runloop spins.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• platform/RunLoop.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-06-21  Brady Eidson  <beidson@apple.com>
+
+        <rdar://problem/11718988> and https://bugs.webkit.org/show_bug.cgi?id=89673
+        showModalDialog fix creates risk of never returning from RunLoop::performWork, potentially blocking other event sources
+
+        In case handling a function on the queue places additional functions on the queue, we should
+        limit the number of functions each invocation of performWork() performs so it can return and
+        other event sources have a chance to spin.
+
+        The showModalDialog fix in question is http://trac.webkit.org/changeset/120879
+
+        Reviewed by Darin Adler and Anders Carlson.
+
+        * platform/RunLoop.cpp:
+        (WebCore::RunLoop::performWork): If there are only N functions in the queue when performWork is called,
+          only handle up to N functions before returning. Any additional functions will be handled the next time
+          the runloop spins.
+
 2012-06-21  Tim Horton  <timothy_horton@apple.com>
 

trunk/Source/WebCore/platform/RunLoop.cpp

@@ -58 +58 @@
 void RunLoop::performWork()
 {
+    // It is important to handle the functions in the queue one at a time because while inside one of these
+    // functions we might re-enter RunLoop::performWork() and we need to be able to pick up where we left off.
+    // See http://webkit.org/b/89590 for more discussion.
+
+    // One possible scenario when handling the function queue is as follows:
+    // - RunLoop::performWork() is invoked with 1 function on the queue
+    // - Handling that function results in 1 more function being enqueued
+    // - Handling that one results in yet another being enqueued
+    // - And so on
+    //
+    // In this situation one invocation of performWork() never returns so all other event sources are blocked.
+    // By only handling up to the number of functions that were in the queue when performWork() is called
+    // we guarantee to occasionally return from the run loop so other event sources will be allowed to spin.
+
     Function<void()> function;
-    
-    while (true) {
-        // It is important to handle the functions in the queue one at a time because while inside one of these
-        // functions we might re-enter RunLoop::performWork() and we need to be able to pick up where we left off.
-        // See http://webkit.org/b/89590 for more discussion.
+    size_t functionsToHandle = 0;
 
+    {
+        MutexLocker locker(m_functionQueueLock);
+        functionsToHandle = m_functionQueue.size();
+
+        if (m_functionQueue.isEmpty())
+            return;
+
+        function = m_functionQueue.takeFirst();
+    }
+
+    function();
+
+    for (size_t functionsHandled = 1; functionsHandled < functionsToHandle; ++functionsHandled) {
         {
             MutexLocker locker(m_functionQueueLock);
+
+            // Even if we start off with N functions to handle and we've only handled less than N functions, the queue
+            // still might be empty because those functions might have been handled in an inner RunLoop::performWork().
+            // In that case we should bail here.
             if (m_functionQueue.isEmpty())
                 break;