Clearing Website data for a given session should not shut down cached processes for...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 12 Oct 2019 16:17:52 +0000 (16:17 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 12 Oct 2019 16:17:52 +0000 (16:17 +0000)
https://bugs.webkit.org/show_bug.cgi?id=202865
<rdar://problem/56202912>

Reviewed by Antti Koivisto.

When clearing Website data for a given data store, we now only clear cached processes
(either if BackForwardCache or WebProcessCache) if they are associated with this
particular data store. It is very wasteful otherwise.

* UIProcess/ProvisionalPageProxy.cpp:
(WebKit::ProvisionalPageProxy::~ProvisionalPageProxy):
* UIProcess/SuspendedPageProxy.cpp:
(WebKit::SuspendedPageProxy::~SuspendedPageProxy):
ProvisionalPageProxy & SuspendedPageProxy's destructors no longer call
WebProcessProxy::maybeShutdown() asynchronously. We now call maybeShutdown()
synchronously in WebProcessProxy::decrementSuspendedPageCount() and
WebProcessProxy::removeProvisionalPageProxy() instead. This makes things a
lot more predictable.

* UIProcess/WebBackForwardCache.cpp:
(WebKit::WebBackForwardCache::removeEntriesForSession):
Add new removeEntriesForSession() method to clear only back / forward cache
entries associated with a particular session.

(WebKit::WebBackForwardCache::clear):
Stop taking a parameter indicating if we allow the processes to enter the
process cache. Now that we call maybeShutdown() synchronously when destroying
a SuspendedPageProxy, we can simply allow the processes to enter the process
cache unconditionally. If the caller does not want this processes in the page
cache, they can clear the process cache afterwards.

* UIProcess/WebBackForwardCache.h:
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::receivedNavigationPolicyDecision):
Now that destroying a SuspendedPageProxy or a ProvisionalPageProxy may shutdown
their process synchronously, add a scope here to prevent shutdown of the process
for the duration of this scope, since we're about to use it for a navigation and
we don't want it to get shutdown, even if there is no longer anything using it.

(WebKit::WebPageProxy::continueNavigationInNewProcess):
Add new assertion and rename parameter for consistency.

* UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::handleMemoryPressureWarning):

* UIProcess/WebProcessProxy.cpp:
(WebKit::WebProcessProxy::addProvisionalPageProxy):
Add new assertion.

(WebKit::WebProcessProxy::removeProvisionalPageProxy):
Call maybeShutDown() if this was the last provisional page.

(WebKit::WebProcessProxy::maybeShutDown):
Drop parameter indicating if we want to allow the process to enter the process
cache and allow caching unconditionally.

(WebKit::WebProcessProxy::canTerminateAuxiliaryProcess):
Prevent termination if there is a ScopePreventingShutdown.

(WebKit::WebProcessProxy::decrementSuspendedPageCount):
Call maybeShutDown() if this was the last suspended page.

* UIProcess/WebProcessProxy.h:
(WebKit::WebProcessProxy::ScopePreventingShutdown::ScopePreventingShutdown):
(WebKit::WebProcessProxy::ScopePreventingShutdown::~ScopePreventingShutdown):
(WebKit::WebProcessProxy::makeScopePreventingShutdown):
Add new facility to prevent shutdown of a process for the duration of the scope.

* UIProcess/WebsiteData/WebsiteDataStore.cpp:
(WebKit::WebsiteDataStore::removeData):
When removing website data, only clear cached processes if they are associated with
the current data store.

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

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/ProvisionalPageProxy.cpp
Source/WebKit/UIProcess/SuspendedPageProxy.cpp
Source/WebKit/UIProcess/WebBackForwardCache.cpp
Source/WebKit/UIProcess/WebBackForwardCache.h
Source/WebKit/UIProcess/WebPageProxy.cpp
Source/WebKit/UIProcess/WebProcessPool.cpp
Source/WebKit/UIProcess/WebProcessProxy.cpp
Source/WebKit/UIProcess/WebProcessProxy.h
Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp

index 1d34944..6a3688c 100644 (file)
@@ -1,3 +1,79 @@
+2019-10-12  Chris Dumez  <cdumez@apple.com>
+
+        Clearing Website data for a given session should not shut down cached processes for other sessions
+        https://bugs.webkit.org/show_bug.cgi?id=202865
+        <rdar://problem/56202912>
+
+        Reviewed by Antti Koivisto.
+
+        When clearing Website data for a given data store, we now only clear cached processes
+        (either if BackForwardCache or WebProcessCache) if they are associated with this
+        particular data store. It is very wasteful otherwise.
+
+        * UIProcess/ProvisionalPageProxy.cpp:
+        (WebKit::ProvisionalPageProxy::~ProvisionalPageProxy):
+        * UIProcess/SuspendedPageProxy.cpp:
+        (WebKit::SuspendedPageProxy::~SuspendedPageProxy):
+        ProvisionalPageProxy & SuspendedPageProxy's destructors no longer call
+        WebProcessProxy::maybeShutdown() asynchronously. We now call maybeShutdown()
+        synchronously in WebProcessProxy::decrementSuspendedPageCount() and
+        WebProcessProxy::removeProvisionalPageProxy() instead. This makes things a
+        lot more predictable.
+
+        * UIProcess/WebBackForwardCache.cpp:
+        (WebKit::WebBackForwardCache::removeEntriesForSession):
+        Add new removeEntriesForSession() method to clear only back / forward cache
+        entries associated with a particular session.
+
+        (WebKit::WebBackForwardCache::clear):
+        Stop taking a parameter indicating if we allow the processes to enter the
+        process cache. Now that we call maybeShutdown() synchronously when destroying
+        a SuspendedPageProxy, we can simply allow the processes to enter the process
+        cache unconditionally. If the caller does not want this processes in the page
+        cache, they can clear the process cache afterwards.
+
+        * UIProcess/WebBackForwardCache.h:
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::receivedNavigationPolicyDecision):
+        Now that destroying a SuspendedPageProxy or a ProvisionalPageProxy may shutdown
+        their process synchronously, add a scope here to prevent shutdown of the process
+        for the duration of this scope, since we're about to use it for a navigation and
+        we don't want it to get shutdown, even if there is no longer anything using it.
+
+        (WebKit::WebPageProxy::continueNavigationInNewProcess):
+        Add new assertion and rename parameter for consistency.
+
+        * UIProcess/WebProcessPool.cpp:
+        (WebKit::WebProcessPool::handleMemoryPressureWarning):
+
+        * UIProcess/WebProcessProxy.cpp:
+        (WebKit::WebProcessProxy::addProvisionalPageProxy):
+        Add new assertion.
+
+        (WebKit::WebProcessProxy::removeProvisionalPageProxy):
+        Call maybeShutDown() if this was the last provisional page.
+
+        (WebKit::WebProcessProxy::maybeShutDown):
+        Drop parameter indicating if we want to allow the process to enter the process
+        cache and allow caching unconditionally.
+
+        (WebKit::WebProcessProxy::canTerminateAuxiliaryProcess):
+        Prevent termination if there is a ScopePreventingShutdown.
+
+        (WebKit::WebProcessProxy::decrementSuspendedPageCount):
+        Call maybeShutDown() if this was the last suspended page.
+
+        * UIProcess/WebProcessProxy.h:
+        (WebKit::WebProcessProxy::ScopePreventingShutdown::ScopePreventingShutdown):
+        (WebKit::WebProcessProxy::ScopePreventingShutdown::~ScopePreventingShutdown):
+        (WebKit::WebProcessProxy::makeScopePreventingShutdown):
+        Add new facility to prevent shutdown of a process for the duration of the scope.
+
+        * UIProcess/WebsiteData/WebsiteDataStore.cpp:
+        (WebKit::WebsiteDataStore::removeData):
+        When removing website data, only clear cached processes if they are associated with
+        the current data store.
+
 2019-10-12  Chris Fleizach  <cfleizach@apple.com>
 
         AX: Make AXIsolatedTree compile again
index a2a09dc..e98ec0e 100644 (file)
@@ -88,21 +88,16 @@ ProvisionalPageProxy::ProvisionalPageProxy(WebPageProxy& page, Ref<WebProcessPro
 
 ProvisionalPageProxy::~ProvisionalPageProxy()
 {
-    m_process->removeProvisionalPageProxy(*this);
-
-    if (m_wasCommitted)
-        return;
-
-    if (&m_process->websiteDataStore() != &m_page.websiteDataStore())
-        m_process->processPool().pageEndUsingWebsiteDataStore(m_page.identifier(), m_process->websiteDataStore());
+    if (!m_wasCommitted) {
+        if (&m_process->websiteDataStore() != &m_page.websiteDataStore())
+            m_process->processPool().pageEndUsingWebsiteDataStore(m_page.identifier(), m_process->websiteDataStore());
 
-    m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_webPageID);
-    m_process->send(Messages::WebPage::Close(), m_webPageID);
-    m_process->removeVisitedLinkStoreUser(m_page.visitedLinkStore(), m_page.identifier());
+        m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_webPageID);
+        m_process->send(Messages::WebPage::Close(), m_webPageID);
+        m_process->removeVisitedLinkStoreUser(m_page.visitedLinkStore(), m_page.identifier());
+    }
 
-    RunLoop::main().dispatch([process = m_process.copyRef()] {
-        process->maybeShutDown();
-    });
+    m_process->removeProvisionalPageProxy(*this);
 }
 
 void ProvisionalPageProxy::processDidTerminate()
index 5c5b86e..8358d2f 100644 (file)
@@ -116,7 +116,6 @@ SuspendedPageProxy::SuspendedPageProxy(WebPageProxy& page, Ref<WebProcessProxy>&
 
 SuspendedPageProxy::~SuspendedPageProxy()
 {
-    m_process->decrementSuspendedPageCount();
     allSuspendedPages().remove(this);
 
     if (m_readyToUnsuspendHandler) {
@@ -125,21 +124,16 @@ SuspendedPageProxy::~SuspendedPageProxy()
         });
     }
 
-    if (m_suspensionState == SuspensionState::Resumed)
-        return;
-
-    // If the suspended page was not consumed before getting destroyed, then close the corresponding page
-    // on the WebProcess side.
-    close();
+    if (m_suspensionState != SuspensionState::Resumed) {
+        // If the suspended page was not consumed before getting destroyed, then close the corresponding page
+        // on the WebProcess side.
+        close();
 
-    if (m_suspensionState == SuspensionState::Suspending)
-        m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_webPageID);
+        if (m_suspensionState == SuspensionState::Suspending)
+            m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_webPageID);
+    }
 
-    // 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();
-    });
+    m_process->decrementSuspendedPageCount();
 }
 
 void SuspendedPageProxy::setBackForwardListItem(WebBackForwardListItem& item)
index 5de186e..4648764 100644 (file)
@@ -30,6 +30,7 @@
 #include "WebBackForwardListItem.h"
 #include "WebPageProxy.h"
 #include "WebProcessProxy.h"
+#include "WebsiteDataStore.h"
 
 namespace WebKit {
 
@@ -83,6 +84,13 @@ void WebBackForwardCache::removeEntriesForProcess(WebProcessProxy& process)
     });
 }
 
+void WebBackForwardCache::removeEntriesForSession(PAL::SessionID sessionID)
+{
+    removeEntriesMatching([sessionID](auto& item) {
+        return item.suspendedPage()->process().websiteDataStore().sessionID() == sessionID;
+    });
+}
+
 void WebBackForwardCache::removeEntriesForPage(WebPageProxy& page)
 {
     removeEntriesMatching([pageID = page.identifier()](auto& item) {
@@ -102,14 +110,11 @@ void WebBackForwardCache::removeEntriesMatching(const Function<bool(WebBackForwa
     }
 }
 
-void WebBackForwardCache::clear(AllowProcessCaching allowProcessCaching)
+void WebBackForwardCache::clear()
 {
     auto itemsWithCachedPage = WTFMove(m_itemsWithCachedPage);
-    for (auto* item : itemsWithCachedPage) {
-        auto process = makeRef(item->suspendedPage()->process());
+    for (auto* item : itemsWithCachedPage)
         item->setSuspendedPage(nullptr);
-        process->maybeShutDown(allowProcessCaching);
-    }
 }
 
 } // namespace WebKit.
index 6e02ab1..4df19ad 100644 (file)
@@ -25,6 +25,7 @@
 
 #pragma once
 
+#include <pal/SessionID.h>
 #include <wtf/Forward.h>
 #include <wtf/ListHashSet.h>
 
@@ -34,7 +35,6 @@ class SuspendedPageProxy;
 class WebBackForwardListItem;
 class WebPageProxy;
 class WebProcessProxy;
-enum class AllowProcessCaching;
 
 class WebBackForwardCache {
     WTF_MAKE_FAST_ALLOCATED;
@@ -46,9 +46,10 @@ public:
     unsigned capacity() const { return m_capacity; }
     unsigned size() const { return m_itemsWithCachedPage.size(); }
 
-    void clear(AllowProcessCaching);
+    void clear();
     void removeEntriesForProcess(WebProcessProxy&);
     void removeEntriesForPage(WebPageProxy&);
+    void removeEntriesForSession(PAL::SessionID);
 
     void addEntry(WebBackForwardListItem&, std::unique_ptr<SuspendedPageProxy>&&);
     void removeEntry(WebBackForwardListItem&);
index e251b36..01876c6 100644 (file)
@@ -2954,6 +2954,9 @@ void WebPageProxy::receivedNavigationPolicyDecision(PolicyAction policyAction, A
             RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "decidePolicyForNavigationAction: keep using process %i for navigation, reason: %{public}s", processIdentifier(), reason.utf8().data());
 
         if (shouldProcessSwap) {
+            // Make sure the process to be used for the navigation does not get shutDown now due to destroying SuspendedPageProxy or ProvisionalPageProxy objects.
+            auto preventNavigationProcessShutdown = processForNavigation->makeScopePreventingShutdown();
+
             auto suspendedPage = destinationSuspendedPage ? backForwardCache().takeEntry(*destinationSuspendedPage->backForwardListItem()) : nullptr;
             if (suspendedPage && suspendedPage->pageIsClosedOrClosing())
                 suspendedPage = nullptr;
@@ -3035,10 +3038,11 @@ void WebPageProxy::commitProvisionalPage(FrameIdentifier frameID, uint64_t navig
     m_provisionalPage = nullptr;
 }
 
-void WebPageProxy::continueNavigationInNewProcess(API::Navigation& navigation, std::unique_ptr<SuspendedPageProxy>&& suspendedPageProxy, Ref<WebProcessProxy>&& newProcess, ProcessSwapRequestedByClient processSwapRequestedByClient, Optional<WebsitePoliciesData>&& websitePolicies)
+void WebPageProxy::continueNavigationInNewProcess(API::Navigation& navigation, std::unique_ptr<SuspendedPageProxy>&& suspendedPage, Ref<WebProcessProxy>&& newProcess, ProcessSwapRequestedByClient processSwapRequestedByClient, Optional<WebsitePoliciesData>&& websitePolicies)
 {
-    RELEASE_LOG_IF_ALLOWED(Loading, "continueNavigationInNewProcess: newProcessPID = %i, hasSuspendedPage = %i", newProcess->processIdentifier(), !!suspendedPageProxy);
+    RELEASE_LOG_IF_ALLOWED(Loading, "continueNavigationInNewProcess: newProcessPID = %i, hasSuspendedPage = %i", newProcess->processIdentifier(), !!suspendedPage);
     LOG(Loading, "Continuing navigation %" PRIu64 " '%s' in a new web process", navigation.navigationID(), navigation.loggingString());
+    RELEASE_ASSERT(!newProcess->isInProcessCache());
 
     if (m_provisionalPage) {
         RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "continueNavigationInNewProcess: There is already a pending provisional load, cancelling it (provisonalNavigationID: %llu, navigationID: %llu)", m_provisionalPage->navigationID(), navigation.navigationID());
@@ -3047,7 +3051,7 @@ void WebPageProxy::continueNavigationInNewProcess(API::Navigation& navigation, s
         m_provisionalPage = nullptr;
     }
 
-    m_provisionalPage = makeUnique<ProvisionalPageProxy>(*this, newProcess.copyRef(), WTFMove(suspendedPageProxy), navigation.navigationID(), navigation.currentRequestIsRedirect(), navigation.currentRequest(), processSwapRequestedByClient);
+    m_provisionalPage = makeUnique<ProvisionalPageProxy>(*this, newProcess.copyRef(), WTFMove(suspendedPage), navigation.navigationID(), navigation.currentRequestIsRedirect(), navigation.currentRequest(), processSwapRequestedByClient);
 
     if (auto* item = navigation.targetItem()) {
         LOG(Loading, "WebPageProxy %p continueNavigationInNewProcess to back item URL %s", this, item->url().utf8().data());
index 8408a6e..b72d3d4 100644 (file)
@@ -1386,8 +1386,9 @@ void WebProcessPool::handleMemoryPressureWarning(Critical)
 {
     RELEASE_LOG(PerformanceLogging, "%p - WebProcessPool::handleMemoryPressureWarning", this);
 
-
-    m_backForwardCache->clear(AllowProcessCaching::No);
+    // Clear back/forward cache first as processes removed from the back/forward cache will likely
+    // be added to the WebProcess cache.
+    m_backForwardCache->clear();
     m_webProcessCache->clear();
 
     if (m_prewarmedProcess)
index 749525a..9960acf 100644 (file)
@@ -248,6 +248,7 @@ void WebProcessProxy::updateRegistrationWithDataStore()
 
 void WebProcessProxy::addProvisionalPageProxy(ProvisionalPageProxy& provisionalPage)
 {
+    ASSERT(!m_isInProcessCache);
     ASSERT(!m_provisionalPages.contains(&provisionalPage));
     m_provisionalPages.add(&provisionalPage);
     updateRegistrationWithDataStore();
@@ -258,6 +259,8 @@ void WebProcessProxy::removeProvisionalPageProxy(ProvisionalPageProxy& provision
     ASSERT(m_provisionalPages.contains(&provisionalPage));
     m_provisionalPages.remove(&provisionalPage);
     updateRegistrationWithDataStore();
+    if (m_provisionalPages.isEmpty())
+        maybeShutDown();
 }
 
 void WebProcessProxy::getLaunchOptions(ProcessLauncher::LaunchOptions& launchOptions)
@@ -937,7 +940,7 @@ bool WebProcessProxy::canBeAddedToWebProcessCache() const
     return true;
 }
 
-void WebProcessProxy::maybeShutDown(AllowProcessCaching allowProcessCaching)
+void WebProcessProxy::maybeShutDown()
 {
     if (processPool().dummyProcessProxy() == this && m_pageMap.isEmpty()) {
         ASSERT(state() == State::Terminated);
@@ -948,7 +951,7 @@ void WebProcessProxy::maybeShutDown(AllowProcessCaching allowProcessCaching)
     if (state() == State::Terminated || !canTerminateAuxiliaryProcess())
         return;
 
-    if (allowProcessCaching == AllowProcessCaching::Yes && canBeAddedToWebProcessCache() && processPool().webProcessCache().addProcessIfPossible(*this))
+    if (canBeAddedToWebProcessCache() && processPool().webProcessCache().addProcessIfPossible(*this))
         return;
 
     shutDown();
@@ -956,7 +959,7 @@ void WebProcessProxy::maybeShutDown(AllowProcessCaching allowProcessCaching)
 
 bool WebProcessProxy::canTerminateAuxiliaryProcess()
 {
-    if (!m_pageMap.isEmpty() || m_suspendedPageCount || !m_provisionalPages.isEmpty() || m_isInProcessCache)
+    if (!m_pageMap.isEmpty() || m_suspendedPageCount || !m_provisionalPages.isEmpty() || m_isInProcessCache || m_shutdownPreventingScopeCount)
         return false;
 
     if (!m_processPool->shouldTerminate(this))
@@ -1483,8 +1486,10 @@ void WebProcessProxy::decrementSuspendedPageCount()
 {
     ASSERT(m_suspendedPageCount);
     --m_suspendedPageCount;
-    if (!m_suspendedPageCount)
+    if (!m_suspendedPageCount) {
         send(Messages::WebProcess::SetHasSuspendedPageProxy(false), 0);
+        maybeShutDown();
+    }
 }
 
 WebProcessPool* WebProcessProxy::processPoolIfExists() const
index 5b05218..832349a 100644 (file)
@@ -94,8 +94,6 @@ typedef RefCounter<BackgroundWebProcessCounterType> BackgroundWebProcessCounter;
 typedef BackgroundWebProcessCounter::Token BackgroundWebProcessToken;
 #endif
 
-enum class AllowProcessCaching { No, Yes };
-
 class WebProcessProxy : public AuxiliaryProcessProxy, public ResponsivenessTimer::Client, public ThreadSafeRefCounted<WebProcessProxy>, public CanMakeWeakPtr<WebProcessProxy>, private ProcessThrottlerClient {
 public:
     typedef HashMap<WebCore::FrameIdentifier, RefPtr<WebFrameProxy>> WebFrameProxyMap;
@@ -272,7 +270,27 @@ 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(AllowProcessCaching = AllowProcessCaching::Yes);
+
+    class ScopePreventingShutdown {
+    public:
+        explicit ScopePreventingShutdown(WebProcessProxy& process)
+            : m_process(process)
+        {
+            ++(m_process->m_shutdownPreventingScopeCount);
+        }
+
+        ~ScopePreventingShutdown()
+        {
+            ASSERT(m_process->m_shutdownPreventingScopeCount);
+            if (!--(m_process->m_shutdownPreventingScopeCount))
+                m_process->maybeShutDown();
+        }
+
+    private:
+        Ref<WebProcessProxy> m_process;
+    };
+
+    ScopePreventingShutdown makeScopePreventingShutdown() { return ScopePreventingShutdown { *this }; }
 
     void didStartProvisionalLoadForMainFrame(const URL&);
 
@@ -404,6 +422,8 @@ private:
     
     void updateRegistrationWithDataStore();
 
+    void maybeShutDown();
+
     enum class IsWeak { No, Yes };
     template<typename T> class WeakOrStrongPtr {
     public:
@@ -482,6 +502,7 @@ private:
 #endif
 
     unsigned m_suspendedPageCount { 0 };
+    unsigned m_shutdownPreventingScopeCount { 0 };
     bool m_hasCommittedAnyProvisionalLoads { false };
     bool m_isPrewarmed;
     bool m_hasAudibleWebPage { false };
index 2a23b2a..83751e9 100644 (file)
@@ -726,8 +726,10 @@ void WebsiteDataStore::removeData(OptionSet<WebsiteDataType> dataTypes, WallTime
     auto webProcessAccessType = computeWebProcessAccessTypeForDataRemoval(dataTypes, !isPersistent());
     if (webProcessAccessType != ProcessAccessType::None) {
         for (auto& processPool : processPools()) {
-            processPool->backForwardCache().clear(AllowProcessCaching::No);
-            processPool->webProcessCache().clear();
+            // Clear back/forward cache first as processes removed from the back/forward cache will likely
+            // be added to the WebProcess cache.
+            processPool->backForwardCache().removeEntriesForSession(sessionID());
+            processPool->webProcessCache().clearAllProcessesForSession(sessionID());
         }
 
         for (auto& process : processes()) {