- Revision
- 240725
- Author
- [email protected]
- Date
- 2019-01-30 12:27:37 -0800 (Wed, 30 Jan 2019)
Log Message
Regression(PSON) Load hang can occur on history navigation
https://bugs.webkit.org/show_bug.cgi?id=194030
<rdar://problem/47656939>
Reviewed by Antti Koivisto.
Source/WebKit:
We do not support having more than one WebPage in a WebProcess with the same page ID. As a result,
if we decide to reuse an existing process on process-swap, we need to make sure that we either use
its suspended page (when possible, meaning that it is for the right HistoryItem / page) or we need
make sure we drop the existing suspended page for this process / pagePID combination, so that the
WebPage on WebProcess side gets closed before we attempt to do the new load.
We were doing this correctly in 2 places in WebProcessPool::processForNavigationInternal() but failed
to do so in a third place, when doing back to a HistoryItem which does not have a SuspendedPage but
whose process is still alive (presumably because it is kept alive by another suspended page). This
patch fixes this third place to remove any suspended page in the process for the current page before
reusing the process. An assertion was also added to the call site in
WebPageProxy::receivedNavigationPolicyDecision() to make sure we catch this more easily in the
future.
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::receivedNavigationPolicyDecision):
* UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::processForNavigationInternal):
(WebKit::WebProcessPool::removeAllSuspendedPagesForPage):
(WebKit::WebProcessPool::hasSuspendedPageFor const):
* UIProcess/WebProcessPool.h:
Tools:
Add API test coverage.
* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (240724 => 240725)
--- trunk/Source/WebKit/ChangeLog 2019-01-30 20:08:16 UTC (rev 240724)
+++ trunk/Source/WebKit/ChangeLog 2019-01-30 20:27:37 UTC (rev 240725)
@@ -1,3 +1,33 @@
+2019-01-30 Chris Dumez <[email protected]>
+
+ Regression(PSON) Load hang can occur on history navigation
+ https://bugs.webkit.org/show_bug.cgi?id=194030
+ <rdar://problem/47656939>
+
+ Reviewed by Antti Koivisto.
+
+ We do not support having more than one WebPage in a WebProcess with the same page ID. As a result,
+ if we decide to reuse an existing process on process-swap, we need to make sure that we either use
+ its suspended page (when possible, meaning that it is for the right HistoryItem / page) or we need
+ make sure we drop the existing suspended page for this process / pagePID combination, so that the
+ WebPage on WebProcess side gets closed before we attempt to do the new load.
+
+ We were doing this correctly in 2 places in WebProcessPool::processForNavigationInternal() but failed
+ to do so in a third place, when doing back to a HistoryItem which does not have a SuspendedPage but
+ whose process is still alive (presumably because it is kept alive by another suspended page). This
+ patch fixes this third place to remove any suspended page in the process for the current page before
+ reusing the process. An assertion was also added to the call site in
+ WebPageProxy::receivedNavigationPolicyDecision() to make sure we catch this more easily in the
+ future.
+
+ * UIProcess/WebPageProxy.cpp:
+ (WebKit::WebPageProxy::receivedNavigationPolicyDecision):
+ * UIProcess/WebProcessPool.cpp:
+ (WebKit::WebProcessPool::processForNavigationInternal):
+ (WebKit::WebProcessPool::removeAllSuspendedPagesForPage):
+ (WebKit::WebProcessPool::hasSuspendedPageFor const):
+ * UIProcess/WebProcessPool.h:
+
2019-01-30 Carlos Garcia Campos <[email protected]>
[GTK][Wayland] REGRESSION(r240712): Clear the GL context if it's the current one on dispose
Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.cpp (240724 => 240725)
--- trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2019-01-30 20:08:16 UTC (rev 240724)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2019-01-30 20:27:37 UTC (rev 240725)
@@ -2722,6 +2722,11 @@
if (!shouldProcessSwap)
return;
+ // FIXME: Architecturally we do not currently support multiple WebPage's with the same ID in a given WebProcess.
+ // In the case where the destination WebProcess has a SuspendedPageProxy for this WebPage, we should have thrown
+ // it away to support WebProcess re-use.
+ ASSERT(destinationSuspendedPage || !process().processPool().hasSuspendedPageFor(processForNavigation, this));
+
auto suspendedPage = destinationSuspendedPage ? process().processPool().takeSuspendedPage(*destinationSuspendedPage) : nullptr;
if (suspendedPage && suspendedPage->failedToSuspend())
suspendedPage = nullptr;
Modified: trunk/Source/WebKit/UIProcess/WebProcessPool.cpp (240724 => 240725)
--- trunk/Source/WebKit/UIProcess/WebProcessPool.cpp 2019-01-30 20:08:16 UTC (rev 240724)
+++ trunk/Source/WebKit/UIProcess/WebProcessPool.cpp 2019-01-30 20:27:37 UTC (rev 240725)
@@ -2190,8 +2190,14 @@
});
}
- if (auto* process = WebProcessProxy::processForIdentifier(targetItem->itemID().processIdentifier))
- return completionHandler(*process, nullptr, "Using target back/forward item's process"_s);
+ if (RefPtr<WebProcessProxy> process = WebProcessProxy::processForIdentifier(targetItem->itemID().processIdentifier)) {
+ // FIXME: Architecturally we do not currently support multiple WebPage's with the same ID in a given WebProcess.
+ // In the case where this WebProcess has a SuspendedPageProxy for this WebPage, we can throw it away to support
+ // WebProcess re-use.
+ removeAllSuspendedPagesForPage(page, process.get());
+
+ return completionHandler(process.releaseNonNull(), nullptr, "Using target back/forward item's process"_s);
+ }
}
if (navigation.treatAsSameOriginNavigation())
@@ -2225,9 +2231,7 @@
// In the case where this WebProcess has a SuspendedPageProxy for this WebPage, we can throw it away to support
// WebProcess re-use.
// In the future it would be great to refactor-out this limitation (https://bugs.webkit.org/show_bug.cgi?id=191166).
- m_suspendedPages.removeAllMatching([&](auto& suspendedPage) {
- return &suspendedPage->page() == &page && &suspendedPage->process() == process;
- });
+ removeAllSuspendedPagesForPage(page, process);
return completionHandler(makeRef(*process), nullptr, reason);
}
@@ -2263,10 +2267,10 @@
m_suspendedPages.append(WTFMove(suspendedPage));
}
-void WebProcessPool::removeAllSuspendedPagesForPage(WebPageProxy& page)
+void WebProcessPool::removeAllSuspendedPagesForPage(WebPageProxy& page, WebProcessProxy* process)
{
- m_suspendedPages.removeAllMatching([&page](auto& suspendedPage) {
- return &suspendedPage->page() == &page;
+ m_suspendedPages.removeAllMatching([&page, process](auto& suspendedPage) {
+ return &suspendedPage->page() == &page && (!process || &suspendedPage->process() == process);
});
}
@@ -2294,10 +2298,10 @@
m_suspendedPages.remove(it);
}
-bool WebProcessPool::hasSuspendedPageFor(WebProcessProxy& process) const
+bool WebProcessPool::hasSuspendedPageFor(WebProcessProxy& process, WebPageProxy* page) const
{
- return m_suspendedPages.findIf([&process](auto& suspendedPage) {
- return &suspendedPage->process() == &process;
+ return m_suspendedPages.findIf([&process, page](auto& suspendedPage) {
+ return &suspendedPage->process() == &process && (!page || &suspendedPage->page() == page);
}) != m_suspendedPages.end();
}
Modified: trunk/Source/WebKit/UIProcess/WebProcessPool.h (240724 => 240725)
--- trunk/Source/WebKit/UIProcess/WebProcessPool.h 2019-01-30 20:08:16 UTC (rev 240724)
+++ trunk/Source/WebKit/UIProcess/WebProcessPool.h 2019-01-30 20:27:37 UTC (rev 240725)
@@ -446,11 +446,11 @@
// SuspendedPageProxy management.
void addSuspendedPage(std::unique_ptr<SuspendedPageProxy>&&);
- void removeAllSuspendedPagesForPage(WebPageProxy&);
+ void removeAllSuspendedPagesForPage(WebPageProxy&, WebProcessProxy* = nullptr);
void closeFailedSuspendedPagesForPage(WebPageProxy&);
std::unique_ptr<SuspendedPageProxy> takeSuspendedPage(SuspendedPageProxy&);
void removeSuspendedPage(SuspendedPageProxy&);
- bool hasSuspendedPageFor(WebProcessProxy&) const;
+ bool hasSuspendedPageFor(WebProcessProxy&, WebPageProxy* = nullptr) const;
unsigned maxSuspendedPageCount() const { return m_maxSuspendedPageCount; }
void didReachGoodTimeToPrewarm();
Modified: trunk/Tools/ChangeLog (240724 => 240725)
--- trunk/Tools/ChangeLog 2019-01-30 20:08:16 UTC (rev 240724)
+++ trunk/Tools/ChangeLog 2019-01-30 20:27:37 UTC (rev 240725)
@@ -1,3 +1,15 @@
+2019-01-30 Chris Dumez <[email protected]>
+
+ Regression(PSON) Load hang can occur on history navigation
+ https://bugs.webkit.org/show_bug.cgi?id=194030
+ <rdar://problem/47656939>
+
+ Reviewed by Antti Koivisto.
+
+ Add API test coverage.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
2019-01-30 Zalan Bujtas <[email protected]>
[LFC] Expand tests coverage.
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (240724 => 240725)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2019-01-30 20:08:16 UTC (rev 240724)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2019-01-30 20:27:37 UTC (rev 240725)
@@ -2311,6 +2311,64 @@
EXPECT_WK_STREQ(@"pson://www.google.com/main.html", [backForwardList.forwardList[1].URL absoluteString]);
}
+TEST(ProcessSwap, GoToSecondItemInBackHistory)
+{
+ auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
+ [processPoolConfiguration setProcessSwapsOnNavigation:YES];
+ 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/main1.html" toData:withSubframesTestBytes];
+ [handler addMappingFromURLString:@"pson://www.webkit.org/main2.html" toData:withSubframesTestBytes];
+ [handler addMappingFromURLString:@"pson://www.apple.com/main.html" toData:withSubframesTestBytes];
+ [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"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/main1.html"]];
+ [webView loadRequest:request];
+
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ auto webkitPID = [webView _webProcessIdentifier];
+
+ request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main2.html"]];
+ [webView loadRequest:request];
+
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
+
+ request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main.html"]];
+ [webView loadRequest:request];
+
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ auto applePID = [webView _webProcessIdentifier];
+ EXPECT_NE(webkitPID, applePID);
+
+ [webView goToBackForwardListItem:webView.get().backForwardList.backList[0]];
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
+ EXPECT_WK_STREQ(@"pson://www.webkit.org/main1.html", [[webView URL] absoluteString]);
+
+ [webView goToBackForwardListItem:webView.get().backForwardList.forwardList[1]];
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ EXPECT_EQ(applePID, [webView _webProcessIdentifier]);
+ EXPECT_WK_STREQ(@"pson://www.apple.com/main.html", [[webView URL] absoluteString]);
+}
+
static const char* keepNavigatingFrameBytes = R"PSONRESOURCE(
<body>
<iframe id="testFrame1" src=""
