⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 276157 in webkit


Ignore:
Timestamp:
Apr 16, 2021, 12:26:19 PM (5 years ago)
Author:
Russell Epstein
Message:

Cherry-pick r274565. rdar://problem/76412930

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):

git-svn-id: ​https://svn.webkit.org/repository/webkit/trunk@274565 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Location:
branches/safari-611-branch
Files:
11 edited

Legend:

Unmodified
Added
Removed

  • branches/safari-611-branch/Source/WebKit/ChangeLog

    r276151 r276157  
     12021-04-16  Russell Epstein  <repstein@apple.com>
     2
     3        Cherry-pick r274565. rdar://problem/76412930
     4
     5    Maybe-regression(STP121): window.open flakily returning null
     6    https://bugs.webkit.org/show_bug.cgi?id=222590
     7    <rdar://problem/75211786>
     8   
     9    Reviewed by Geoffrey Garen.
     10   
     11    Source/WebKit:
     12   
     13    This was an IPC ordering bug. WebPageProxy::DidCommitLoadForFrame is async and WebPageProxy::CreateNewPage is
     14    sync. As a result, it was possible for the WebPageProxy::CreateNewPage to get processed *BEFORE* the
     15    WebPageProxy::DidCommitLoadForFrame IPC. This was causing trouble because Safari rejects the popup opening if
     16    the main frame is doing a provisional load.
     17   
     18    To address the issue, introduce a new IPC::SendSyncOption::MaintainOrderingWithAsyncMessages flag and
     19    use it on WebPageProxy::CreateNewPage sync IPC so that it gets processed in order with surrounding async
     20    messages.
     21   
     22    * Platform/IPC/Connection.cpp:
     23    (IPC::Connection::SyncMessageState::processIncomingMessage):
     24    (IPC::Connection::sendSyncMessage):
     25    * Platform/IPC/Connection.h:
     26    * Platform/IPC/Decoder.cpp:
     27    (IPC::Decoder::shouldMaintainOrderingWithAsyncMessages const):
     28    * Platform/IPC/Decoder.h:
     29    * Platform/IPC/Encoder.cpp:
     30    (IPC::Encoder::setShouldMaintainOrderingWithAsyncMessages):
     31    * Platform/IPC/Encoder.h:
     32    * Platform/IPC/MessageFlags.h:
     33    * WebProcess/WebCoreSupport/WebChromeClient.cpp:
     34    (WebKit::WebChromeClient::createWindow):
     35   
     36    Tools:
     37   
     38    Add API test coverage.
     39   
     40    * TestWebKitAPI/Tests/WebKit/ModalAlertsSPI.cpp:
     41    (TestWebKitAPI::TEST):
     42    (TestWebKitAPI::checkFrameLoadStateAndCreateNewPage):
     43   
     44    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@274565 268f45cc-cd09-0410-ab3c-d52691b4dbfc
     45
     46    2021-03-17  Chris Dumez  <cdumez@apple.com>
     47
     48            Maybe-regression(STP121): window.open flakily returning null
     49            https://bugs.webkit.org/show_bug.cgi?id=222590
     50            <rdar://problem/75211786>
     51
     52            Reviewed by Geoffrey Garen.
     53
     54            This was an IPC ordering bug. WebPageProxy::DidCommitLoadForFrame is async and WebPageProxy::CreateNewPage is
     55            sync. As a result, it was possible for the WebPageProxy::CreateNewPage to get processed *BEFORE* the
     56            WebPageProxy::DidCommitLoadForFrame IPC. This was causing trouble because Safari rejects the popup opening if
     57            the main frame is doing a provisional load.
     58
     59            To address the issue, introduce a new IPC::SendSyncOption::MaintainOrderingWithAsyncMessages flag and
     60            use it on WebPageProxy::CreateNewPage sync IPC so that it gets processed in order with surrounding async
     61            messages.
     62
     63            * Platform/IPC/Connection.cpp:
     64            (IPC::Connection::SyncMessageState::processIncomingMessage):
     65            (IPC::Connection::sendSyncMessage):
     66            * Platform/IPC/Connection.h:
     67            * Platform/IPC/Decoder.cpp:
     68            (IPC::Decoder::shouldMaintainOrderingWithAsyncMessages const):
     69            * Platform/IPC/Decoder.h:
     70            * Platform/IPC/Encoder.cpp:
     71            (IPC::Encoder::setShouldMaintainOrderingWithAsyncMessages):
     72            * Platform/IPC/Encoder.h:
     73            * Platform/IPC/MessageFlags.h:
     74            * WebProcess/WebCoreSupport/WebChromeClient.cpp:
     75            (WebKit::WebChromeClient::createWindow):
     76
    1772021-04-15  Russell Epstein  <repstein@apple.com>
    278

  • branches/safari-611-branch/Source/WebKit/Platform/IPC/Connection.cpp

    r271243 r276157  
    148148        auto locker = holdLock(m_mutex);
    149149        shouldDispatch = m_didScheduleDispatchMessagesWorkSet.add(&connection).isNewEntry;
     150        ASSERT(connection.m_incomingMessagesMutex.isHeld());
     151        if (message->shouldMaintainOrderingWithAsyncMessages()) {
     152            // This sync message should maintain ordering with async messages so we need to process the pending async messages first.
     153            while (!connection.m_incomingMessages.isEmpty())
     154                m_messagesToDispatchWhileWaitingForSyncReply.append(ConnectionAndIncomingMessage { connection, connection.m_incomingMessages.takeFirst() });
     155        }
    150156        m_messagesToDispatchWhileWaitingForSyncReply.append(ConnectionAndIncomingMessage { connection, WTFMove(message) });
    151157    }
    … …  
    615621        sendOptions = sendOptions | IPC::SendOption::DispatchMessageEvenWhenWaitingForUnboundedSyncReply;
    616622
     623    if (sendSyncOptions.contains(IPC::SendSyncOption::MaintainOrderingWithAsyncMessages))
     624        encoder->setShouldMaintainOrderingWithAsyncMessages();
     625
    617626    auto messageName = encoder->messageName();
    618627    sendMessage(WTFMove(encoder), sendOptions);

  • branches/safari-611-branch/Source/WebKit/Platform/IPC/Connection.h

    r271282 r276157  
    7070    UseFullySynchronousModeForTesting = 1 << 1,
    7171    ForceDispatchWhenDestinationIsWaitingForUnboundedSyncReply = 1 << 2,
     72    MaintainOrderingWithAsyncMessages = 1 << 3,
    7273};
    7374

  • branches/safari-611-branch/Source/WebKit/Platform/IPC/Decoder.cpp

    r273395 r276157  
    137137}
    138138
     139bool Decoder::shouldMaintainOrderingWithAsyncMessages() const
     140{
     141    return m_messageFlags.contains(MessageFlags::MaintainOrderingWithAsyncMessages);
     142}
     143
    139144#if PLATFORM(MAC)
    140145void Decoder::setImportanceAssertion(std::unique_ptr<ImportanceAssertion> assertion)

  • branches/safari-611-branch/Source/WebKit/Platform/IPC/Decoder.h

    r273395 r276157  
    6161    ShouldDispatchWhenWaitingForSyncReply shouldDispatchMessageWhenWaitingForSyncReply() const;
    6262    bool shouldUseFullySynchronousModeForTesting() const;
     63    bool shouldMaintainOrderingWithAsyncMessages() const;
    6364
    6465#if PLATFORM(MAC)

  • branches/safari-611-branch/Source/WebKit/Platform/IPC/Encoder.cpp

    r271243 r276157  
    123123}
    124124
     125void Encoder::setShouldMaintainOrderingWithAsyncMessages()
     126{
     127    messageFlags().add(MessageFlags::MaintainOrderingWithAsyncMessages);
     128}
     129
    125130void Encoder::wrapForTesting(std::unique_ptr<Encoder> original)
    126131{

  • branches/safari-611-branch/Source/WebKit/Platform/IPC/Encoder.h

    r271243 r276157  
    5757
    5858    void setFullySynchronousModeForTesting();
     59    void setShouldMaintainOrderingWithAsyncMessages();
    5960
    6061    void wrapForTesting(std::unique_ptr<Encoder>);

  • branches/safari-611-branch/Source/WebKit/Platform/IPC/MessageFlags.h

    r271243 r276157  
    3232    DispatchMessageWhenWaitingForUnboundedSyncReply = 1 << 1,
    3333    UseFullySynchronousModeForTesting = 1 << 2,
     34    MaintainOrderingWithAsyncMessages = 1 << 3,
    3435};
    3536
    … …  
    4950        IPC::MessageFlags::DispatchMessageWhenWaitingForSyncReply,
    5051        IPC::MessageFlags::DispatchMessageWhenWaitingForUnboundedSyncReply,
    51         IPC::MessageFlags::UseFullySynchronousModeForTesting
     52        IPC::MessageFlags::UseFullySynchronousModeForTesting,
     53        IPC::MessageFlags::MaintainOrderingWithAsyncMessages
    5254    >;
    5355};

  • branches/safari-611-branch/Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp

    r276151 r276157  
    281281    Optional<PageIdentifier> newPageID;
    282282    Optional<WebPageCreationParameters> parameters;
    283     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()))
     283    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))
    284284        return nullptr;
    285285

  • branches/safari-611-branch/Tools/ChangeLog

    r276089 r276157  
     12021-04-16  Russell Epstein  <repstein@apple.com>
     2
     3        Cherry-pick r274565. rdar://problem/76412930
     4
     5    Maybe-regression(STP121): window.open flakily returning null
     6    https://bugs.webkit.org/show_bug.cgi?id=222590
     7    <rdar://problem/75211786>
     8   
     9    Reviewed by Geoffrey Garen.
     10   
     11    Source/WebKit:
     12   
     13    This was an IPC ordering bug. WebPageProxy::DidCommitLoadForFrame is async and WebPageProxy::CreateNewPage is
     14    sync. As a result, it was possible for the WebPageProxy::CreateNewPage to get processed *BEFORE* the
     15    WebPageProxy::DidCommitLoadForFrame IPC. This was causing trouble because Safari rejects the popup opening if
     16    the main frame is doing a provisional load.
     17   
     18    To address the issue, introduce a new IPC::SendSyncOption::MaintainOrderingWithAsyncMessages flag and
     19    use it on WebPageProxy::CreateNewPage sync IPC so that it gets processed in order with surrounding async
     20    messages.
     21   
     22    * Platform/IPC/Connection.cpp:
     23    (IPC::Connection::SyncMessageState::processIncomingMessage):
     24    (IPC::Connection::sendSyncMessage):
     25    * Platform/IPC/Connection.h:
     26    * Platform/IPC/Decoder.cpp:
     27    (IPC::Decoder::shouldMaintainOrderingWithAsyncMessages const):
     28    * Platform/IPC/Decoder.h:
     29    * Platform/IPC/Encoder.cpp:
     30    (IPC::Encoder::setShouldMaintainOrderingWithAsyncMessages):
     31    * Platform/IPC/Encoder.h:
     32    * Platform/IPC/MessageFlags.h:
     33    * WebProcess/WebCoreSupport/WebChromeClient.cpp:
     34    (WebKit::WebChromeClient::createWindow):
     35   
     36    Tools:
     37   
     38    Add API test coverage.
     39   
     40    * TestWebKitAPI/Tests/WebKit/ModalAlertsSPI.cpp:
     41    (TestWebKitAPI::TEST):
     42    (TestWebKitAPI::checkFrameLoadStateAndCreateNewPage):
     43   
     44    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@274565 268f45cc-cd09-0410-ab3c-d52691b4dbfc
     45
     46    2021-03-17  Chris Dumez  <cdumez@apple.com>
     47
     48            Maybe-regression(STP121): window.open flakily returning null
     49            https://bugs.webkit.org/show_bug.cgi?id=222590
     50            <rdar://problem/75211786>
     51
     52            Reviewed by Geoffrey Garen.
     53
     54            Add API test coverage.
     55
     56            * TestWebKitAPI/Tests/WebKit/ModalAlertsSPI.cpp:
     57            (TestWebKitAPI::TEST):
     58            (TestWebKitAPI::checkFrameLoadStateAndCreateNewPage):
     59
    1602021-04-15  Russell Epstein  <repstein@apple.com>
    261

  • branches/safari-611-branch/Tools/TestWebKitAPI/Tests/WebKit/ModalAlertsSPI.cpp

    r248846 r276157  
    138138
    139139    Util::run(&done);
     140    openedWebView = nil;
     141}
     142
     143static WKPageRef checkFrameLoadStateAndCreateNewPage(WKPageRef page, WKURLRequestRef urlRequest, WKDictionaryRef features, WKEventModifiers modifiers, WKEventMouseButton mouseButton, const void *clientInfo)
     144{
     145    auto mainFrame = WKPageGetMainFrame(page);
     146    ASSERT(mainFrame);
     147    EXPECT_EQ(kWKFrameLoadStateCommitted, WKFrameGetFrameLoadState(mainFrame));
     148    done = true;
     149    return createNewPage(page, urlRequest, features, modifiers, mouseButton, clientInfo);
     150}
     151
     152TEST(WebKit, CreateNewPageDelegateFrameLoadState)
     153{
     154    for (unsigned i = 0; i < 25; ++i) {
     155        done = false;
     156        auto context = adoptWK(WKContextCreateWithConfiguration(nullptr));
     157        PlatformWebView webView(context.get());
     158
     159        WKPageUIClientV5 uiClient;
     160        memset(&uiClient, 0, sizeof(uiClient));
     161        uiClient.base.version = 5;
     162        uiClient.createNewPage = checkFrameLoadStateAndCreateNewPage;
     163        WKPageSetPageUIClient(webView.page(), &uiClient.base);
     164
     165        auto htmlString = Util::toWK("<script>open('about:blank', '_blank')</script>");
     166        WKPageLoadHTMLString(webView.page(), htmlString.get(), nullptr);
     167        Util::run(&done);
     168        openedWebView = nil;
     169    }
    140170}
    141171
Note: See TracChangeset for help on using the changeset viewer.