Attachment 370146 Details for Bug 197995 – Patch

[patch] Patch

bug-197995-20190517135013.patch (text/plain), 8.09 KB, created by Brady Eidson on 2019-05-17 13:50:13 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Brady Eidson

Created: 2019-05-17 13:50:13 PDT

Size: 8.09 KB

patch

obsolete

>Subversion Revision: 245474
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 695de904e006d293fa9fc00cc8e0afb103d15e40..1f83202d21d3c929962eb14940059b82dbfe4514 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -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
>diff --git a/Source/WebKit/NetworkProcess/Downloads/DownloadMap.cpp b/Source/WebKit/NetworkProcess/Downloads/DownloadMap.cpp
>index 3299253048f42ce396c35c3da05da50a1531b92a..e2e12854809fb968ee802b160a1007007fe54011 100644
>--- a/Source/WebKit/NetworkProcess/Downloads/DownloadMap.cpp
>+++ b/Source/WebKit/NetworkProcess/Downloads/DownloadMap.cpp
>@@ -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;
>diff --git a/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp b/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
>index 4510e9268a22c205c8c6b035a7b0fcfe478e06e2..fe26536f21c3ee9a701898ca05932d55d15d34b0 100644
>--- a/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp
>@@ -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);
>diff --git a/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp b/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp
>index f399c5a8e98acb275e3edbf4671d3254ed01dbc7..63a49b567db8da53f5799a50344f31251f5e9684 100644
>--- a/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp
>+++ b/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp
>@@ -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;
> }
> 
>diff --git a/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.h b/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.h
>index be38ccb82fb55939f442e812a8f530d3400f5057..854dcc790a0892776f38545c5df95002213fbfc2 100644
>--- a/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.h
>+++ b/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.h
>@@ -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;

Actions: View | Formatted Diff | Diff

Attachments on bug 197995: 370138 | 370140 | 370146