Title: [239638] trunk/Source/WebKit
- Revision
- 239638
- Author
- [email protected]
- Date
- 2019-01-04 14:10:21 -0800 (Fri, 04 Jan 2019)
Log Message
Crash under WebPageProxy::continueNavigationInNewProcess()
https://bugs.webkit.org/show_bug.cgi?id=193113
<rdar://problem/46938686>
Reviewed by Brady Eidson.
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):
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (239637 => 239638)
--- trunk/Source/WebKit/ChangeLog 2019-01-04 21:40:40 UTC (rev 239637)
+++ trunk/Source/WebKit/ChangeLog 2019-01-04 22:10:21 UTC (rev 239638)
@@ -1,3 +1,41 @@
+2019-01-04 Chris Dumez <[email protected]>
+
+ Crash under WebPageProxy::continueNavigationInNewProcess()
+ https://bugs.webkit.org/show_bug.cgi?id=193113
+ <rdar://problem/46938686>
+
+ Reviewed by Brady Eidson.
+
+ 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-04 Tim Horton <[email protected]>
Remove some nonexistent files from the WebKit Xcode project
Modified: trunk/Source/WebKit/UIProcess/API/APINavigation.h (239637 => 239638)
--- trunk/Source/WebKit/UIProcess/API/APINavigation.h 2019-01-04 21:40:40 UTC (rev 239637)
+++ trunk/Source/WebKit/UIProcess/API/APINavigation.h 2019-01-04 22:10:21 UTC (rev 239638)
@@ -97,7 +97,6 @@
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; }
Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.cpp (239637 => 239638)
--- trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2019-01-04 21:40:40 UTC (rev 239637)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2019-01-04 22:10:21 UTC (rev 239638)
@@ -4379,18 +4379,15 @@
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);
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
