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.
Created attachment 350678 [details] Patch
Comment on attachment 350678 [details] Patch Clearing flags on attachment: 350678 Committed r236471: <https://trac.webkit.org/changeset/236471>
All reviewed patches have been landed. Closing bug.
<rdar://problem/44770430>
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.
Reverted r236471 and r236480 for reason: Seems to be causing some flaky crashes Committed r236492: <https://trac.webkit.org/changeset/236492>
Created attachment 350868 [details] Patch
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.
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.
Comment on attachment 350868 [details] Patch Clearing flags on attachment: 350868 Committed r236512: <https://trac.webkit.org/changeset/236512>
(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>.
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
(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.
(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.