- Revision
- 239333
- Author
- [email protected]
- Date
- 2018-12-18 07:26:50 -0800 (Tue, 18 Dec 2018)
Log Message
Regression(r239182) SuspendedPage's process reuse for link navigation optimization sometimes broken
https://bugs.webkit.org/show_bug.cgi?id=192772
Reviewed by Antti Koivisto.
Source/WebKit:
With r239182, if the page in the previous process would fail to enter PageCache, we would destroy
the corresponding SuspendedPageProxy, which would potentially terminate the process. This would
regress performance when trying to navigate back in history to that page. This would also regress
performance when link-navigating to the same domain as we would have previously reused the suspended
page's process for such navigation.
Address the issue by keeping the SuspendedPageProxy alive even if the WebPage fails to suspend.
When trying to reuse a SuspendedPageProxy, if the page failed to suspend, reuse its process but
not the suspended page itself.
* UIProcess/SuspendedPageProxy.cpp:
(WebKit::SuspendedPageProxy::~SuspendedPageProxy):
(WebKit::SuspendedPageProxy::waitUntilReadyToUnsuspend):
(WebKit::SuspendedPageProxy::unsuspend):
(WebKit::SuspendedPageProxy::didSuspend):
(WebKit::SuspendedPageProxy::didFailToSuspend):
(WebKit::SuspendedPageProxy::loggingString const):
* UIProcess/SuspendedPageProxy.h:
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::swapToWebProcess):
* UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::processForNavigationInternal):
Tools:
Add API test coverage.
* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (239332 => 239333)
--- trunk/Source/WebKit/ChangeLog 2018-12-18 15:25:32 UTC (rev 239332)
+++ trunk/Source/WebKit/ChangeLog 2018-12-18 15:26:50 UTC (rev 239333)
@@ -1,3 +1,33 @@
+2018-12-18 Chris Dumez <[email protected]>
+
+ Regression(r239182) SuspendedPage's process reuse for link navigation optimization sometimes broken
+ https://bugs.webkit.org/show_bug.cgi?id=192772
+
+ Reviewed by Antti Koivisto.
+
+ With r239182, if the page in the previous process would fail to enter PageCache, we would destroy
+ the corresponding SuspendedPageProxy, which would potentially terminate the process. This would
+ regress performance when trying to navigate back in history to that page. This would also regress
+ performance when link-navigating to the same domain as we would have previously reused the suspended
+ page's process for such navigation.
+
+ Address the issue by keeping the SuspendedPageProxy alive even if the WebPage fails to suspend.
+ When trying to reuse a SuspendedPageProxy, if the page failed to suspend, reuse its process but
+ not the suspended page itself.
+
+ * UIProcess/SuspendedPageProxy.cpp:
+ (WebKit::SuspendedPageProxy::~SuspendedPageProxy):
+ (WebKit::SuspendedPageProxy::waitUntilReadyToUnsuspend):
+ (WebKit::SuspendedPageProxy::unsuspend):
+ (WebKit::SuspendedPageProxy::didSuspend):
+ (WebKit::SuspendedPageProxy::didFailToSuspend):
+ (WebKit::SuspendedPageProxy::loggingString const):
+ * UIProcess/SuspendedPageProxy.h:
+ * UIProcess/WebPageProxy.cpp:
+ (WebKit::WebPageProxy::swapToWebProcess):
+ * UIProcess/WebProcessPool.cpp:
+ (WebKit::WebProcessPool::processForNavigationInternal):
+
2018-12-17 Jiewen Tan <[email protected]>
[Mac] Layout Test http/wpt/webauthn/public-key-credential-create-success-hid.https.html and http/wpt/webauthn/public-key-credential-get-success-hid.https.html are flaky
Modified: trunk/Source/WebKit/UIProcess/SuspendedPageProxy.cpp (239332 => 239333)
--- trunk/Source/WebKit/UIProcess/SuspendedPageProxy.cpp 2018-12-18 15:25:32 UTC (rev 239332)
+++ trunk/Source/WebKit/UIProcess/SuspendedPageProxy.cpp 2018-12-18 15:26:50 UTC (rev 239333)
@@ -94,7 +94,7 @@
if (m_readyToUnsuspendHandler)
m_readyToUnsuspendHandler(nullptr);
- if (!m_isSuspended)
+ if (m_suspensionState == SuspensionState::Resumed)
return;
// If the suspended page was not consumed before getting destroyed, then close the corresponding page
@@ -114,18 +114,26 @@
if (m_readyToUnsuspendHandler)
m_readyToUnsuspendHandler(nullptr);
- if (!m_finishedSuspending)
+ switch (m_suspensionState) {
+ case SuspensionState::Suspending:
m_readyToUnsuspendHandler = WTFMove(completionHandler);
- else
+ break;
+ case SuspensionState::FailedToSuspend:
+ case SuspensionState::Suspended:
completionHandler(this);
+ break;
+ case SuspensionState::Resumed:
+ ASSERT_NOT_REACHED();
+ completionHandler(nullptr);
+ break;
+ }
}
void SuspendedPageProxy::unsuspend()
{
- ASSERT(m_isSuspended);
- ASSERT(m_finishedSuspending);
+ ASSERT(m_suspensionState == SuspensionState::Suspended);
- m_isSuspended = false;
+ m_suspensionState = SuspensionState::Resumed;
m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID());
m_process->send(Messages::WebPage::SetIsSuspended(false), m_page.pageID());
}
@@ -134,7 +142,8 @@
{
LOG(ProcessSwapping, "SuspendedPageProxy %s from process %i finished transition to suspended", loggingString(), m_process->processIdentifier());
- m_finishedSuspending = true;
+ ASSERT(m_suspensionState == SuspensionState::Suspending);
+ m_suspensionState = SuspensionState::Suspended;
#if PLATFORM(IOS_FAMILY)
m_suspensionToken = nullptr;
@@ -146,11 +155,11 @@
void SuspendedPageProxy::didFailToSuspend()
{
- // We are unusable due to failure to suspend so remove ourselves from the WebProcessPool.
- auto protectedThis = m_process->processPool().takeSuspendedPage(*this);
+ ASSERT(m_suspensionState == SuspensionState::Suspending);
+ m_suspensionState = SuspensionState::FailedToSuspend;
if (m_readyToUnsuspendHandler)
- m_readyToUnsuspendHandler(nullptr);
+ m_readyToUnsuspendHandler(this);
}
void SuspendedPageProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder& decoder)
@@ -180,7 +189,7 @@
#if !LOG_DISABLED
const char* SuspendedPageProxy::loggingString() const
{
- return debugString("(", String::format("%p", this), " page ID ", String::number(m_page.pageID()), ", m_finishedSuspending ", String::number(m_finishedSuspending), ")");
+ return debugString("(", String::format("%p", this), " page ID ", String::number(m_page.pageID()), ", m_suspensionState ", String::number(static_cast<unsigned>(m_suspensionState)), ")");
}
#endif
Modified: trunk/Source/WebKit/UIProcess/SuspendedPageProxy.h (239332 => 239333)
--- trunk/Source/WebKit/UIProcess/SuspendedPageProxy.h 2018-12-18 15:25:32 UTC (rev 239332)
+++ trunk/Source/WebKit/UIProcess/SuspendedPageProxy.h 2018-12-18 15:26:50 UTC (rev 239333)
@@ -47,6 +47,8 @@
uint64_t mainFrameID() const { return m_mainFrameID; }
const String& registrableDomain() const { return m_registrableDomain; }
+ bool failedToSuspend() const { return m_suspensionState == SuspensionState::FailedToSuspend; }
+
void waitUntilReadyToUnsuspend(CompletionHandler<void(SuspendedPageProxy*)>&&);
void unsuspend();
@@ -67,9 +69,8 @@
uint64_t m_mainFrameID;
String m_registrableDomain;
- bool m_isSuspended { true };
-
- bool m_finishedSuspending { false };
+ enum class SuspensionState : uint8_t { Suspending, FailedToSuspend, Suspended, Resumed };
+ SuspensionState m_suspensionState { SuspensionState::Suspending };
CompletionHandler<void(SuspendedPageProxy*)> m_readyToUnsuspendHandler;
#if PLATFORM(IOS_FAMILY)
ProcessThrottler::BackgroundActivityToken m_suspensionToken;
Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.cpp (239332 => 239333)
--- trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2018-12-18 15:25:32 UTC (rev 239332)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2018-12-18 15:26:50 UTC (rev 239333)
@@ -780,11 +780,16 @@
// case, we need to initialize m_mainFrame to reflect the fact the the WebProcess' WebPage
// already exists and already has a main frame.
if (destinationSuspendedPage) {
- ASSERT(!m_mainFrame);
- ASSERT(&destinationSuspendedPage->process() == m_process.ptr());
- destinationSuspendedPage->unsuspend();
- m_mainFrame = WebFrameProxy::create(*this, destinationSuspendedPage->mainFrameID());
- m_process->frameCreated(destinationSuspendedPage->mainFrameID(), *m_mainFrame);
+ if (!destinationSuspendedPage->failedToSuspend()) {
+ ASSERT(!m_mainFrame);
+ ASSERT(&destinationSuspendedPage->process() == m_process.ptr());
+ destinationSuspendedPage->unsuspend();
+ m_mainFrame = WebFrameProxy::create(*this, destinationSuspendedPage->mainFrameID());
+ m_process->frameCreated(destinationSuspendedPage->mainFrameID(), *m_mainFrame);
+ } else {
+ // We failed to suspend the page so destroy it now and merely reuse its WebContent process.
+ destinationSuspendedPage = nullptr;
+ }
}
m_process->addExistingWebPage(*this, m_pageID, WebProcessProxy::BeginsUsingDataStore::No);
Modified: trunk/Source/WebKit/UIProcess/WebProcessPool.cpp (239332 => 239333)
--- trunk/Source/WebKit/UIProcess/WebProcessPool.cpp 2018-12-18 15:25:32 UTC (rev 239332)
+++ trunk/Source/WebKit/UIProcess/WebProcessPool.cpp 2018-12-18 15:26:50 UTC (rev 239333)
@@ -2177,7 +2177,7 @@
if (auto* suspendedPage = backForwardListItem->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 process failed to suspend"_s);
+ 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);
});
Modified: trunk/Tools/ChangeLog (239332 => 239333)
--- trunk/Tools/ChangeLog 2018-12-18 15:25:32 UTC (rev 239332)
+++ trunk/Tools/ChangeLog 2018-12-18 15:26:50 UTC (rev 239333)
@@ -1,3 +1,14 @@
+2018-12-18 Chris Dumez <[email protected]>
+
+ Regression(r239182) SuspendedPage's process reuse for link navigation optimization sometimes broken
+ https://bugs.webkit.org/show_bug.cgi?id=192772
+
+ Reviewed by Antti Koivisto.
+
+ Add API test coverage.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
2018-12-18 Philippe Normand <[email protected]>
Unreviewed, JHBuild GTK build fix attempt
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (239332 => 239333)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2018-12-18 15:25:32 UTC (rev 239332)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2018-12-18 15:26:50 UTC (rev 239333)
@@ -1792,6 +1792,110 @@
EXPECT_EQ(applePID, [webView _webProcessIdentifier]);
}
+static const char* failsToEnterPageCacheTestBytes = R"PSONRESOURCE(
+<body>
+<script>
+// Pages with dedicated workers do not go into page cache.
+var myWorker = new Worker('worker.js');
+</script>
+</body>
+)PSONRESOURCE";
+
+TEST(ProcessSwap, ReuseSuspendedProcessEvenIfPageCacheFails)
+{
+ 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:failsToEnterPageCacheTestBytes];
+ [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.apple.com/main1.html"]];
+ [webView loadRequest:request];
+
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ auto applePID = [webView _webProcessIdentifier];
+
+ EXPECT_NE(webkitPID, applePID);
+
+ request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main2.html"]];
+ [webView loadRequest:request];
+
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ // We should have gone back to the webkit.org process for this load since we reuse SuspendedPages' process when possible.
+ EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
+
+ request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main2.html"]];
+ [webView loadRequest:request];
+
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ // We should have gone back to the apple.com process for this load since we reuse SuspendedPages' process when possible.
+ EXPECT_EQ(applePID, [webView _webProcessIdentifier]);
+}
+
+TEST(ProcessSwap, ReuseSuspendedProcessOnBackEvenIfPageCacheFails)
+{
+ 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:failsToEnterPageCacheTestBytes];
+ [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.apple.com/main1.html"]];
+ [webView loadRequest:request];
+
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ auto applePID = [webView _webProcessIdentifier];
+
+ EXPECT_NE(webkitPID, applePID);
+
+ [webView goBack];
+
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
+}
+
static const char* mainFramesOnlyMainFrame = R"PSONRESOURCE(
<script>
function loaded() {
