Bug 89590 – showModalDialog message handling is flaky in WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

showModalDialog is flaky in WebKit2

The problem is that CoreIPC messages are randomly not handled while the modal dialog is displaying.  Those messages are in the RunLoop's function queue to be handled, but they are blocked on WebPageProxy::runModal() returning.

To fix this, we have to make two changes:
1 - Change RunLoop::performWork to not copy its queue to a temporary queue but rather to pull each function off the queue one at a time.
2 - Have WebPageProxy::runModal() wake up the runloop right before calling out to the UIClient so one the nested modal runloop has been entered there's a guarantee it will wake up to handle the remaining messages.

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

[Brady Eidson]

Created attachment 148621 [details]
Patch v1 - Proposed fix

I'm in the process of running layout tests on this but the only possible change in behavior should be limited to showing modal dialogs...  and any changes should be progressions!

[WebKit Review Bot]

Attachment 148621 [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:63:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/platform/RunLoop.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 8 files


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

[Jon Lee]

r=me, if i had reviewer privileges.

[Brady Eidson]

(In reply to comment #2)
> Attachment 148621 [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:63:  This { should be at the end of the previous line  [whitespace/braces] [4]
> Source/WebCore/platform/RunLoop.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 2 in 8 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Filed bug 89591 on the bogus warning about RunLoop.cpp:63 - That is our standard MutexLocker idiom throughout the codebase.

Will fix the alphabetical ordering before landing.

[Darin Adler]

Comment on attachment 148621 [details]
Patch v1 - Proposed fix

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

> Source/WebCore/platform/RunLoop.cpp:59
>  void RunLoop::performWork()
>  {

This function needs a why comment explaining the importance of only fetching one function from the queue at a time.

The change log does say that this is needed to fix the bug, but the why comment could say something that specifically talks about the fact that during one of the functions we may make another call to RunLoop::performWork and that has to pick up where we left off.

>> Source/WebCore/platform/RunLoop.cpp:63
>> +        {
> 
> This { should be at the end of the previous line  [whitespace/braces] [4]

This is a stylebot bug and should be reported to the stylebot folks.

>> Source/WebCore/platform/RunLoop.h:36
>> +#include <wtf/Deque.h>
> 
> Alphabetical sorting problem.  [build/include_order] [4]

Please sort this correctly.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:3625
> +    process()->connection()->wakeUpRunLoop();

This needs a why comment. Even the change log doesn’t say why, and in fact, I don’t know why! I’m sure it’s easy to explain.

[Brady Eidson]

(In reply to comment #5)
> (From update of attachment 148621 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148621&action=review
> 
> > Source/WebCore/platform/RunLoop.cpp:59
> >  void RunLoop::performWork()
> >  {
> 
> This function needs a why comment explaining the importance of only fetching one function from the queue at a time.
> 
> The change log does say that this is needed to fix the bug, but the why comment could say something that specifically talks about the fact that during one of the functions we may make another call to RunLoop::performWork and that has to pick up where we left off.

Will do!

> 
> >> Source/WebCore/platform/RunLoop.cpp:63
> >> +        {
> > 
> > This { should be at the end of the previous line  [whitespace/braces] [4]
> 
> This is a stylebot bug and should be reported to the stylebot folks.

Had already been filed as https://bugs.webkit.org/show_bug.cgi?id=89591

> 
> >> Source/WebCore/platform/RunLoop.h:36
> >> +#include <wtf/Deque.h>
> > 
> > Alphabetical sorting problem.  [build/include_order] [4]
> 
> Please sort this correctly.

Already done locally.

> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:3625
> > +    process()->connection()->wakeUpRunLoop();
> 
> This needs a why comment. Even the change log doesn’t say why, and in fact, I don’t know why! I’m sure it’s easy to explain.

The ChangeLog does say, actually!  But I'll make it more clear both there and in a comment.

Thanks for the review!

[Brady Eidson]

Landed in http://trac.webkit.org/changeset/120879