WebProcessPool::clearWebProcessHasUploads cannot assume its given processIdentifier...
authoryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 15 Jun 2019 01:07:16 +0000 (01:07 +0000)
committeryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 15 Jun 2019 01:07:16 +0000 (01:07 +0000)
https://bugs.webkit.org/show_bug.cgi?id=198865
<rdar://problem/51618878>

Reviewed by Brady Eidson.

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):

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

Source/WebKit/ChangeLog
Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp
Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h
Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.messages.in
Source/WebKit/NetworkProcess/NetworkResourceLoadMap.cpp
Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h
Source/WebKit/UIProcess/WebProcessPool.cpp
Source/WebKit/UIProcess/WebProcessProxy.cpp
Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp
Source/WebKit/WebProcess/Network/WebLoaderStrategy.h
Source/WebKit/WebProcess/WebProcess.cpp

index c65ca91..f04e1bf 100644 (file)
@@ -1,5 +1,44 @@
 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 Brady Eidson.
+
+        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
         https://bugs.webkit.org/show_bug.cgi?id=198814
 
index 560e6fd..5992639 100644 (file)
@@ -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);
index c0bafe8..19e7672 100644 (file)
@@ -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
index 22077ea..7709e2f 100644 (file)
@@ -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)
index e3332ce..65fd58f 100644 (file)
 #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);
 }
 
index 6d4079c..3efc77c 100644 (file)
@@ -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
index 43d7700..f158268 100644 (file)
@@ -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);
index 5603c8a..3a0c12d 100644 (file)
@@ -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);
 
index 1525673..e21f4a1 100644 (file)
@@ -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)
index ac52496..dc54f2d 100644 (file)
@@ -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
index 2805513..a4bd7af 100644 (file)
@@ -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.