Changeset 246452 in webkit

Timestamp:

Jun 14, 2019 6:07:16 PM (5 years ago)

Author:

youenn@apple.com

Message:

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

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• NetworkProcess/NetworkConnectionToWebProcess.cpp (modified) (2 diffs)
• NetworkProcess/NetworkConnectionToWebProcess.h (modified) (2 diffs)
• NetworkProcess/NetworkConnectionToWebProcess.messages.in (modified) (1 diff)
• NetworkProcess/NetworkResourceLoadMap.cpp (modified) (2 diffs)
• NetworkProcess/NetworkResourceLoadMap.h (modified) (2 diffs)
• UIProcess/WebProcessPool.cpp (modified) (2 diffs)
• UIProcess/WebProcessProxy.cpp (modified) (1 diff)
• WebProcess/Network/WebLoaderStrategy.cpp (modified) (5 diffs)
• WebProcess/Network/WebLoaderStrategy.h (modified) (1 diff)
• WebProcess/WebProcess.cpp (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp

@@ -84 +84 @@
     : m_connection(IPC::Connection::createServerConnection(connectionIdentifier, *this))
     , m_networkProcess(networkProcess)
-    , m_networkResourceLoaders(*this)
 #if ENABLE(WEB_RTC)
     , m_mdnsRegister(*this)
@@ -900 +899 @@
 #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)
 {

trunk/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h

@@ -143 +143 @@
     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);
@@ -319 +315 @@
     std::unique_ptr<WebPaymentCoordinatorProxy> m_paymentCoordinator;
 #endif
-
-    WebCore::ProcessIdentifier m_webProcessIdentifier;
-    bool m_connectionHasUploads { false };
 };
 

trunk/Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.messages.in

@@ -87 +87 @@
 #endif
 
-    SetWebProcessIdentifier(WebCore::ProcessIdentifier processIdentifier)
-
     WebPageWasAdded(PAL::SessionID sessionID, WebCore::PageIdentifier pageID, WebCore::PageIdentifier oldPageID)
     WebPageWasRemoved(PAL::SessionID sessionID, WebCore::PageIdentifier pageID)

trunk/Source/WebKit/NetworkProcess/NetworkResourceLoadMap.cpp

@@ -27 +27 @@
 #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));
 }
 
@@ -55 +45 @@
     if (!loader)
         return nullptr;
-
-    if ((*loader)->originalRequest().hasUpload()) {
-        m_loadersWithUploads.remove(loader->ptr());
-        if (m_loadersWithUploads.isEmpty())
-            m_connectionToWebProcess.clearConnectionHasUploads();
-    }
-
     return WTFMove(*loader);
 }

trunk/Source/WebKit/NetworkProcess/NetworkResourceLoadMap.h

@@ -44 +44 @@
     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); }
@@ -60 +55 @@
 
 private:
-    NetworkConnectionToWebProcess& m_connectionToWebProcess;
     MapType m_loaders;
-    HashSet<NetworkResourceLoader*> m_loadersWithUploads;
 };
 

trunk/Source/WebKit/UIProcess/WebProcessPool.cpp

@@ -2532 +2532 @@
 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());
@@ -2554 +2558 @@
 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);

trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp

@@ -169 +169 @@
     ASSERT(m_pageURLRetainCountMap.isEmpty());
 
+    if (m_processPool)
+        m_processPool->clearWebProcessHasUploads(coreProcessIdentifier());
+
     auto result = allProcesses().remove(coreProcessIdentifier());
     ASSERT_UNUSED(result, result);

trunk/Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp

@@ -42 +42 @@
 #include "WebPageProxyMessages.h"
 #include "WebProcess.h"
+#include "WebProcessPoolMessages.h"
 #include "WebResourceLoader.h"
 #include "WebServiceWorkerProvider.h"
@@ -358 +359 @@
     }
 
-    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));
 }
 
@@ -423 +431 @@
 
     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)
@@ -555 +566 @@
     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);
@@ -562 +577 @@
         error = internalError(request.url());
     }
+
+    if (shouldNotifyOfUpload)
+        WebProcess::singleton().parentProcessConnection()->send(Messages::WebProcessPool::ClearWebProcessHasUploads { Process::identifier() }, 0);
 }
 

trunk/Source/WebKit/WebProcess/Network/WebLoaderStrategy.h

@@ -123 +123 @@
     Vector<Function<void(bool)>> m_onlineStateChangeListeners;
     bool m_isOnLine { true };
+    HashSet<WebResourceLoader*> m_loadersWithUploads;
 };
 

trunk/Source/WebKit/WebProcess/WebProcess.cpp

@@ -1247 +1247 @@
 
         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.