Bug 89673 – showModalDialog fix creates risk of never returning from RunLoop::performWork, potentially blocking other event sources

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

showModalDialog fix creates risk of never returning from RunLoop::performWork, potentially starving other event sources

The fix in question was for bug 89590 and can be seen at http://trac.webkit.org/changeset/120879

The scenario is as follows:
-RunLoop::performWork() is invoked and there is 1 function on the queue
-Handling that one function results in 1 more function being placed on the queue
-Handling that one results in yet another being placed on the queue
-Ad infinitum

Therefore that invocation of performWork() never returns and all other event sources are blocked.

Before r120879 this could never happen because we only ever did the amount of work that was in the queue when performWork() was invoked.  Any additional work that was enqueued would have to wait until the next time performWork() was invoked which would be after other event sources had a chance to spin.

We can approximate that same behavior with a simple count.  Patch is coming up.

In radar as <rdar://problem/11718988>

[Brady Eidson]

Created attachment 148832 [details]
Patch v1 - Proposed fix

This patch implements a rule where we only perform the number of functions in the queue at the time performWork was called.  Additional functions will wait until the next time performWork is called.

This very closely approximates our behavior before http://trac.webkit.org/changeset/120879.

I originally re-wrote the loop to be a for loop but that required acquiring the lock before the for loop *just* to get the function count.

By far the common case in RunLoop::performWork is that it is handling 1 function.  So the for-loop solution would've required  acquiring and releasing the lock twice just to perform one function.

Having this simple branch inside the loop seems much more efficient than having that addition lock acquisition in the common case of 1 function, but I'm open to input on this.

[WebKit Review Bot]

Attachment 148832 [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/platform/RunLoop.cpp:82:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

(In reply to comment #1)
> Created an attachment (id=148832) [details]

> By far the common case in RunLoop::performWork is that it is handling 1 function.  So the for-loop solution would've required  acquiring and releasing the lock twice just to perform one function.
> 
> Having this simple branch inside the loop seems much more efficient than having that addition lock acquisition in the common case of 1 function, but I'm open to input on this.

I thought of a better way.  New patch coming.

[Brady Eidson]

Created attachment 148837 [details]
Patch v2 - Avoid taking the lock twice for the common case *and* avoid the branch comparison in Patch v1

[Darin Adler]

Comment on attachment 148837 [details]
Patch v2 - Avoid taking the lock twice for the common case *and* avoid the branch comparison in Patch v1

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

I know Anders will review too. Looks fine to me, thought.

> Source/WebCore/platform/RunLoop.cpp:72
> +    // we guarantee that we will occassionally return from the run loop and other event sources will have

Typo: occasionally has only one s

[Jon Lee]

Comment on attachment 148837 [details]
Patch v2 - Avoid taking the lock twice for the common case *and* avoid the branch comparison in Patch v1

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

> Source/WebCore/platform/RunLoop.cpp:90
> +    for (size_t functionsHandled = 1; functionsHandled < functionsToHandle; functionsHandled++) {

pre, not post, plz 2 fix

[Brady Eidson]

(In reply to comment #6)
> (From update of attachment 148837 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148837&action=review
> 
> > Source/WebCore/platform/RunLoop.cpp:90
> > +    for (size_t functionsHandled = 1; functionsHandled < functionsToHandle; functionsHandled++) {
> 
> pre, not post, plz 2 fix

Definitely fixed locally.

Waiting to land until after Anders has also taken a look

[Brady Eidson]

http://trac.webkit.org/changeset/120954