Changeset 236471 in webkit


Ignore:
Timestamp:
Sep 25, 2018 12:41:41 PM (6 years ago)
Author:
Chris Dumez
Message:

UIProcess should process incoming sync IPC from WebProcess when waiting for a sync IPC reply from it
​https://bugs.webkit.org/show_bug.cgi?id=189927

Reviewed by Alex Christensen.

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 bugs caused by re-entering WebCore at unsafe times.

The reason the UIProcess previously did not do out of order sync IPC process was to avoid processing
a synchronous policy decision IPC for a frameID it did not know about yet, due to the DidCreateMainFrame /
DidCreateSubframe IPC messages being asynchronous. To address this issue, the decidePolicyForNavigationActionSync
IPC handler now calls didCreateMainFrame() / didCreateSubframe() as needed if it does not know about
the frame yet. Note that synchronous policy decisions are rare and are currently only needed by initial
about:blank and fragment navigations.

  • UIProcess/WebPageProxy.cpp:

(WebKit::WebPageProxy::didCreateMainFrame):
(WebKit::WebPageProxy::didCreateSubframe):
(WebKit::WebPageProxy::decidePolicyForNavigationActionAsync):
(WebKit::WebPageProxy::decidePolicyForNavigationAction):
(WebKit::WebPageProxy::decidePolicyForNavigationActionSync):

  • UIProcess/WebPageProxy.h:
  • UIProcess/WebPageProxy.messages.in:
  • WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:

(WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):

  • WebProcess/WebProcess.cpp:

(WebKit::WebProcess::initializeConnection):

Location:
trunk/Source/WebKit
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r236464 r236471  
     12018-09-25  Chris Dumez  <cdumez@apple.com>
     2
     3        UIProcess should process incoming sync IPC from WebProcess when waiting for a sync IPC reply from it
     4        https://bugs.webkit.org/show_bug.cgi?id=189927
     5
     6        Reviewed by Alex Christensen.
     7
     8        UIProcess should process incoming sync IPC from WebProcess when waiting for a sync IPC reply from it
     9        in order to avoid deadlocks. This is not an issue currently because the WebProcess does process
     10        incoming sync IPC when waiting for a sync IPC reply. However, we plan to change this in the future
     11        in order to avoid bugs caused by re-entering WebCore at unsafe times.
     12
     13        The reason the UIProcess previously did not do out of order sync IPC process was to avoid processing
     14        a synchronous policy decision IPC for a frameID it did not know about yet, due to the DidCreateMainFrame /
     15        DidCreateSubframe IPC messages being asynchronous. To address this issue, the decidePolicyForNavigationActionSync
     16        IPC handler now calls didCreateMainFrame() / didCreateSubframe() as needed if it does not know about
     17        the frame yet. Note that synchronous policy decisions are rare and are currently only needed by initial
     18        about:blank and fragment navigations.
     19
     20        * UIProcess/WebPageProxy.cpp:
     21        (WebKit::WebPageProxy::didCreateMainFrame):
     22        (WebKit::WebPageProxy::didCreateSubframe):
     23        (WebKit::WebPageProxy::decidePolicyForNavigationActionAsync):
     24        (WebKit::WebPageProxy::decidePolicyForNavigationAction):
     25        (WebKit::WebPageProxy::decidePolicyForNavigationActionSync):
     26        * UIProcess/WebPageProxy.h:
     27        * UIProcess/WebPageProxy.messages.in:
     28        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
     29        (WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
     30        * WebProcess/WebProcess.cpp:
     31        (WebKit::WebProcess::initializeConnection):
     32
    1332018-09-25  Chris Dumez  <cdumez@apple.com>
    234

  • trunk/Source/WebKit/UIProcess/WebPageProxy.cpp

    r236257 r236471  
    33583358void WebPageProxy::didCreateMainFrame(uint64_t frameID)
    33593359{
     3360    if (m_mainFrame && m_mainFrame->frameID() == frameID)
     3361        return;
     3362
    33603363    PageClientProtector protector(pageClient());
    33613364
    … …  
    33803383
    33813384    MESSAGE_CHECK(m_mainFrame);
     3385
     3386    if (m_process->webFrame(frameID))
     3387        return;
     3388
    33823389    MESSAGE_CHECK(m_process->canCreateFrame(frameID));
    33833390   
    … …  
    39974004void WebPageProxy::decidePolicyForNavigationActionAsync(uint64_t frameID, const WebCore::SecurityOriginData& frameSecurityOrigin, uint64_t navigationID, NavigationActionData&& navigationActionData, const FrameInfoData& frameInfoData, uint64_t originatingPageID, const WebCore::ResourceRequest& originalRequest, WebCore::ResourceRequest&& request, WebCore::ResourceResponse&& redirectResponse, const UserData& userData, WebCore::ShouldSkipSafeBrowsingCheck shouldSkipSafeBrowsingCheck, uint64_t listenerID)
    39984005{
    3999     decidePolicyForNavigationAction(frameID, frameSecurityOrigin, navigationID, WTFMove(navigationActionData), frameInfoData, originatingPageID, originalRequest, WTFMove(request), WTFMove(redirectResponse), userData, shouldSkipSafeBrowsingCheck, PolicyDecisionSender::create([this, protectedThis = makeRef(*this), frameID, listenerID] (auto... args) {
     4006    auto* frame = m_process->webFrame(frameID);
     4007    MESSAGE_CHECK(frame);
     4008
     4009    decidePolicyForNavigationAction(*frame, frameSecurityOrigin, navigationID, WTFMove(navigationActionData), frameInfoData, originatingPageID, originalRequest, WTFMove(request), WTFMove(redirectResponse), userData, shouldSkipSafeBrowsingCheck, PolicyDecisionSender::create([this, protectedThis = makeRef(*this), frameID, listenerID] (auto... args) {
    40004010        m_process->send(Messages::WebPage::DidReceivePolicyDecision(frameID, listenerID, args...), m_pageID);
    40014011    }));
    40024012}
    40034013
    4004 void WebPageProxy::decidePolicyForNavigationAction(uint64_t frameID, const WebCore::SecurityOriginData& frameSecurityOrigin, uint64_t navigationID, NavigationActionData&& navigationActionData, const FrameInfoData& originatingFrameInfoData, uint64_t originatingPageID, const WebCore::ResourceRequest& originalRequest, WebCore::ResourceRequest&& request, WebCore::ResourceResponse&& redirectResponse, const UserData& userData, WebCore::ShouldSkipSafeBrowsingCheck shouldSkipSafeBrowsingCheck, Ref<PolicyDecisionSender>&& sender)
     4014void WebPageProxy::decidePolicyForNavigationAction(WebFrameProxy& frame, const WebCore::SecurityOriginData& frameSecurityOrigin, uint64_t navigationID, NavigationActionData&& navigationActionData, const FrameInfoData& originatingFrameInfoData, uint64_t originatingPageID, const WebCore::ResourceRequest& originalRequest, WebCore::ResourceRequest&& request, WebCore::ResourceResponse&& redirectResponse, const UserData& userData, WebCore::ShouldSkipSafeBrowsingCheck shouldSkipSafeBrowsingCheck, Ref<PolicyDecisionSender>&& sender)
    40054015{
    40064016    LOG(Loading, "WebPageProxy::decidePolicyForNavigationAction - Original URL %s, current target URL %s", originalRequest.url().string().utf8().data(), request.url().string().utf8().data());
    … …  
    40144024        m_pageLoadState.clearPendingAPIRequestURL(transaction);
    40154025
    4016     WebFrameProxy* frame = m_process->webFrame(frameID);
    4017     MESSAGE_CHECK(frame);
    40184026    MESSAGE_CHECK_URL(request.url());
    40194027    MESSAGE_CHECK_URL(originalRequest.url());
    … …  
    40524060
    40534061#if ENABLE(CONTENT_FILTERING)
    4054     if (frame->didHandleContentFilterUnblockNavigation(request))
     4062    if (frame.didHandleContentFilterUnblockNavigation(request))
    40554063        return receivedPolicyDecision(PolicyAction::Ignore, m_navigationState->navigation(newNavigationID), std::nullopt, WTFMove(sender));
    40564064#else
    … …  
    40584066#endif
    40594067
    4060     auto listener = makeRef(frame->setUpPolicyListenerProxy([this, protectedThis = makeRef(*this), frame = makeRef(*frame), sender = WTFMove(sender), navigation] (WebCore::PolicyAction policyAction, API::WebsitePolicies* policies, ProcessSwapRequestedByClient processSwapRequestedByClient, Vector<Ref<SafeBrowsingResult>>&&) mutable {
     4068    auto listener = makeRef(frame.setUpPolicyListenerProxy([this, protectedThis = makeRef(*this), frame = makeRef(frame), sender = WTFMove(sender), navigation] (WebCore::PolicyAction policyAction, API::WebsitePolicies* policies, ProcessSwapRequestedByClient processSwapRequestedByClient, Vector<Ref<SafeBrowsingResult>>&&) mutable {
    40614069        // FIXME: do something with the SafeBrowsingResults.
    40624070        receivedNavigationPolicyDecision(policyAction, navigation.get(), processSwapRequestedByClient, frame, policies, WTFMove(sender));
    … …  
    40654073        beginSafeBrowsingCheck(request.url(), listener);
    40664074
    4067     API::Navigation* mainFrameNavigation = frame->isMainFrame() ? navigation.get() : nullptr;
     4075    API::Navigation* mainFrameNavigation = frame.isMainFrame() ? navigation.get() : nullptr;
    40684076    WebFrameProxy* originatingFrame = m_process->webFrame(originatingFrameInfoData.frameID);
    40694077
    40704078    if (auto* resourceLoadStatisticsStore = websiteDataStore().resourceLoadStatistics())
    4071         resourceLoadStatisticsStore->logFrameNavigation(*frame, URL(URL(), m_pageLoadState.url()), request, redirectResponse.url());
     4079        resourceLoadStatisticsStore->logFrameNavigation(frame, URL(URL(), m_pageLoadState.url()), request, redirectResponse.url());
    40724080
    40734081    if (m_policyClient)
    4074         m_policyClient->decidePolicyForNavigationAction(*this, frame, WTFMove(navigationActionData), originatingFrame, originalRequest, WTFMove(request), WTFMove(listener), m_process->transformHandlesToObjects(userData.object()).get());
     4082        m_policyClient->decidePolicyForNavigationAction(*this, &frame, WTFMove(navigationActionData), originatingFrame, originalRequest, WTFMove(request), WTFMove(listener), m_process->transformHandlesToObjects(userData.object()).get());
    40754083    else {
    4076         auto destinationFrameInfo = API::FrameInfo::create(*frame, frameSecurityOrigin.securityOrigin());
     4084        auto destinationFrameInfo = API::FrameInfo::create(frame, frameSecurityOrigin.securityOrigin());
    40774085        RefPtr<API::FrameInfo> sourceFrameInfo;
    4078         if (!fromAPI && originatingFrame == frame)
     4086        if (!fromAPI && originatingFrame == &frame)
    40794087            sourceFrameInfo = destinationFrameInfo.copyRef();
    40804088        else if (!fromAPI)
    … …  
    40924100}
    40934101
    4094 void WebPageProxy::decidePolicyForNavigationActionSync(uint64_t frameID, const WebCore::SecurityOriginData& frameSecurityOrigin, uint64_t navigationID, NavigationActionData&& navigationActionData, const FrameInfoData& frameInfoData, uint64_t originatingPageID, const WebCore::ResourceRequest& originalRequest, WebCore::ResourceRequest&& request, WebCore::ResourceResponse&& redirectResponse, const UserData& userData, WebCore::ShouldSkipSafeBrowsingCheck shouldSkipSafeBrowsingCheck, Messages::WebPageProxy::DecidePolicyForNavigationActionSync::DelayedReply&& reply)
     4102void WebPageProxy::decidePolicyForNavigationActionSync(uint64_t frameID, bool isMainFrame, const WebCore::SecurityOriginData& frameSecurityOrigin, uint64_t navigationID, NavigationActionData&& navigationActionData, const FrameInfoData& frameInfoData, uint64_t originatingPageID, const WebCore::ResourceRequest& originalRequest, WebCore::ResourceRequest&& request, WebCore::ResourceResponse&& redirectResponse, const UserData& userData, WebCore::ShouldSkipSafeBrowsingCheck shouldSkipSafeBrowsingCheck, Messages::WebPageProxy::DecidePolicyForNavigationActionSync::DelayedReply&& reply)
    40954103{
    40964104    auto sender = PolicyDecisionSender::create(WTFMove(reply));
     4105
     4106    auto* frame = m_process->webFrame(frameID);
     4107    if (!frame) {
     4108        // This synchronous IPC message was processed before the asynchronous DidCreateMainFrame / DidCreateSubframe one so we do not know about this frameID yet.
     4109        if (isMainFrame)
     4110            didCreateMainFrame(frameID);
     4111        else
     4112            didCreateSubframe(frameID);
     4113
     4114        frame = m_process->webFrame(frameID);
     4115        RELEASE_ASSERT(frame);
     4116    }
    40974117   
    4098     decidePolicyForNavigationAction(frameID, frameSecurityOrigin, navigationID, WTFMove(navigationActionData), frameInfoData, originatingPageID, originalRequest, WTFMove(request), WTFMove(redirectResponse), userData, shouldSkipSafeBrowsingCheck, sender.copyRef());
     4118    decidePolicyForNavigationAction(*frame, frameSecurityOrigin, navigationID, WTFMove(navigationActionData), frameInfoData, originatingPageID, originalRequest, WTFMove(request), WTFMove(redirectResponse), userData, shouldSkipSafeBrowsingCheck, sender.copyRef());
    40994119
    41004120    // If the client did not respond synchronously, proceed with the load.

  • trunk/Source/WebKit/UIProcess/WebPageProxy.h

    r236257 r236471  
    14351435    void didDestroyNavigation(uint64_t navigationID);
    14361436
    1437     void decidePolicyForNavigationAction(uint64_t frameID, const WebCore::SecurityOriginData& frameSecurityOrigin, uint64_t navigationID, NavigationActionData&&, const FrameInfoData&, uint64_t originatingPageID, const WebCore::ResourceRequest& originalRequest, WebCore::ResourceRequest&&, WebCore::ResourceResponse&& redirectResponse, const UserData&, WebCore::ShouldSkipSafeBrowsingCheck, Ref<PolicyDecisionSender>&&);
     1437    void decidePolicyForNavigationAction(WebFrameProxy&, const WebCore::SecurityOriginData& frameSecurityOrigin, uint64_t navigationID, NavigationActionData&&, const FrameInfoData&, uint64_t originatingPageID, const WebCore::ResourceRequest& originalRequest, WebCore::ResourceRequest&&, WebCore::ResourceResponse&& redirectResponse, const UserData&, WebCore::ShouldSkipSafeBrowsingCheck, Ref<PolicyDecisionSender>&&);
    14381438    void decidePolicyForNavigationActionAsync(uint64_t frameID, const WebCore::SecurityOriginData& frameSecurityOrigin, uint64_t navigationID, NavigationActionData&&, const FrameInfoData&, uint64_t originatingPageID, const WebCore::ResourceRequest& originalRequest, WebCore::ResourceRequest&&, WebCore::ResourceResponse&& redirectResponse, const UserData&, WebCore::ShouldSkipSafeBrowsingCheck, uint64_t listenerID);
    1439     void decidePolicyForNavigationActionSync(uint64_t frameID, const WebCore::SecurityOriginData& frameSecurityOrigin, uint64_t navigationID, NavigationActionData&&, const FrameInfoData&, uint64_t originatingPageID, const WebCore::ResourceRequest& originalRequest, WebCore::ResourceRequest&&, WebCore::ResourceResponse&& redirectResponse, const UserData&, WebCore::ShouldSkipSafeBrowsingCheck, Messages::WebPageProxy::DecidePolicyForNavigationActionSync::DelayedReply&&);
     1439    void decidePolicyForNavigationActionSync(uint64_t frameID, bool isMainFrame, const WebCore::SecurityOriginData& frameSecurityOrigin, uint64_t navigationID, NavigationActionData&&, const FrameInfoData&, uint64_t originatingPageID, const WebCore::ResourceRequest& originalRequest, WebCore::ResourceRequest&&, WebCore::ResourceResponse&& redirectResponse, const UserData&, WebCore::ShouldSkipSafeBrowsingCheck, Messages::WebPageProxy::DecidePolicyForNavigationActionSync::DelayedReply&&);
    14401440    void decidePolicyForNewWindowAction(uint64_t frameID, const WebCore::SecurityOriginData& frameSecurityOrigin, NavigationActionData&&, WebCore::ResourceRequest&&, const String& frameName, uint64_t listenerID, const UserData&);
    14411441    void decidePolicyForResponse(uint64_t frameID, const WebCore::SecurityOriginData& frameSecurityOrigin, uint64_t navigationID, const WebCore::ResourceResponse&, const WebCore::ResourceRequest&, bool canShowMIMEType, uint64_t listenerID, const UserData&);

  • trunk/Source/WebKit/UIProcess/WebPageProxy.messages.in

    r236257 r236471  
    109109    DecidePolicyForResponse(uint64_t frameID, struct WebCore::SecurityOriginData frameSecurityOrigin, uint64_t navigationID, WebCore::ResourceResponse response, WebCore::ResourceRequest request, bool canShowMIMEType, uint64_t listenerID, WebKit::UserData userData)
    110110    DecidePolicyForNavigationActionAsync(uint64_t frameID, struct WebCore::SecurityOriginData frameSecurityOrigin, uint64_t navigationID, struct WebKit::NavigationActionData navigationActionData, struct WebKit::FrameInfoData originatingFrameInfoData, uint64_t originatingPageID, WebCore::ResourceRequest originalRequest, WebCore::ResourceRequest request, WebCore::ResourceResponse redirectResponse, WebKit::UserData userData, enum WebCore::ShouldSkipSafeBrowsingCheck shouldSkipSafeBrowsingCheck, uint64_t listenerID)
    111     DecidePolicyForNavigationActionSync(uint64_t frameID, struct WebCore::SecurityOriginData frameSecurityOrigin, uint64_t navigationID, struct WebKit::NavigationActionData navigationActionData, struct WebKit::FrameInfoData originatingFrameInfoData, uint64_t originatingPageID, WebCore::ResourceRequest originalRequest, WebCore::ResourceRequest request, WebCore::ResourceResponse redirectResponse, WebKit::UserData userData, enum WebCore::ShouldSkipSafeBrowsingCheck shouldSkipSafeBrowsingCheck) -> (enum WebCore::PolicyAction policyAction, uint64_t newNavigationID, WebKit::DownloadID downloadID, std::optional<WebKit::WebsitePoliciesData> websitePolicies) Delayed
     111    DecidePolicyForNavigationActionSync(uint64_t frameID, bool isMainFrame, struct WebCore::SecurityOriginData frameSecurityOrigin, uint64_t navigationID, struct WebKit::NavigationActionData navigationActionData, struct WebKit::FrameInfoData originatingFrameInfoData, uint64_t originatingPageID, WebCore::ResourceRequest originalRequest, WebCore::ResourceRequest request, WebCore::ResourceResponse redirectResponse, WebKit::UserData userData, enum WebCore::ShouldSkipSafeBrowsingCheck shouldSkipSafeBrowsingCheck) -> (enum WebCore::PolicyAction policyAction, uint64_t newNavigationID, WebKit::DownloadID downloadID, std::optional<WebKit::WebsitePoliciesData> websitePolicies) Delayed
    112112    DecidePolicyForNewWindowAction(uint64_t frameID, struct WebCore::SecurityOriginData frameSecurityOrigin, struct WebKit::NavigationActionData navigationActionData, WebCore::ResourceRequest request, String frameName, uint64_t listenerID, WebKit::UserData userData)
    113113    UnableToImplementPolicy(uint64_t frameID, WebCore::ResourceError error, WebKit::UserData userData)

  • trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp

    r236069 r236471  
    895895        std::optional<WebsitePoliciesData> websitePolicies;
    896896
    897         if (!webPage->sendSync(Messages::WebPageProxy::DecidePolicyForNavigationActionSync(m_frame->frameID(), SecurityOriginData::fromFrame(coreFrame), documentLoader->navigationID(), navigationActionData, originatingFrameInfoData, originatingPageID, navigationAction.resourceRequest(), request, redirectResponse, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get()), shouldSkipSafeBrowsingCheck), Messages::WebPageProxy::DecidePolicyForNavigationActionSync::Reply(policyAction, newNavigationID, downloadID, websitePolicies))) {
     897        if (!webPage->sendSync(Messages::WebPageProxy::DecidePolicyForNavigationActionSync(m_frame->frameID(), m_frame->isMainFrame(), SecurityOriginData::fromFrame(coreFrame), documentLoader->navigationID(), navigationActionData, originatingFrameInfoData, originatingPageID, navigationAction.resourceRequest(), request, redirectResponse, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get()), shouldSkipSafeBrowsingCheck), Messages::WebPageProxy::DecidePolicyForNavigationActionSync::Reply(policyAction, newNavigationID, downloadID, websitePolicies))) {
    898898            m_frame->didReceivePolicyDecision(listenerID, PolicyAction::Ignore, 0, { }, { });
    899899            return;

  • trunk/Source/WebKit/WebProcess/WebProcess.cpp

    r236035 r236471  
    252252
    253253    m_webConnection = WebConnectionToUIProcess::create(this);
    254 
    255     // In order to ensure that the asynchronous messages that are used for notifying the UI process
    256     // about when WebFrame objects come and go are always delivered before the synchronous policy messages,
    257     // use this flag to force synchronous messages to be treated as asynchronous messages in the UI process
    258     // unless when doing so would lead to a deadlock.
    259     connection->setOnlySendMessagesAsDispatchWhenWaitingForSyncReplyWhenProcessingSuchAMessage(true);
    260254}
    261255
Note: See TracChangeset for help on using the changeset viewer.