- Revision
- 245555
- Author
- [email protected]
- Date
- 2019-05-20 20:21:24 -0700 (Mon, 20 May 2019)
Log Message
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
Modified Paths
Diff
Modified: branches/safari-608.1.24.20-branch/Source/WebKit/ChangeLog (245554 => 245555)
--- 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)
@@ -1,5 +1,70 @@
2019-05-20 Kocsen Chung <[email protected]>
+ 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 <[email protected]>
+
+ 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 <[email protected]>
+
Cherry-pick r245465. rdar://problem/50252398
[iOS] Respect scrolling="no" on composited frames
Modified: branches/safari-608.1.24.20-branch/Source/WebKit/NetworkProcess/Downloads/DownloadMap.cpp (245554 => 245555)
--- 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)
@@ -55,10 +55,13 @@
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 @@
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;
Modified: branches/safari-608.1.24.20-branch/Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp (245554 => 245555)
--- 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)
@@ -322,6 +322,8 @@
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);
Modified: branches/safari-608.1.24.20-branch/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp (245554 => 245555)
--- 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)
@@ -84,9 +84,17 @@
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 @@
{
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 @@
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 @@
}
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;
}
Modified: branches/safari-608.1.24.20-branch/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.h (245554 => 245555)
--- 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)
@@ -72,7 +72,9 @@
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;
