Attachment 362564 Details for Bug 194881 – Patch

[patch] Patch

bug-194881-20190220161920.patch (text/plain), 12.39 KB, created by Chris Dumez on 2019-02-20 16:19:21 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-02-20 16:19:21 PST

Size: 12.39 KB

patch

obsolete

>Subversion Revision: 241842
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index dd17d82d65ab0f86114fc17fb6e517db5fb25e04..1dedcf375b9853d35a10aa6990f7394a7911426f 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,37 @@
>+2019-02-20  Chris Dumez  <cdumez@apple.com>
>+
>+        [PSON] Make sure hung processes are not kept alive by suspended pages or process caching
>+        https://bugs.webkit.org/show_bug.cgi?id=194881
>+        <rdar://problem/48249014>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        After we construct a SuspendedPageProxy and before we send the IPC to the WebProcess to
>+        ask it to suspend, start a 10 seconds timer. If the process does not answer the request
>+        to suspend before the timer fires, we destroy the SuspendedPageProxy so that we do not
>+        keep a hung process around.
>+
>+        For the WebProcessCache, we now call WebProcessProxy::isResponsive() on the process
>+        before adding it to the cache. Internally, this relies on an IPC handshake with the
>+        WebProcess. If the process is not responsive, we do not add it to the cache and we
>+        shut it down. If it is responsive then we proceed normally with adding it to the
>+        cache.
>+
>+        * UIProcess/SuspendedPageProxy.cpp:
>+        (WebKit::SuspendedPageProxy::SuspendedPageProxy):
>+        (WebKit::SuspendedPageProxy::didProcessRequestToSuspend):
>+        (WebKit::SuspendedPageProxy::suspensionTimedOut):
>+        * UIProcess/SuspendedPageProxy.h:
>+        * UIProcess/WebProcessCache.cpp:
>+        (WebKit::WebProcessCache::addProcessIfPossible):
>+        (WebKit::WebProcessCache::addProcess):
>+        * UIProcess/WebProcessCache.h:
>+        * UIProcess/WebProcessProxy.cpp:
>+        (WebKit::WebProcessProxy::processDidTerminateOrFailedToLaunch):
>+        (WebKit::WebProcessProxy::maybeShutDown):
>+        (WebKit::WebProcessProxy::isResponsive):
>+        * UIProcess/WebProcessProxy.h:
>+
> 2019-02-20  Andy Estes  <aestes@apple.com>
> 
>         [Xcode] Add SDKVariant.xcconfig to various Xcode projects
>diff --git a/Source/WebKit/UIProcess/SuspendedPageProxy.cpp b/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
>index 59e0e0b0325e61ad81d85d1e1257562fcc749d50..f3540402a031dc9c6f7f95571e55b68868da76a5 100644
>--- a/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/SuspendedPageProxy.cpp
>@@ -41,6 +41,8 @@
> namespace WebKit {
> using namespace WebCore;
> 
>+static const Seconds suspensionTimeout { 10_s };
>+
> #if !LOG_DISABLED
> static const HashSet<IPC::StringReference>& messageNamesToIgnoreWhileSuspended()
> {
>@@ -81,6 +83,7 @@ SuspendedPageProxy::SuspendedPageProxy(WebPageProxy& page, Ref<WebProcessProxy>&
>     , m_process(WTFMove(process))
>     , m_mainFrameID(mainFrameID)
>     , m_registrableDomain(toRegistrableDomain(URL(URL(), item.url())))
>+    , m_suspensionTimeoutTimer(RunLoop::main(), this, &SuspendedPageProxy::suspensionTimedOut)
> #if PLATFORM(IOS_FAMILY)
>     , m_suspensionToken(m_process->throttler().backgroundActivityToken())
> #endif
>@@ -88,6 +91,7 @@ SuspendedPageProxy::SuspendedPageProxy(WebPageProxy& page, Ref<WebProcessProxy>&
>     item.setSuspendedPage(this);
>     m_process->addMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_page.pageID(), *this);
> 
>+    m_suspensionTimeoutTimer.startOneShot(suspensionTimeout);
>     m_process->send(Messages::WebPage::SetIsSuspended(true), m_page.pageID());
> }
> 
>@@ -164,6 +168,8 @@ void SuspendedPageProxy::didProcessRequestToSuspend(SuspensionState newSuspensio
> 
>     m_suspensionState = newSuspensionState;
> 
>+    m_suspensionTimeoutTimer.stop();
>+
> #if PLATFORM(IOS_FAMILY)
>     m_suspensionToken = nullptr;
> #endif
>@@ -183,6 +189,12 @@ void SuspendedPageProxy::didProcessRequestToSuspend(SuspensionState newSuspensio
>         m_readyToUnsuspendHandler(this);
> }
> 
>+void SuspendedPageProxy::suspensionTimedOut()
>+{
>+    RELEASE_LOG_ERROR(ProcessSwapping, "%p - SuspendedPageProxy::suspensionTimedOut() destroying the suspended page because it failed to suspend in time", this);
>+    m_process->processPool().removeSuspendedPage(*this); // Will destroy |this|.
>+}
>+
> void SuspendedPageProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder& decoder)
> {
>     ASSERT(decoder.messageReceiverName() == Messages::WebPageProxy::messageReceiverName());
>diff --git a/Source/WebKit/UIProcess/SuspendedPageProxy.h b/Source/WebKit/UIProcess/SuspendedPageProxy.h
>index 86ac0206ba38bca1188d3cb46352915c12fbe7bd..3ad19ea6ade484b96f76552efd187a93b66656f1 100644
>--- a/Source/WebKit/UIProcess/SuspendedPageProxy.h
>+++ b/Source/WebKit/UIProcess/SuspendedPageProxy.h
>@@ -61,6 +61,7 @@ public:
> private:
>     enum class SuspensionState : uint8_t { Suspending, FailedToSuspend, Suspended, Resumed };
>     void didProcessRequestToSuspend(SuspensionState);
>+    void suspensionTimedOut();
> 
>     // IPC::MessageReceiver
>     void didReceiveMessage(IPC::Connection&, IPC::Decoder&) final;
>@@ -74,6 +75,7 @@ private:
> 
>     SuspensionState m_suspensionState { SuspensionState::Suspending };
>     CompletionHandler<void(SuspendedPageProxy*)> m_readyToUnsuspendHandler;
>+    RunLoop::Timer<SuspendedPageProxy> m_suspensionTimeoutTimer;
> #if PLATFORM(IOS_FAMILY)
>     ProcessThrottler::BackgroundActivityToken m_suspensionToken;
> #endif
>diff --git a/Source/WebKit/UIProcess/WebProcessCache.cpp b/Source/WebKit/UIProcess/WebProcessCache.cpp
>index b20c0e201e88b72cfd80e2313363bc5784867ece..3e9e17e0ad6e0de19195c9be43d01ef132bf2c66 100644
>--- a/Source/WebKit/UIProcess/WebProcessCache.cpp
>+++ b/Source/WebKit/UIProcess/WebProcessCache.cpp
>@@ -44,13 +44,47 @@ WebProcessCache::WebProcessCache(WebProcessPool& processPool)
>     platformInitialize();
> }
> 
>-bool WebProcessCache::addProcess(const String& registrableDomain, Ref<WebProcessProxy>&& process)
>+bool WebProcessCache::addProcessIfPossible(const String& registrableDomain, Ref<WebProcessProxy>&& process)
> {
>     ASSERT(!registrableDomain.isEmpty());
>     ASSERT(!process->pageCount());
>     ASSERT(!process->provisionalPageCount());
>     ASSERT(!process->processPool().hasSuspendedPageFor(process));
> 
>+    if (!capacity())
>+        return false;
>+
>+    if (MemoryPressureHandler::singleton().isUnderMemoryPressure()) {
>+        RELEASE_LOG(ProcessSwapping, "%p - WebProcessCache::addProcessIfPossible(): Not caching process %i because we are under memory pressure", this, process->processIdentifier());
>+        return false;
>+    }
>+
>+    if (m_processesPerRegistrableDomain.contains(registrableDomain)) {
>+        RELEASE_LOG(ProcessSwapping, "%p - WebProcessCache::addProcessIfPossible(): Not caching process %i because we already have a cached process for this domain", this, process->processIdentifier());
>+        return false;
>+    }
>+
>+    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(), registrableDomain](bool isResponsive) {
>+        process->setIsInProcessCache(false);
>+        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();
>+            return;
>+        }
>+        if (!process->processPool().webProcessCache().addProcess(registrableDomain, process.copyRef()))
>+            process->shutDown();
>+    });
>+    return true;
>+}
>+
>+bool WebProcessCache::addProcess(const String& registrableDomain, Ref<WebProcessProxy>&& process)
>+{
>+    ASSERT(!process->pageCount());
>+    ASSERT(!process->provisionalPageCount());
>+    ASSERT(!process->processPool().hasSuspendedPageFor(process));
>+
>     if (!capacity())
>         return false;
> 
>@@ -68,8 +102,10 @@ bool WebProcessCache::addProcess(const String& registrableDomain, Ref<WebProcess
>     auto addResult = m_processesPerRegistrableDomain.ensure(registrableDomain, [process = process.copyRef()]() mutable {
>         return std::make_unique<CachedProcess>(WTFMove(process));
>     });
>-    if (!addResult.isNewEntry)
>+    if (!addResult.isNewEntry) {
>+        RELEASE_LOG(ProcessSwapping, "%p - WebProcessCache::addProcess(): Not caching process %i because we already have a cached process for this domain", this, process->processIdentifier());
>         return false;
>+    }
> 
>     RELEASE_LOG(ProcessSwapping, "%p - WebProcessCache::addProcess: Adding process %i to WebProcess cache, cache size: [%u / %u]", this, process->processIdentifier(), size(), capacity());
>     return true;
>diff --git a/Source/WebKit/UIProcess/WebProcessCache.h b/Source/WebKit/UIProcess/WebProcessCache.h
>index 19adf617a73f385950cdcbbfc5519d504685e603..c52225f37a9bfac770bb9dee1c5302d896e779f8 100644
>--- a/Source/WebKit/UIProcess/WebProcessCache.h
>+++ b/Source/WebKit/UIProcess/WebProcessCache.h
>@@ -41,7 +41,7 @@ class WebProcessCache {
> public:
>     explicit WebProcessCache(WebProcessPool&);
> 
>-    bool addProcess(const String& registrableDomain, Ref<WebProcessProxy>&&);
>+    bool addProcessIfPossible(const String& registrableDomain, Ref<WebProcessProxy>&&);
>     RefPtr<WebProcessProxy> takeProcess(const String& registrableDomain, WebsiteDataStore&);
> 
>     void updateCapacity(WebProcessPool&);
>@@ -58,6 +58,7 @@ private:
> 
>     void evictProcess(WebProcessProxy&);
>     void platformInitialize();
>+    bool addProcess(const String& registrableDomain, Ref<WebProcessProxy>&&);
> 
>     unsigned m_capacity { 0 };
> 
>diff --git a/Source/WebKit/UIProcess/WebProcessProxy.cpp b/Source/WebKit/UIProcess/WebProcessProxy.cpp
>index d1441cb4da1ffa5077cd8926897170cecfdf5f31..9f08d1c47ce43b50165c81835777ddf3eb89f73f 100644
>--- a/Source/WebKit/UIProcess/WebProcessProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebProcessProxy.cpp
>@@ -636,6 +636,10 @@ void WebProcessProxy::processDidTerminateOrFailedToLaunch()
>     auto pages = copyToVectorOf<RefPtr<WebPageProxy>>(m_pageMap.values());
>     auto provisionalPages = WTF::map(m_provisionalPages, [](auto* provisionalPage) { return makeWeakPtr(provisionalPage); });
> 
>+    auto isResponsiveCallbacks = WTFMove(m_isResponsiveCallbacks);
>+    for (auto& callback : isResponsiveCallbacks)
>+        callback(false);
>+
>     if (m_isInProcessCache) {
>         auto removedProcess = processPool().webProcessCache().takeProcess(registrableDomain(), websiteDataStore());
>         ASSERT_UNUSED(removedProcess, removedProcess.get() == this);
>@@ -839,7 +843,7 @@ void WebProcessProxy::maybeShutDown()
>     if (state() == State::Terminated || !canTerminateAuxiliaryProcess())
>         return;
> 
>-    if (canBeAddedToWebProcessCache() && processPool().webProcessCache().addProcess(registrableDomain(), *this))
>+    if (canBeAddedToWebProcessCache() && processPool().webProcessCache().addProcessIfPossible(registrableDomain(), *this))
>         return;
> 
>     shutDown();
>@@ -1183,11 +1187,11 @@ void WebProcessProxy::setIsHoldingLockedFiles(bool isHoldingLockedFiles)
>     }
> }
> 
>-void WebProcessProxy::isResponsive(WTF::Function<void(bool isWebProcessResponsive)>&& callback)
>+void WebProcessProxy::isResponsive(CompletionHandler<void(bool isWebProcessResponsive)>&& callback)
> {
>     if (m_isResponsive == NoOrMaybe::No) {
>         if (callback) {
>-            RunLoop::main().dispatch([callback = WTFMove(callback)] {
>+            RunLoop::main().dispatch([callback = WTFMove(callback)]() mutable {
>                 bool isWebProcessResponsive = false;
>                 callback(isWebProcessResponsive);
>             });
>diff --git a/Source/WebKit/UIProcess/WebProcessProxy.h b/Source/WebKit/UIProcess/WebProcessProxy.h
>index 837bf8d5bd3744a755a596ba960b435b09ab695e..94c98d2692483bcb0b87118b2b5e9ad86dcdde9a 100644
>--- a/Source/WebKit/UIProcess/WebProcessProxy.h
>+++ b/Source/WebKit/UIProcess/WebProcessProxy.h
>@@ -206,7 +206,7 @@ public:
> 
>     ProcessThrottler& throttler() { return m_throttler; }
> 
>-    void isResponsive(WTF::Function<void(bool isWebProcessResponsive)>&&);
>+    void isResponsive(CompletionHandler<void(bool isWebProcessResponsive)>&&);
>     void isResponsiveWithLazyStop();
>     void didReceiveMainThreadPing();
>     void didReceiveBackgroundResponsivenessPing();
>@@ -409,7 +409,7 @@ private:
>     bool m_isInProcessCache { false };
> 
>     enum class NoOrMaybe { No, Maybe } m_isResponsive;
>-    Vector<WTF::Function<void(bool webProcessIsResponsive)>> m_isResponsiveCallbacks;
>+    Vector<CompletionHandler<void(bool webProcessIsResponsive)>> m_isResponsiveCallbacks;
> 
>     VisibleWebPageCounter m_visiblePageCounter;
>

Actions: View | Formatted Diff | Diff

Attachments on bug 194881: 362564