Attachment 350145 Details for Bug 189763 – Patch

[patch] Patch

bug-189763-20180919131936.patch (text/plain), 9.77 KB, created by Chris Dumez on 2018-09-19 13:19:37 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-09-19 13:19:37 PDT

Size: 9.77 KB

patch

obsolete

>Subversion Revision: 236210
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index b58444249c044ca9dcbef2f0f4634dad583c2c24..c55af649f9a9337b88b785d0716ecfef5d1c91dc 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,31 @@
>+2018-09-19  Chris Dumez  <cdumez@apple.com>
>+
>+        Crash under WebPageProxy::decidePolicyForNavigationAction()
>+        https://bugs.webkit.org/show_bug.cgi?id=189763
>+        <rdar://problem/44597111>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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):
>+
> 2018-09-18  Brent Fulgham  <bfulgham@apple.com>
> 
>         [iOS] Allow WebContent process to check the "Protocol Characteristics" of files to which it has access
>diff --git a/Source/WebKit/UIProcess/WebNavigationState.cpp b/Source/WebKit/UIProcess/WebNavigationState.cpp
>index 9ffabb2a842a09d8e6f273cc766f4ca2120d0fe8..5e3f2414cff89cc5258c35d5f0ef433218ee7180 100644
>--- a/Source/WebKit/UIProcess/WebNavigationState.cpp
>+++ b/Source/WebKit/UIProcess/WebNavigationState.cpp
>@@ -77,20 +77,20 @@ Ref<API::Navigation> WebNavigationState::createLoadDataNavigation()
>     return navigation;
> }
> 
>-API::Navigation& WebNavigationState::navigation(uint64_t navigationID)
>+API::Navigation* WebNavigationState::navigation(uint64_t navigationID)
> {
>     ASSERT(navigationID);
>     ASSERT(m_navigations.contains(navigationID));
> 
>-    return *m_navigations.get(navigationID);
>+    return m_navigations.get(navigationID);
> }
> 
>-Ref<API::Navigation> WebNavigationState::takeNavigation(uint64_t navigationID)
>+RefPtr<API::Navigation> WebNavigationState::takeNavigation(uint64_t navigationID)
> {
>     ASSERT(navigationID);
>     ASSERT(m_navigations.contains(navigationID));
>     
>-    return m_navigations.take(navigationID).releaseNonNull();
>+    return m_navigations.take(navigationID);
> }
> 
> void WebNavigationState::didDestroyNavigation(uint64_t navigationID)
>diff --git a/Source/WebKit/UIProcess/WebNavigationState.h b/Source/WebKit/UIProcess/WebNavigationState.h
>index 1366e3cc8dc43cdc69b2afba120fc016bf20ad9c..dffe9e030c055d9d0a38202f983b8694140a3e76 100644
>--- a/Source/WebKit/UIProcess/WebNavigationState.h
>+++ b/Source/WebKit/UIProcess/WebNavigationState.h
>@@ -53,8 +53,8 @@ public:
>     Ref<API::Navigation> createReloadNavigation();
>     Ref<API::Navigation> createLoadDataNavigation();
> 
>-    API::Navigation& navigation(uint64_t navigationID);
>-    Ref<API::Navigation> takeNavigation(uint64_t navigationID);
>+    API::Navigation* navigation(uint64_t navigationID);
>+    RefPtr<API::Navigation> takeNavigation(uint64_t navigationID);
>     void didDestroyNavigation(uint64_t navigationID);
>     void clearAllNavigations();
> 
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index e750d80130397f5fdad507d7ed3a157481208da6..1afd58c967f313216e5cab3bb4277cc0744e014d 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -3460,7 +3460,7 @@ void WebPageProxy::didStartProvisionalLoadForFrame(uint64_t frameID, uint64_t na
>     // FIXME: We should message check that navigationID is not zero here, but it's currently zero for some navigations through the page cache.
>     RefPtr<API::Navigation> navigation;
>     if (frame->isMainFrame() && navigationID)
>-        navigation = &navigationState().navigation(navigationID);
>+        navigation = navigationState().navigation(navigationID);
> 
>     // If this seemingly new load is actually continuing a server redirect for a previous navigation in a new process,
>     // then we ignore this notification.
>@@ -3507,7 +3507,7 @@ void WebPageProxy::didReceiveServerRedirectForProvisionalLoadForFrame(uint64_t f
>     // FIXME: We should message check that navigationID is not zero here, but it's currently zero for some navigations through the page cache.
>     RefPtr<API::Navigation> navigation;
>     if (navigationID) {
>-        navigation = &navigationState().navigation(navigationID);
>+        navigation = navigationState().navigation(navigationID);
>         navigation->appendRedirectionURL(request.url());
>     }
> 
>@@ -3634,7 +3634,7 @@ void WebPageProxy::didCommitLoadForFrame(uint64_t frameID, uint64_t navigationID
>     // FIXME: We should message check that navigationID is not zero here, but it's currently zero for some navigations through the page cache.
>     RefPtr<API::Navigation> navigation;
>     if (frame->isMainFrame() && navigationID)
>-        navigation = &navigationState().navigation(navigationID);
>+        navigation = navigationState().navigation(navigationID);
> 
>     m_hasCommittedAnyProvisionalLoads = true;
>     m_process->didCommitProvisionalLoad();
>@@ -3726,7 +3726,7 @@ void WebPageProxy::didFinishDocumentLoadForFrame(uint64_t frameID, uint64_t navi
>     // FIXME: We should message check that navigationID is not zero here, but it's currently zero for some navigations through the page cache.
>     RefPtr<API::Navigation> navigation;
>     if (frame->isMainFrame() && navigationID)
>-        navigation = &navigationState().navigation(navigationID);
>+        navigation = navigationState().navigation(navigationID);
> 
>     if (frame->isMainFrame())
>         m_navigationClient->didFinishDocumentLoad(*this, navigation.get(), m_process->transformHandlesToObjects(userData.object()).get());
>@@ -3744,7 +3744,7 @@ void WebPageProxy::didFinishLoadForFrame(uint64_t frameID, uint64_t navigationID
>     // FIXME: We should message check that navigationID is not zero here, but it's currently zero for some navigations through the page cache.
>     RefPtr<API::Navigation> navigation;
>     if (frame->isMainFrame() && navigationID)
>-        navigation = &navigationState().navigation(navigationID);
>+        navigation = navigationState().navigation(navigationID);
> 
>     auto transaction = m_pageLoadState.transaction();
> 
>@@ -3787,7 +3787,7 @@ void WebPageProxy::didFailLoadForFrame(uint64_t frameID, uint64_t navigationID,
>     // FIXME: We should message check that navigationID is not zero here, but it's currently zero for some navigations through the page cache.
>     RefPtr<API::Navigation> navigation;
>     if (frame->isMainFrame() && navigationID)
>-        navigation = &navigationState().navigation(navigationID);
>+        navigation = navigationState().navigation(navigationID);
> 
>     clearLoadDependentCallbacks();
> 
>@@ -3828,7 +3828,7 @@ void WebPageProxy::didSameDocumentNavigationForFrame(uint64_t frameID, uint64_t
>     // FIXME: We should message check that navigationID is not zero here, but it's currently zero for some navigations through the page cache.
>     RefPtr<API::Navigation> navigation;
>     if (frame->isMainFrame() && navigationID)
>-        navigation = &navigationState().navigation(navigationID);
>+        navigation = navigationState().navigation(navigationID);
> 
>     auto transaction = m_pageLoadState.transaction();
> 
>@@ -4002,7 +4002,7 @@ void WebPageProxy::decidePolicyForNavigationAction(uint64_t frameID, const WebCo
> 
>     RefPtr<API::Navigation> navigation;
>     if (navigationID)
>-        navigation = makeRef(m_navigationState->navigation(navigationID));
>+        navigation = m_navigationState->navigation(navigationID);
> 
>     if (auto targetBackForwardItemIdentifier = navigationActionData.targetBackForwardItemIdentifier) {
>         if (auto* item = m_backForwardList->itemForID(*navigationActionData.targetBackForwardItemIdentifier)) {
>@@ -4034,7 +4034,7 @@ void WebPageProxy::decidePolicyForNavigationAction(uint64_t frameID, const WebCo
> 
> #if ENABLE(CONTENT_FILTERING)
>     if (frame->didHandleContentFilterUnblockNavigation(request))
>-        return receivedPolicyDecision(PolicyAction::Ignore, &m_navigationState->navigation(newNavigationID), std::nullopt, WTFMove(sender));
>+        return receivedPolicyDecision(PolicyAction::Ignore, m_navigationState->navigation(newNavigationID), std::nullopt, WTFMove(sender));
> #else
>     UNUSED_PARAM(newNavigationID);
> #endif
>@@ -4128,7 +4128,7 @@ void WebPageProxy::decidePolicyForResponse(uint64_t frameID, const SecurityOrigi
>     MESSAGE_CHECK_URL(request.url());
>     MESSAGE_CHECK_URL(response.url());
> 
>-    RefPtr<API::Navigation> navigation = navigationID ? &m_navigationState->navigation(navigationID) : nullptr;
>+    RefPtr<API::Navigation> navigation = navigationID ? m_navigationState->navigation(navigationID) : nullptr;
>     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 {
>         // FIXME: Assert the API::WebsitePolicies* is nullptr here once clients of WKFramePolicyListenerUseWithPolicies go away.
>         RELEASE_ASSERT(processSwapRequestedByClient == ProcessSwapRequestedByClient::No);

Actions: View | Formatted Diff | Diff

Attachments on bug 189763: 350145