Changeset 236227 in webkit


Ignore:
Timestamp:
Sep 19, 2018 2:58:45 PM (6 years ago)
Author:
Chris Dumez
Message:

Crash under WebPageProxy::decidePolicyForNavigationAction()
​https://bugs.webkit.org/show_bug.cgi?id=189763
<rdar://problem/44597111>

Reviewed by Alex Christensen.

Update WebNavigationState::navigation() / WebNavigationState::takeNavigation()
to return a pointer instead of a reference as we have evidence that they can
return null. I kept the debug assertions to try and catch the cases where we
return null but at least we stop crashing in release builds.

  • UIProcess/WebNavigationState.cpp:

(WebKit::WebNavigationState::navigation):
(WebKit::WebNavigationState::takeNavigation):

  • UIProcess/WebNavigationState.h:
  • UIProcess/WebPageProxy.cpp:

(WebKit::WebPageProxy::didStartProvisionalLoadForFrame):
(WebKit::WebPageProxy::didReceiveServerRedirectForProvisionalLoadForFrame):
(WebKit::WebPageProxy::didCommitLoadForFrame):
(WebKit::WebPageProxy::didFinishDocumentLoadForFrame):
(WebKit::WebPageProxy::didFinishLoadForFrame):
(WebKit::WebPageProxy::didFailLoadForFrame):
(WebKit::WebPageProxy::didSameDocumentNavigationForFrame):
(WebKit::WebPageProxy::decidePolicyForNavigationAction):
(WebKit::WebPageProxy::decidePolicyForResponse):

Location:
trunk/Source/WebKit
Files:
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r236226 r236227  
     12018-09-19  Chris Dumez  <cdumez@apple.com>
     2
     3        Crash under WebPageProxy::decidePolicyForNavigationAction()
     4        https://bugs.webkit.org/show_bug.cgi?id=189763
     5        <rdar://problem/44597111>
     6
     7        Reviewed by Alex Christensen.
     8
     9        Update WebNavigationState::navigation() / WebNavigationState::takeNavigation()
     10        to return a pointer instead of a reference as we have evidence that they can
     11        return null. I kept the debug assertions to try and catch the cases where we
     12        return null but at least we stop crashing in release builds.
     13
     14        * UIProcess/WebNavigationState.cpp:
     15        (WebKit::WebNavigationState::navigation):
     16        (WebKit::WebNavigationState::takeNavigation):
     17        * UIProcess/WebNavigationState.h:
     18        * UIProcess/WebPageProxy.cpp:
     19        (WebKit::WebPageProxy::didStartProvisionalLoadForFrame):
     20        (WebKit::WebPageProxy::didReceiveServerRedirectForProvisionalLoadForFrame):
     21        (WebKit::WebPageProxy::didCommitLoadForFrame):
     22        (WebKit::WebPageProxy::didFinishDocumentLoadForFrame):
     23        (WebKit::WebPageProxy::didFinishLoadForFrame):
     24        (WebKit::WebPageProxy::didFailLoadForFrame):
     25        (WebKit::WebPageProxy::didSameDocumentNavigationForFrame):
     26        (WebKit::WebPageProxy::decidePolicyForNavigationAction):
     27        (WebKit::WebPageProxy::decidePolicyForResponse):
     28
    1292018-09-19  Chris Dumez  <cdumez@apple.com>
    230

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

    r235265 r236227  
    7878}
    7979
    80 API::Navigation& WebNavigationState::navigation(uint64_t navigationID)
     80API::Navigation* WebNavigationState::navigation(uint64_t navigationID)
    8181{
    8282    ASSERT(navigationID);
    8383    ASSERT(m_navigations.contains(navigationID));
    8484
    85     return *m_navigations.get(navigationID);
     85    return m_navigations.get(navigationID);
    8686}
    8787
    88 Ref<API::Navigation> WebNavigationState::takeNavigation(uint64_t navigationID)
     88RefPtr<API::Navigation> WebNavigationState::takeNavigation(uint64_t navigationID)
    8989{
    9090    ASSERT(navigationID);
    9191    ASSERT(m_navigations.contains(navigationID));
    9292   
    93     return m_navigations.take(navigationID).releaseNonNull();
     93    return m_navigations.take(navigationID);
    9494}
    9595

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

    r230834 r236227  
    5454    Ref<API::Navigation> createLoadDataNavigation();
    5555
    56     API::Navigation& navigation(uint64_t navigationID);
    57     Ref<API::Navigation> takeNavigation(uint64_t navigationID);
     56    API::Navigation* navigation(uint64_t navigationID);
     57    RefPtr<API::Navigation> takeNavigation(uint64_t navigationID);
    5858    void didDestroyNavigation(uint64_t navigationID);
    5959    void clearAllNavigations();

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

    r236157 r236227  
    34613461    RefPtr<API::Navigation> navigation;
    34623462    if (frame->isMainFrame() && navigationID)
    3463         navigation = &navigationState().navigation(navigationID);
     3463        navigation = navigationState().navigation(navigationID);
    34643464
    34653465    // If this seemingly new load is actually continuing a server redirect for a previous navigation in a new process,
    … …  
    35083508    RefPtr<API::Navigation> navigation;
    35093509    if (navigationID) {
    3510         navigation = &navigationState().navigation(navigationID);
     3510        navigation = navigationState().navigation(navigationID);
    35113511        navigation->appendRedirectionURL(request.url());
    35123512    }
    … …  
    36353635    RefPtr<API::Navigation> navigation;
    36363636    if (frame->isMainFrame() && navigationID)
    3637         navigation = &navigationState().navigation(navigationID);
     3637        navigation = navigationState().navigation(navigationID);
    36383638
    36393639    m_hasCommittedAnyProvisionalLoads = true;
    … …  
    37273727    RefPtr<API::Navigation> navigation;
    37283728    if (frame->isMainFrame() && navigationID)
    3729         navigation = &navigationState().navigation(navigationID);
     3729        navigation = navigationState().navigation(navigationID);
    37303730
    37313731    if (frame->isMainFrame())
    … …  
    37453745    RefPtr<API::Navigation> navigation;
    37463746    if (frame->isMainFrame() && navigationID)
    3747         navigation = &navigationState().navigation(navigationID);
     3747        navigation = navigationState().navigation(navigationID);
    37483748
    37493749    auto transaction = m_pageLoadState.transaction();
    … …  
    37883788    RefPtr<API::Navigation> navigation;
    37893789    if (frame->isMainFrame() && navigationID)
    3790         navigation = &navigationState().navigation(navigationID);
     3790        navigation = navigationState().navigation(navigationID);
    37913791
    37923792    clearLoadDependentCallbacks();
    … …  
    38293829    RefPtr<API::Navigation> navigation;
    38303830    if (frame->isMainFrame() && navigationID)
    3831         navigation = &navigationState().navigation(navigationID);
     3831        navigation = navigationState().navigation(navigationID);
    38323832
    38333833    auto transaction = m_pageLoadState.transaction();
    … …  
    40034003    RefPtr<API::Navigation> navigation;
    40044004    if (navigationID)
    4005         navigation = makeRef(m_navigationState->navigation(navigationID));
     4005        navigation = m_navigationState->navigation(navigationID);
    40064006
    40074007    if (auto targetBackForwardItemIdentifier = navigationActionData.targetBackForwardItemIdentifier) {
    … …  
    40354035#if ENABLE(CONTENT_FILTERING)
    40364036    if (frame->didHandleContentFilterUnblockNavigation(request))
    4037         return receivedPolicyDecision(PolicyAction::Ignore, &m_navigationState->navigation(newNavigationID), std::nullopt, WTFMove(sender));
     4037        return receivedPolicyDecision(PolicyAction::Ignore, m_navigationState->navigation(newNavigationID), std::nullopt, WTFMove(sender));
    40384038#else
    40394039    UNUSED_PARAM(newNavigationID);
    … …  
    41294129    MESSAGE_CHECK_URL(response.url());
    41304130
    4131     RefPtr<API::Navigation> navigation = navigationID ? &m_navigationState->navigation(navigationID) : nullptr;
     4131    RefPtr<API::Navigation> navigation = navigationID ? m_navigationState->navigation(navigationID) : nullptr;
    41324132    auto listener = makeRef(frame->setUpPolicyListenerProxy([this, protectedThis = makeRef(*this), frameID, listenerID, navigation = WTFMove(navigation)] (WebCore::PolicyAction policyAction, API::WebsitePolicies*, ProcessSwapRequestedByClient processSwapRequestedByClient, Vector<Ref<SafeBrowsingResult>>&& safeBrowsingResults) mutable {
    41334133        // FIXME: Assert the API::WebsitePolicies* is nullptr here once clients of WKFramePolicyListenerUseWithPolicies go away.
Note: See TracChangeset for help on using the changeset viewer.