Diff
Modified: trunk/Source/WebCore/ChangeLog (242902 => 242903)
--- trunk/Source/WebCore/ChangeLog 2019-03-13 20:27:54 UTC (rev 242902)
+++ trunk/Source/WebCore/ChangeLog 2019-03-13 20:32:48 UTC (rev 242903)
@@ -1,3 +1,40 @@
+2019-03-13 Chris Dumez <[email protected]>
+
+ REGRESSION(PSON, r240660): Navigation over process boundary is flashy when using Cmd-left/right arrow to navigate
+ https://bugs.webkit.org/show_bug.cgi?id=195684
+ <rdar://problem/48294714>
+
+ Reviewed by Antti Koivisto.
+
+ The issue was caused by us failing to suspend the current page on navigation because the source and
+ target WebBackForwardListItem are identical. The source WebBackForwardListItem was wrong.
+
+ When a navigation is triggered by the WebContent process (and not the UIProcess), we create the Navigation
+ object in WebPageProxy::decidePolicyForNavigationAction(). For the navigation's targetItem, we use the
+ target item identifier provided by the WebContent process via the NavigationActionData. However,
+ for the source item, we would use the WebBackForwardList's currentItem in the UIProcess. The issue
+ is that the WebBackForwardList's currentItem usually has already been updated to be the target
+ item via a WebPageProxy::BackForwardGoToItem() synchronous IPC.
+
+ To avoid raciness and given that the current history management is fragile (as it is managed by
+ both the UIProcess and the WebProcess), I am now passing the source item identifier in
+ addition to the target item identifier in the NavigationActionData that is sent by the WebProcess.
+ This is a lot less error prone, the WebProcess knows more accurately which history items it is going
+ from and to.
+
+ * loader/FrameLoader.cpp:
+ (WebCore::FrameLoader::loadURLIntoChildFrame):
+ (WebCore::FrameLoader::loadDifferentDocumentItem):
+ (WebCore::FrameLoader::loadItem):
+ (WebCore::FrameLoader::retryAfterFailedCacheOnlyMainResourceLoad):
+ * loader/FrameLoader.h:
+ * loader/HistoryController.cpp:
+ (WebCore::HistoryController::recursiveGoToItem):
+ * loader/NavigationAction.cpp:
+ (WebCore::NavigationAction::setSourceBackForwardItem):
+ * loader/NavigationAction.h:
+ (WebCore::NavigationAction::sourceBackForwardItemIdentifier const):
+
2019-03-13 Jer Noble <[email protected]>
Add AggregateLogger, a Logger specialization for singleton classes.
Modified: trunk/Source/WebCore/loader/FrameLoader.cpp (242902 => 242903)
--- trunk/Source/WebCore/loader/FrameLoader.cpp 2019-03-13 20:27:54 UTC (rev 242902)
+++ trunk/Source/WebCore/loader/FrameLoader.cpp 2019-03-13 20:32:48 UTC (rev 242903)
@@ -974,7 +974,7 @@
if (parentItem && parentItem->children().size() && isBackForwardLoadType(loadType()) && !m_frame.document()->loadEventFinished()) {
if (auto* childItem = parentItem->childItemWithTarget(childFrame->tree().uniqueName())) {
childFrame->loader().m_requestedHistoryItem = childItem;
- childFrame->loader().loadDifferentDocumentItem(*childItem, loadType(), MayAttemptCacheOnlyLoadForFormSubmissionItem, ShouldTreatAsContinuingLoad::No);
+ childFrame->loader().loadDifferentDocumentItem(*childItem, nullptr, loadType(), MayAttemptCacheOnlyLoadForFormSubmissionItem, ShouldTreatAsContinuingLoad::No);
return;
}
}
@@ -3686,7 +3686,7 @@
// FIXME: This function should really be split into a couple pieces, some of
// which should be methods of HistoryController and some of which should be
// methods of FrameLoader.
-void FrameLoader::loadDifferentDocumentItem(HistoryItem& item, FrameLoadType loadType, FormSubmissionCacheLoadPolicy cacheLoadPolicy, ShouldTreatAsContinuingLoad shouldTreatAsContinuingLoad)
+void FrameLoader::loadDifferentDocumentItem(HistoryItem& item, HistoryItem* fromItem, FrameLoadType loadType, FormSubmissionCacheLoadPolicy cacheLoadPolicy, ShouldTreatAsContinuingLoad shouldTreatAsContinuingLoad)
{
RELEASE_LOG_IF_ALLOWED("loadDifferentDocumentItem: frame load started (frame = %p, main = %d)", &m_frame, m_frame.isMainFrame());
@@ -3706,6 +3706,7 @@
auto action = "" { *m_frame.document(), documentLoader->request(), initiatedByMainFrame, loadType, false };
action.setTargetBackForwardItem(item);
+ action.setSourceBackForwardItem(fromItem);
documentLoader->setTriggeringAction(WTFMove(action));
documentLoader->setLastCheckedRequest(ResourceRequest());
@@ -3796,12 +3797,13 @@
}
action.setTargetBackForwardItem(item);
+ action.setSourceBackForwardItem(fromItem);
loadWithNavigationAction(request, WTFMove(action), LockHistory::No, loadType, { }, AllowNavigationToInvalidURL::Yes);
}
// Loads content into this frame, as specified by history item
-void FrameLoader::loadItem(HistoryItem& item, FrameLoadType loadType, ShouldTreatAsContinuingLoad shouldTreatAsContinuingLoad)
+void FrameLoader::loadItem(HistoryItem& item, HistoryItem* fromItem, FrameLoadType loadType, ShouldTreatAsContinuingLoad shouldTreatAsContinuingLoad)
{
m_requestedHistoryItem = &item;
HistoryItem* currentItem = history().currentItem();
@@ -3810,7 +3812,7 @@
if (sameDocumentNavigation)
loadSameDocumentItem(item);
else
- loadDifferentDocumentItem(item, loadType, MayAttemptCacheOnlyLoadForFormSubmissionItem, shouldTreatAsContinuingLoad);
+ loadDifferentDocumentItem(item, fromItem, loadType, MayAttemptCacheOnlyLoadForFormSubmissionItem, shouldTreatAsContinuingLoad);
}
void FrameLoader::retryAfterFailedCacheOnlyMainResourceLoad()
@@ -3825,7 +3827,7 @@
HistoryItem& item = *history().provisionalItem();
stopAllLoaders(ShouldNotClearProvisionalItem);
- loadDifferentDocumentItem(item, loadType, MayNotAttemptCacheOnlyLoadForFormSubmissionItem, ShouldTreatAsContinuingLoad::No);
+ loadDifferentDocumentItem(item, history().currentItem(), loadType, MayNotAttemptCacheOnlyLoadForFormSubmissionItem, ShouldTreatAsContinuingLoad::No);
}
ResourceError FrameLoader::cancelledError(const ResourceRequest& request) const
Modified: trunk/Source/WebCore/loader/FrameLoader.h (242902 => 242903)
--- trunk/Source/WebCore/loader/FrameLoader.h 2019-03-13 20:27:54 UTC (rev 242902)
+++ trunk/Source/WebCore/loader/FrameLoader.h 2019-03-13 20:32:48 UTC (rev 242903)
@@ -133,7 +133,7 @@
WEBCORE_EXPORT void reloadWithOverrideEncoding(const String& overrideEncoding);
void open(CachedFrameBase&);
- void loadItem(HistoryItem&, FrameLoadType, ShouldTreatAsContinuingLoad);
+ void loadItem(HistoryItem&, HistoryItem* fromItem, FrameLoadType, ShouldTreatAsContinuingLoad);
HistoryItem* requestedHistoryItem() const { return m_requestedHistoryItem.get(); }
void retryAfterFailedCacheOnlyMainResourceLoad();
@@ -335,7 +335,7 @@
void checkCompletenessNow();
void loadSameDocumentItem(HistoryItem&);
- void loadDifferentDocumentItem(HistoryItem&, FrameLoadType, FormSubmissionCacheLoadPolicy, ShouldTreatAsContinuingLoad);
+ void loadDifferentDocumentItem(HistoryItem&, HistoryItem* fromItem, FrameLoadType, FormSubmissionCacheLoadPolicy, ShouldTreatAsContinuingLoad);
void loadProvisionalItemFromCachedPage();
Modified: trunk/Source/WebCore/loader/HistoryController.cpp (242902 => 242903)
--- trunk/Source/WebCore/loader/HistoryController.cpp 2019-03-13 20:27:54 UTC (rev 242902)
+++ trunk/Source/WebCore/loader/HistoryController.cpp 2019-03-13 20:32:48 UTC (rev 242903)
@@ -751,7 +751,7 @@
void HistoryController::recursiveGoToItem(HistoryItem& item, HistoryItem* fromItem, FrameLoadType type, ShouldTreatAsContinuingLoad shouldTreatAsContinuingLoad)
{
if (!itemsAreClones(item, fromItem)) {
- m_frame.loader().loadItem(item, type, shouldTreatAsContinuingLoad);
+ m_frame.loader().loadItem(item, fromItem, type, shouldTreatAsContinuingLoad);
return;
}
Modified: trunk/Source/WebCore/loader/NavigationAction.cpp (242902 => 242903)
--- trunk/Source/WebCore/loader/NavigationAction.cpp 2019-03-13 20:27:54 UTC (rev 242902)
+++ trunk/Source/WebCore/loader/NavigationAction.cpp 2019-03-13 20:32:48 UTC (rev 242903)
@@ -144,4 +144,9 @@
m_targetBackForwardItemIdentifier = item.identifier();
}
+void NavigationAction::setSourceBackForwardItem(HistoryItem* item)
+{
+ m_sourceBackForwardItemIdentifier = item ? makeOptional(item->identifier()) : WTF::nullopt;
}
+
+}
Modified: trunk/Source/WebCore/loader/NavigationAction.h (242902 => 242903)
--- trunk/Source/WebCore/loader/NavigationAction.h 2019-03-13 20:27:54 UTC (rev 242902)
+++ trunk/Source/WebCore/loader/NavigationAction.h 2019-03-13 20:32:48 UTC (rev 242903)
@@ -129,6 +129,9 @@
void setTargetBackForwardItem(HistoryItem&);
const Optional<BackForwardItemIdentifier>& targetBackForwardItemIdentifier() const { return m_targetBackForwardItemIdentifier; }
+ void setSourceBackForwardItem(HistoryItem*);
+ const Optional<BackForwardItemIdentifier>& sourceBackForwardItemIdentifier() const { return m_sourceBackForwardItemIdentifier; }
+
LockHistory lockHistory() const { return m_lockHistory; }
void setLockHistory(LockHistory lockHistory) { m_lockHistory = lockHistory; }
@@ -154,6 +157,7 @@
bool m_hasOpenedFrames { false };
bool m_openedByDOMWithOpener { false };
Optional<BackForwardItemIdentifier> m_targetBackForwardItemIdentifier;
+ Optional<BackForwardItemIdentifier> m_sourceBackForwardItemIdentifier;
LockHistory m_lockHistory { LockHistory::No };
LockBackForwardList m_lockBackForwardList { LockBackForwardList::No };
Optional<AdClickAttribution> m_adClickAttribution;
Modified: trunk/Source/WebKit/ChangeLog (242902 => 242903)
--- trunk/Source/WebKit/ChangeLog 2019-03-13 20:27:54 UTC (rev 242902)
+++ trunk/Source/WebKit/ChangeLog 2019-03-13 20:32:48 UTC (rev 242903)
@@ -1,5 +1,23 @@
2019-03-13 Chris Dumez <[email protected]>
+ REGRESSION(PSON, r240660): Navigation over process boundary is flashy when using Cmd-left/right arrow to navigate
+ https://bugs.webkit.org/show_bug.cgi?id=195684
+ <rdar://problem/48294714>
+
+ Reviewed by Antti Koivisto.
+
+ * Shared/NavigationActionData.cpp:
+ (WebKit::NavigationActionData::encode const):
+ (WebKit::NavigationActionData::decode):
+ * Shared/NavigationActionData.h:
+ * UIProcess/WebPageProxy.cpp:
+ (WebKit::WebPageProxy::decidePolicyForNavigationAction):
+ (WebKit::WebPageProxy::backForwardAddItem):
+ * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
+ (WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
+
+2019-03-13 Chris Dumez <[email protected]>
+
Drop legacy WebCore::toRegistrableDomain() utility function
https://bugs.webkit.org/show_bug.cgi?id=195637
Modified: trunk/Source/WebKit/Shared/NavigationActionData.cpp (242902 => 242903)
--- trunk/Source/WebKit/Shared/NavigationActionData.cpp 2019-03-13 20:27:54 UTC (rev 242902)
+++ trunk/Source/WebKit/Shared/NavigationActionData.cpp 2019-03-13 20:32:48 UTC (rev 242903)
@@ -50,6 +50,7 @@
encoder << openedByDOMWithOpener;
encoder << requesterOrigin;
encoder << targetBackForwardItemIdentifier;
+ encoder << sourceBackForwardItemIdentifier;
encoder.encodeEnum(lockHistory);
encoder.encodeEnum(lockBackForwardList);
encoder << clientRedirectSourceForHistory;
@@ -127,6 +128,11 @@
if (!targetBackForwardItemIdentifier)
return WTF::nullopt;
+ Optional<Optional<WebCore::BackForwardItemIdentifier>> sourceBackForwardItemIdentifier;
+ decoder >> sourceBackForwardItemIdentifier;
+ if (!sourceBackForwardItemIdentifier)
+ return WTF::nullopt;
+
WebCore::LockHistory lockHistory;
if (!decoder.decodeEnum(lockHistory))
return WTF::nullopt;
@@ -148,7 +154,7 @@
return {{ WTFMove(navigationType), modifiers, WTFMove(mouseButton), WTFMove(syntheticClickType), WTFMove(*userGestureTokenIdentifier),
WTFMove(*canHandleRequest), WTFMove(shouldOpenExternalURLsPolicy), WTFMove(*downloadAttribute), WTFMove(clickLocationInRootViewCoordinates),
WTFMove(*isRedirect), *treatAsSameOriginNavigation, *hasOpenedFrames, *openedByDOMWithOpener, WTFMove(*requesterOrigin),
- WTFMove(*targetBackForwardItemIdentifier), lockHistory, lockBackForwardList, WTFMove(*clientRedirectSourceForHistory), WTFMove(*adClickAttribution) }};
+ WTFMove(*targetBackForwardItemIdentifier), WTFMove(*sourceBackForwardItemIdentifier), lockHistory, lockBackForwardList, WTFMove(*clientRedirectSourceForHistory), WTFMove(*adClickAttribution) }};
}
} // namespace WebKit
Modified: trunk/Source/WebKit/Shared/NavigationActionData.h (242902 => 242903)
--- trunk/Source/WebKit/Shared/NavigationActionData.h 2019-03-13 20:27:54 UTC (rev 242902)
+++ trunk/Source/WebKit/Shared/NavigationActionData.h 2019-03-13 20:32:48 UTC (rev 242903)
@@ -58,6 +58,7 @@
bool openedByDOMWithOpener { false };
WebCore::SecurityOriginData requesterOrigin;
Optional<WebCore::BackForwardItemIdentifier> targetBackForwardItemIdentifier;
+ Optional<WebCore::BackForwardItemIdentifier> sourceBackForwardItemIdentifier;
WebCore::LockHistory lockHistory;
WebCore::LockBackForwardList lockBackForwardList;
WTF::String clientRedirectSourceForHistory;
Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.cpp (242902 => 242903)
--- trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2019-03-13 20:27:54 UTC (rev 242902)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2019-03-13 20:32:48 UTC (rev 242903)
@@ -4515,8 +4515,13 @@
if (!navigation) {
if (auto targetBackForwardItemIdentifier = navigationActionData.targetBackForwardItemIdentifier) {
- if (auto* item = m_backForwardList->itemForID(*targetBackForwardItemIdentifier))
- navigation = m_navigationState->createBackForwardNavigation(*item, m_backForwardList->currentItem(), FrameLoadType::IndexedBackForward);
+ if (auto* item = m_backForwardList->itemForID(*targetBackForwardItemIdentifier)) {
+ auto* fromItem = navigationActionData.sourceBackForwardItemIdentifier ? m_backForwardList->itemForID(*navigationActionData.sourceBackForwardItemIdentifier) : nullptr;
+ if (!fromItem)
+ fromItem = m_backForwardList->currentItem();
+ WTFLogAlways("WebPageProxy::decidePolicyForNavigationAction() creates back/forward navigation from item %s", fromItem ? fromItem->url().utf8().data() : "null");
+ navigation = m_navigationState->createBackForwardNavigation(*item, fromItem, FrameLoadType::IndexedBackForward);
+ }
}
if (!navigation)
navigation = m_navigationState->createLoadRequestNavigation(ResourceRequest(request), m_backForwardList->currentItem());
@@ -5579,7 +5584,8 @@
void WebPageProxy::backForwardAddItem(BackForwardListItemState&& itemState)
{
- m_backForwardList->addItem(WebBackForwardListItem::create(WTFMove(itemState), pageID()));
+ auto item = WebBackForwardListItem::create(WTFMove(itemState), pageID());
+ m_backForwardList->addItem(WTFMove(item));
}
void WebPageProxy::backForwardGoToItem(const BackForwardItemIdentifier& itemID, SandboxExtension::Handle& sandboxExtensionHandle)
Modified: trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp (242902 => 242903)
--- trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp 2019-03-13 20:27:54 UTC (rev 242902)
+++ trunk/Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp 2019-03-13 20:32:48 UTC (rev 242903)
@@ -880,6 +880,7 @@
if (auto& requester = navigationAction.requester())
navigationActionData.requesterOrigin = requester->securityOrigin().data();
navigationActionData.targetBackForwardItemIdentifier = navigationAction.targetBackForwardItemIdentifier();
+ navigationActionData.sourceBackForwardItemIdentifier = navigationAction.sourceBackForwardItemIdentifier();
navigationActionData.lockHistory = navigationAction.lockHistory();
navigationActionData.lockBackForwardList = navigationAction.lockBackForwardList();
navigationActionData.adClickAttribution = navigationAction.adClickAttribution();
Modified: trunk/Tools/ChangeLog (242902 => 242903)
--- trunk/Tools/ChangeLog 2019-03-13 20:27:54 UTC (rev 242902)
+++ trunk/Tools/ChangeLog 2019-03-13 20:32:48 UTC (rev 242903)
@@ -1,3 +1,15 @@
+2019-03-13 Chris Dumez <[email protected]>
+
+ REGRESSION(PSON, r240660): Navigation over process boundary is flashy when using Cmd-left/right arrow to navigate
+ https://bugs.webkit.org/show_bug.cgi?id=195684
+ <rdar://problem/48294714>
+
+ Reviewed by Antti Koivisto.
+
+ Add API test coverage.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
2019-03-13 Aakash Jain <[email protected]>
[ews-app] Remove unused patch view
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (242902 => 242903)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2019-03-13 20:27:54 UTC (rev 242902)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2019-03-13 20:32:48 UTC (rev 242903)
@@ -3137,6 +3137,75 @@
EXPECT_EQ(2u, seenPIDs.size());
}
+TEST(ProcessSwap, PageCacheWhenNavigatingFromJS)
+{
+ auto processPoolConfiguration = psonProcessPoolConfiguration();
+ auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
+
+ auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
+ [webViewConfiguration setProcessPool:processPool.get()];
+ auto handler = adoptNS([[PSONScheme alloc] init]);
+ [handler addMappingFromURLString:@"pson://www.webkit.org/main.html" toData:pageCache1Bytes];
+ [handler addMappingFromURLString:@"pson://www.apple.com/main.html" toData:pageCache1Bytes];
+ [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
+
+ auto messageHandler = adoptNS([[PSONMessageHandler alloc] init]);
+ [[webViewConfiguration userContentController] addScriptMessageHandler:messageHandler.get() name:@"pson"];
+
+ auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
+ auto delegate = adoptNS([[PSONNavigationDelegate alloc] init]);
+ [webView setNavigationDelegate:delegate.get()];
+
+ NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
+
+ [webView loadRequest:request];
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ auto pidAfterLoad1 = [webView _webProcessIdentifier];
+
+ EXPECT_EQ(1u, [processPool _webProcessCountIgnoringPrewarmedAndCached]);
+
+ request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main.html"]];
+
+ [webView loadRequest:request];
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ auto pidAfterLoad2 = [webView _webProcessIdentifier];
+
+ EXPECT_EQ(2u, [processPool _webProcessCountIgnoringPrewarmedAndCached]);
+ EXPECT_NE(pidAfterLoad1, pidAfterLoad2);
+
+ [webView evaluateJavaScript:@"history.back()" completionHandler: nil];
+ TestWebKitAPI::Util::run(&receivedMessage);
+ receivedMessage = false;
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ auto pidAfterLoad3 = [webView _webProcessIdentifier];
+
+ EXPECT_EQ(2u, [processPool _webProcessCountIgnoringPrewarmedAndCached]);
+ EXPECT_EQ(pidAfterLoad1, pidAfterLoad3);
+ EXPECT_EQ(1u, [receivedMessages count]);
+ EXPECT_TRUE([receivedMessages.get()[0] isEqualToString:@"Was persisted" ]);
+ EXPECT_EQ(2u, seenPIDs.size());
+
+ [webView evaluateJavaScript:@"history.forward()" completionHandler: nil];
+ TestWebKitAPI::Util::run(&receivedMessage);
+ receivedMessage = false;
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ auto pidAfterLoad4 = [webView _webProcessIdentifier];
+
+ EXPECT_EQ(2u, [processPool _webProcessCountIgnoringPrewarmedAndCached]);
+ EXPECT_EQ(pidAfterLoad2, pidAfterLoad4);
+ EXPECT_EQ(2u, [receivedMessages count]);
+ EXPECT_TRUE([receivedMessages.get()[1] isEqualToString:@"Was persisted" ]);
+ EXPECT_EQ(2u, seenPIDs.size());
+}
+
TEST(ProcessSwap, NumberOfPrewarmedProcesses)
{
auto processPoolConfiguration = psonProcessPoolConfiguration();
