Modified: trunk/Source/WebKit/ChangeLog (236226 => 236227)
--- trunk/Source/WebKit/ChangeLog 2018-09-19 21:33:51 UTC (rev 236226)
+++ trunk/Source/WebKit/ChangeLog 2018-09-19 21:58:45 UTC (rev 236227)
@@ -1,5 +1,33 @@
2018-09-19 Chris Dumez <[email protected]>
+ 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):
+
+2018-09-19 Chris Dumez <[email protected]>
+
Crash under WebProcessProxy::suspendedPageWasDestroyed(WebKit::SuspendedPageProxy&)
https://bugs.webkit.org/show_bug.cgi?id=189721
<rdar://problem/44359788>
Modified: trunk/Source/WebKit/UIProcess/WebNavigationState.cpp (236226 => 236227)
--- trunk/Source/WebKit/UIProcess/WebNavigationState.cpp 2018-09-19 21:33:51 UTC (rev 236226)
+++ trunk/Source/WebKit/UIProcess/WebNavigationState.cpp 2018-09-19 21:58:45 UTC (rev 236227)
@@ -77,20 +77,20 @@
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)
Modified: trunk/Source/WebKit/UIProcess/WebNavigationState.h (236226 => 236227)
--- trunk/Source/WebKit/UIProcess/WebNavigationState.h 2018-09-19 21:33:51 UTC (rev 236226)
+++ trunk/Source/WebKit/UIProcess/WebNavigationState.h 2018-09-19 21:58:45 UTC (rev 236227)
@@ -53,8 +53,8 @@
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();
Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.cpp (236226 => 236227)
--- trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2018-09-19 21:33:51 UTC (rev 236226)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2018-09-19 21:58:45 UTC (rev 236227)
@@ -3460,7 +3460,7 @@
// 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 @@
// 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 @@
// 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 @@
// 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 @@
// 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 @@
// 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 @@
// 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 @@
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 @@
#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 @@
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);
