Bug 189927 – UIProcess should process incoming sync IPC from WebProcess when waiting for a sync IPC reply from it

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

UIProcess should process incoming sync IPC from WebProcess when waiting for a sync IPC reply from it in order to avoid deadlocks. This is not an issue currently because the WebProcess does process incoming sync IPC when waiting for a sync IPC reply. However, we plan to change this in the future in order to avoid security bugs caused by re-entering WebCore at unsafe times.

[Chris Dumez]

Created attachment 350678 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 350678 [details]
Patch

Clearing flags on attachment: 350678

Committed r236471: <https://trac.webkit.org/changeset/236471>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44770430>

[Chris Dumez]

I think this patch is causing the following flaky crash:
ASSERTION FAILED: frame
/Volumes/Data/WebKit/OpenSource/Source/WebKit/UIProcess/WebPageProxy.cpp(4017) : void WebKit::WebPageProxy::decidePolicyForNavigationAction(uint64_t, const WebCore::SecurityOriginData &, uint64_t, WebKit::NavigationActionData &&, const WebKit::FrameInfoData &, uint64_t, const WebCore::ResourceRequest &, WebCore::ResourceRequest &&, WebCore::ResourceResponse &&, const WebKit::UserData &, WebCore::ShouldSkipSafeBrowsingCheck, Ref<WebKit::WebPageProxy::PolicyDecisionSender> &&)
1   0x102077b59 WTFCrash
2   0x106c54b6b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x1074e07d3 WebKit::WebPageProxy::decidePolicyForNavigationAction(unsigned long long, WebCore::SecurityOriginData const&, unsigned long long, WebKit::NavigationActionData&&, WebKit::FrameInfoData const&, unsigned long long, WebCore::ResourceRequest const&, WebCore::ResourceRequest&&, WebCore::ResourceResponse&&, WebKit::UserData const&, WebCore::ShouldSkipSafeBrowsingCheck, WTF::Ref<WebKit::WebPageProxy::PolicyDecisionSender, WTF::DumbPtrTraits<WebKit::WebPageProxy::PolicyDecisionSender> >&&)
4   0x1074e0521 WebKit::WebPageProxy::decidePolicyForNavigationActionAsync(unsigned long long, WebCore::SecurityOriginData const&, unsigned long long, WebKit::NavigationActionData&&, WebKit::FrameInfoData const&, unsigned long long, WebCore::ResourceRequest const&, WebCore::ResourceRequest&&, WebCore::ResourceResponse&&, WebKit::UserData const&, WebCore::ShouldSkipSafeBrowsingCheck, unsigned long long)
5   0x107db34b6 void IPC::callMemberFunctionImpl<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(unsigned long long, WebCore::SecurityOriginData const&, unsigned long long, WebKit::NavigationActionData&&, WebKit::FrameInfoData const&, unsigned long long, WebCore::ResourceRequest const&, WebCore::ResourceRequest&&, WebCore::ResourceResponse&&, WebKit::UserData const&, WebCore::ShouldSkipSafeBrowsingCheck, unsigned long long), std::__1::tuple<unsigned long long, WebCore::SecurityOriginData, unsigned long long, WebKit::NavigationActionData, WebKit::FrameInfoData, unsigned long long, WebCore::ResourceRequest, WebCore::ResourceRequest, WebCore::ResourceResponse, WebKit::UserData, WebCore::ShouldSkipSafeBrowsingCheck, unsigned long long>, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul, 6ul, 7ul, 8ul, 9ul, 10ul, 11ul>(WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(unsigned long long, WebCore::SecurityOriginData const&, unsigned long long, WebKit::NavigationActionData&&, WebKit::FrameInfoData const&, unsigned long long, WebCore::ResourceRequest const&, WebCore::ResourceRequest&&, WebCore::ResourceResponse&&, WebKit::UserData const&, WebCore::ShouldSkipSafeBrowsingCheck, unsigned long long), std::__1::tuple<unsigned long long, WebCore::SecurityOriginData, unsigned long long, WebKit::NavigationActionData, WebKit::FrameInfoData, unsigned long long, WebCore::ResourceRequest, WebCore::ResourceRequest, WebCore::ResourceResponse, WebKit::UserData, WebCore::ShouldSkipSafeBrowsingCheck, unsigned long long>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul, 6ul, 7ul, 8ul, 9ul, 10ul, 11ul>)
6   0x107db24b0 void IPC::callMemberFunction<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(unsigned long long, WebCore::SecurityOriginData const&, unsigned long long, WebKit::NavigationActionData&&, WebKit::FrameInfoData const&, unsigned long long, WebCore::ResourceRequest const&, WebCore::ResourceRequest&&, WebCore::ResourceResponse&&, WebKit::UserData const&, WebCore::ShouldSkipSafeBrowsingCheck, unsigned long long), std::__1::tuple<unsigned long long, WebCore::SecurityOriginData, unsigned long long, WebKit::NavigationActionData, WebKit::FrameInfoData, unsigned long long, WebCore::ResourceRequest, WebCore::ResourceRequest, WebCore::ResourceResponse, WebKit::UserData, WebCore::ShouldSkipSafeBrowsingCheck, unsigned long long>, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul, 4ul, 5ul, 6ul, 7ul, 8ul, 9ul, 10ul, 11ul> >(std::__1::tuple<unsigned long long, WebCore::SecurityOriginData, unsigned long long, WebKit::NavigationActionData, WebKit::FrameInfoData, unsigned long long, WebCore::ResourceRequest, WebCore::ResourceRequest, WebCore::ResourceResponse, WebKit::UserData, WebCore::ShouldSkipSafeBrowsingCheck, unsigned long long>&&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(unsigned long long, WebCore::SecurityOriginData const&, unsigned long long, WebKit::NavigationActionData&&, WebKit::FrameInfoData const&, unsigned long long, WebCore::ResourceRequest const&, WebCore::ResourceRequest&&, WebCore::ResourceResponse&&, WebKit::UserData const&, WebCore::ShouldSkipSafeBrowsingCheck, unsigned long long))
7   0x107d953e5 void IPC::handleMessage<Messages::WebPageProxy::DecidePolicyForNavigationActionAsync, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(unsigned long long, WebCore::SecurityOriginData const&, unsigned long long, WebKit::NavigationActionData&&, WebKit::FrameInfoData const&, unsigned long long, WebCore::ResourceRequest const&, WebCore::ResourceRequest&&, WebCore::ResourceResponse&&, WebKit::UserData const&, WebCore::ShouldSkipSafeBrowsingCheck, unsigned long long)>(IPC::Decoder&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(unsigned long long, WebCore::SecurityOriginData const&, unsigned long long, WebKit::NavigationActionData&&, WebKit::FrameInfoData const&, unsigned long long, WebCore::ResourceRequest const&, WebCore::ResourceRequest&&, WebCore::ResourceResponse&&, WebKit::UserData const&, WebCore::ShouldSkipSafeBrowsingCheck, unsigned long long))
8   0x107d8c692 WebKit::WebPageProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
9   0x106cef60a IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&)
10  0x1072d2dc4 WebKit::ChildProcessProxy::dispatchMessage(IPC::Connection&, IPC::Decoder&)
11  0x107542b3d WebKit::WebProcessProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
12  0x106c9b65a IPC::Connection::dispatchMessage(IPC::Decoder&)
13  0x106c8e291 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)
14  0x106c99255 IPC::Connection::dispatchIncomingMessages()
15  0x106cb8bc2 IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14::operator()()
16  0x106cb8ae9 WTF::Function<void ()>::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14>::call()
17  0x10209ed4d WTF::Function<void ()>::operator()() const
18  0x1020f6ca3 WTF::RunLoop::performWork()
19  0x1020f7634 WTF::RunLoop::performWork(void*)


Which indicates that the decidePolicyForNavigationActionAsync IPC is processed *before* the DidCreateMainFrame / DidCreateSubFrame one :( This is likely due to DidCreateMainFrame / DidCreateSubFrame async IPC getting sent with IPC::SendOption::DispatchMessageEvenWhenWaitingForSyncReply option, similarly to synchronous IPC.

[Chris Dumez]

Reverted r236471 and r236480 for reason:

Seems to be causing some flaky crashes

Committed r236492: <https://trac.webkit.org/changeset/236492>

[Chris Dumez]

Created attachment 350868 [details]
Patch

[Chris Dumez]

Comment on attachment 350868 [details]
Patch

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

> Source/WebKit/ChangeLog:34
> +        Drop DispatchMessageEvenWhenWaitingForSyncReply SendOption when sending the

This is the change I made compared to the previous iteration of the patch, in order to address the flaky crashes on the bots.

[Geoffrey Garen]

Comment on attachment 350868 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/WebPageProxy.cpp:3361
> +    if (m_mainFrame && m_mainFrame->frameID() == frameID)
> +        return;

Probably deserves a comment that we do this because of decidePolicyForNavigationActionSync.

> Source/WebKit/UIProcess/WebPageProxy.cpp:3387
> +    if (m_process->webFrame(frameID))
> +        return;

Ditto.

[WebKit Commit Bot]

Comment on attachment 350868 [details]
Patch

Clearing flags on attachment: 350868

Committed r236512: <https://trac.webkit.org/changeset/236512>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

(In reply to Geoffrey Garen from comment #9)
> Comment on attachment 350868 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350868&action=review
> 
> r=me
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:3361
> > +    if (m_mainFrame && m_mainFrame->frameID() == frameID)
> > +        return;
> 
> Probably deserves a comment that we do this because of
> decidePolicyForNavigationActionSync.
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:3387
> > +    if (m_process->webFrame(frameID))
> > +        return;
> 
> Ditto.

Done in <https://trac.webkit.org/changeset/236513>.

[Truitt Savell]

Looks like after https://trac.webkit.org/changeset/236512/webkit two tests have become flakey failures. 

editing/pasteboard/emacs-ctrl-a-k-y.html
fast/scrolling/scroll-animator-overlay-scrollbars-clicked.html

These two tests seem to be failing due to a race condition

Diff:
--- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/fast/scrolling/scroll-animator-overlay-scrollbars-clicked-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/fast/scrolling/scroll-animator-overlay-scrollbars-clicked-actual.txt
@@ -1,9 +1,9 @@
 CONSOLE MESSAGE: line 14: MainFrameView: didAddVerticalScrollbar
 CONSOLE MESSAGE: line 14: MainFrameView: didAddHorizontalScrollbar
 CONSOLE MESSAGE: line 14: FrameView: didAddVerticalScrollbar
-CONSOLE MESSAGE: line 14: FrameView: willRemoveVerticalScrollbar
 CONSOLE MESSAGE: line 14: MainFrameView: mouseEnteredContentArea
 CONSOLE MESSAGE: line 14: MainFrameView: mouseMovedInContentArea
+CONSOLE MESSAGE: line 14: FrameView: willRemoveVerticalScrollbar
 CONSOLE MESSAGE: line 16: MainFrameView: mouseEnteredVerticalScrollbar
 CONSOLE MESSAGE: line 16: FrameView: mouseEnteredContentArea
 CONSOLE MESSAGE: line 16: MainFrameView: mouseMovedInContentArea

I was able to confirm this flakiness using 
run-webkit-tests --root testbuild-236512 editing/pasteboard/emacs-ctrl-a-k-y.html --iterations 500 -f

running this on 236511 causes no failures. 

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fscrolling%2Fscroll-animator-overlay-scrollbars-clicked.html%20editing%2Fpasteboard%2Femacs-ctrl-a-k-y.html

[Chris Dumez]

(In reply to Truitt Savell from comment #13)
> Looks like after https://trac.webkit.org/changeset/236512/webkit two tests
> have become flakey failures. 
> 
> editing/pasteboard/emacs-ctrl-a-k-y.html
> fast/scrolling/scroll-animator-overlay-scrollbars-clicked.html
> 
> These two tests seem to be failing due to a race condition
> 
> Diff:
> ---
> /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/
> fast/scrolling/scroll-animator-overlay-scrollbars-clicked-expected.txt
> +++
> /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/
> fast/scrolling/scroll-animator-overlay-scrollbars-clicked-actual.txt
> @@ -1,9 +1,9 @@
>  CONSOLE MESSAGE: line 14: MainFrameView: didAddVerticalScrollbar
>  CONSOLE MESSAGE: line 14: MainFrameView: didAddHorizontalScrollbar
>  CONSOLE MESSAGE: line 14: FrameView: didAddVerticalScrollbar
> -CONSOLE MESSAGE: line 14: FrameView: willRemoveVerticalScrollbar
>  CONSOLE MESSAGE: line 14: MainFrameView: mouseEnteredContentArea
>  CONSOLE MESSAGE: line 14: MainFrameView: mouseMovedInContentArea
> +CONSOLE MESSAGE: line 14: FrameView: willRemoveVerticalScrollbar
>  CONSOLE MESSAGE: line 16: MainFrameView: mouseEnteredVerticalScrollbar
>  CONSOLE MESSAGE: line 16: FrameView: mouseEnteredContentArea
>  CONSOLE MESSAGE: line 16: MainFrameView: mouseMovedInContentArea
> 
> I was able to confirm this flakiness using 
> run-webkit-tests --root testbuild-236512
> editing/pasteboard/emacs-ctrl-a-k-y.html --iterations 500 -f
> 
> running this on 236511 causes no failures. 
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=fast%2Fscrolling%2Fscroll-animator-overlay-
> scrollbars-clicked.html%20editing%2Fpasteboard%2Femacs-ctrl-a-k-y.html

Ok, I will look into it. Thanks.

[Chris Dumez]

(In reply to Chris Dumez from comment #14)
> (In reply to Truitt Savell from comment #13)
> > Looks like after https://trac.webkit.org/changeset/236512/webkit two tests
> > have become flakey failures. 
> > 
> > editing/pasteboard/emacs-ctrl-a-k-y.html
> > fast/scrolling/scroll-animator-overlay-scrollbars-clicked.html
> > 
> > These two tests seem to be failing due to a race condition
> > 
> > Diff:
> > ---
> > /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/
> > fast/scrolling/scroll-animator-overlay-scrollbars-clicked-expected.txt
> > +++
> > /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/
> > fast/scrolling/scroll-animator-overlay-scrollbars-clicked-actual.txt
> > @@ -1,9 +1,9 @@
> >  CONSOLE MESSAGE: line 14: MainFrameView: didAddVerticalScrollbar
> >  CONSOLE MESSAGE: line 14: MainFrameView: didAddHorizontalScrollbar
> >  CONSOLE MESSAGE: line 14: FrameView: didAddVerticalScrollbar
> > -CONSOLE MESSAGE: line 14: FrameView: willRemoveVerticalScrollbar
> >  CONSOLE MESSAGE: line 14: MainFrameView: mouseEnteredContentArea
> >  CONSOLE MESSAGE: line 14: MainFrameView: mouseMovedInContentArea
> > +CONSOLE MESSAGE: line 14: FrameView: willRemoveVerticalScrollbar
> >  CONSOLE MESSAGE: line 16: MainFrameView: mouseEnteredVerticalScrollbar
> >  CONSOLE MESSAGE: line 16: FrameView: mouseEnteredContentArea
> >  CONSOLE MESSAGE: line 16: MainFrameView: mouseMovedInContentArea
> > 
> > I was able to confirm this flakiness using 
> > run-webkit-tests --root testbuild-236512
> > editing/pasteboard/emacs-ctrl-a-k-y.html --iterations 500 -f
> > 
> > running this on 236511 causes no failures. 
> > 
> > History:
> > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> > html#showAllRuns=true&tests=fast%2Fscrolling%2Fscroll-animator-overlay-
> > scrollbars-clicked.html%20editing%2Fpasteboard%2Femacs-ctrl-a-k-y.html
> 
> Ok, I will look into it. Thanks.

Please do not rollout the patch on its own as it would cause IPC deadlocks because of the follow-up patches that landed.