Avoid races in taking networking assertions for downloads by having both Networking...
authorbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 17 May 2019 21:37:07 +0000 (21:37 +0000)
committerbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 17 May 2019 21:37:07 +0000 (21:37 +0000)
<rdar://problem/50564630> and https://bugs.webkit.org/show_bug.cgi?id=197995

Reviewed by Chris Dumez.

There's a fairly indeterminant time gap between when the UIProcess decides a load becomes a download
and when the NetworkProcess Download object is created, and therefore the download assertion to be taken.

The time gap can be long enough for the Networking process to suspend before the download actually starts.

There's the reverse race when the UIProcess tells a download to stop, as well.

By having both the UIProcess and NetworkProcess take an assertion on behalf of the NetworkProcess we
avoid the race.

* NetworkProcess/Downloads/DownloadMap.cpp:
(WebKit::DownloadMap::add):
(WebKit::DownloadMap::remove):

* NetworkProcess/NetworkResourceLoader.cpp:
(WebKit::NetworkResourceLoader::convertToDownload):

* UIProcess/Downloads/DownloadProxyMap.cpp:
(WebKit::DownloadProxyMap::createDownloadProxy):
(WebKit::DownloadProxyMap::downloadFinished):
(WebKit::DownloadProxyMap::invalidate):
* UIProcess/Downloads/DownloadProxyMap.h:

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

Source/WebKit/ChangeLog
Source/WebKit/NetworkProcess/Downloads/DownloadMap.cpp
Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp
Source/WebKit/UIProcess/Downloads/DownloadProxyMap.h

index 695de90..1f83202 100644 (file)
@@ -1,3 +1,33 @@
+2019-05-17  Brady Eidson  <beidson@apple.com>
+
+        Avoid races in taking networking assertions for downloads by having both Networking and UIProcess do it.
+        <rdar://problem/50564630> and https://bugs.webkit.org/show_bug.cgi?id=197995
+
+        Reviewed by Chris Dumez.
+
+        There's a fairly indeterminant time gap between when the UIProcess decides a load becomes a download
+        and when the NetworkProcess Download object is created, and therefore the download assertion to be taken.
+
+        The time gap can be long enough for the Networking process to suspend before the download actually starts.
+
+        There's the reverse race when the UIProcess tells a download to stop, as well.
+
+        By having both the UIProcess and NetworkProcess take an assertion on behalf of the NetworkProcess we
+        avoid the race.
+
+        * NetworkProcess/Downloads/DownloadMap.cpp:
+        (WebKit::DownloadMap::add):
+        (WebKit::DownloadMap::remove):
+
+        * NetworkProcess/NetworkResourceLoader.cpp:
+        (WebKit::NetworkResourceLoader::convertToDownload):
+
+        * UIProcess/Downloads/DownloadProxyMap.cpp:
+        (WebKit::DownloadProxyMap::createDownloadProxy):
+        (WebKit::DownloadProxyMap::downloadFinished):
+        (WebKit::DownloadProxyMap::invalidate):
+        * UIProcess/Downloads/DownloadProxyMap.h:
+
 2019-05-17  Keith Rollin  <krollin@apple.com>
 
         Re-enable generate-xcfilelists
index 3299253..e2e1285 100644 (file)
@@ -55,10 +55,13 @@ bool DownloadMap::contains(DownloadID downloadID) const
 
 DownloadMap::DownloadMapType::AddResult DownloadMap::add(DownloadID downloadID, std::unique_ptr<Download>&& download)
 {
+    RELEASE_LOG(Loading, "Adding download %" PRIu64 " to NetworkProcess DownloadMap", downloadID.downloadID());
+
     auto result = m_downloads.add(downloadID, WTFMove(download));
     if (m_downloads.size() == 1) {
         ASSERT(!m_downloadAssertion);
         m_downloadAssertion = std::make_unique<ProcessAssertion>(getpid(), "WebKit downloads"_s, AssertionState::UnboundedNetworking);
+        RELEASE_LOG(ProcessSuspension, "Took 'WebKit downloads' assertion in NetworkProcess");
     }
 
     return result;
@@ -66,10 +69,13 @@ DownloadMap::DownloadMapType::AddResult DownloadMap::add(DownloadID downloadID,
 
 bool DownloadMap::remove(DownloadID downloadID)
 {
+    RELEASE_LOG(Loading, "Removing download %" PRIu64 " from NetworkProcess DownloadMap", downloadID.downloadID());
+
     auto result = m_downloads.remove(downloadID);
     if (m_downloads.isEmpty()) {
         ASSERT(m_downloadAssertion);
         m_downloadAssertion = nullptr;
+        RELEASE_LOG(ProcessSuspension, "Dropped 'WebKit downloads' assertion in NetworkProcess");
     }
     
     return result;
index 4510e92..fe26536 100644 (file)
@@ -322,6 +322,8 @@ void NetworkResourceLoader::cleanup(LoadResult result)
 
 void NetworkResourceLoader::convertToDownload(DownloadID downloadID, const ResourceRequest& request, const ResourceResponse& response)
 {
+    RELEASE_LOG(Loading, "Converting NetworkResourceLoader %p to download %" PRIu64 " (pageID = %" PRIu64 ", frameID = %" PRIu64 ", resourceID = %" PRIu64 ")", this, downloadID.downloadID(), m_parameters.webPageID, m_parameters.webFrameID, m_parameters.identifier);
+    
     // This can happen if the resource came from the disk cache.
     if (!m_networkLoad) {
         m_connection->networkProcess().downloadManager().startDownload(m_parameters.sessionID, downloadID, request);
index f399c5a..63a49b5 100644 (file)
@@ -84,9 +84,17 @@ DownloadProxy& DownloadProxyMap::createDownloadProxy(WebProcessPool& processPool
     auto downloadProxy = DownloadProxy::create(*this, processPool, resourceRequest);
     m_downloads.set(downloadProxy->downloadID(), downloadProxy.copyRef());
 
+    RELEASE_LOG(Loading, "Adding download %" PRIu64 " to UIProcess DownloadProxyMap", downloadProxy->downloadID().downloadID());
+
     if (m_downloads.size() == 1 && m_shouldTakeAssertion) {
-        ASSERT(!m_downloadAssertion);
-        m_downloadAssertion = std::make_unique<ProcessAssertion>(getCurrentProcessID(), "WebKit downloads"_s, AssertionState::UnboundedNetworking);
+        ASSERT(!m_downloadUIAssertion);
+        m_downloadUIAssertion = std::make_unique<ProcessAssertion>(getCurrentProcessID(), "WebKit downloads"_s, AssertionState::UnboundedNetworking);
+
+        ASSERT(!m_downloadNetworkingAssertion);
+        RELEASE_ASSERT(m_process);
+        m_downloadNetworkingAssertion = std::make_unique<ProcessAssertion>(m_process->processIdentifier(), "WebKit downloads"_s, AssertionState::UnboundedNetworking);
+
+        RELEASE_LOG(ProcessSuspension, "UIProcess took 'WebKit downloads' assertions for UIProcess and NetworkProcess");
     }
 
     m_process->addMessageReceiver(Messages::DownloadProxy::messageReceiverName(), downloadProxy->downloadID().downloadID(), downloadProxy.get());
@@ -98,6 +106,8 @@ void DownloadProxyMap::downloadFinished(DownloadProxy& downloadProxy)
 {
     auto downloadID = downloadProxy.downloadID();
 
+    RELEASE_LOG(Loading, "Removing download %" PRIu64 " from UIProcess DownloadProxyMap", downloadID.downloadID());
+
     // The DownloadProxy may be holding the last reference to the process pool.
     auto protectedProcessPool = makeRefPtr(m_process->processPool());
 
@@ -108,8 +118,11 @@ void DownloadProxyMap::downloadFinished(DownloadProxy& downloadProxy)
     m_downloads.remove(downloadID);
 
     if (m_downloads.isEmpty() && m_shouldTakeAssertion) {
-        ASSERT(m_downloadAssertion);
-        m_downloadAssertion = nullptr;
+        ASSERT(m_downloadUIAssertion);
+        ASSERT(m_downloadNetworkingAssertion);
+        m_downloadUIAssertion = nullptr;
+        m_downloadNetworkingAssertion = nullptr;
+        RELEASE_LOG(ProcessSuspension, "UIProcess released 'WebKit downloads' assertions for UIProcess and NetworkProcess");
     }
 }
 
@@ -123,7 +136,10 @@ void DownloadProxyMap::invalidate()
     }
 
     m_downloads.clear();
-    m_downloadAssertion = nullptr;
+    m_downloadUIAssertion = nullptr;
+    m_downloadNetworkingAssertion = nullptr;
+    RELEASE_LOG(ProcessSuspension, "UIProcess DownloadProxyMap invalidated - Released 'WebKit downloads' assertions for UIProcess and NetworkProcess");
+
     m_process = nullptr;
 }
 
index be38ccb..854dcc7 100644 (file)
@@ -72,7 +72,9 @@ private:
     HashMap<DownloadID, RefPtr<DownloadProxy>> m_downloads;
 
     bool m_shouldTakeAssertion { false };
-    std::unique_ptr<ProcessAssertion> m_downloadAssertion;
+    std::unique_ptr<ProcessAssertion> m_downloadUIAssertion;
+    std::unique_ptr<ProcessAssertion> m_downloadNetworkingAssertion;
+
 #if PLATFORM(IOS_FAMILY)
     RetainPtr<id> m_backgroundObserver;
     RetainPtr<id> m_foregroundObserver;