Changeset 274565 in webkit

Timestamp:

Mar 17, 2021, 10:09:04 AM (5 years ago)

Author:

Chris Dumez

Message:

Maybe-regression(STP121): window.open flakily returning null
​https://bugs.webkit.org/show_bug.cgi?id=222590
<rdar://problem/75211786>

Reviewed by Geoffrey Garen.

Source/WebKit:

This was an IPC ordering bug. WebPageProxy::DidCommitLoadForFrame is async and WebPageProxy::CreateNewPage is
sync. As a result, it was possible for the WebPageProxy::CreateNewPage to get processed *BEFORE* the
WebPageProxy::DidCommitLoadForFrame IPC. This was causing trouble because Safari rejects the popup opening if
the main frame is doing a provisional load.

To address the issue, introduce a new IPC::SendSyncOption::MaintainOrderingWithAsyncMessages flag and
use it on WebPageProxy::CreateNewPage sync IPC so that it gets processed in order with surrounding async
messages.

• Platform/IPC/Connection.cpp:

(IPC::Connection::SyncMessageState::processIncomingMessage):
(IPC::Connection::sendSyncMessage):

• Platform/IPC/Connection.h:
• Platform/IPC/Decoder.cpp:

(IPC::Decoder::shouldMaintainOrderingWithAsyncMessages const):

• Platform/IPC/Decoder.h:
• Platform/IPC/Encoder.cpp:

(IPC::Encoder::setShouldMaintainOrderingWithAsyncMessages):

• Platform/IPC/Encoder.h:
• Platform/IPC/MessageFlags.h:
• WebProcess/WebCoreSupport/WebChromeClient.cpp:

(WebKit::WebChromeClient::createWindow):

Tools:

Add API test coverage.

• TestWebKitAPI/Tests/WebKit/ModalAlertsSPI.cpp:

(TestWebKitAPI::TEST):
(TestWebKitAPI::checkFrameLoadStateAndCreateNewPage):

Location:

trunk

Files:

• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/Platform/IPC/Connection.cpp (modified) (2 diffs)
• Source/WebKit/Platform/IPC/Connection.h (modified) (1 diff)
• Source/WebKit/Platform/IPC/Decoder.cpp (modified) (1 diff)
• Source/WebKit/Platform/IPC/Decoder.h (modified) (1 diff)
• Source/WebKit/Platform/IPC/Encoder.cpp (modified) (1 diff)
• Source/WebKit/Platform/IPC/Encoder.h (modified) (1 diff)
• Source/WebKit/Platform/IPC/MessageFlags.h (modified) (2 diffs)
• Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKit/ModalAlertsSPI.cpp (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2021-03-17  Chris Dumez  <cdumez@apple.com>
+
+        Maybe-regression(STP121): window.open flakily returning null
+        https://bugs.webkit.org/show_bug.cgi?id=222590
+        <rdar://problem/75211786>
+
+        Reviewed by Geoffrey Garen.
+
+        This was an IPC ordering bug. WebPageProxy::DidCommitLoadForFrame is async and WebPageProxy::CreateNewPage is
+        sync. As a result, it was possible for the WebPageProxy::CreateNewPage to get processed *BEFORE* the
+        WebPageProxy::DidCommitLoadForFrame IPC. This was causing trouble because Safari rejects the popup opening if
+        the main frame is doing a provisional load.
+
+        To address the issue, introduce a new IPC::SendSyncOption::MaintainOrderingWithAsyncMessages flag and
+        use it on WebPageProxy::CreateNewPage sync IPC so that it gets processed in order with surrounding async
+        messages.
+
+        * Platform/IPC/Connection.cpp:
+        (IPC::Connection::SyncMessageState::processIncomingMessage):
+        (IPC::Connection::sendSyncMessage):
+        * Platform/IPC/Connection.h:
+        * Platform/IPC/Decoder.cpp:
+        (IPC::Decoder::shouldMaintainOrderingWithAsyncMessages const):
+        * Platform/IPC/Decoder.h:
+        * Platform/IPC/Encoder.cpp:
+        (IPC::Encoder::setShouldMaintainOrderingWithAsyncMessages):
+        * Platform/IPC/Encoder.h:
+        * Platform/IPC/MessageFlags.h:
+        * WebProcess/WebCoreSupport/WebChromeClient.cpp:
+        (WebKit::WebChromeClient::createWindow):
+
 2021-03-17  Peng Liu  <peng.liu6@apple.com>
 

trunk/Source/WebKit/Platform/IPC/Connection.cpp

@@ -150 +150 @@
         auto locker = holdLock(m_mutex);
         shouldDispatch = m_didScheduleDispatchMessagesWorkSet.add(&connection).isNewEntry;
+        ASSERT(connection.m_incomingMessagesMutex.isHeld());
+        if (message->shouldMaintainOrderingWithAsyncMessages()) {
+            // This sync message should maintain ordering with async messages so we need to process the pending async messages first.
+            while (!connection.m_incomingMessages.isEmpty())
+                m_messagesToDispatchWhileWaitingForSyncReply.append(ConnectionAndIncomingMessage { connection, connection.m_incomingMessages.takeFirst() });
+        }
         m_messagesToDispatchWhileWaitingForSyncReply.append(ConnectionAndIncomingMessage { connection, WTFMove(message) });
     }
@@ -604 +610 @@
         sendOptions = sendOptions | IPC::SendOption::DispatchMessageEvenWhenWaitingForUnboundedSyncReply;
 
+    if (sendSyncOptions.contains(IPC::SendSyncOption::MaintainOrderingWithAsyncMessages))
+        encoder->setShouldMaintainOrderingWithAsyncMessages();
+
     auto messageName = encoder->messageName();
     sendMessage(WTFMove(encoder), sendOptions);

trunk/Source/WebKit/Platform/IPC/Connection.h

@@ -72 +72 @@
     UseFullySynchronousModeForTesting = 1 << 1,
     ForceDispatchWhenDestinationIsWaitingForUnboundedSyncReply = 1 << 2,
+    MaintainOrderingWithAsyncMessages = 1 << 3,
 };
 

trunk/Source/WebKit/Platform/IPC/Decoder.cpp

@@ -148 +148 @@
 }
 
+bool Decoder::shouldMaintainOrderingWithAsyncMessages() const
+{
+    return m_messageFlags.contains(MessageFlags::MaintainOrderingWithAsyncMessages);
+}
+
 #if PLATFORM(MAC)
 void Decoder::setImportanceAssertion(std::unique_ptr<ImportanceAssertion> assertion)

trunk/Source/WebKit/Platform/IPC/Decoder.h

@@ -65 +65 @@
     ShouldDispatchWhenWaitingForSyncReply shouldDispatchMessageWhenWaitingForSyncReply() const;
     bool shouldUseFullySynchronousModeForTesting() const;
+    bool shouldMaintainOrderingWithAsyncMessages() const;
 
 #if PLATFORM(MAC)

trunk/Source/WebKit/Platform/IPC/Encoder.cpp

@@ -124 +124 @@
 }
 
+void Encoder::setShouldMaintainOrderingWithAsyncMessages()
+{
+    messageFlags().add(MessageFlags::MaintainOrderingWithAsyncMessages);
+}
+
 void Encoder::wrapForTesting(UniqueRef<Encoder>&& original)
 {

trunk/Source/WebKit/Platform/IPC/Encoder.h

@@ -57 +57 @@
 
     void setFullySynchronousModeForTesting();
+    void setShouldMaintainOrderingWithAsyncMessages();
 
     void wrapForTesting(UniqueRef<Encoder>&&);

trunk/Source/WebKit/Platform/IPC/MessageFlags.h

@@ -32 +32 @@
     DispatchMessageWhenWaitingForUnboundedSyncReply = 1 << 1,
     UseFullySynchronousModeForTesting = 1 << 2,
+    MaintainOrderingWithAsyncMessages = 1 << 3,
 };
 
@@ -49 +50 @@
         IPC::MessageFlags::DispatchMessageWhenWaitingForSyncReply,
         IPC::MessageFlags::DispatchMessageWhenWaitingForUnboundedSyncReply,
-        IPC::MessageFlags::UseFullySynchronousModeForTesting
+        IPC::MessageFlags::UseFullySynchronousModeForTesting,
+        IPC::MessageFlags::MaintainOrderingWithAsyncMessages
     >;
 };

trunk/Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp

@@ -284 +284 @@
     Optional<PageIdentifier> newPageID;
     Optional<WebPageCreationParameters> parameters;
-    if (!webProcess.parentProcessConnection()->sendSync(Messages::WebPageProxy::CreateNewPage(webFrame->info(), webFrame->page()->webPageProxyIdentifier(), navigationAction.resourceRequest(), windowFeatures, navigationActionData), Messages::WebPageProxy::CreateNewPage::Reply(newPageID, parameters), m_page.identifier()))
+    if (!webProcess.parentProcessConnection()->sendSync(Messages::WebPageProxy::CreateNewPage(webFrame->info(), webFrame->page()->webPageProxyIdentifier(), navigationAction.resourceRequest(), windowFeatures, navigationActionData), Messages::WebPageProxy::CreateNewPage::Reply(newPageID, parameters), m_page.identifier(), IPC::Timeout::infinity(), IPC::SendSyncOption::MaintainOrderingWithAsyncMessages))
         return nullptr;
 

trunk/Tools/ChangeLog

@@ +1 @@
+2021-03-17  Chris Dumez  <cdumez@apple.com>
+
+        Maybe-regression(STP121): window.open flakily returning null
+        https://bugs.webkit.org/show_bug.cgi?id=222590
+        <rdar://problem/75211786>
+
+        Reviewed by Geoffrey Garen.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKit/ModalAlertsSPI.cpp:
+        (TestWebKitAPI::TEST):
+        (TestWebKitAPI::checkFrameLoadStateAndCreateNewPage):
+
 2021-03-17  Brent Fulgham  <bfulgham@apple.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKit/ModalAlertsSPI.cpp

@@ -138 +138 @@
 
     Util::run(&done);
+    openedWebView = nil;
+}
+
+static WKPageRef checkFrameLoadStateAndCreateNewPage(WKPageRef page, WKURLRequestRef urlRequest, WKDictionaryRef features, WKEventModifiers modifiers, WKEventMouseButton mouseButton, const void *clientInfo)
+{
+    auto mainFrame = WKPageGetMainFrame(page);
+    ASSERT(mainFrame);
+    EXPECT_EQ(kWKFrameLoadStateCommitted, WKFrameGetFrameLoadState(mainFrame));
+    done = true;
+    return createNewPage(page, urlRequest, features, modifiers, mouseButton, clientInfo);
+}
+
+TEST(WebKit, CreateNewPageDelegateFrameLoadState)
+{
+    for (unsigned i = 0; i < 25; ++i) {
+        done = false;
+        auto context = adoptWK(WKContextCreateWithConfiguration(nullptr));
+        PlatformWebView webView(context.get());
+
+        WKPageUIClientV5 uiClient;
+        memset(&uiClient, 0, sizeof(uiClient));
+        uiClient.base.version = 5;
+        uiClient.createNewPage = checkFrameLoadStateAndCreateNewPage;
+        WKPageSetPageUIClient(webView.page(), &uiClient.base);
+
+        auto htmlString = Util::toWK("<script>open('about:blank', '_blank')</script>");
+        WKPageLoadHTMLString(webView.page(), htmlString.get(), nullptr);
+        Util::run(&done);
+        openedWebView = nil;
+    }
 }