Suspended page persists even after back/forward list item is gone
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 10 Nov 2018 00:24:08 +0000 (00:24 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 10 Nov 2018 00:24:08 +0000 (00:24 +0000)
https://bugs.webkit.org/show_bug.cgi?id=191488
<rdar://problem/45953006>

Reviewed by Geoffrey Garen.

Source/WebKit:

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:

Tools:

Add API test coverage.

* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@238061 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/Shared/WebBackForwardListItem.cpp
Source/WebKit/Shared/WebBackForwardListItem.h
Source/WebKit/UIProcess/SuspendedPageProxy.cpp
Source/WebKit/UIProcess/WebBackForwardList.cpp
Source/WebKit/UIProcess/WebPageProxy.cpp
Source/WebKit/UIProcess/WebProcessPool.cpp
Source/WebKit/UIProcess/WebProcessPool.h
Source/WebKit/UIProcess/WebProcessProxy.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm

index 3acff34..fca5ade 100644 (file)
@@ -1,3 +1,35 @@
+2018-11-09  Chris Dumez  <cdumez@apple.com>
+
+        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  <wenson_hsieh@apple.com>
 
         [Cocoa] Implement SPI on WKWebView to increase and decrease list levels
index 612db23..a666236 100644 (file)
@@ -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 @@ WebBackForwardListItem::~WebBackForwardListItem()
 {
     ASSERT(allItems().get(m_itemState.identifier) == this);
     allItems().remove(m_itemState.identifier);
+
+    removeSuspendedPageFromProcessPool();
 }
 
 HashMap<BackForwardItemIdentifier, WebBackForwardListItem*>& WebBackForwardListItem::allItems()
@@ -113,11 +117,24 @@ bool WebBackForwardListItem::itemIsInSameDocument(const WebBackForwardListItem&
     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()
 {
index 4376648..3dad9b7 100644 (file)
@@ -69,7 +69,7 @@ public:
     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 @@ public:
 private:
     explicit WebBackForwardListItem(BackForwardListItemState&&, uint64_t pageID);
 
+    void removeSuspendedPageFromProcessPool();
+
     BackForwardListItemState m_itemState;
     uint64_t m_pageID;
     WeakPtr<SuspendedPageProxy> m_suspendedPage;
index 9495826..ddf0e15 100644 (file)
@@ -80,7 +80,7 @@ SuspendedPageProxy::SuspendedPageProxy(WebPageProxy& page, Ref<WebProcessProxy>&
     , 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());
index bc3b4ac..22ca4c3 100644 (file)
@@ -467,6 +467,7 @@ void WebBackForwardList::didRemoveItem(WebBackForwardListItem& backForwardListIt
 {
     m_page->backForwardRemovedItem(backForwardListItem.itemID());
 
+    backForwardListItem.setSuspendedPage(nullptr);
 #if PLATFORM(COCOA)
     backForwardListItem.setSnapshot(nullptr);
 #endif
index 8375ca3..f6fda75 100644 (file)
@@ -729,7 +729,7 @@ bool WebPageProxy::suspendCurrentPageIfPossible(API::Navigation& navigation, std
 
     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 @@ void WebPageProxy::swapToWebProcess(Ref<WebProcessProxy>&& process, API::Navigat
     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 @@ void WebPageProxy::close()
 
     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);
index e4a8770..a42b013 100644 (file)
@@ -2248,7 +2248,7 @@ Ref<WebProcessProxy> WebProcessPool::processForNavigationInternal(WebPageProxy&
     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,21 +2256,30 @@ void WebProcessPool::addSuspendedPageProxy(std::unique_ptr<SuspendedPageProxy>&&
     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;
     });
 }
 
-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;
     });
 }
 
-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;
index d3a8d8e..553dac5 100644 (file)
@@ -446,10 +446,11 @@ public:
     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();
index 199c5a0..1c05f34 100644 (file)
@@ -865,7 +865,7 @@ void WebProcessProxy::maybeShutDown()
 
 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))
index d3ee532..c058a2a 100644 (file)
@@ -1,3 +1,15 @@
+2018-11-09  Chris Dumez  <cdumez@apple.com>
+
+        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  <wenson_hsieh@apple.com>
 
         [Cocoa] Implement SPI on WKWebView to increase and decrease list levels
index b2ec925..fbe8acc 100644 (file)
@@ -623,6 +623,65 @@ TEST(ProcessSwap, Back)
         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)
 {