Bug 78486 - [chromium] check that we're not running multiple modal dialogs at the same time
Summary: [chromium] check that we're not running multiple modal dialogs at the same time
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: jochen
URL:
Keywords:
Depends on:
Blocks: 78240
  Show dependency treegraph
 
Reported: 2012-02-13 04:48 PST by jochen
Modified: 2012-02-20 01:30 PST (History)
3 users (show)

See Also:


Attachments
Patch (3.35 KB, patch)
2012-02-13 04:49 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (3.52 KB, patch)
2012-02-14 02:37 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (3.67 KB, patch)
2012-02-17 02:21 PST, jochen
no flags Details | Formatted Diff | Diff
Patch (3.72 KB, patch)
2012-02-20 00:20 PST, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2012-02-13 04:48:44 PST
[chromium] check that we're not running multiple modal dialogs at the same time
Comment 1 jochen 2012-02-13 04:49:28 PST
Created attachment 126755 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-02-13 11:16:18 PST
Comment on attachment 126755 [details]
Patch

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

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:903
> +    ASSERT(!m_shell->isDisplayingModalDialog());
> +    m_shell->setIsDisplayingModalDialog(true);
>      bool oldState = webkit_support::MessageLoopNestableTasksAllowed();
>      webkit_support::MessageLoopSetNestableTasksAllowed(true);
>      m_inModalLoop = true;
>      webkit_support::RunMessageLoop();
>      webkit_support::MessageLoopSetNestableTasksAllowed(oldState);
> +    m_shell->setIsDisplayingModalDialog(false);

This feels like we should have a RIAA to manage this...  Do we do this anywhere else?
Comment 3 jochen 2012-02-13 11:30:52 PST
What's a RIAA?
Comment 4 Kent Tamura 2012-02-13 17:11:04 PST
(In reply to comment #3)
> What's a RIAA?

Probably Eric meant RAII.
Comment 5 Kent Tamura 2012-02-13 21:35:02 PST
Comment on attachment 126755 [details]
Patch

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

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:897
> +    ASSERT(!m_shell->isDisplayingModalDialog());
> +    m_shell->setIsDisplayingModalDialog(true);

ASSERT(!m_inModalLoop) isn't enough?
Comment 6 jochen 2012-02-14 02:37:15 PST
Created attachment 126942 [details]
Patch
Comment 7 jochen 2012-02-14 02:38:40 PST
(In reply to comment #5)
> (From update of attachment 126755 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126755&action=review
> 
> > Tools/DumpRenderTree/chromium/WebViewHost.cpp:897
> > +    ASSERT(!m_shell->isDisplayingModalDialog());
> > +    m_shell->setIsDisplayingModalDialog(true);
> 
> ASSERT(!m_inModalLoop) isn't enough?

No, that's not a global state. but belongs to a given WebViewHost. If you have several, you can still start two modal dialogs at the same time.

In 78240 I've attached a test case that does exactly does this - DRT then spins of two nested message loops and fails to terminate them, so it times out.
Comment 8 WebKit Review Bot 2012-02-14 02:40:14 PST
Attachment 126942 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
	M	Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py
	M	Tools/ChangeLog
r107696 = aac87443871f9579cd8351bf27615eb9f00b9362 (refs/remotes/origin/master)
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168766 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Kent Tamura 2012-02-14 03:44:22 PST
Comment on attachment 126942 [details]
Patch

ok, I understand.
Comment 10 WebKit Review Bot 2012-02-14 04:36:25 PST
Comment on attachment 126942 [details]
Patch

Clearing flags on attachment: 126942

Committed r107704: <http://trac.webkit.org/changeset/107704>
Comment 11 WebKit Review Bot 2012-02-14 04:36:29 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Adam Klein 2012-02-15 17:02:24 PST
This causes several modal dialog related tests to fail on the debug bots (and locally). Here's a stack trace of fast/harness/show-modal-dialog.html:

Program received signal SIGSEGV, Segmentation fault.
0x00000000005475a5 in TestShell::setIsDisplayingModalDialog (this=0x3636363636363636, isDisplayingModalDialog=false)
    at ../../third_party/WebKit/Tools/DumpRenderTree/chromium/TestShell.h:166
166         void setIsDisplayingModalDialog(bool isDisplayingModalDialog) { m_isDisplayingModalDialog = isDisplayingModalDialog; }
(gdb) bt   
#0  0x00000000005475a5 in TestShell::setIsDisplayingModalDialog (this=0x3636363636363636, isDisplayingModalDialog=false)
    at ../../third_party/WebKit/Tools/DumpRenderTree/chromium/TestShell.h:166
#1  0x0000000000541742 in WebViewHost::runModal (this=0x7fffe2291500)
    at ../../third_party/WebKit/Tools/DumpRenderTree/chromium/WebViewHost.cpp:907
#2  0x00007ffff13d8002 in WebKit::ChromeClientImpl::runModal (this=0x7fffe2301310)
    at ../../third_party/WebKit/Source/WebKit/chromium/src/ChromeClientImpl.cpp:321
#3  0x00007ffff2445a54 in WebCore::Chrome::runModal (this=0x7fffe2272f00) at ../../third_party/WebKit/Source/WebCore/page/Chrome.cpp:233
#4  0x00007ffff245f6e7 in WebCore::DOMWindow::showModalDialog (this=0x7fffe2276900, urlString=..., dialogFeaturesString=..., 
    activeWindow=0x7fffe2276900, firstWindow=0x7fffe2276900, function=0x7ffff208e9b9 <WebCore::setUpDialog(WebCore::DOMWindow*, void*)>, 
    functionContext=0x7fffffffb0c0) at ../../third_party/WebKit/Source/WebCore/page/DOMWindow.cpp:1903
#5  0x00007ffff208eaf5 in WebCore::V8DOMWindow::showModalDialogCallback (args=...)
    at ../../third_party/WebKit/Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:441
Comment 13 Adam Klein 2012-02-15 17:07:02 PST
Reverted r107704 for reason:

Caused

Committed r107858: <http://trac.webkit.org/changeset/107858>
Comment 15 jochen 2012-02-17 02:21:02 PST
Created attachment 127557 [details]
Patch
Comment 16 jochen 2012-02-17 02:22:11 PST
Comment on attachment 127557 [details]
Patch

The issue was that the WebViewHost might get deleted before RunMessageLoop() returns, so I can't access member variables afterwards.
Comment 17 Kent Tamura 2012-02-19 20:40:27 PST
Comment on attachment 127557 [details]
Patch

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

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:906
> +    // This WebViewHost might get deleted before RunMessageLoop() returns, so keep a copy of the m_shell member variable around.
> +    TestShell* shell = m_shell;
>      webkit_support::RunMessageLoop();
>      webkit_support::MessageLoopSetNestableTasksAllowed(oldState);
> +    shell->setIsDisplayingModalDialog(false);

When the WebViewHost is deleted, the TestShell is also deleted.
Comment 18 jochen 2012-02-20 00:20:07 PST
Created attachment 127771 [details]
Patch
Comment 19 jochen 2012-02-20 00:21:23 PST
(In reply to comment #17)
> (From update of attachment 127557 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127557&action=review
> 
> > Tools/DumpRenderTree/chromium/WebViewHost.cpp:906
> > +    // This WebViewHost might get deleted before RunMessageLoop() returns, so keep a copy of the m_shell member variable around.
> > +    TestShell* shell = m_shell;
> >      webkit_support::RunMessageLoop();
> >      webkit_support::MessageLoopSetNestableTasksAllowed(oldState);
> > +    shell->setIsDisplayingModalDialog(false);
> 
> When the WebViewHost is deleted, the TestShell is also deleted.

Not in this case: runModal() is only executed during showModalDialog which creates a new WebViewHost.

I added an ASSERT() to make this clear.
Comment 20 Kent Tamura 2012-02-20 00:43:55 PST
(In reply to comment #19)
> > When the WebViewHost is deleted, the TestShell is also deleted.
> 
> Not in this case: runModal() is only executed during showModalDialog which creates a new WebViewHost.

Oh, I see.
Comment 21 WebKit Review Bot 2012-02-20 01:30:41 PST
Comment on attachment 127771 [details]
Patch

Clearing flags on attachment: 127771

Committed r108225: <http://trac.webkit.org/changeset/108225>
Comment 22 WebKit Review Bot 2012-02-20 01:30:46 PST
All reviewed patches have been landed.  Closing bug.