<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[245480] trunk/Source/WebKit</title>
</head>
<body>

<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt;  }
#msg dl a { font-weight: bold}
#msg dl a:link    { color:#fc3; }
#msg dl a:active  { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff  {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/245480">245480</a></dd>
<dt>Author</dt> <dd>beidson@apple.com</dd>
<dt>Date</dt> <dd>2019-05-17 14:37:07 -0700 (Fri, 17 May 2019)</dd>
</dl>

<h3>Log Message</h3>
<pre>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:</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWebKitChangeLog">trunk/Source/WebKit/ChangeLog</a></li>
<li><a href="#trunkSourceWebKitNetworkProcessDownloadsDownloadMapcpp">trunk/Source/WebKit/NetworkProcess/Downloads/DownloadMap.cpp</a></li>
<li><a href="#trunkSourceWebKitNetworkProcessNetworkResourceLoadercpp">trunk/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp</a></li>
<li><a href="#trunkSourceWebKitUIProcessDownloadsDownloadProxyMapcpp">trunk/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp</a></li>
<li><a href="#trunkSourceWebKitUIProcessDownloadsDownloadProxyMaph">trunk/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.h</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWebKitChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit/ChangeLog (245479 => 245480)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit/ChangeLog    2019-05-17 21:31:06 UTC (rev 245479)
+++ trunk/Source/WebKit/ChangeLog       2019-05-17 21:37:07 UTC (rev 245480)
</span><span class="lines">@@ -1,3 +1,33 @@
</span><ins>+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:
+
</ins><span class="cx"> 2019-05-17  Keith Rollin  <krollin@apple.com>
</span><span class="cx"> 
</span><span class="cx">         Re-enable generate-xcfilelists
</span></span></pre></div>
<a id="trunkSourceWebKitNetworkProcessDownloadsDownloadMapcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit/NetworkProcess/Downloads/DownloadMap.cpp (245479 => 245480)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit/NetworkProcess/Downloads/DownloadMap.cpp     2019-05-17 21:31:06 UTC (rev 245479)
+++ trunk/Source/WebKit/NetworkProcess/Downloads/DownloadMap.cpp        2019-05-17 21:37:07 UTC (rev 245480)
</span><span class="lines">@@ -55,10 +55,13 @@
</span><span class="cx"> 
</span><span class="cx"> DownloadMap::DownloadMapType::AddResult DownloadMap::add(DownloadID downloadID, std::unique_ptr<Download>&& download)
</span><span class="cx"> {
</span><ins>+    RELEASE_LOG(Loading, "Adding download %" PRIu64 " to NetworkProcess DownloadMap", downloadID.downloadID());
+
</ins><span class="cx">     auto result = m_downloads.add(downloadID, WTFMove(download));
</span><span class="cx">     if (m_downloads.size() == 1) {
</span><span class="cx">         ASSERT(!m_downloadAssertion);
</span><span class="cx">         m_downloadAssertion = std::make_unique<ProcessAssertion>(getpid(), "WebKit downloads"_s, AssertionState::UnboundedNetworking);
</span><ins>+        RELEASE_LOG(ProcessSuspension, "Took 'WebKit downloads' assertion in NetworkProcess");
</ins><span class="cx">     }
</span><span class="cx"> 
</span><span class="cx">     return result;
</span><span class="lines">@@ -66,10 +69,13 @@
</span><span class="cx"> 
</span><span class="cx"> bool DownloadMap::remove(DownloadID downloadID)
</span><span class="cx"> {
</span><ins>+    RELEASE_LOG(Loading, "Removing download %" PRIu64 " from NetworkProcess DownloadMap", downloadID.downloadID());
+
</ins><span class="cx">     auto result = m_downloads.remove(downloadID);
</span><span class="cx">     if (m_downloads.isEmpty()) {
</span><span class="cx">         ASSERT(m_downloadAssertion);
</span><span class="cx">         m_downloadAssertion = nullptr;
</span><ins>+        RELEASE_LOG(ProcessSuspension, "Dropped 'WebKit downloads' assertion in NetworkProcess");
</ins><span class="cx">     }
</span><span class="cx">     
</span><span class="cx">     return result;
</span></span></pre></div>
<a id="trunkSourceWebKitNetworkProcessNetworkResourceLoadercpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp (245479 => 245480)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp     2019-05-17 21:31:06 UTC (rev 245479)
+++ trunk/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp        2019-05-17 21:37:07 UTC (rev 245480)
</span><span class="lines">@@ -322,6 +322,8 @@
</span><span class="cx"> 
</span><span class="cx"> void NetworkResourceLoader::convertToDownload(DownloadID downloadID, const ResourceRequest& request, const ResourceResponse& response)
</span><span class="cx"> {
</span><ins>+    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);
+    
</ins><span class="cx">     // This can happen if the resource came from the disk cache.
</span><span class="cx">     if (!m_networkLoad) {
</span><span class="cx">         m_connection->networkProcess().downloadManager().startDownload(m_parameters.sessionID, downloadID, request);
</span></span></pre></div>
<a id="trunkSourceWebKitUIProcessDownloadsDownloadProxyMapcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp (245479 => 245480)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp     2019-05-17 21:31:06 UTC (rev 245479)
+++ trunk/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp        2019-05-17 21:37:07 UTC (rev 245480)
</span><span class="lines">@@ -84,9 +84,17 @@
</span><span class="cx">     auto downloadProxy = DownloadProxy::create(*this, processPool, resourceRequest);
</span><span class="cx">     m_downloads.set(downloadProxy->downloadID(), downloadProxy.copyRef());
</span><span class="cx"> 
</span><ins>+    RELEASE_LOG(Loading, "Adding download %" PRIu64 " to UIProcess DownloadProxyMap", downloadProxy->downloadID().downloadID());
+
</ins><span class="cx">     if (m_downloads.size() == 1 && m_shouldTakeAssertion) {
</span><del>-        ASSERT(!m_downloadAssertion);
-        m_downloadAssertion = std::make_unique<ProcessAssertion>(getCurrentProcessID(), "WebKit downloads"_s, AssertionState::UnboundedNetworking);
</del><ins>+        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");
</ins><span class="cx">     }
</span><span class="cx"> 
</span><span class="cx">     m_process->addMessageReceiver(Messages::DownloadProxy::messageReceiverName(), downloadProxy->downloadID().downloadID(), downloadProxy.get());
</span><span class="lines">@@ -98,6 +106,8 @@
</span><span class="cx"> {
</span><span class="cx">     auto downloadID = downloadProxy.downloadID();
</span><span class="cx"> 
</span><ins>+    RELEASE_LOG(Loading, "Removing download %" PRIu64 " from UIProcess DownloadProxyMap", downloadID.downloadID());
+
</ins><span class="cx">     // The DownloadProxy may be holding the last reference to the process pool.
</span><span class="cx">     auto protectedProcessPool = makeRefPtr(m_process->processPool());
</span><span class="cx"> 
</span><span class="lines">@@ -108,8 +118,11 @@
</span><span class="cx">     m_downloads.remove(downloadID);
</span><span class="cx"> 
</span><span class="cx">     if (m_downloads.isEmpty() && m_shouldTakeAssertion) {
</span><del>-        ASSERT(m_downloadAssertion);
-        m_downloadAssertion = nullptr;
</del><ins>+        ASSERT(m_downloadUIAssertion);
+        ASSERT(m_downloadNetworkingAssertion);
+        m_downloadUIAssertion = nullptr;
+        m_downloadNetworkingAssertion = nullptr;
+        RELEASE_LOG(ProcessSuspension, "UIProcess released 'WebKit downloads' assertions for UIProcess and NetworkProcess");
</ins><span class="cx">     }
</span><span class="cx"> }
</span><span class="cx"> 
</span><span class="lines">@@ -123,7 +136,10 @@
</span><span class="cx">     }
</span><span class="cx"> 
</span><span class="cx">     m_downloads.clear();
</span><del>-    m_downloadAssertion = nullptr;
</del><ins>+    m_downloadUIAssertion = nullptr;
+    m_downloadNetworkingAssertion = nullptr;
+    RELEASE_LOG(ProcessSuspension, "UIProcess DownloadProxyMap invalidated - Released 'WebKit downloads' assertions for UIProcess and NetworkProcess");
+
</ins><span class="cx">     m_process = nullptr;
</span><span class="cx"> }
</span><span class="cx"> 
</span></span></pre></div>
<a id="trunkSourceWebKitUIProcessDownloadsDownloadProxyMaph"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.h (245479 => 245480)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.h       2019-05-17 21:31:06 UTC (rev 245479)
+++ trunk/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.h  2019-05-17 21:37:07 UTC (rev 245480)
</span><span class="lines">@@ -72,7 +72,9 @@
</span><span class="cx">     HashMap<DownloadID, RefPtr<DownloadProxy>> m_downloads;
</span><span class="cx"> 
</span><span class="cx">     bool m_shouldTakeAssertion { false };
</span><del>-    std::unique_ptr<ProcessAssertion> m_downloadAssertion;
</del><ins>+    std::unique_ptr<ProcessAssertion> m_downloadUIAssertion;
+    std::unique_ptr<ProcessAssertion> m_downloadNetworkingAssertion;
+
</ins><span class="cx"> #if PLATFORM(IOS_FAMILY)
</span><span class="cx">     RetainPtr<id> m_backgroundObserver;
</span><span class="cx">     RetainPtr<id> m_foregroundObserver;
</span></span></pre>
</div>
</div>

</body>
</html>