Bug 89590 - showModalDialog message handling is flaky in WebKit2
: showModalDialog message handling is flaky in WebKit2
Status: RESOLVED FIXED
: WebKit
WebKit2
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2012-06-20 11:45 PST by
Modified: 2012-06-20 16:05 PST (History)


Attachments
Patch v1 - Proposed fix (6.58 KB, patch)
2012-06-20 12:00 PST, Brady Eidson
darin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-06-20 11:45:45 PST
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>
------- Comment #1 From 2012-06-20 12:00:28 PST -------
Created an attachment (id=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!
------- Comment #2 From 2012-06-20 12:02:59 PST -------
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.
------- Comment #3 From 2012-06-20 12:05:06 PST -------
r=me, if i had reviewer privileges.
------- Comment #4 From 2012-06-20 12:06:53 PST -------
(In reply to comment #2)
> Attachment 148621 [details] [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.
------- Comment #5 From 2012-06-20 12:25:25 PST -------
(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.

>> 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.
------- Comment #6 From 2012-06-20 12:34:22 PST -------
(In reply to comment #5)
> (From update of attachment 148621 [details] [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!
------- Comment #7 From 2012-06-20 16:05:58 PST -------
Landed in http://trac.webkit.org/changeset/120879