[PSON] Cap number of SuspendedPageProxy objects and allow a WebPageProxy to have...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 18 Oct 2018 15:33:54 +0000 (15:33 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 18 Oct 2018 15:33:54 +0000 (15:33 +0000)
https://bugs.webkit.org/show_bug.cgi?id=190688
<rdar://problem/45354095>

Reviewed by Antti Koivisto.

Source/WebKit:

Cap number of SuspendedPageProxy objects to 3 to avoid accumulating too many "suspended" processes.

Also allow a WebPageProxy to have more than one SuspendedPageProxy so that PageCache now works for
more than 1 history navigation (up to 3 with the suspended page limit in this patch).

The following cleanup / refactoring was made to support this:
- The SuspendedPageProxy objects are now owned by the WebProcessPool instead of the WebPageProxy.
  The WebProcessPool is in charge of limiting the number of SuspendedPageProxy objects by dropping
  the oldest one whenever a WebPageProxy tries to add a new one and we've already reached the limit.
- WebProcessProxy no longer needs to know anything about suspended pages, which simplifies the
  code quite a bit. Instead, the SuspendedPageProxy objects now register themselves as
  IPC::MessageReceivers with their WebProcessProxy. The SuspendedPageProxy also take care of
  calling maybeShutdown() on their WebProcessProxy when they get destroyed.

* UIProcess/SuspendedPageProxy.cpp:
(WebKit::SuspendedPageProxy::~SuspendedPageProxy):
(WebKit::SuspendedPageProxy::tearDownDrawingAreaInWebProcess):
(WebKit::SuspendedPageProxy::unsuspend):
(WebKit::SuspendedPageProxy::didFinishLoad):
(WebKit::SuspendedPageProxy::didReceiveSyncMessage):
* UIProcess/SuspendedPageProxy.h:
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::suspendCurrentPageIfPossible):
(WebKit::WebPageProxy::swapToWebProcess):
(WebKit::WebPageProxy::close):
(WebKit::WebPageProxy::continueNavigationInNewProcess):
(WebKit::WebPageProxy::didCompletePageTransition):
(WebKit::WebPageProxy::enterAcceleratedCompositingMode):
* UIProcess/WebPageProxy.h:
* UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::disconnectProcess):
(WebKit::WebProcessPool::processForNavigationInternal):
(WebKit::WebProcessPool::addSuspendedPageProxy):
(WebKit::WebProcessPool::removeAllSuspendedPageProxiesForPage):
(WebKit::WebProcessPool::takeSuspendedPageProxy):
(WebKit::WebProcessPool::hasSuspendedPageProxyFor):
* UIProcess/WebProcessPool.h:
* UIProcess/WebProcessProxy.cpp:
(WebKit::WebProcessProxy::didReceiveMessage):
(WebKit::WebProcessProxy::processDidTerminateOrFailedToLaunch):
(WebKit::WebProcessProxy::canTerminateChildProcess):
(WebKit::WebProcessProxy::requestTermination):
* UIProcess/WebProcessProxy.h:

Tools:

Extended API test coverage to confirm that:
- We do not accumulate more than 3 suspended processes.
- We can navigate back 3 times and use the page cache for each of these loads.

* TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:

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

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

index 3ca86d9..c188ead 100644 (file)
@@ -1,3 +1,55 @@
+2018-10-18  Chris Dumez  <cdumez@apple.com>
+
+        [PSON] Cap number of SuspendedPageProxy objects and allow a WebPageProxy to have more than one
+        https://bugs.webkit.org/show_bug.cgi?id=190688
+        <rdar://problem/45354095>
+
+        Reviewed by Antti Koivisto.
+
+        Cap number of SuspendedPageProxy objects to 3 to avoid accumulating too many "suspended" processes.
+
+        Also allow a WebPageProxy to have more than one SuspendedPageProxy so that PageCache now works for
+        more than 1 history navigation (up to 3 with the suspended page limit in this patch).
+
+        The following cleanup / refactoring was made to support this:
+        - The SuspendedPageProxy objects are now owned by the WebProcessPool instead of the WebPageProxy.
+          The WebProcessPool is in charge of limiting the number of SuspendedPageProxy objects by dropping
+          the oldest one whenever a WebPageProxy tries to add a new one and we've already reached the limit.
+        - WebProcessProxy no longer needs to know anything about suspended pages, which simplifies the
+          code quite a bit. Instead, the SuspendedPageProxy objects now register themselves as
+          IPC::MessageReceivers with their WebProcessProxy. The SuspendedPageProxy also take care of
+          calling maybeShutdown() on their WebProcessProxy when they get destroyed.
+
+        * UIProcess/SuspendedPageProxy.cpp:
+        (WebKit::SuspendedPageProxy::~SuspendedPageProxy):
+        (WebKit::SuspendedPageProxy::tearDownDrawingAreaInWebProcess):
+        (WebKit::SuspendedPageProxy::unsuspend):
+        (WebKit::SuspendedPageProxy::didFinishLoad):
+        (WebKit::SuspendedPageProxy::didReceiveSyncMessage):
+        * UIProcess/SuspendedPageProxy.h:
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::suspendCurrentPageIfPossible):
+        (WebKit::WebPageProxy::swapToWebProcess):
+        (WebKit::WebPageProxy::close):
+        (WebKit::WebPageProxy::continueNavigationInNewProcess):
+        (WebKit::WebPageProxy::didCompletePageTransition):
+        (WebKit::WebPageProxy::enterAcceleratedCompositingMode):
+        * UIProcess/WebPageProxy.h:
+        * UIProcess/WebProcessPool.cpp:
+        (WebKit::WebProcessPool::disconnectProcess):
+        (WebKit::WebProcessPool::processForNavigationInternal):
+        (WebKit::WebProcessPool::addSuspendedPageProxy):
+        (WebKit::WebProcessPool::removeAllSuspendedPageProxiesForPage):
+        (WebKit::WebProcessPool::takeSuspendedPageProxy):
+        (WebKit::WebProcessPool::hasSuspendedPageProxyFor):
+        * UIProcess/WebProcessPool.h:
+        * UIProcess/WebProcessProxy.cpp:
+        (WebKit::WebProcessProxy::didReceiveMessage):
+        (WebKit::WebProcessProxy::processDidTerminateOrFailedToLaunch):
+        (WebKit::WebProcessProxy::canTerminateChildProcess):
+        (WebKit::WebProcessProxy::requestTermination):
+        * UIProcess/WebProcessProxy.h:
+
 2018-10-17  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         Enable the datalist element by default on iOS and macOS
index 02ac661..14c5327 100644 (file)
@@ -81,44 +81,44 @@ SuspendedPageProxy::SuspendedPageProxy(WebPageProxy& page, Ref<WebProcessProxy>&
     , m_origin(SecurityOriginData::fromURL({ { }, item.url() }))
 {
     item.setSuspendedPage(*this);
-    m_process->processPool().registerSuspendedPageProxy(*this);
+    m_process->addMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID(), *this);
+
     m_process->send(Messages::WebPage::SetIsSuspended(true), m_page.pageID());
 }
 
 SuspendedPageProxy::~SuspendedPageProxy()
 {
-    if (auto process = m_process) {
-        process->send(Messages::WebPage::SetIsSuspended(false), m_page.pageID());
-        process->suspendedPageWasDestroyed(*this);
-        process->processPool().unregisterSuspendedPageProxy(*this);
-    }
-}
-
-void SuspendedPageProxy::webProcessDidClose(WebProcessProxy& process)
-{
-    ASSERT_UNUSED(process, &process == m_process);
+    if (!m_isSuspended)
+        return;
 
-    m_process->processPool().unregisterSuspendedPageProxy(*this);
-    m_process = nullptr;
+    // If the suspended page was not consumed before getting destroyed, then close the corresponding page
+    // on the WebProcess side.
+    m_process->send(Messages::WebPage::Close(), m_page.pageID());
+    m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID());
 
-    // This will destroy |this|.
-    m_page.suspendedPageClosed(*this);
+    // We call maybeShutDown() asynchronously since the SuspendedPage is currently being removed from the WebProcessPool
+    // and we want to avoid re-entering WebProcessPool methods.
+    RunLoop::main().dispatch([process = m_process.copyRef()] {
+        process->maybeShutDown();
+    });
 }
 
-void SuspendedPageProxy::destroyWebPageInWebProcess()
+void SuspendedPageProxy::tearDownDrawingAreaInWebProcess()
 {
-    m_process->send(Messages::WebPage::Close(), m_page.pageID());
-    m_page.suspendedPageClosed(*this);
+    m_process->send(Messages::WebPage::TearDownDrawingAreaForSuspend(), m_page.pageID());
 }
 
-void SuspendedPageProxy::tearDownDrawingAreaInWebProcess()
+void SuspendedPageProxy::unsuspend()
 {
-    m_process->send(Messages::WebPage::TearDownDrawingAreaForSuspend(), m_page.pageID());
+    ASSERT(m_isSuspended);
+
+    m_isSuspended = false;
+    m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID());
+    m_process->send(Messages::WebPage::SetIsSuspended(false), m_page.pageID());
 }
 
 void SuspendedPageProxy::didFinishLoad()
 {
-    ASSERT(m_process);
     LOG(ProcessSwapping, "SuspendedPageProxy %s from process %i finished transition to suspended", loggingString(), m_process->processIdentifier());
 
 #if !LOG_DISABLED
@@ -142,6 +142,10 @@ void SuspendedPageProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder& decod
 #endif
 }
 
+void SuspendedPageProxy::didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder>&)
+{
+}
+
 #if !LOG_DISABLED
 const char* SuspendedPageProxy::loggingString() const
 {
index 04c4ecf..0d5585e 100644 (file)
@@ -36,22 +36,20 @@ namespace WebKit {
 class WebPageProxy;
 class WebProcessProxy;
 
-class SuspendedPageProxy : public CanMakeWeakPtr<SuspendedPageProxy> {
+class SuspendedPageProxy final: public IPC::MessageReceiver, public CanMakeWeakPtr<SuspendedPageProxy> {
 public:
     SuspendedPageProxy(WebPageProxy&, Ref<WebProcessProxy>&&, WebBackForwardListItem&, uint64_t mainFrameID);
     ~SuspendedPageProxy();
 
-    void didReceiveMessage(IPC::Connection&, IPC::Decoder&);
-
     WebPageProxy& page() const { return m_page; }
-    WebProcessProxy* process() const { return m_process.get(); }
+    WebProcessProxy& process() { return m_process.get(); }
     uint64_t mainFrameID() const { return m_mainFrameID; }
     const WebCore::SecurityOriginData& origin() const { return m_origin; }
 
-    void webProcessDidClose(WebProcessProxy&);
-    void destroyWebPageInWebProcess();
     void tearDownDrawingAreaInWebProcess();
 
+    void unsuspend();
+
 #if !LOG_DISABLED
     const char* loggingString() const;
 #endif
@@ -59,11 +57,17 @@ public:
 private:
     void didFinishLoad();
 
+    // IPC::MessageReceiver
+    void didReceiveMessage(IPC::Connection&, IPC::Decoder&) final;
+    void didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder>&) final;
+
     WebPageProxy& m_page;
-    RefPtr<WebProcessProxy> m_process;
+    Ref<WebProcessProxy> m_process;
     uint64_t m_mainFrameID;
     WebCore::SecurityOriginData m_origin;
 
+    bool m_isSuspended { true };
+
 #if !LOG_DISABLED
     bool m_finishedSuspending { false };
 #endif
index bcf91b2..7ecf37e 100644 (file)
@@ -711,27 +711,22 @@ void WebPageProxy::reattachToWebProcess()
     finishAttachingToWebProcess();
 }
 
-SuspendedPageProxy* WebPageProxy::maybeCreateSuspendedPage(WebProcessProxy& process, API::Navigation& navigation, uint64_t mainFrameID)
+void WebPageProxy::suspendCurrentPageIfPossible(API::Navigation& navigation, std::optional<uint64_t> mainFrameID)
 {
-    ASSERT(!m_suspendedPage || m_suspendedPage->process() != &process);
+    if (!mainFrameID)
+        return;
 
     auto* currentItem = navigation.fromItem();
     if (!currentItem) {
-        LOG(ProcessSwapping, "WebPageProxy %" PRIu64 " unable to create suspended page for process pid %i - No current back/forward item", pageID(), process.processIdentifier());
-        return nullptr;
+        LOG(ProcessSwapping, "WebPageProxy %" PRIu64 " unable to create suspended page for process pid %i - No current back/forward item", pageID(), m_process->processIdentifier());
+        return;
     }
 
-    m_suspendedPage = std::make_unique<SuspendedPageProxy>(*this, process, *currentItem, mainFrameID);
-
-    LOG(ProcessSwapping, "WebPageProxy %" PRIu64 " created suspended page %s for process pid %i, back/forward item %s" PRIu64, pageID(), m_suspendedPage->loggingString(), process.processIdentifier(), currentItem->itemID().logString());
+    auto suspendedPage = std::make_unique<SuspendedPageProxy>(*this, m_process.copyRef(), *currentItem, *mainFrameID);
+    m_lastSuspendedPage = makeWeakPtr(suspendedPage.get());
+    m_process->processPool().addSuspendedPageProxy(WTFMove(suspendedPage));
 
-    return m_suspendedPage.get();
-}
-
-void WebPageProxy::suspendedPageClosed(SuspendedPageProxy& page)
-{
-    ASSERT_UNUSED(page, &page == m_suspendedPage.get());
-    m_suspendedPage = nullptr;
+    LOG(ProcessSwapping, "WebPageProxy %" PRIu64 " created suspended page %s for process pid %i, back/forward item %s" PRIu64, pageID(), m_lastSuspendedPage->loggingString(), m_process->processIdentifier(), currentItem->itemID().logString());
 }
 
 void WebPageProxy::swapToWebProcess(Ref<WebProcessProxy>&& process, API::Navigation& navigation, std::optional<uint64_t> mainFrameIDInPreviousProcess)
@@ -739,11 +734,18 @@ void WebPageProxy::swapToWebProcess(Ref<WebProcessProxy>&& process, API::Navigat
     ASSERT(!m_isClosed);
     RELEASE_LOG_IF_ALLOWED(Process, "%p WebPageProxy::swapToWebProcess\n", this);
 
-    // If the process we're attaching to is kept alive solely by our current suspended page,
-    // we need to maintain that by temporarily keeping the suspended page alive.
-    auto currentSuspendedPage = WTFMove(m_suspendedPage);
-    if (mainFrameIDInPreviousProcess)
-        m_process->suspendWebPageProxy(*this, navigation, *mainFrameIDInPreviousProcess);
+    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());
+            ASSERT(destinationSuspendedPage);
+            destinationSuspendedPage->unsuspend();
+        }
+    }
+
+    m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_pageID);
+    suspendCurrentPageIfPossible(navigation, mainFrameIDInPreviousProcess);
+    m_process->removeWebPage(*this, m_pageID);
 
     m_process = WTFMove(process);
     m_isValid = true;
@@ -752,10 +754,11 @@ void WebPageProxy::swapToWebProcess(Ref<WebProcessProxy>&& process, API::Navigat
     // WebPageProxy::didCreateMainFrame() will not be called to initialize m_mainFrame. In such
     // case, we need to initialize m_mainFrame to reflect the fact the the WebProcess' WebPage
     // already exists and already has a main frame.
-    if (currentSuspendedPage && currentSuspendedPage->process() == m_process.ptr()) {
+    if (destinationSuspendedPage) {
         ASSERT(!m_mainFrame);
-        m_mainFrame = WebFrameProxy::create(this, currentSuspendedPage->mainFrameID());
-        m_process->frameCreated(currentSuspendedPage->mainFrameID(), *m_mainFrame);
+        ASSERT(&destinationSuspendedPage->process() == m_process.ptr());
+        m_mainFrame = WebFrameProxy::create(this, destinationSuspendedPage->mainFrameID());
+        m_process->frameCreated(destinationSuspendedPage->mainFrameID(), *m_mainFrame);
     }
 
     finishAttachingToWebProcess();
@@ -928,6 +931,8 @@ void WebPageProxy::close()
 
     m_webProcessLifetimeTracker.pageWasInvalidated();
 
+    m_process->processPool().removeAllSuspendedPageProxiesForPage(*this);
+
     m_process->send(Messages::WebPage::Close(), m_pageID);
     m_process->removeWebPage(*this, m_pageID);
     m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_pageID);
@@ -2554,7 +2559,7 @@ void WebPageProxy::continueNavigationInNewProcess(API::Navigation& navigation, R
 
         auto itemStates = m_backForwardList->filteredItemStates([this, targetItem = item](WebBackForwardListItem& item) {
             if (auto* page = item.suspendedPage()) {
-                if (page->process() == m_process.ptr())
+                if (&page->process() == m_process.ptr())
                     return false;
             }
             return &item != targetItem;
@@ -3490,7 +3495,7 @@ void WebPageProxy::didFinishProgress()
 void WebPageProxy::didCompletePageTransition(bool isInitialEmptyDocument)
 {
     // Attach drawing area for the initial empty document only if this is not a process swap.
-    if (isInitialEmptyDocument && m_suspendedPage)
+    if (isInitialEmptyDocument && m_lastSuspendedPage)
         return;
 
 #if PLATFORM(MAC)
@@ -3500,8 +3505,8 @@ void WebPageProxy::didCompletePageTransition(bool isInitialEmptyDocument)
     // Drawing area for the suspended page will be torn down when the attach completes.
     m_drawingArea->attachInWebProcess();
 #else
-    if (m_suspendedPage)
-        m_suspendedPage->tearDownDrawingAreaInWebProcess();
+    if (m_lastSuspendedPage)
+        m_lastSuspendedPage->tearDownDrawingAreaInWebProcess();
 #endif
 }
 
@@ -6395,8 +6400,8 @@ void WebPageProxy::enterAcceleratedCompositingMode(const LayerTreeContext& layer
     pageClient().enterAcceleratedCompositingMode(layerTreeContext);
 
     // We have completed the page transition and can tear down the layers in the suspended process.
-    if (m_suspendedPage)
-        m_suspendedPage->tearDownDrawingAreaInWebProcess();
+    if (m_lastSuspendedPage)
+        m_lastSuspendedPage->tearDownDrawingAreaInWebProcess();
 }
 
 void WebPageProxy::exitAcceleratedCompositingMode()
index 70fb9b5..b8579d1 100644 (file)
@@ -1355,10 +1355,6 @@ public:
 
     WebPreferencesStore preferencesStore() const;
 
-    SuspendedPageProxy* maybeCreateSuspendedPage(WebProcessProxy&, API::Navigation&, uint64_t mainFrameID);
-    SuspendedPageProxy* suspendedPage() const { return m_suspendedPage.get(); }
-    void suspendedPageClosed(SuspendedPageProxy&);
-
     void setDefersLoadingForTesting(bool);
 
     WebCore::IntRect syncRootViewToScreen(const WebCore::IntRect& viewRect);
@@ -1379,6 +1375,8 @@ private:
     void updateThrottleState();
     void updateHiddenPageThrottlingAutoIncreases();
 
+    void suspendCurrentPageIfPossible(API::Navigation&, std::optional<uint64_t> mainFrameID);
+
     enum class ResetStateReason {
         PageInvalidated,
         WebProcessExited,
@@ -2260,10 +2258,8 @@ private:
 
     std::optional<MonotonicTime> m_pageLoadStart;
 
-    // FIXME: Support more than one suspended page per WebPageProxy,
-    // and have a global collection of them per process pool
-    // (e.g. for that process pool's page cache)
-    std::unique_ptr<SuspendedPageProxy> m_suspendedPage;
+    // FIXME: We should try and get rid of this data member.
+    WeakPtr<SuspendedPageProxy> m_lastSuspendedPage;
 
     RunLoop::Timer<WebPageProxy> m_resetRecentCrashCountTimer;
     unsigned m_recentCrashCount { 0 };
index a72b571..4a479ad 100644 (file)
@@ -124,7 +124,8 @@ using namespace WebCore;
 
 DEFINE_DEBUG_ONLY_GLOBAL(WTF::RefCountedLeakCounter, processPoolCounter, ("WebProcessPool"));
 
-static const Seconds serviceWorkerTerminationDelay { 5_s };
+const Seconds serviceWorkerTerminationDelay { 5_s };
+const unsigned maximumSuspendedPagesCount { 3 };
 
 static uint64_t generateListenerIdentifier()
 {
@@ -1022,6 +1023,10 @@ void WebProcessPool::disconnectProcess(WebProcessProxy* process)
     if (m_processWithPageCache == process)
         m_processWithPageCache = nullptr;
 
+    m_suspendedPages.removeAllMatching([process](auto& suspendedPage) {
+        return &suspendedPage->process() == process;
+    });
+
 #if ENABLE(SERVICE_WORKER)
     if (is<ServiceWorkerProcessProxy>(*process)) {
         auto* removedProcess = m_serviceWorkerProcesses.take(downcast<ServiceWorkerProcessProxy>(*process).securityOrigin());
@@ -1291,6 +1296,8 @@ void WebProcessPool::handleMemoryPressureWarning(Critical)
     if (m_prewarmedProcess)
         m_prewarmedProcess->shutDown();
     ASSERT(!m_prewarmedProcess);
+
+    m_suspendedPages.clear();
 }
 
 #if ENABLE(NETSCAPE_PLUGIN_API)
@@ -2117,10 +2124,9 @@ Ref<WebProcessProxy> WebProcessPool::processForNavigationInternal(WebPageProxy&
 
     if (auto* backForwardListItem = navigation.targetItem()) {
         if (auto* suspendedPage = backForwardListItem->suspendedPage()) {
-            ASSERT(suspendedPage->process());
             action = PolicyAction::Suspend;
             reason = "Using target back/forward item's process"_s;
-            return *suspendedPage->process();
+            return suspendedPage->process();
         }
 
         // If the target back/forward item and the current back/forward item originated
@@ -2171,10 +2177,7 @@ Ref<WebProcessProxy> WebProcessPool::processForNavigationInternal(WebPageProxy&
                 // 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.
-                if (auto* suspendedPage = page.suspendedPage()) {
-                    LOG(ProcessSwapping, "(ProcessSwapping) Destroying suspended page for that swap");
-                    suspendedPage->destroyWebPageInWebProcess();
-                }
+                page.process().processPool().removeAllSuspendedPageProxiesForPage(page);
 
                 return makeRef(*process);
             }
@@ -2191,30 +2194,33 @@ Ref<WebProcessProxy> WebProcessPool::processForNavigationInternal(WebPageProxy&
     return createNewWebProcess(page.websiteDataStore());
 }
 
-void WebProcessPool::registerSuspendedPageProxy(SuspendedPageProxy& page)
+void WebProcessPool::addSuspendedPageProxy(std::unique_ptr<SuspendedPageProxy>&& suspendedPage)
 {
-    auto& vector = m_suspendedPages.ensure(page.origin(), [] {
-        return Vector<SuspendedPageProxy*> { };
-    }).iterator->value;
-
-    vector.append(&page);
+    if (m_suspendedPages.size() >= maximumSuspendedPagesCount)
+        m_suspendedPages.removeFirst();
 
-#if !LOG_DISABLED
-    if (vector.size() > 5)
-        LOG(ProcessSwapping, "Security origin %s now has %zu suspended pages (this seems unexpected)", page.origin().debugString().utf8().data(), vector.size());
-#endif
+    m_suspendedPages.append(WTFMove(suspendedPage));
 }
 
-void WebProcessPool::unregisterSuspendedPageProxy(SuspendedPageProxy& page)
+void WebProcessPool::removeAllSuspendedPageProxiesForPage(WebPageProxy& page)
 {
-    auto iterator = m_suspendedPages.find(page.origin());
-    ASSERT(iterator != m_suspendedPages.end());
+    m_suspendedPages.removeAllMatching([&page](auto& suspendedPage) {
+        return &suspendedPage->page() == &page;
+    });
+}
 
-    auto result = iterator->value.removeFirst(&page);
-    ASSERT_UNUSED(result, result);
+std::unique_ptr<SuspendedPageProxy> WebProcessPool::takeSuspendedPageProxy(SuspendedPageProxy& suspendedPage)
+{
+    return m_suspendedPages.takeFirst([&suspendedPage](auto& item) {
+        return item.get() == &suspendedPage;
+    });
+}
 
-    if (iterator->value.isEmpty())
-        m_suspendedPages.remove(iterator);
+bool WebProcessPool::hasSuspendedPageProxyFor(WebProcessProxy& process)
+{
+    return m_suspendedPages.findIf([&process](auto& suspendedPage) {
+        return &suspendedPage->process() == &process;
+    }) != m_suspendedPages.end();
 }
 
 void WebProcessPool::addMockMediaDevice(const MockMediaDevice& device)
index 682ac2f..f2c1a2a 100644 (file)
@@ -440,8 +440,13 @@ public:
 #endif
 
     Ref<WebProcessProxy> processForNavigation(WebPageProxy&, const API::Navigation&, ProcessSwapRequestedByClient, WebCore::PolicyAction&, String& reason);
-    void registerSuspendedPageProxy(SuspendedPageProxy&);
-    void unregisterSuspendedPageProxy(SuspendedPageProxy&);
+
+    // SuspendedPageProxy management.
+    void addSuspendedPageProxy(std::unique_ptr<SuspendedPageProxy>&&);
+    void removeAllSuspendedPageProxiesForPage(WebPageProxy&);
+    std::unique_ptr<SuspendedPageProxy> takeSuspendedPageProxy(SuspendedPageProxy&);
+    bool hasSuspendedPageProxyFor(WebProcessProxy&);
+
     void didReachGoodTimeToPrewarm();
 
     void didCollectPrewarmInformation(const String& registrableDomain, const WebCore::PrewarmInformation&);
@@ -703,7 +708,7 @@ private:
 #endif
 #endif
 
-    HashMap<WebCore::SecurityOriginData, Vector<SuspendedPageProxy*>> m_suspendedPages;
+    Deque<std::unique_ptr<SuspendedPageProxy>> m_suspendedPages;
     HashMap<String, RefPtr<WebProcessProxy>> m_swappedProcessesPerRegistrableDomain;
 
     HashMap<String, std::unique_ptr<WebCore::PrewarmInformation>> m_prewarmInformationPerRegistrableDomain;
index fd998e1..8b6e36e 100644 (file)
@@ -34,7 +34,6 @@
 #include "Logging.h"
 #include "PluginInfoStore.h"
 #include "PluginProcessManager.h"
-#include "SuspendedPageProxy.h"
 #include "TextChecker.h"
 #include "TextCheckerState.h"
 #include "UIMessagePortChannelProvider.h"
@@ -444,27 +443,6 @@ void WebProcessProxy::addExistingWebPage(WebPageProxy& webPage, uint64_t pageID)
     updateBackgroundResponsivenessTimer();
 }
 
-void WebProcessProxy::suspendWebPageProxy(WebPageProxy& webPage, API::Navigation& navigation, uint64_t mainFrameID)
-{
-    if (auto* suspendedPage = webPage.maybeCreateSuspendedPage(*this, navigation, mainFrameID)) {
-        LOG(ProcessSwapping, "WebProcessProxy pid %i added suspended page %s", processIdentifier(), suspendedPage->loggingString());
-        m_suspendedPageMap.set(webPage.pageID(), suspendedPage);
-    }
-
-    removeWebPage(webPage, webPage.pageID());
-    removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), webPage.pageID());
-}
-
-void WebProcessProxy::suspendedPageWasDestroyed(SuspendedPageProxy& suspendedPage)
-{
-    LOG(ProcessSwapping, "WebProcessProxy pid %i suspended page %s was destroyed", processIdentifier(), suspendedPage.loggingString());
-
-    ASSERT(m_suspendedPageMap.contains(suspendedPage.page().pageID()));
-    m_suspendedPageMap.remove(suspendedPage.page().pageID());
-
-    maybeShutDown();
-}
-
 void WebProcessProxy::markIsNoLongerInPrewarmedPool()
 {
     ASSERT(m_isPrewarmed);
@@ -673,15 +651,6 @@ void WebProcessProxy::didReceiveMessage(IPC::Connection& connection, IPC::Decode
         return;
     }
 
-    // WebPageProxy messages are normally handled by the normal "dispatchMessage" up above.
-    // If they were not handled there, then they may potentially be handled by SuspendedPageProxy objects.
-    if (decoder.messageReceiverName() == Messages::WebPageProxy::messageReceiverName()) {
-        if (auto* suspendedPage = m_suspendedPageMap.get(decoder.destinationID())) {
-            suspendedPage->didReceiveMessage(connection, decoder);
-            return;
-        }
-    }
-
     // FIXME: Add unhandled message logging.
 }
 
@@ -729,11 +698,6 @@ void WebProcessProxy::processDidTerminateOrFailedToLaunch()
     }
 #endif
 
-    for (auto* suspendedPage : copyToVectorOf<SuspendedPageProxy*>(m_suspendedPageMap.values()))
-        suspendedPage->webProcessDidClose(*this);
-
-    m_suspendedPageMap.clear();
-
     for (auto& page : pages)
         page->processDidTerminate(ProcessTerminationReason::Crash);
 }
@@ -901,7 +865,7 @@ void WebProcessProxy::maybeShutDown()
 
 bool WebProcessProxy::canTerminateChildProcess()
 {
-    if (!m_pageMap.isEmpty() || !m_suspendedPageMap.isEmpty())
+    if (!m_pageMap.isEmpty() || m_processPool->hasSuspendedPageProxyFor(*this))
         return false;
 
     if (!m_processPool->shouldTerminate(this))
@@ -1015,11 +979,6 @@ void WebProcessProxy::requestTermination(ProcessTerminationReason reason)
 
     shutDown();
 
-    for (auto* suspendedPage : copyToVectorOf<SuspendedPageProxy*>(m_suspendedPageMap.values()))
-        suspendedPage->webProcessDidClose(*this);
-
-    m_suspendedPageMap.clear();
-
     for (auto& page : pages)
         page->processDidTerminate(reason);
 }
index d0e7ecb..acc51f1 100644 (file)
@@ -67,7 +67,6 @@ namespace WebKit {
 class NetworkProcessProxy;
 class ObjCObjectGraph;
 class PageClient;
-class SuspendedPageProxy;
 class UserMediaCaptureManagerProxy;
 class VisitedLinkStore;
 class WebBackForwardListItem;
@@ -208,9 +207,6 @@ public:
     void didCommitProvisionalLoad() { m_hasCommittedAnyProvisionalLoads = true; }
     bool hasCommittedAnyProvisionalLoads() const { return m_hasCommittedAnyProvisionalLoads; }
 
-    void suspendWebPageProxy(WebPageProxy&, API::Navigation&, uint64_t mainFrameID);
-    void suspendedPageWasDestroyed(SuspendedPageProxy&);
-
 #if PLATFORM(WATCHOS)
     void takeBackgroundActivityTokenForFullscreenInput();
     void releaseBackgroundActivityTokenForFullscreenInput();
@@ -232,6 +228,7 @@ public:
     // Called when the web process has crashed or we know that it will terminate soon.
     // Will potentially cause the WebProcessProxy object to be freed.
     void shutDown();
+    void maybeShutDown();
 
 protected:
     static uint64_t generatePageID();
@@ -251,8 +248,6 @@ protected:
 #endif
 
 private:
-    void maybeShutDown();
-
     // IPC message handlers.
     void updateBackForwardItem(const BackForwardListItemState&);
     void didDestroyFrame(uint64_t);
@@ -361,7 +356,6 @@ private:
     HashSet<String> m_localPathsWithAssumedReadAccess;
 
     WebPageProxyMap m_pageMap;
-    HashMap<uint64_t, SuspendedPageProxy*> m_suspendedPageMap;
     WebFrameProxyMap m_frameMap;
     UserInitiatedActionMap m_userInitiatedActionMap;
 
index a545abd..c7ff8e0 100644 (file)
@@ -1,3 +1,17 @@
+2018-10-18  Chris Dumez  <cdumez@apple.com>
+
+        [PSON] Cap number of SuspendedPageProxy objects and allow a WebPageProxy to have more than one
+        https://bugs.webkit.org/show_bug.cgi?id=190688
+        <rdar://problem/45354095>
+
+        Reviewed by Antti Koivisto.
+
+        Extended API test coverage to confirm that:
+        - We do not accumulate more than 3 suspended processes.
+        - We can navigate back 3 times and use the page cache for each of these loads.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
+
 2018-10-17  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         Enable the datalist element by default on iOS and macOS
index 8448428..0ed0a4b 100644 (file)
@@ -412,6 +412,7 @@ TEST(ProcessSwap, NoSwappingForeTLDPlus2)
     EXPECT_EQ(numberOfDecidePolicyCalls, 2);
 }
 
+// We currently keep 3 suspended pages around so we can go back 3 times and use the page cache for each load.
 TEST(ProcessSwap, Back)
 {
     auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
@@ -422,6 +423,8 @@ TEST(ProcessSwap, Back)
     [webViewConfiguration setProcessPool:processPool.get()];
     auto handler = adoptNS([[PSONScheme alloc] init]);
     [handler addMappingFromURLString:@"pson://www.webkit.org/main.html" toData:testBytes];
+    [handler addMappingFromURLString:@"pson://www.apple.com/main.html" toData:testBytes];
+    [handler addMappingFromURLString:@"pson://www.google.com/main.html" toData:testBytes];
     [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"PSON"];
 
     RetainPtr<PSONMessageHandler> messageHandler = adoptNS([[PSONMessageHandler alloc] init]);
@@ -439,7 +442,7 @@ TEST(ProcessSwap, Back)
     TestWebKitAPI::Util::run(&done);
     done = false;
 
-    auto pid1 = [webView _webProcessIdentifier];
+    auto webkitPID = [webView _webProcessIdentifier];
 
     request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.apple.com/main.html"]];
     [webView loadRequest:request];
@@ -447,30 +450,70 @@ TEST(ProcessSwap, Back)
     TestWebKitAPI::Util::run(&done);
     done = false;
 
-    auto pid2 = [webView _webProcessIdentifier];
+    auto applePID = [webView _webProcessIdentifier];
+    EXPECT_NE(webkitPID, applePID);
 
-    [webView goBack];
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.google.com/main.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto googlePID = [webView _webProcessIdentifier];
+    EXPECT_NE(applePID, googlePID);
+
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.bing.com/main.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto bingPID = [webView _webProcessIdentifier];
+    EXPECT_NE(googlePID, bingPID);
+
+    [webView goBack]; // Back to google.com.
 
     TestWebKitAPI::Util::run(&receivedMessage);
     receivedMessage = false;
     TestWebKitAPI::Util::run(&done);
     done = false;
 
-    auto pid3 = [webView _webProcessIdentifier];
+    auto pidAfterFirstBackNavigation = [webView _webProcessIdentifier];
+    EXPECT_EQ(googlePID, pidAfterFirstBackNavigation);
 
-    // 3 loads, 3 decidePolicy calls (e.g. any load that performs a process swap should not have generated an
-    // additional decidePolicy call as a result of the process swap)
-    EXPECT_EQ(numberOfDecidePolicyCalls, 3);
+    [webView goBack]; // Back to apple.com.
 
-    EXPECT_EQ([receivedMessages count], 2u);
-    EXPECT_TRUE([receivedMessages.get()[0] isEqualToString:@"PageShow called. Persisted: false, and window.history.state is: null"]);
-    EXPECT_TRUE([receivedMessages.get()[1] isEqualToString:@"PageShow called. Persisted: true, and window.history.state is: onloadCalled"]);
+    TestWebKitAPI::Util::run(&receivedMessage);
+    receivedMessage = false;
+    TestWebKitAPI::Util::run(&done);
+    done = false;
 
-    EXPECT_EQ(2u, seenPIDs.size());
+    auto pidAfterSecondBackNavigation = [webView _webProcessIdentifier];
+    EXPECT_EQ(applePID, pidAfterSecondBackNavigation);
 
-    EXPECT_FALSE(pid1 == pid2);
-    EXPECT_FALSE(pid2 == pid3);
-    EXPECT_TRUE(pid1 == pid3);
+    [webView goBack]; // Back to webkit.org.
+
+    TestWebKitAPI::Util::run(&receivedMessage);
+    receivedMessage = false;
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    auto pidAfterThirdBackNavigation = [webView _webProcessIdentifier];
+    EXPECT_EQ(webkitPID, pidAfterThirdBackNavigation);
+
+    // 7 loads, 7 decidePolicy calls (e.g. any load that performs a process swap should not have generated an
+    // additional decidePolicy call as a result of the process swap)
+    EXPECT_EQ(7, numberOfDecidePolicyCalls);
+
+    EXPECT_EQ(6u, [receivedMessages count]);
+    EXPECT_WK_STREQ(@"PageShow called. Persisted: false, and window.history.state is: null", receivedMessages.get()[0]);
+    EXPECT_WK_STREQ(@"PageShow called. Persisted: false, and window.history.state is: null", receivedMessages.get()[1]);
+    EXPECT_WK_STREQ(@"PageShow called. Persisted: false, and window.history.state is: null", receivedMessages.get()[2]);
+    EXPECT_WK_STREQ(@"PageShow called. Persisted: true, and window.history.state is: onloadCalled", receivedMessages.get()[3]);
+    EXPECT_WK_STREQ(@"PageShow called. Persisted: true, and window.history.state is: onloadCalled", receivedMessages.get()[4]);
+    EXPECT_WK_STREQ(@"PageShow called. Persisted: true, and window.history.state is: onloadCalled", receivedMessages.get()[5]);
+
+    EXPECT_EQ(4u, seenPIDs.size());
 }
 
 TEST(ProcessSwap, BackWithoutSuspendedPage)
@@ -1114,7 +1157,7 @@ TEST(ProcessSwap, MainFramesOnly)
     EXPECT_EQ(1u, seenPIDs.size());
 }
 
-TEST(ProcessSwap, OnePreviousProcessRemains)
+TEST(ProcessSwap, ThreePreviousProcessesRemains)
 {
     auto processPoolConfiguration = adoptNS([[_WKProcessPoolConfiguration alloc] init]);
     [processPoolConfiguration setProcessSwapsOnNavigation:YES];
@@ -1147,11 +1190,23 @@ TEST(ProcessSwap, OnePreviousProcessRemains)
     TestWebKitAPI::Util::run(&done);
     done = false;
 
-    // Navigations to 3 different domains, we expect to have seen 3 different PIDs
-    EXPECT_EQ(3u, seenPIDs.size());
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.bing.com/main.html"]];
+    [webView loadRequest:request];
 
-    // But only 2 of those processes should still be alive
-    EXPECT_EQ(2u, [processPool _webProcessCountIgnoringPrewarmed]);
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.yahoo.com/main.html"]];
+    [webView loadRequest:request];
+
+    TestWebKitAPI::Util::run(&done);
+    done = false;
+
+    // Navigations to 5 different domains, we expect to have seen 5 different PIDs
+    EXPECT_EQ(5u, seenPIDs.size());
+
+    // But only 4 of those processes should still be alive (1 visible, 3 suspended).
+    EXPECT_EQ(4u, [processPool _webProcessCountIgnoringPrewarmed]);
 }
 
 static const char* pageCache1Bytes = R"PSONRESOURCE(