WebKit Bugzilla
Attachment 357479 Details for
Bug 192772
: Regression(r239182) SuspendedPage's process reuse for link navigation optimization sometimes broken
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
bug-192772-20181217144322.patch (text/plain), 14.57 KB, created by
Chris Dumez
on 2018-12-17 14:43:23 PST
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Chris Dumez
Created:
2018-12-17 14:43:23 PST
Size:
14.57 KB
patch
obsolete
>Subversion Revision: 239282 >diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog >index cc9c4ab4fa4c7dca456962bff2591f4c082d317b..b7138215b8a81bdc50666af52a9661167f8cde95 100644 >--- a/Source/WebKit/ChangeLog >+++ b/Source/WebKit/ChangeLog >@@ -1,3 +1,33 @@ >+2018-12-17 Chris Dumez <cdumez@apple.com> >+ >+ Regression(r239182) SuspendedPage's process reuse for link navigation optimization sometimes broken >+ https://bugs.webkit.org/show_bug.cgi?id=192772 >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ 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 Zan Dobersek <zdobersek@igalia.com> > > Unreviewed WPE build fix after r239277. >diff --git a/Source/WebKit/UIProcess/SuspendedPageProxy.cpp b/Source/WebKit/UIProcess/SuspendedPageProxy.cpp >index 7ed22380405003c06f2b637eab0fd1c4f0c23863..61a0af9ac4c4cd07949b9ca9b0d8af6b20cc2c8a 100644 >--- a/Source/WebKit/UIProcess/SuspendedPageProxy.cpp >+++ b/Source/WebKit/UIProcess/SuspendedPageProxy.cpp >@@ -94,7 +94,7 @@ SuspendedPageProxy::~SuspendedPageProxy() > 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 @@ void SuspendedPageProxy::waitUntilReadyToUnsuspend(CompletionHandler<void(Suspen > 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 @@ void SuspendedPageProxy::didSuspend() > { > 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::didSuspend() > > 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 @@ void SuspendedPageProxy::didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, > #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 > >diff --git a/Source/WebKit/UIProcess/SuspendedPageProxy.h b/Source/WebKit/UIProcess/SuspendedPageProxy.h >index b661f52a86c5236cd7f182adba2576fbf86a0153..1b56d06af6681da2b636b994f875bd19e24b05b9 100644 >--- a/Source/WebKit/UIProcess/SuspendedPageProxy.h >+++ b/Source/WebKit/UIProcess/SuspendedPageProxy.h >@@ -47,6 +47,8 @@ public: > 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 @@ private: > 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; >diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp >index 50224ed1d166e5b9878df95bac909cac4fc2b27f..fcb4c8373f323cb3ee364d77773aa236dde3befc 100644 >--- a/Source/WebKit/UIProcess/WebPageProxy.cpp >+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp >@@ -780,11 +780,16 @@ void WebPageProxy::swapToWebProcess(Ref<WebProcessProxy>&& process, std::unique_ > // 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); >diff --git a/Source/WebKit/UIProcess/WebProcessPool.cpp b/Source/WebKit/UIProcess/WebProcessPool.cpp >index fa43a3d1e783092bb7c0218147fd164f827b03c0..82830a53f61494b12020d48152078a5c93e2a5ca 100644 >--- a/Source/WebKit/UIProcess/WebProcessPool.cpp >+++ b/Source/WebKit/UIProcess/WebProcessPool.cpp >@@ -2177,7 +2177,7 @@ void WebProcessPool::processForNavigationInternal(WebPageProxy& page, const API: > 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); > }); >diff --git a/Tools/ChangeLog b/Tools/ChangeLog >index e7c0324a8cc02f13896c2d323169ed189ed6796a..ff857b1ef58bdf9bfe946369c5786b567c706053 100644 >--- a/Tools/ChangeLog >+++ b/Tools/ChangeLog >@@ -1,3 +1,14 @@ >+2018-12-17 Chris Dumez <cdumez@apple.com> >+ >+ Regression(r239182) SuspendedPage's process reuse for link navigation optimization sometimes broken >+ https://bugs.webkit.org/show_bug.cgi?id=192772 >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ Add API test coverage. >+ >+ * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm: >+ > 2018-12-17 Daniel Bates <dabates@apple.com> > > Support concatenating StringView with other string types >diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm >index 0701dc4be64909b96c2db8f5316221002c236619..2434bcec8c1e11b543bdb78f8d5f2fe5be97f7a3 100644 >--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm >+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm >@@ -1792,6 +1792,110 @@ TEST(ProcessSwap, ReuseSuspendedProcess) > 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() {
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 192772
: 357479