WebProcessCache should keep track of processes being added
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 11 Mar 2019 19:30:42 +0000 (19:30 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 11 Mar 2019 19:30:42 +0000 (19:30 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195538

Reviewed by Geoffrey Garen.

WebProcessCache should keep track of processes being added, while they are being
checked for responsiveness. This is useful so that:
- Requests to clear the cache also clear processes being added
- Requests to remove a given process from the cache (either because it crashed
  or because it is being used for a history navigation) actually remove the
  process if it is still being checked for responsiveness.
- The cached process eviction timer applies to such processes in case something
  goes wrong with the code and the pending request does not get processed.

* UIProcess/WebProcessCache.cpp:
(WebKit::generateAddRequestIdentifier):
(WebKit::WebProcessCache::addProcessIfPossible):
(WebKit::WebProcessCache::addProcess):
(WebKit::WebProcessCache::clear):
(WebKit::WebProcessCache::clearAllProcessesForSession):
(WebKit::WebProcessCache::removeProcess):
(WebKit::WebProcessCache::CachedProcess::evictionTimerFired):
(WebKit::WebProcessCache::evictProcess): Deleted.
* UIProcess/WebProcessCache.h:
(WebKit::WebProcessCache::size const):
* UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::processForNavigationInternal):
* UIProcess/WebProcessProxy.cpp:
(WebKit::WebProcessProxy::processDidTerminateOrFailedToLaunch):

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

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/API/Cocoa/WKProcessPool.mm
Source/WebKit/UIProcess/WebProcessCache.cpp
Source/WebKit/UIProcess/WebProcessCache.h
Source/WebKit/UIProcess/WebProcessPool.cpp
Source/WebKit/UIProcess/WebProcessProxy.cpp

index 755a0d2..8331f4c 100644 (file)
@@ -1,3 +1,35 @@
+2019-03-11  Chris Dumez  <cdumez@apple.com>
+
+        WebProcessCache should keep track of processes being added
+        https://bugs.webkit.org/show_bug.cgi?id=195538
+
+        Reviewed by Geoffrey Garen.
+
+        WebProcessCache should keep track of processes being added, while they are being
+        checked for responsiveness. This is useful so that:
+        - Requests to clear the cache also clear processes being added
+        - Requests to remove a given process from the cache (either because it crashed
+          or because it is being used for a history navigation) actually remove the
+          process if it is still being checked for responsiveness.
+        - The cached process eviction timer applies to such processes in case something
+          goes wrong with the code and the pending request does not get processed.
+
+        * UIProcess/WebProcessCache.cpp:
+        (WebKit::generateAddRequestIdentifier):
+        (WebKit::WebProcessCache::addProcessIfPossible):
+        (WebKit::WebProcessCache::addProcess):
+        (WebKit::WebProcessCache::clear):
+        (WebKit::WebProcessCache::clearAllProcessesForSession):
+        (WebKit::WebProcessCache::removeProcess):
+        (WebKit::WebProcessCache::CachedProcess::evictionTimerFired):
+        (WebKit::WebProcessCache::evictProcess): Deleted.
+        * UIProcess/WebProcessCache.h:
+        (WebKit::WebProcessCache::size const):
+        * UIProcess/WebProcessPool.cpp:
+        (WebKit::WebProcessPool::processForNavigationInternal):
+        * UIProcess/WebProcessProxy.cpp:
+        (WebKit::WebProcessProxy::processDidTerminateOrFailedToLaunch):
+
 2019-03-11  Alex Christensen  <achristensen@webkit.org>
 
         REGRESSION: ( r240978-r240985 ) [ iOS Release ] Layout Test imported/w3c/web-platform-tests/xhr/send-redirect-post-upload.htm is crashing
index 8ffb0c9..48d6152 100644 (file)
@@ -476,7 +476,12 @@ static NSDictionary *policiesHashMapToDictionary(const HashMap<String, HashMap<S
 
 - (size_t)_webProcessCountIgnoringPrewarmedAndCached
 {
-    return [self _webProcessCount] - ([self _hasPrewarmedWebProcess] ? 1 : 0) - _processPool->webProcessCache().size();
+    size_t count = 0;
+    for (auto& process : _processPool->processes()) {
+        if (!process->isInProcessCache() && !process->isPrewarmed())
+            ++count;
+    }
+    return count;
 }
 
 - (size_t)_webPageContentProcessCount
index 5ebfa20..72ed3e0 100644 (file)
@@ -37,6 +37,12 @@ namespace WebKit {
 Seconds WebProcessCache::cachedProcessLifetime { 30_min };
 Seconds WebProcessCache::clearingDelayAfterApplicationResignsActive { 5_min };
 
+static uint64_t generateAddRequestIdentifier()
+{
+    static uint64_t identifier = 0;
+    return ++identifier;
+}
+
 WebProcessCache::WebProcessCache(WebProcessPool& processPool)
     : m_evictionTimer(RunLoop::main(), this, &WebProcessCache::clear)
 {
@@ -73,38 +79,47 @@ bool WebProcessCache::addProcessIfPossible(const String& registrableDomain, Ref<
     if (!canCacheProcess(process))
         return false;
 
+    uint64_t requestIdentifier = generateAddRequestIdentifier();
+    m_pendingAddRequests.add(requestIdentifier, std::make_unique<CachedProcess>(process.copyRef()));
+
     RELEASE_LOG(ProcessSwapping, "%p - WebProcessCache::addProcessIfPossible(): Checking if process %i is responsive before caching it...", this, process->processIdentifier());
-    process->setIsInProcessCache(true);
-    process->isResponsive([process = process.copyRef(), processPool = makeRef(process->processPool()), registrableDomain](bool isResponsive) {
-        process->setIsInProcessCache(false);
+    process->isResponsive([this, processPool = makeRef(process->processPool()), requestIdentifier](bool isResponsive) {
+        auto cachedProcess = m_pendingAddRequests.take(requestIdentifier);
+        if (!cachedProcess)
+            return;
+
         if (!isResponsive) {
-            RELEASE_LOG_ERROR(ProcessSwapping, "%p - WebProcessCache::addProcessIfPossible(): Not caching process %i because it is not responsive", &process->processPool().webProcessCache(), process->processIdentifier());
-            process->shutDown();
+            RELEASE_LOG_ERROR(ProcessSwapping, "%p - WebProcessCache::addProcessIfPossible(): Not caching process %i because it is not responsive", &processPool->webProcessCache(), cachedProcess->process().processIdentifier());
             return;
         }
-        if (!processPool->webProcessCache().addProcess(registrableDomain, process.copyRef()))
-            process->shutDown();
+        processPool->webProcessCache().addProcess(WTFMove(cachedProcess));
     });
     return true;
 }
 
-bool WebProcessCache::addProcess(const String& registrableDomain, Ref<WebProcessProxy>&& process)
+bool WebProcessCache::addProcess(std::unique_ptr<CachedProcess>&& cachedProcess)
 {
-    ASSERT(!process->pageCount());
-    ASSERT(!process->provisionalPageCount());
-    ASSERT(!process->suspendedPageCount());
+    ASSERT(!cachedProcess->process().pageCount());
+    ASSERT(!cachedProcess->process().provisionalPageCount());
+    ASSERT(!cachedProcess->process().suspendedPageCount());
 
-    if (!canCacheProcess(process))
+    if (!canCacheProcess(cachedProcess->process()))
         return false;
 
+    auto registrableDomain = cachedProcess->process().registrableDomain();
+    RELEASE_ASSERT(!registrableDomain.isEmpty());
+
+    if (auto previousProcess = m_processesPerRegistrableDomain.take(registrableDomain))
+        RELEASE_LOG(ProcessSwapping, "%p - WebProcessCache::addProcess(): Evicting process %i from WebProcess cache because a new process was added for the same domain", this, previousProcess->process().processIdentifier());
+
     while (m_processesPerRegistrableDomain.size() >= capacity()) {
         auto it = m_processesPerRegistrableDomain.random();
-        RELEASE_LOG(ProcessSwapping, "%p - WebProcessCache::addProcess(): Evicting process %i from WebProcess cache", this, it->value->process().processIdentifier());
+        RELEASE_LOG(ProcessSwapping, "%p - WebProcessCache::addProcess(): Evicting process %i from WebProcess cache because capacity was reached", this, it->value->process().processIdentifier());
         m_processesPerRegistrableDomain.remove(it);
     }
 
-    m_processesPerRegistrableDomain.set(registrableDomain, std::make_unique<CachedProcess>(process.copyRef()));
-    RELEASE_LOG(ProcessSwapping, "%p - WebProcessCache::addProcess: Adding process %i to WebProcess cache, cache size: [%u / %u]", this, process->processIdentifier(), size(), capacity());
+    RELEASE_LOG(ProcessSwapping, "%p - WebProcessCache::addProcess: Added process %i to WebProcess cache, cache size: [%u / %u]", this, cachedProcess->process().processIdentifier(), size() + 1, capacity());
+    m_processesPerRegistrableDomain.add(registrableDomain, WTFMove(cachedProcess));
 
     return true;
 }
@@ -155,10 +170,11 @@ void WebProcessCache::updateCapacity(WebProcessPool& processPool)
 
 void WebProcessCache::clear()
 {
-    if (m_processesPerRegistrableDomain.isEmpty())
+    if (m_pendingAddRequests.isEmpty() && m_processesPerRegistrableDomain.isEmpty())
         return;
 
-    RELEASE_LOG(ProcessSwapping, "%p - WebProcessCache::clear() evicting %u processes", this, m_processesPerRegistrableDomain.size());
+    RELEASE_LOG(ProcessSwapping, "%p - WebProcessCache::clear() evicting %u processes", this, m_pendingAddRequests.size() + m_processesPerRegistrableDomain.size());
+    m_pendingAddRequests.clear();
     m_processesPerRegistrableDomain.clear();
 }
 
@@ -173,6 +189,16 @@ void WebProcessCache::clearAllProcessesForSession(PAL::SessionID sessionID)
     }
     for (auto& key : keysToRemove)
         m_processesPerRegistrableDomain.remove(key);
+
+    Vector<uint64_t> pendingRequestsToRemove;
+    for (auto& pair : m_pendingAddRequests) {
+        if (pair.value->process().websiteDataStore().sessionID() == sessionID) {
+            RELEASE_LOG(ProcessSwapping, "%p - WebProcessCache::clearAllProcessesForSession() evicting process %i because its session was destroyed", this, pair.value->process().processIdentifier());
+            pendingRequestsToRemove.append(pair.key);
+        }
+    }
+    for (auto& key : pendingRequestsToRemove)
+        m_pendingAddRequests.remove(key);
 }
 
 void WebProcessCache::setApplicationIsActive(bool isActive)
@@ -184,16 +210,32 @@ void WebProcessCache::setApplicationIsActive(bool isActive)
         m_evictionTimer.startOneShot(clearingDelayAfterApplicationResignsActive);
 }
 
-void WebProcessCache::evictProcess(WebProcessProxy& process)
+void WebProcessCache::removeProcess(WebProcessProxy& process, ShouldShutDownProcess shouldShutDownProcess)
 {
     RELEASE_ASSERT(!process.registrableDomain().isEmpty());
-    auto it = m_processesPerRegistrableDomain.find(process.registrableDomain());
-    ASSERT(it != m_processesPerRegistrableDomain.end());
-    ASSERT(&it->value->process() == &process);
-
     RELEASE_LOG(ProcessSwapping, "%p - WebProcessCache::evictProcess(): Evicting process %i from WebProcess cache because it expired", this, process.processIdentifier());
 
-    m_processesPerRegistrableDomain.remove(it);
+    std::unique_ptr<CachedProcess> cachedProcess;
+    auto it = m_processesPerRegistrableDomain.find(process.registrableDomain());
+    if (it != m_processesPerRegistrableDomain.end() && &it->value->process() == &process) {
+        cachedProcess = WTFMove(it->value);
+        m_processesPerRegistrableDomain.remove(it);
+    } else {
+        for (auto& pair : m_pendingAddRequests) {
+            if (&pair.value->process() == &process) {
+                cachedProcess = WTFMove(pair.value);
+                m_pendingAddRequests.remove(pair.key);
+                break;
+            }
+        }
+    }
+    ASSERT(cachedProcess);
+    if (!cachedProcess)
+        return;
+
+    ASSERT(&cachedProcess->process() == &process);
+    if (shouldShutDownProcess == ShouldShutDownProcess::No)
+        cachedProcess->takeProcess();
 }
 
 WebProcessCache::CachedProcess::CachedProcess(Ref<WebProcessProxy>&& process)
@@ -228,7 +270,7 @@ Ref<WebProcessProxy> WebProcessCache::CachedProcess::takeProcess()
 void WebProcessCache::CachedProcess::evictionTimerFired()
 {
     ASSERT(m_process);
-    m_process->processPool().webProcessCache().evictProcess(*m_process);
+    m_process->processPool().webProcessCache().removeProcess(*m_process, ShouldShutDownProcess::Yes);
 }
 
 #if !PLATFORM(COCOA)
index 691b8e5..0a30d53 100644 (file)
@@ -55,17 +55,13 @@ public:
 
     void clearAllProcessesForSession(PAL::SessionID);
 
+    enum class ShouldShutDownProcess { No, Yes };
+    void removeProcess(WebProcessProxy&, ShouldShutDownProcess);
+
 private:
     static Seconds cachedProcessLifetime;
     static Seconds clearingDelayAfterApplicationResignsActive;
 
-    bool canCacheProcess(WebProcessProxy&) const;
-    void evictProcess(WebProcessProxy&);
-    void platformInitialize();
-    bool addProcess(const String& registrableDomain, Ref<WebProcessProxy>&&);
-
-    unsigned m_capacity { 0 };
-
     class CachedProcess {
         WTF_MAKE_FAST_ALLOCATED;
     public:
@@ -82,6 +78,13 @@ private:
         RunLoop::Timer<CachedProcess> m_evictionTimer;
     };
 
+    bool canCacheProcess(WebProcessProxy&) const;
+    void platformInitialize();
+    bool addProcess(std::unique_ptr<CachedProcess>&&);
+
+    unsigned m_capacity { 0 };
+
+    HashMap<uint64_t, std::unique_ptr<CachedProcess>> m_pendingAddRequests;
     HashMap<String, std::unique_ptr<CachedProcess>> m_processesPerRegistrableDomain;
     RunLoop::Timer<WebProcessCache> m_evictionTimer;
 };
index 6d8fe86..e8ffa7a 100644 (file)
@@ -2260,8 +2260,8 @@ void WebProcessPool::processForNavigationInternal(WebPageProxy& page, const API:
 
             // Make sure we remove the process from the cache if it is in there since we're about to use it.
             if (process->isInProcessCache()) {
-                auto removedProcess = webProcessCache().takeProcess(process->registrableDomain(), process->websiteDataStore());
-                ASSERT_UNUSED(removedProcess, removedProcess.get() == process.get());
+                webProcessCache().removeProcess(*process, WebProcessCache::ShouldShutDownProcess::No);
+                ASSERT(!process->isInProcessCache());
             }
 
             return completionHandler(process.releaseNonNull(), nullptr, "Using target back/forward item's process"_s);
index 6df95dd..50de216 100644 (file)
@@ -649,8 +649,8 @@ void WebProcessProxy::processDidTerminateOrFailedToLaunch()
         callback(false);
 
     if (m_isInProcessCache) {
-        auto removedProcess = processPool().webProcessCache().takeProcess(registrableDomain(), websiteDataStore());
-        ASSERT_UNUSED(removedProcess, removedProcess.get() == this);
+        processPool().webProcessCache().removeProcess(*this, WebProcessCache::ShouldShutDownProcess::No);
+        ASSERT(!m_isInProcessCache);
     }
 
     shutDown();