Attachment 358262 Details for Bug 193113 – Patch

[patch] Patch

bug-193113-20190103110315.patch (text/plain), 5.12 KB, created by Chris Dumez on 2019-01-03 11:03:16 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-01-03 11:03:16 PST

Size: 5.12 KB

patch

obsolete

>Subversion Revision: 239579
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index d377a849d96db69c106d13ce6f283e969e5769b0..03c3dea385567be5fe5addb363d453d0128be63c 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,41 @@
>+2019-01-03  Chris Dumez  <cdumez@apple.com>
>+
>+        Crash under WebPageProxy::continueNavigationInNewProcess()
>+        https://bugs.webkit.org/show_bug.cgi?id=193113
>+        <rdar://problem/46938686>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The crash was happening in continueNavigationInNewProcess() when dereferencing
>+        the Optional<> value returned by API::Navigation::backForwardFrameLoadType(), after verifying
>+        that API::Navigation::targetItem() is not null.
>+
>+        When constructing an API::Navigation object with a targetItem, you HAVE to pass
>+        in a backForwardFrameLoadType as well so this normally is not possible. However,
>+        it can happen because API::Navigation::setTargetItem() can get called later on and
>+        set a target item on a Navigation object which potentially does not have a
>+        backForwardFrameLoadType. This setter was only called in one place in
>+        decidePolicyForNavigationAction() to update an existing Navigation object using
>+        the targetItem provided by a NavigationAction. This logic was added with PSON
>+        support.
>+
>+        Because I was unable to write a test case reproducing this and because I do not know
>+        how it can happen in practice that we'd have a NavigationAction with a targetItem
>+        even though the Navigation object itself is not for a back/forward navigation, I have
>+        chosen to drop the unsafe API::Navigation::setTargetItem() setter and the call site.
>+        When the call site was added, with ProcessSwap.NavigateToDataURLThenBack API test,
>+        the intention was to create a back/forward navigation object instead of a standard load
>+        navigation one if there is currently no existing Navigation object in the UIProcess.
>+        This can happen when the back/forward navigation is triggered by the WebProcess via
>+        JS (e.g. history.back()) and this is what the API test covers. The part of the logic
>+        that updates an existing Navigation object with a targetItem coming from the
>+        NavigationAction is untested and I have no evidence it does anything useful. However,
>+        we DO have evidence that it can cause crashes.
>+
>+        * UIProcess/API/APINavigation.h:
>+        * UIProcess/WebPageProxy.cpp:
>+        (WebKit::WebPageProxy::decidePolicyForNavigationAction):
>+
> 2019-01-03  Chris Dumez  <cdumez@apple.com>
> 
>         Crash under WebProcessPool::addSuspendedPage()
>diff --git a/Source/WebKit/UIProcess/API/APINavigation.h b/Source/WebKit/UIProcess/API/APINavigation.h
>index 841d5ac544055ee986c189961e6b139eded15972..b15030d4e71331b8aa8c30a25398b6be4efc632a 100644
>--- a/Source/WebKit/UIProcess/API/APINavigation.h
>+++ b/Source/WebKit/UIProcess/API/APINavigation.h
>@@ -97,7 +97,6 @@ public:
> 
>     bool currentRequestIsRedirect() const { return m_lastNavigationAction.isRedirect; }
> 
>-    void setTargetItem(WebKit::WebBackForwardListItem& item) { m_targetItem = &item; }
>     WebKit::WebBackForwardListItem* targetItem() const { return m_targetItem.get(); }
>     WebKit::WebBackForwardListItem* fromItem() const { return m_fromItem.get(); }
>     Optional<WebCore::FrameLoadType> backForwardFrameLoadType() const { return m_backForwardFrameLoadType; }
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index 9db70710d990100c5cb058bfde92bc77ea51b11d..467a806b313d7cda32659da703e60ded3d1bb7f4 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -4379,18 +4379,15 @@ void WebPageProxy::decidePolicyForNavigationAction(WebFrameProxy& frame, WebCore
>         frameSecurityOrigin = navigation->destinationFrameSecurityOrigin();
>     }
> 
>-    if (auto targetBackForwardItemIdentifier = navigationActionData.targetBackForwardItemIdentifier) {
>-        if (auto* item = m_backForwardList->itemForID(*navigationActionData.targetBackForwardItemIdentifier)) {
>-            if (!navigation)
>+    if (!navigation) {
>+        if (auto targetBackForwardItemIdentifier = navigationActionData.targetBackForwardItemIdentifier) {
>+            if (auto* item = m_backForwardList->itemForID(*targetBackForwardItemIdentifier))
>                 navigation = m_navigationState->createBackForwardNavigation(*item, m_backForwardList->currentItem(), FrameLoadType::IndexedBackForward);
>-            else
>-                navigation->setTargetItem(*item);
>         }
>+        if (!navigation)
>+            navigation = m_navigationState->createLoadRequestNavigation(ResourceRequest(request), m_backForwardList->currentItem());
>     }
> 
>-    if (!navigation)
>-        navigation = m_navigationState->createLoadRequestNavigation(ResourceRequest(request), m_backForwardList->currentItem());
>-
>     uint64_t newNavigationID = navigation->navigationID();
>     navigation->setCurrentRequest(ResourceRequest(request), m_process->coreProcessIdentifier());
>     navigation->setLastNavigationAction(navigationActionData);

Actions: View | Formatted Diff | Diff

Attachments on bug 193113: 358262