Diff
Modified: trunk/Source/WebKit/ChangeLog (238060 => 238061)
--- trunk/Source/WebKit/ChangeLog 2018-11-09 23:10:47 UTC (rev 238060)
+++ trunk/Source/WebKit/ChangeLog 2018-11-10 00:24:08 UTC (rev 238061)
@@ -1,3 +1,35 @@
+2018-11-09 Chris Dumez <[email protected]>
+
+ Suspended page persists even after back/forward list item is gone
+ https://bugs.webkit.org/show_bug.cgi?id=191488
+ <rdar://problem/45953006>
+
+ Reviewed by Geoffrey Garen.
+
+ Currently, the WebProcessPool owns the SuspendedPageProxy objects and makes sure we cap how
+ many we can have. The WebBackForwardListItem merely has a WeakPtr to its associated
+ SuspendedPageProxy. However, there is no point in having the WebProcessPool keeping a
+ SuspendedPageProxy object alive if there is no longer any WebBackForwardListItem pointing
+ to it.
+
+ To address the issue, WebBackForwardListItem nows tells the WebProcessPool to destroy
+ its SuspendedPageProxy when necessary. WebBackForwardList also takes care of nulling
+ out the WebBackForwardListItem's SuspendedPageProxy after the item has been removed
+ from the list (in case somebody keeps the item alive).
+
+ * Shared/WebBackForwardListItem.cpp:
+ (WebKit::WebBackForwardListItem::~WebBackForwardListItem):
+ (WebKit::WebBackForwardListItem::setSuspendedPage):
+ (WebKit::WebBackForwardListItem::suspendedPageIsNoLongerNeeded):
+ * Shared/WebBackForwardListItem.h:
+ * UIProcess/SuspendedPageProxy.cpp:
+ (WebKit::SuspendedPageProxy::SuspendedPageProxy):
+ * UIProcess/WebBackForwardList.cpp:
+ (WebKit::WebBackForwardList::didRemoveItem):
+ * UIProcess/WebProcessPool.cpp:
+ (WebKit::WebProcessPool::removeSuspendedPageProxy):
+ * UIProcess/WebProcessPool.h:
+
2018-11-09 Wenson Hsieh <[email protected]>
[Cocoa] Implement SPI on WKWebView to increase and decrease list levels
Modified: trunk/Source/WebKit/Shared/WebBackForwardListItem.cpp (238060 => 238061)
--- trunk/Source/WebKit/Shared/WebBackForwardListItem.cpp 2018-11-09 23:10:47 UTC (rev 238060)
+++ trunk/Source/WebKit/Shared/WebBackForwardListItem.cpp 2018-11-10 00:24:08 UTC (rev 238061)
@@ -27,6 +27,8 @@
#include "WebBackForwardListItem.h"
#include "SuspendedPageProxy.h"
+#include "WebProcessPool.h"
+#include "WebProcessProxy.h"
#include <WebCore/URL.h>
#include <wtf/DebugUtilities.h>
@@ -51,6 +53,8 @@
{
ASSERT(allItems().get(m_itemState.identifier) == this);
allItems().remove(m_itemState.identifier);
+
+ removeSuspendedPageFromProcessPool();
}
HashMap<BackForwardItemIdentifier, WebBackForwardListItem*>& WebBackForwardListItem::allItems()
@@ -113,11 +117,24 @@
return documentTreesAreEqual(mainFrameState, otherMainFrameState);
}
-void WebBackForwardListItem::setSuspendedPage(SuspendedPageProxy& page)
+void WebBackForwardListItem::setSuspendedPage(SuspendedPageProxy* page)
{
+ if (m_suspendedPage == page)
+ return;
+
+ removeSuspendedPageFromProcessPool();
m_suspendedPage = makeWeakPtr(page);
}
+void WebBackForwardListItem::removeSuspendedPageFromProcessPool()
+{
+ if (!m_suspendedPage)
+ return;
+
+ m_suspendedPage->process().processPool().removeSuspendedPage(*m_suspendedPage);
+ ASSERT(!m_suspendedPage);
+}
+
#if !LOG_DISABLED
const char* WebBackForwardListItem::loggingString()
{
Modified: trunk/Source/WebKit/Shared/WebBackForwardListItem.h (238060 => 238061)
--- trunk/Source/WebKit/Shared/WebBackForwardListItem.h 2018-11-09 23:10:47 UTC (rev 238060)
+++ trunk/Source/WebKit/Shared/WebBackForwardListItem.h 2018-11-10 00:24:08 UTC (rev 238061)
@@ -69,7 +69,7 @@
ViewSnapshot* snapshot() const { return m_itemState.snapshot.get(); }
void setSnapshot(RefPtr<ViewSnapshot>&& snapshot) { m_itemState.snapshot = WTFMove(snapshot); }
#endif
- void setSuspendedPage(SuspendedPageProxy&);
+ void setSuspendedPage(SuspendedPageProxy*);
SuspendedPageProxy* suspendedPage() const { return m_suspendedPage.get(); }
#if !LOG_DISABLED
@@ -79,6 +79,8 @@
private:
explicit WebBackForwardListItem(BackForwardListItemState&&, uint64_t pageID);
+ void removeSuspendedPageFromProcessPool();
+
BackForwardListItemState m_itemState;
uint64_t m_pageID;
WeakPtr<SuspendedPageProxy> m_suspendedPage;
Modified: trunk/Source/WebKit/UIProcess/SuspendedPageProxy.cpp (238060 => 238061)
--- trunk/Source/WebKit/UIProcess/SuspendedPageProxy.cpp 2018-11-09 23:10:47 UTC (rev 238060)
+++ trunk/Source/WebKit/UIProcess/SuspendedPageProxy.cpp 2018-11-10 00:24:08 UTC (rev 238061)
@@ -80,7 +80,7 @@
, m_mainFrameID(mainFrameID)
, m_registrableDomain(toRegistrableDomain(URL(URL(), item.url())))
{
- item.setSuspendedPage(*this);
+ item.setSuspendedPage(this);
m_process->addMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID(), *this);
m_process->send(Messages::WebPage::SetIsSuspended(true), m_page.pageID());
Modified: trunk/Source/WebKit/UIProcess/WebBackForwardList.cpp (238060 => 238061)
--- trunk/Source/WebKit/UIProcess/WebBackForwardList.cpp 2018-11-09 23:10:47 UTC (rev 238060)
+++ trunk/Source/WebKit/UIProcess/WebBackForwardList.cpp 2018-11-10 00:24:08 UTC (rev 238061)
@@ -467,6 +467,7 @@
{
m_page->backForwardRemovedItem(backForwardListItem.itemID());
+ backForwardListItem.setSuspendedPage(nullptr);
#if PLATFORM(COCOA)
backForwardListItem.setSnapshot(nullptr);
#endif
Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.cpp (238060 => 238061)
--- trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2018-11-09 23:10:47 UTC (rev 238060)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2018-11-10 00:24:08 UTC (rev 238061)
@@ -729,7 +729,7 @@
LOG(ProcessSwapping, "WebPageProxy %" PRIu64 " created suspended page %s for process pid %i, back/forward item %s" PRIu64, pageID(), suspendedPage->loggingString(), m_process->processIdentifier(), currentItem->itemID().logString());
- m_process->processPool().addSuspendedPageProxy(WTFMove(suspendedPage));
+ m_process->processPool().addSuspendedPage(WTFMove(suspendedPage));
return true;
}
@@ -741,7 +741,7 @@
std::unique_ptr<SuspendedPageProxy> destinationSuspendedPage;
if (auto* backForwardListItem = navigation.targetItem()) {
if (backForwardListItem->suspendedPage() && &backForwardListItem->suspendedPage()->process() == process.ptr()) {
- destinationSuspendedPage = this->process().processPool().takeSuspendedPageProxy(*backForwardListItem->suspendedPage());
+ destinationSuspendedPage = this->process().processPool().takeSuspendedPage(*backForwardListItem->suspendedPage());
ASSERT(destinationSuspendedPage);
destinationSuspendedPage->unsuspend();
}
@@ -940,7 +940,7 @@
m_webProcessLifetimeTracker.pageWasInvalidated();
- m_process->processPool().removeAllSuspendedPageProxiesForPage(*this);
+ m_process->processPool().removeAllSuspendedPagesForPage(*this);
m_process->send(Messages::WebPage::Close(), m_pageID);
m_process->removeWebPage(*this, m_pageID);
Modified: trunk/Source/WebKit/UIProcess/WebProcessPool.cpp (238060 => 238061)
--- trunk/Source/WebKit/UIProcess/WebProcessPool.cpp 2018-11-09 23:10:47 UTC (rev 238060)
+++ trunk/Source/WebKit/UIProcess/WebProcessPool.cpp 2018-11-10 00:24:08 UTC (rev 238061)
@@ -2248,7 +2248,7 @@
return createNewWebProcess(page.websiteDataStore());
}
-void WebProcessPool::addSuspendedPageProxy(std::unique_ptr<SuspendedPageProxy>&& suspendedPage)
+void WebProcessPool::addSuspendedPage(std::unique_ptr<SuspendedPageProxy>&& suspendedPage)
{
if (m_suspendedPages.size() >= m_maxSuspendedPageCount)
m_suspendedPages.removeFirst();
@@ -2256,7 +2256,7 @@
m_suspendedPages.append(WTFMove(suspendedPage));
}
-void WebProcessPool::removeAllSuspendedPageProxiesForPage(WebPageProxy& page)
+void WebProcessPool::removeAllSuspendedPagesForPage(WebPageProxy& page)
{
m_suspendedPages.removeAllMatching([&page](auto& suspendedPage) {
return &suspendedPage->page() == &page;
@@ -2263,7 +2263,7 @@
});
}
-std::unique_ptr<SuspendedPageProxy> WebProcessPool::takeSuspendedPageProxy(SuspendedPageProxy& suspendedPage)
+std::unique_ptr<SuspendedPageProxy> WebProcessPool::takeSuspendedPage(SuspendedPageProxy& suspendedPage)
{
return m_suspendedPages.takeFirst([&suspendedPage](auto& item) {
return item.get() == &suspendedPage;
@@ -2270,8 +2270,17 @@
});
}
-bool WebProcessPool::hasSuspendedPageProxyFor(WebProcessProxy& process) const
+void WebProcessPool::removeSuspendedPage(SuspendedPageProxy& suspendedPage)
{
+ auto it = m_suspendedPages.findIf([&suspendedPage](auto& item) {
+ return item.get() == &suspendedPage;
+ });
+ if (it != m_suspendedPages.end())
+ m_suspendedPages.remove(it);
+}
+
+bool WebProcessPool::hasSuspendedPageFor(WebProcessProxy& process) const
+{
return m_suspendedPages.findIf([&process](auto& suspendedPage) {
return &suspendedPage->process() == &process;
}) != m_suspendedPages.end();
Modified: trunk/Source/WebKit/UIProcess/WebProcessPool.h (238060 => 238061)
--- trunk/Source/WebKit/UIProcess/WebProcessPool.h 2018-11-09 23:10:47 UTC (rev 238060)
+++ trunk/Source/WebKit/UIProcess/WebProcessPool.h 2018-11-10 00:24:08 UTC (rev 238061)
@@ -446,10 +446,11 @@
Ref<WebProcessProxy> processForNavigation(WebPageProxy&, const API::Navigation&, ProcessSwapRequestedByClient, String& reason);
// SuspendedPageProxy management.
- void addSuspendedPageProxy(std::unique_ptr<SuspendedPageProxy>&&);
- void removeAllSuspendedPageProxiesForPage(WebPageProxy&);
- std::unique_ptr<SuspendedPageProxy> takeSuspendedPageProxy(SuspendedPageProxy&);
- bool hasSuspendedPageProxyFor(WebProcessProxy&) const;
+ void addSuspendedPage(std::unique_ptr<SuspendedPageProxy>&&);
+ void removeAllSuspendedPagesForPage(WebPageProxy&);
+ std::unique_ptr<SuspendedPageProxy> takeSuspendedPage(SuspendedPageProxy&);
+ void removeSuspendedPage(SuspendedPageProxy&);
+ bool hasSuspendedPageFor(WebProcessProxy&) const;
unsigned maxSuspendedPageCount() const { return m_maxSuspendedPageCount; }
void didReachGoodTimeToPrewarm();
Modified: trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp (238060 => 238061)
--- trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp 2018-11-09 23:10:47 UTC (rev 238060)
+++ trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp 2018-11-10 00:24:08 UTC (rev 238061)
@@ -865,7 +865,7 @@
bool WebProcessProxy::canTerminateChildProcess()
{
- if (!m_pageMap.isEmpty() || m_processPool->hasSuspendedPageProxyFor(*this))
+ if (!m_pageMap.isEmpty() || m_processPool->hasSuspendedPageFor(*this))
return false;
if (!m_processPool->shouldTerminate(this))
Modified: trunk/Tools/ChangeLog (238060 => 238061)
--- trunk/Tools/ChangeLog 2018-11-09 23:10:47 UTC (rev 238060)
+++ trunk/Tools/ChangeLog 2018-11-10 00:24:08 UTC (rev 238061)
@@ -1,3 +1,15 @@
+2018-11-09 Chris Dumez <[email protected]>
+
+ Suspended page persists even after back/forward list item is gone
+ https://bugs.webkit.org/show_bug.cgi?id=191488
+ <rdar://problem/45953006>
+
+ Reviewed by Geoffrey Garen.
+
+ Add API test coverage.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
2018-11-09 Wenson Hsieh <[email protected]>
[Cocoa] Implement SPI on WKWebView to increase and decrease list levels
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm (238060 => 238061)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2018-11-09 23:10:47 UTC (rev 238060)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm 2018-11-10 00:24:08 UTC (rev 238061)
@@ -623,6 +623,65 @@
EXPECT_EQ(5u, seenPIDs.size());
}
+TEST(ProcessSwap, SuspendedPageDiesAfterBackForwardListItemIsGone)
+{
+ auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
+ processPoolConfiguration.get().processSwapsOnNavigation = 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]);
+ [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()];
+
+ EXPECT_GT([processPool _maximumSuspendedPageCount], 0U);
+
+ NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
+ [webView loadRequest:request];
+
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ auto 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);
+
+ // webkit.org + apple.com processes.
+ EXPECT_EQ(2U, [processPool _webProcessCountIgnoringPrewarmed]);
+
+ [webView goBack]; // Back to webkit.org.
+ TestWebKitAPI::Util::run(&done);
+ done = false;
+
+ EXPECT_EQ(webkitPID, [webView _webProcessIdentifier]);
+
+ // webkit.org + apple.com processes.
+ EXPECT_EQ(2U, [processPool _webProcessCountIgnoringPrewarmed]);
+
+ 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]);
+
+ // apple.com is not longer present in the back/forward list and there should therefore be no-suspended page for it.
+ while ([processPool _webProcessCountIgnoringPrewarmed] > 1u)
+ TestWebKitAPI::Util::spinRunLoop();
+}
+
#if PLATFORM(MAC)
TEST(ProcessSwap, SuspendedPagesInActivityMonitor)
{
