Bug 78486 – [chromium] check that we're not running multiple modal dialogs at the same time

• Component: New Bugs
• Status: RESOLVED FIXED

[chromium] check that we're not running multiple modal dialogs at the same time

Created attachment 126755 [details]
Patch

[Eric Seidel (no email)]

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?

What's a RIAA?

[Kent Tamura]

(In reply to comment #3)
> What's a RIAA?

Probably Eric meant RAII.

[Kent Tamura]

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?

Created attachment 126942 [details]
Patch

(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.

[WebKit Review Bot]

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.

[Kent Tamura]

Comment on attachment 126942 [details]
Patch

ok, I understand.

[WebKit Review Bot]

Comment on attachment 126942 [details]
Patch

Clearing flags on attachment: 126942

Committed r107704: <http://trac.webkit.org/changeset/107704>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Klein]

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

[Adam Klein]

Reverted r107704 for reason:

Caused

Committed r107858: <http://trac.webkit.org/changeset/107858>

[Adam Klein]

See also the flakiness dashboard: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Fharness%2Fshow-modal-dialog.html

Created attachment 127557 [details]
Patch

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.

[Kent Tamura]

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.

Created attachment 127771 [details]
Patch

(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.

[Kent Tamura]

(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.

[WebKit Review Bot]

Comment on attachment 127771 [details]
Patch

Clearing flags on attachment: 127771

Committed r108225: <http://trac.webkit.org/changeset/108225>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.