Title: [239623] trunk
- Revision
- 239623
- Author
- [email protected]
- Date
- 2019-01-04 11:02:19 -0800 (Fri, 04 Jan 2019)
Log Message
[PSON] Calling history.back() from inside the load event handler prevents process-swapping
https://bugs.webkit.org/show_bug.cgi?id=193120
Reviewed by Alex Christensen.
Source/WebKit:
A HistoryItem is created only *after* we've fired the load event. As a result, if you call
history.back() in JS from inside the load event handler, the current HistoryItem and and
the target HistoryItem will be the same. This is normally not an issue. However, there was
logic inside of WebProcessPool::processForNavigationInternal() which would compare the
processID of the source and destination BackForwardListItems and which would force a process
reuse if both BackForwardListItems came from the same WebContent process. So even though
we swapped when doing a standard load from site A to site B, we would fail to swap if site
B called history.back() from inside its load event handler.
To address the issue, stop relying on the source backforward item's processID. Instead, just
use the WebContent process matching the destination backforward item's processID if it still
exists.
* UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::processForNavigationInternal):
Tools:
Add API test coverage.
* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (239622 => 239623)
--- trunk/Source/WebKit/ChangeLog 2019-01-04 19:02:00 UTC (rev 239622)
+++ trunk/Source/WebKit/ChangeLog 2019-01-04 19:02:19 UTC (rev 239623)
@@ -1,3 +1,26 @@
+2019-01-04 Chris Dumez <[email protected]>
+
+ [PSON] Calling history.back() from inside the load event handler prevents process-swapping
+ https://bugs.webkit.org/show_bug.cgi?id=193120
+
+ Reviewed by Alex Christensen.
+
+ A HistoryItem is created only *after* we've fired the load event. As a result, if you call
+ history.back() in JS from inside the load event handler, the current HistoryItem and and
+ the target HistoryItem will be the same. This is normally not an issue. However, there was
+ logic inside of WebProcessPool::processForNavigationInternal() which would compare the
+ processID of the source and destination BackForwardListItems and which would force a process
+ reuse if both BackForwardListItems came from the same WebContent process. So even though
+ we swapped when doing a standard load from site A to site B, we would fail to swap if site
+ B called history.back() from inside its load event handler.
+
+ To address the issue, stop relying on the source backforward item's processID. Instead, just
+ use the WebContent process matching the destination backforward item's processID if it still
+ exists.
+
+ * UIProcess/WebProcessPool.cpp:
+ (WebKit::WebProcessPool::processForNavigationInternal):
+
2019-01-04 Keith Rollin <[email protected]>
Bring back parent processID for logging
Modified: trunk/Source/WebKit/UIProcess/WebProcessPool.cpp (239622 => 239623)
--- trunk/Source/WebKit/UIProcess/WebProcessPool.cpp 2019-01-04 19:02:00 UTC (rev 239622)
+++ trunk/Source/WebKit/UIProcess/WebProcessPool.cpp 2019-01-04 19:02:19 UTC (rev 239623)
@@ -2172,25 +2172,18 @@
if (navigation.hasOpenedFrames())
return completionHandler(page.process(), nullptr, "Browsing context has opened other windows"_s);
- if (auto* backForwardListItem = navigation.targetItem()) {
- if (auto* suspendedPage = backForwardListItem->suspendedPage()) {
+ if (auto* targetItem = navigation.targetItem()) {
+ if (auto* suspendedPage = targetItem->suspendedPage()) {
return suspendedPage->waitUntilReadyToUnsuspend([createNewProcess = WTFMove(createNewProcess), completionHandler = WTFMove(completionHandler)](SuspendedPageProxy* suspendedPage) mutable {
if (!suspendedPage)
return completionHandler(createNewProcess(), nullptr, "Using new process because target back/forward item's suspended page is not reusable"_s);
Ref<WebProcessProxy> process = suspendedPage->process();
- completionHandler(WTFMove(process), suspendedPage, "Using target back/forward item's process"_s);
+ completionHandler(WTFMove(process), suspendedPage, "Using target back/forward item's process and suspended page"_s);
});
}
- // If the target back/forward item and the current back/forward item originated
- // in the same WebProcess then we should reuse the current WebProcess.
- if (auto* fromItem = navigation.fromItem()) {
- auto uiProcessIdentifier = Process::identifier();
- // In case of session restore, the item's process identifier is the UIProcess' identifier, in which case we do not want to do this check
- // or we'd never swap after a session restore.
- if (fromItem->itemID().processIdentifier == backForwardListItem->itemID().processIdentifier && backForwardListItem->itemID().processIdentifier != uiProcessIdentifier)
- return completionHandler(page.process(), nullptr, "Source and target back/forward item originated in the same process"_s);
- }
+ if (auto* process = WebProcessProxy::processForIdentifier(targetItem->itemID().processIdentifier))
+ return completionHandler(*process, nullptr, "Using target back/forward item's process"_s);
}
if (navigation.treatAsSameOriginNavigation())
Modified: trunk/Tools/ChangeLog (239622 => 239623)
--- trunk/Tools/ChangeLog 2019-01-04 19:02:00 UTC (rev 239622)
+++ trunk/Tools/ChangeLog 2019-01-04 19:02:19 UTC (rev 239623)
@@ -1,5 +1,16 @@
2019-01-04 Chris Dumez <[email protected]>
+ [PSON] Calling history.back() from inside the load event handler prevents process-swapping
+ https://bugs.webkit.org/show_bug.cgi?id=193120
+
+ Reviewed by Alex Christensen.
+
+ Add API test coverage.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
+2019-01-04 Chris Dumez <[email protected]>
+
Crash under WebProcessPool::addSuspendedPage()
https://bugs.webkit.org/show_bug.cgi?id=193110
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (239622 => 239623)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2019-01-04 19:02:00 UTC (rev 239622)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2019-01-04 19:02:19 UTC (rev 239623)
@@ -2810,6 +2810,62 @@
EXPECT_NE(pid1, pid3);
}
+static const char* navigateToCrossSiteThenBackFromJSBytes = R"PSONRESOURCE(
+<script>
+_onpageshow_ = function(event) {
+ // Location changes need to happen outside the onload handler to generate history entries.
+ setTimeout(function() {
+ window.location.href = ""
+ }, 0);
+}
+
+</script>
+)PSONRESOURCE";
+
+static const char* navigateBackFromJSBytes = R"PSONRESOURCE(
+<body _onload_='history.back()'></body>
+)PSONRESOURCE";
+
+TEST(ProcessSwap, NavigateToCrossSiteThenBackFromJS)
+{
+ auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
+ processPoolConfiguration.get().processSwapsOnNavigation = YES;
+ processPoolConfiguration.get().pageCacheEnabled = NO;
+ 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:navigateToCrossSiteThenBackFromJSBytes]; // Navigates to "pson://www.apple.com/main.html".
+ [handler addMappingFromURLString:@"pson://www.apple.com/main.html" toData:navigateBackFromJSBytes];
+ [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
+
+ auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
+ auto navigationDelegate = adoptNS([[PSONNavigationDelegate alloc] init]);
+ [webView setNavigationDelegate:navigationDelegate.get()];
+
+ numberOfDecidePolicyCalls = 0;
+ [webView loadRequest:[NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]]];
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+ auto webkitPID = [webView _webProcessIdentifier];
+
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+ auto applePID = [webView _webProcessIdentifier];
+ EXPECT_NE(webkitPID, applePID); // Should have process-swapped when going from webkit.org to apple.com.
+
+ // Page now calls history.back() to navigate back to webkit.org.
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ EXPECT_EQ(3, numberOfDecidePolicyCalls);
+
+ // Should have process-swapped when going from apple.com to webkit.org.
+ // PID is not necessarily webkitPID because PageCache is disabled.
+ EXPECT_NE(applePID, [webView _webProcessIdentifier]);
+}
+
#if PLATFORM(MAC)
static const char* saveOpenerTestBytes = R"PSONRESOURCE(
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
