Attachment 372139 Details for Bug 198865 – Patch

[patch] Patch

bug-198865-20190614140854.patch (text/plain), 16.09 KB, created by youenn fablet on 2019-06-14 14:08:55 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: youenn fablet

Created: 2019-06-14 14:08:55 PDT

Size: 16.09 KB

patch

obsolete

>Subversion Revision: 246434
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 5475ae5604acbebbb79072d875280cae4d897867..beeb8aecaf839c19cbe27659f3580c939b9e2d74 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,42 @@
>+2019-06-14  Youenn Fablet  <youenn@apple.com>
>+
>+        WebProcessPool::clearWebProcessHasUploads cannot assume its given processIdentifier is valid
>+        https://bugs.webkit.org/show_bug.cgi?id=198865
>+        <rdar://problem/51618878>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        NetworkProcess currently instructs UIProcess whether a given WebProcess is doing upload.
>+        There is no guarantee though that the WebProcessProxy is still there when the IPC is arriving at UIProcess.
>+        Instead, let WebProcess handles its upload state and notify WebProcessPool about its state.
>+        Make sure WebProcessProxy unregisters itself in case of crash.
>+        In case of NetworkProcess crash, WebProcesses will see all their uploads fail
>+        and will notify automatically UIProcess to update their state.
>+
>+        Since the processID given to WebProcessPool is coming from IPC, we cannot not trust it.
>+        Add early return in case of not finding a WebProcessProxy for WebProcessPool clear/set methods.
>+
>+        * NetworkProcess/NetworkConnectionToWebProcess.cpp:
>+        (WebKit::NetworkConnectionToWebProcess::NetworkConnectionToWebProcess):
>+        * NetworkProcess/NetworkConnectionToWebProcess.h:
>+        * NetworkProcess/NetworkConnectionToWebProcess.messages.in:
>+        * NetworkProcess/NetworkResourceLoadMap.cpp:
>+        (WebKit::NetworkResourceLoadMap::add):
>+        (WebKit::NetworkResourceLoadMap::take):
>+        * NetworkProcess/NetworkResourceLoadMap.h:
>+        * UIProcess/WebProcessPool.cpp:
>+        (WebKit::WebProcessPool::setWebProcessHasUploads):
>+        (WebKit::WebProcessPool::clearWebProcessHasUploads):
>+        * UIProcess/WebProcessProxy.cpp:
>+        (WebKit::WebProcessProxy::~WebProcessProxy):
>+        * WebProcess/Network/WebLoaderStrategy.cpp:
>+        (WebKit::WebLoaderStrategy::scheduleLoadFromNetworkProcess):
>+        (WebKit::WebLoaderStrategy::remove):
>+        (WebKit::WebLoaderStrategy::tryLoadingSynchronouslyUsingURLSchemeHandler):
>+        * WebProcess/Network/WebLoaderStrategy.h:
>+        * WebProcess/WebProcess.cpp:
>+        (WebKit::WebProcess::ensureNetworkProcessConnection):
>+
> 2019-06-14  Youenn Fablet  <youenn@apple.com>
> 
>         WebResourceLoadStatisticsStore should not use its network session if invalidated
>diff --git a/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp b/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp
>index 560e6fd196d9e42651249c2dcf2d522c1e011c30..5992639caa2e1ba3bd12b014098955cb0de73dc9 100644
>--- a/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp
>@@ -83,7 +83,6 @@ Ref<NetworkConnectionToWebProcess> NetworkConnectionToWebProcess::create(Network
> NetworkConnectionToWebProcess::NetworkConnectionToWebProcess(NetworkProcess& networkProcess, IPC::Connection::Identifier connectionIdentifier)
>     : m_connection(IPC::Connection::createServerConnection(connectionIdentifier, *this))
>     , m_networkProcess(networkProcess)
>-    , m_networkResourceLoaders(*this)
> #if ENABLE(WEB_RTC)
>     , m_mdnsRegister(*this)
> #endif
>@@ -899,25 +898,6 @@ void NetworkConnectionToWebProcess::establishSWServerConnection(PAL::SessionID s
> }
> #endif
> 
>-void NetworkConnectionToWebProcess::setWebProcessIdentifier(ProcessIdentifier webProcessIdentifier)
>-{
>-    m_webProcessIdentifier = webProcessIdentifier;
>-}
>-
>-void NetworkConnectionToWebProcess::setConnectionHasUploads()
>-{
>-    ASSERT(!m_connectionHasUploads);
>-    m_connectionHasUploads = true;
>-    m_networkProcess->parentProcessConnection()->send(Messages::WebProcessPool::SetWebProcessHasUploads(m_webProcessIdentifier), 0);
>-}
>-
>-void NetworkConnectionToWebProcess::clearConnectionHasUploads()
>-{
>-    ASSERT(m_connectionHasUploads);
>-    m_connectionHasUploads = false;
>-    m_networkProcess->parentProcessConnection()->send(Messages::WebProcessPool::ClearWebProcessHasUploads(m_webProcessIdentifier), 0);
>-}
>-
> void NetworkConnectionToWebProcess::webPageWasAdded(PAL::SessionID sessionID, PageIdentifier pageID, WebCore::PageIdentifier oldPageID)
> {
>     m_networkProcess->webPageWasAdded(m_connection.get(), sessionID, pageID, oldPageID);
>diff --git a/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h b/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h
>index c0bafe85597d25a5a5291dd06522d3c282c81b40..19e76723781bfeb470fb2622b93eca74e9aeee8c 100644
>--- a/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h
>+++ b/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h
>@@ -142,10 +142,6 @@ public:
>     Vector<RefPtr<WebCore::BlobDataFileReference>> filesInBlob(const URL&);
>     Vector<RefPtr<WebCore::BlobDataFileReference>> resolveBlobReferences(const NetworkResourceLoadParameters&);
> 
>-    void setWebProcessIdentifier(WebCore::ProcessIdentifier);
>-    void setConnectionHasUploads();
>-    void clearConnectionHasUploads();
>-
>     void webPageWasAdded(PAL::SessionID, WebCore::PageIdentifier, WebCore::PageIdentifier oldPageID);
>     void webPageWasRemoved(PAL::SessionID, WebCore::PageIdentifier);
>     void webProcessSessionChanged(PAL::SessionID newSessionID, const Vector<WebCore::PageIdentifier>& pages);
>@@ -318,9 +314,6 @@ private:
> #if ENABLE(APPLE_PAY_REMOTE_UI)
>     std::unique_ptr<WebPaymentCoordinatorProxy> m_paymentCoordinator;
> #endif
>-
>-    WebCore::ProcessIdentifier m_webProcessIdentifier;
>-    bool m_connectionHasUploads { false };
> };
> 
> } // namespace WebKit
>diff --git a/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.messages.in b/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.messages.in
>index 22077eaddbe0e6dec0cbc62f5eeddfa40303cf67..7709e2f65c16488c68fb1e72352b1a249c5d94d4 100644
>--- a/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.messages.in
>+++ b/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.messages.in
>@@ -86,8 +86,6 @@ messages -> NetworkConnectionToWebProcess LegacyReceiver {
>     EstablishSWServerConnection(PAL::SessionID sessionID) -> (WebCore::SWServerConnectionIdentifier serverConnectionIdentifier) Synchronous
> #endif
> 
>-    SetWebProcessIdentifier(WebCore::ProcessIdentifier processIdentifier)
>-
>     WebPageWasAdded(PAL::SessionID sessionID, WebCore::PageIdentifier pageID, WebCore::PageIdentifier oldPageID)
>     WebPageWasRemoved(PAL::SessionID sessionID, WebCore::PageIdentifier pageID)
>     WebProcessSessionChanged(PAL::SessionID newSessionID, Vector<WebCore::PageIdentifier> pages)
>diff --git a/Source/WebKit/NetworkProcess/NetworkResourceLoadMap.cpp b/Source/WebKit/NetworkProcess/NetworkResourceLoadMap.cpp
>index e3332cee14d418aed48ffdeac779a38b1cc4d1af..65fd58fe0a72ae49d98f855f55d092dfe3d84e1b 100644
>--- a/Source/WebKit/NetworkProcess/NetworkResourceLoadMap.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkResourceLoadMap.cpp
>@@ -26,22 +26,12 @@
> #include "config.h"
> #include "NetworkResourceLoadMap.h"
> 
>-#include "NetworkConnectionToWebProcess.h"
>-
> namespace WebKit {
> 
> NetworkResourceLoadMap::MapType::AddResult NetworkResourceLoadMap::add(ResourceLoadIdentifier identifier, Ref<NetworkResourceLoader>&& loader)
> {
>-    auto result = m_loaders.add(identifier, WTFMove(loader));
>-    ASSERT(result.isNewEntry);
>-        
>-    if (result.iterator->value->originalRequest().hasUpload()) {
>-        if (m_loadersWithUploads.isEmpty())
>-            m_connectionToWebProcess.setConnectionHasUploads();
>-        m_loadersWithUploads.add(result.iterator->value.ptr());
>-    }
>-
>-    return result;
>+    ASSERT(!m_loaders.contains(identifier));
>+    return m_loaders.add(identifier, WTFMove(loader));
> }
> 
> bool NetworkResourceLoadMap::remove(ResourceLoadIdentifier identifier)
>@@ -54,13 +44,6 @@ RefPtr<NetworkResourceLoader> NetworkResourceLoadMap::take(ResourceLoadIdentifie
>     auto loader = m_loaders.take(identifier);
>     if (!loader)
>         return nullptr;
>-
>-    if ((*loader)->originalRequest().hasUpload()) {
>-        m_loadersWithUploads.remove(loader->ptr());
>-        if (m_loadersWithUploads.isEmpty())
>-            m_connectionToWebProcess.clearConnectionHasUploads();
>-    }
>-
>     return WTFMove(*loader);
> }
> 
>diff --git a/Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h b/Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h
>index 6d4079ca64522af7b1b6d387f8fe4bb3d13c700d..3efc77c8eb0c9411124452123925c7f95fedcfa4 100644
>--- a/Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h
>+++ b/Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h
>@@ -43,11 +43,6 @@ class NetworkResourceLoadMap {
> public:
>     typedef HashMap<ResourceLoadIdentifier, Ref<NetworkResourceLoader>> MapType;
> 
>-    NetworkResourceLoadMap(NetworkConnectionToWebProcess& connection)
>-        : m_connectionToWebProcess(connection)
>-    {
>-    }
>-
>     bool isEmpty() const { return m_loaders.isEmpty(); }
>     bool contains(ResourceLoadIdentifier identifier) const { return m_loaders.contains(identifier); }
>     MapType::iterator begin() { return m_loaders.begin(); }
>@@ -59,9 +54,7 @@ public:
>     RefPtr<NetworkResourceLoader> take(ResourceLoadIdentifier);
> 
> private:
>-    NetworkConnectionToWebProcess& m_connectionToWebProcess;
>     MapType m_loaders;
>-    HashSet<NetworkResourceLoader*> m_loadersWithUploads;
> };
> 
> } // namespace WebKit
>diff --git a/Source/WebKit/UIProcess/WebProcessPool.cpp b/Source/WebKit/UIProcess/WebProcessPool.cpp
>index 43d7700e5f7f467bebbd99a9eed1577c7b44b21d..f158268b3bb959de918e06734687862b0f79ce91 100644
>--- a/Source/WebKit/UIProcess/WebProcessPool.cpp
>+++ b/Source/WebKit/UIProcess/WebProcessPool.cpp
>@@ -2531,9 +2531,13 @@ void WebProcessPool::didCommitCrossSiteLoadWithDataTransfer(PAL::SessionID sessi
> 
> void WebProcessPool::setWebProcessHasUploads(ProcessIdentifier processID)
> {
>+    ASSERT(processID);
>     auto* process = WebProcessProxy::processForIdentifier(processID);
>     ASSERT(process);
> 
>+    if (!process)
>+        return;
>+
>     RELEASE_LOG(ProcessSuspension, "Web process pid %u now has uploads in progress", (unsigned)process->processIdentifier());
> 
>     if (m_processesWithUploads.isEmpty()) {
>@@ -2553,8 +2557,11 @@ void WebProcessPool::setWebProcessHasUploads(ProcessIdentifier processID)
> 
> void WebProcessPool::clearWebProcessHasUploads(ProcessIdentifier processID)
> {
>+    ASSERT(processID);
>     auto result = m_processesWithUploads.take(processID);
>-    ASSERT_UNUSED(result, result);
>+    ASSERT(result);
>+    if (!result)
>+        return;
> 
>     auto* process = WebProcessProxy::processForIdentifier(processID);
>     ASSERT_UNUSED(process, process);
>diff --git a/Source/WebKit/UIProcess/WebProcessProxy.cpp b/Source/WebKit/UIProcess/WebProcessProxy.cpp
>index 5603c8a4d565b7c8c8b9630139558b235ed4203d..3a0c12d7e381f7d19eef6e6508b29d6bf0b73e33 100644
>--- a/Source/WebKit/UIProcess/WebProcessProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebProcessProxy.cpp
>@@ -168,6 +168,9 @@ WebProcessProxy::~WebProcessProxy()
>     RELEASE_ASSERT(isMainThreadOrCheckDisabled());
>     ASSERT(m_pageURLRetainCountMap.isEmpty());
> 
>+    if (m_processPool)
>+        m_processPool->clearWebProcessHasUploads(coreProcessIdentifier());
>+
>     auto result = allProcesses().remove(coreProcessIdentifier());
>     ASSERT_UNUSED(result, result);
> 
>diff --git a/Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp b/Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp
>index 15256733b5d2b98180fcf53b6fbc7232cb31672c..e21f4a13957c4de987ae47b3e206b8bcd19c8066 100644
>--- a/Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp
>+++ b/Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp
>@@ -41,6 +41,7 @@
> #include "WebPage.h"
> #include "WebPageProxyMessages.h"
> #include "WebProcess.h"
>+#include "WebProcessPoolMessages.h"
> #include "WebResourceLoader.h"
> #include "WebServiceWorkerProvider.h"
> #include "WebURLSchemeHandlerProxy.h"
>@@ -357,7 +358,14 @@ void WebLoaderStrategy::scheduleLoadFromNetworkProcess(ResourceLoader& resourceL
>         return;
>     }
> 
>-    m_webResourceLoaders.set(identifier, WebResourceLoader::create(resourceLoader, trackingParameters));
>+    auto loader = WebResourceLoader::create(resourceLoader, trackingParameters);
>+    if (resourceLoader.originalRequest().hasUpload()) {
>+        if (m_loadersWithUploads.isEmpty())
>+            WebProcess::singleton().parentProcessConnection()->send(Messages::WebProcessPool::SetWebProcessHasUploads(Process::identifier()), 0);
>+        m_loadersWithUploads.add(loader.ptr());
>+    }
>+
>+    m_webResourceLoaders.set(identifier, WTFMove(loader));
> }
> 
> void WebLoaderStrategy::scheduleInternallyFailedLoad(WebCore::ResourceLoader& resourceLoader)
>@@ -423,6 +431,9 @@ void WebLoaderStrategy::remove(ResourceLoader* resourceLoader)
> 
>     WebProcess::singleton().ensureNetworkProcessConnection().connection().send(Messages::NetworkConnectionToWebProcess::RemoveLoadIdentifier(identifier), 0);
> 
>+    if (m_loadersWithUploads.remove(loader.get()) && m_loadersWithUploads.isEmpty())
>+        WebProcess::singleton().parentProcessConnection()->send(Messages::WebProcessPool::ClearWebProcessHasUploads { Process::identifier() }, 0);
>+
>     // It's possible that this WebResourceLoader might be just about to message back to the NetworkProcess (e.g. ContinueWillSendRequest)
>     // but there's no point in doing so anymore.
>     loader->detachFromCoreLoader();
>@@ -554,6 +565,10 @@ void WebLoaderStrategy::loadResourceSynchronously(FrameLoader& frameLoader, unsi
> 
>     HangDetectionDisabler hangDetectionDisabler;
> 
>+    bool shouldNotifyOfUpload = request.hasUpload() && m_loadersWithUploads.isEmpty();
>+    if (shouldNotifyOfUpload)
>+        WebProcess::singleton().parentProcessConnection()->send(Messages::WebProcessPool::SetWebProcessHasUploads { Process::identifier() }, 0);
>+
>     if (!WebProcess::singleton().ensureNetworkProcessConnection().connection().sendSync(Messages::NetworkConnectionToWebProcess::PerformSynchronousLoad(loadParameters), Messages::NetworkConnectionToWebProcess::PerformSynchronousLoad::Reply(error, response, data), 0)) {
>         RELEASE_LOG_ERROR_IF_ALLOWED(sessionID, "loadResourceSynchronously: failed sending synchronous network process message (pageID = %" PRIu64 ", frameID = %" PRIu64 ", resourceID = %lu)", pageID.toUInt64(), frameID, resourceLoadIdentifier);
>         if (auto* page = webPage ? webPage->corePage() : nullptr)
>@@ -561,6 +576,9 @@ void WebLoaderStrategy::loadResourceSynchronously(FrameLoader& frameLoader, unsi
>         response = ResourceResponse();
>         error = internalError(request.url());
>     }
>+
>+    if (shouldNotifyOfUpload)
>+        WebProcess::singleton().parentProcessConnection()->send(Messages::WebProcessPool::ClearWebProcessHasUploads { Process::identifier() }, 0);
> }
> 
> void WebLoaderStrategy::pageLoadCompleted(PageIdentifier webPageID)
>diff --git a/Source/WebKit/WebProcess/Network/WebLoaderStrategy.h b/Source/WebKit/WebProcess/Network/WebLoaderStrategy.h
>index ac524966a8521e0e3d65eb62b9f70e82f78cc217..dc54f2dadc47d7fb1945bf1cd1f6597472e3c892 100644
>--- a/Source/WebKit/WebProcess/Network/WebLoaderStrategy.h
>+++ b/Source/WebKit/WebProcess/Network/WebLoaderStrategy.h
>@@ -122,6 +122,7 @@ private:
>     HashMap<unsigned long, PreconnectCompletionHandler> m_preconnectCompletionHandlers;
>     Vector<Function<void(bool)>> m_onlineStateChangeListeners;
>     bool m_isOnLine { true };
>+    HashSet<WebResourceLoader*> m_loadersWithUploads;
> };
> 
> } // namespace WebKit
>diff --git a/Source/WebKit/WebProcess/WebProcess.cpp b/Source/WebKit/WebProcess/WebProcess.cpp
>index 28055139782cb0c340b2011250b625deb8216329..a4bd7afe30c3815f96d572b4eb4f7f4ddec35212 100644
>--- a/Source/WebKit/WebProcess/WebProcess.cpp
>+++ b/Source/WebKit/WebProcess/WebProcess.cpp
>@@ -1246,7 +1246,6 @@ NetworkProcessConnection& WebProcess::ensureNetworkProcessConnection()
>             CRASH();
> 
>         m_networkProcessConnection = NetworkProcessConnection::create(connectionIdentifier);
>-        m_networkProcessConnection->connection().send(Messages::NetworkConnectionToWebProcess::SetWebProcessIdentifier(Process::identifier()), 0);
> 
>         // To recover web storage, network process needs to know active webpages to prepare session storage.
>         // FIXME: https://bugs.webkit.org/show_bug.cgi?id=198051.

Actions: View | Formatted Diff | Diff

Attachments on bug 198865: 372139 | 372148 | 372217