<!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>[245555] branches/safari-608.1.24.20-branch/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/245555">245555</a></dd>
<dt>Author</dt> <dd>kocsen_chung@apple.com</dd>
<dt>Date</dt> <dd>2019-05-20 20:21:24 -0700 (Mon, 20 May 2019)</dd>
</dl>

<h3>Log Message</h3>
<pre>Cherry-pick <a href="http://trac.webkit.org/projects/webkit/changeset/245480">r245480</a>. rdar://problem/50564630

    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:

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

<h3>Modified Paths</h3>
<ul>
<li><a href="#branchessafari60812420branchSourceWebKitChangeLog">branches/safari-608.1.24.20-branch/Source/WebKit/ChangeLog</a></li>
<li><a href="#branchessafari60812420branchSourceWebKitNetworkProcessDownloadsDownloadMapcpp">branches/safari-608.1.24.20-branch/Source/WebKit/NetworkProcess/Downloads/DownloadMap.cpp</a></li>
<li><a href="#branchessafari60812420branchSourceWebKitNetworkProcessNetworkResourceLoadercpp">branches/safari-608.1.24.20-branch/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp</a></li>
<li><a href="#branchessafari60812420branchSourceWebKitUIProcessDownloadsDownloadProxyMapcpp">branches/safari-608.1.24.20-branch/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp</a></li>
<li><a href="#branchessafari60812420branchSourceWebKitUIProcessDownloadsDownloadProxyMaph">branches/safari-608.1.24.20-branch/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.h</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="branchessafari60812420branchSourceWebKitChangeLog"></a>
<div class="modfile"><h4>Modified: branches/safari-608.1.24.20-branch/Source/WebKit/ChangeLog (245554 => 245555)</h4>
<pre class="diff"><span>
<span class="info">--- branches/safari-608.1.24.20-branch/Source/WebKit/ChangeLog       2019-05-21 03:21:21 UTC (rev 245554)
+++ branches/safari-608.1.24.20-branch/Source/WebKit/ChangeLog  2019-05-21 03:21:24 UTC (rev 245555)
</span><span class="lines">@@ -1,5 +1,70 @@
</span><span class="cx"> 2019-05-20  Kocsen Chung  <kocsen_chung@apple.com>
</span><span class="cx"> 
</span><ins>+        Cherry-pick r245480. rdar://problem/50564630
+
+    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:
+    
+    
+    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@245480 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+    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-20  Kocsen Chung  <kocsen_chung@apple.com>
+
</ins><span class="cx">         Cherry-pick r245465. rdar://problem/50252398
</span><span class="cx"> 
</span><span class="cx">     [iOS] Respect scrolling="no" on composited frames
</span></span></pre></div>
<a id="branchessafari60812420branchSourceWebKitNetworkProcessDownloadsDownloadMapcpp"></a>
<div class="modfile"><h4>Modified: branches/safari-608.1.24.20-branch/Source/WebKit/NetworkProcess/Downloads/DownloadMap.cpp (245554 => 245555)</h4>
<pre class="diff"><span>
<span class="info">--- branches/safari-608.1.24.20-branch/Source/WebKit/NetworkProcess/Downloads/DownloadMap.cpp        2019-05-21 03:21:21 UTC (rev 245554)
+++ branches/safari-608.1.24.20-branch/Source/WebKit/NetworkProcess/Downloads/DownloadMap.cpp   2019-05-21 03:21:24 UTC (rev 245555)
</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="branchessafari60812420branchSourceWebKitNetworkProcessNetworkResourceLoadercpp"></a>
<div class="modfile"><h4>Modified: branches/safari-608.1.24.20-branch/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp (245554 => 245555)</h4>
<pre class="diff"><span>
<span class="info">--- branches/safari-608.1.24.20-branch/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp        2019-05-21 03:21:21 UTC (rev 245554)
+++ branches/safari-608.1.24.20-branch/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp   2019-05-21 03:21:24 UTC (rev 245555)
</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="branchessafari60812420branchSourceWebKitUIProcessDownloadsDownloadProxyMapcpp"></a>
<div class="modfile"><h4>Modified: branches/safari-608.1.24.20-branch/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp (245554 => 245555)</h4>
<pre class="diff"><span>
<span class="info">--- branches/safari-608.1.24.20-branch/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp        2019-05-21 03:21:21 UTC (rev 245554)
+++ branches/safari-608.1.24.20-branch/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp   2019-05-21 03:21:24 UTC (rev 245555)
</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="branchessafari60812420branchSourceWebKitUIProcessDownloadsDownloadProxyMaph"></a>
<div class="modfile"><h4>Modified: branches/safari-608.1.24.20-branch/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.h (245554 => 245555)</h4>
<pre class="diff"><span>
<span class="info">--- branches/safari-608.1.24.20-branch/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.h  2019-05-21 03:21:21 UTC (rev 245554)
+++ branches/safari-608.1.24.20-branch/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.h     2019-05-21 03:21:24 UTC (rev 245555)
</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>