- Revision
- 206987
- Author
- carlo...@webkit.org
- Date
- 2016-10-10 00:28:48 -0700 (Mon, 10 Oct 2016)
Log Message
Network Session: PendingDownload is always nullptr in DownloadManager::dataTaskBecameDownloadTask
https://bugs.webkit.org/show_bug.cgi?id=163006
Reviewed by Alex Christensen.
In DownloadManager::dataTaskBecameDownloadTask() we are supposed to have either a pending download, or a network
data task depending on whether the download was started by startDownload() or convertTaskToDownload. However, in
both cases we do have a data task and never a pending download. In the case of startDownload() the pending
download is removed from m_pendingDownloads in willDecidePendingDownloadDestination(). The task is always
added to m_downloadsWaitingForDestination in willDecidePendingDownloadDestination() and to
m_downloadsAfterDestinationDecided in continueDecidePendingDownloadDestination() in both cases.
* NetworkProcess/Downloads/DownloadManager.cpp:
(WebKit::DownloadManager::dataTaskBecameDownloadTask): Add an ASSERT to ensure we don't have a pending download
at this point and that the download is not already in the map. Remove the download from the
m_downloadsAfterDestinationDecided map, but don't check its result because it might not bein that map if
dataTaskBecameDownloadTask is called synchronously from the didReceiveResponse completion handler.
(WebKit::DownloadManager::willDecidePendingDownloadDestination): Do not take the pending download here, wait
until didReceiveResponse completion handler is called.
(WebKit::DownloadManager::continueDecidePendingDownloadDestination): Take the pending download here ensuring
it's alive while the didReceiveResponse completion handler is called. Also remove invalid early return when
either networkDataTask or completionHandler are nullptr because we are using both unconditionally, we should
return early if both are nullptr to avoid crashes. However, we are checking that the download is in the
m_downloadsWaitingForDestination and in that case we should always have both, so better add asserts there. If
the download is already in the map after the completion handler it means that dataTaskBecameDownloadTask() has
alrady been called, so we can just return in that case.
* NetworkProcess/Downloads/DownloadManager.h:
* NetworkProcess/cocoa/NetworkSessionCocoa.mm:
(-[WKNetworkSessionDelegate URLSession:dataTask:didBecomeDownloadTask:]): Protect the NetworkDataTask that can
be deleted by dataTaskBecameDownloadTask().
Modified Paths
Diff
Modified: trunk/Source/WebKit2/ChangeLog (206986 => 206987)
--- trunk/Source/WebKit2/ChangeLog 2016-10-10 07:13:32 UTC (rev 206986)
+++ trunk/Source/WebKit2/ChangeLog 2016-10-10 07:28:48 UTC (rev 206987)
@@ -1,3 +1,36 @@
+2016-10-10 Carlos Garcia Campos <cgar...@igalia.com>
+
+ Network Session: PendingDownload is always nullptr in DownloadManager::dataTaskBecameDownloadTask
+ https://bugs.webkit.org/show_bug.cgi?id=163006
+
+ Reviewed by Alex Christensen.
+
+ In DownloadManager::dataTaskBecameDownloadTask() we are supposed to have either a pending download, or a network
+ data task depending on whether the download was started by startDownload() or convertTaskToDownload. However, in
+ both cases we do have a data task and never a pending download. In the case of startDownload() the pending
+ download is removed from m_pendingDownloads in willDecidePendingDownloadDestination(). The task is always
+ added to m_downloadsWaitingForDestination in willDecidePendingDownloadDestination() and to
+ m_downloadsAfterDestinationDecided in continueDecidePendingDownloadDestination() in both cases.
+
+ * NetworkProcess/Downloads/DownloadManager.cpp:
+ (WebKit::DownloadManager::dataTaskBecameDownloadTask): Add an ASSERT to ensure we don't have a pending download
+ at this point and that the download is not already in the map. Remove the download from the
+ m_downloadsAfterDestinationDecided map, but don't check its result because it might not bein that map if
+ dataTaskBecameDownloadTask is called synchronously from the didReceiveResponse completion handler.
+ (WebKit::DownloadManager::willDecidePendingDownloadDestination): Do not take the pending download here, wait
+ until didReceiveResponse completion handler is called.
+ (WebKit::DownloadManager::continueDecidePendingDownloadDestination): Take the pending download here ensuring
+ it's alive while the didReceiveResponse completion handler is called. Also remove invalid early return when
+ either networkDataTask or completionHandler are nullptr because we are using both unconditionally, we should
+ return early if both are nullptr to avoid crashes. However, we are checking that the download is in the
+ m_downloadsWaitingForDestination and in that case we should always have both, so better add asserts there. If
+ the download is already in the map after the completion handler it means that dataTaskBecameDownloadTask() has
+ alrady been called, so we can just return in that case.
+ * NetworkProcess/Downloads/DownloadManager.h:
+ * NetworkProcess/cocoa/NetworkSessionCocoa.mm:
+ (-[WKNetworkSessionDelegate URLSession:dataTask:didBecomeDownloadTask:]): Protect the NetworkDataTask that can
+ be deleted by dataTaskBecameDownloadTask().
+
2016-10-10 Tim Horton <timothy_hor...@apple.com>
Share more code between iOS and macOS ViewGestureController
Modified: trunk/Source/WebKit2/NetworkProcess/Downloads/DownloadManager.cpp (206986 => 206987)
--- trunk/Source/WebKit2/NetworkProcess/Downloads/DownloadManager.cpp 2016-10-10 07:13:32 UTC (rev 206986)
+++ trunk/Source/WebKit2/NetworkProcess/Downloads/DownloadManager.cpp 2016-10-10 07:28:48 UTC (rev 206987)
@@ -67,18 +67,12 @@
}
#if USE(NETWORK_SESSION)
-std::pair<RefPtr<NetworkDataTask>, std::unique_ptr<PendingDownload>> DownloadManager::dataTaskBecameDownloadTask(DownloadID downloadID, std::unique_ptr<Download>&& download)
+void DownloadManager::dataTaskBecameDownloadTask(DownloadID downloadID, std::unique_ptr<Download>&& download)
{
- // This is needed for downloads started with startDownload, otherwise it will return nullptr.
- auto pendingDownload = m_pendingDownloads.take(downloadID);
-
- // This is needed for downloads started with convertTaskToDownload, otherwise it will return nullptr.
- auto downloadAfterLocationDecided = m_downloadsAfterDestinationDecided.take(downloadID);
-
- ASSERT(!!pendingDownload != !!downloadAfterLocationDecided);
-
+ ASSERT(!m_pendingDownloads.contains(downloadID));
+ ASSERT(!m_downloads.contains(downloadID));
+ m_downloadsAfterDestinationDecided.remove(downloadID);
m_downloads.add(downloadID, WTFMove(download));
- return std::make_pair(WTFMove(downloadAfterLocationDecided), WTFMove(pendingDownload));
}
#if USE(PROTECTION_SPACE_AUTH_CALLBACK)
@@ -102,8 +96,6 @@
void DownloadManager::willDecidePendingDownloadDestination(NetworkDataTask& networkDataTask, ResponseCompletionHandler&& completionHandler)
{
auto downloadID = networkDataTask.pendingDownloadID();
- auto pendingDownload = m_pendingDownloads.take(downloadID);
- ASSERT(networkDataTask.pendingDownload() == pendingDownload.get());
auto addResult = m_downloadsWaitingForDestination.set(downloadID, std::make_pair<RefPtr<NetworkDataTask>, ResponseCompletionHandler>(&networkDataTask, WTFMove(completionHandler)));
ASSERT_UNUSED(addResult, addResult.isNewEntry);
}
@@ -122,15 +114,22 @@
{
#if USE(NETWORK_SESSION)
if (m_downloadsWaitingForDestination.contains(downloadID)) {
+ auto pendingDownload = m_pendingDownloads.take(downloadID);
auto pair = m_downloadsWaitingForDestination.take(downloadID);
auto networkDataTask = WTFMove(pair.first);
auto completionHandler = WTFMove(pair.second);
- if (!networkDataTask || !completionHandler)
- return;
+ ASSERT(networkDataTask);
+ ASSERT(completionHandler);
+ ASSERT(!pendingDownload || pendingDownload.get() == networkDataTask->pendingDownload());
networkDataTask->setPendingDownloadLocation(destination, sandboxExtensionHandle, allowOverwrite);
completionHandler(PolicyDownload);
+ if (m_downloads.contains(downloadID)) {
+ // The completion handler already called dataTaskBecameDownloadTask().
+ return;
+ }
+
ASSERT(!m_downloadsAfterDestinationDecided.contains(downloadID));
m_downloadsAfterDestinationDecided.set(downloadID, networkDataTask);
return;
Modified: trunk/Source/WebKit2/NetworkProcess/Downloads/DownloadManager.h (206986 => 206987)
--- trunk/Source/WebKit2/NetworkProcess/Downloads/DownloadManager.h 2016-10-10 07:13:32 UTC (rev 206986)
+++ trunk/Source/WebKit2/NetworkProcess/Downloads/DownloadManager.h 2016-10-10 07:28:48 UTC (rev 206987)
@@ -74,7 +74,7 @@
void startDownload(WebCore::SessionID, DownloadID, const WebCore::ResourceRequest&, const String& suggestedName = { });
#if USE(NETWORK_SESSION)
- std::pair<RefPtr<NetworkDataTask>, std::unique_ptr<PendingDownload>> dataTaskBecameDownloadTask(DownloadID, std::unique_ptr<Download>&&);
+ void dataTaskBecameDownloadTask(DownloadID, std::unique_ptr<Download>&&);
#if USE(PROTECTION_SPACE_AUTH_CALLBACK)
void continueCanAuthenticateAgainstProtectionSpace(DownloadID, bool canAuthenticate);
#endif
Modified: trunk/Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm (206986 => 206987)
--- trunk/Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm 2016-10-10 07:13:32 UTC (rev 206986)
+++ trunk/Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm 2016-10-10 07:28:48 UTC (rev 206987)
@@ -295,6 +295,7 @@
{
auto storedCredentials = _withCredentials ? WebCore::StoredCredentials::AllowStoredCredentials : WebCore::StoredCredentials::DoNotAllowStoredCredentials;
if (auto* networkDataTask = _session->dataTaskForIdentifier([dataTask taskIdentifier], storedCredentials)) {
+ Ref<NetworkDataTask> protectedNetworkDataTask(*networkDataTask);
auto downloadID = networkDataTask->pendingDownloadID();
auto& downloadManager = WebKit::NetworkProcess::singleton().downloadManager();
auto download = std::make_unique<WebKit::Download>(downloadManager, downloadID, downloadTask, _session->sessionID(), networkDataTask->suggestedFilename());
@@ -301,8 +302,7 @@
networkDataTask->transferSandboxExtensionToDownload(*download);
ASSERT(WebCore::fileExists(networkDataTask->pendingDownloadLocation()));
download->didCreateDestination(networkDataTask->pendingDownloadLocation());
- auto pendingDownload = downloadManager.dataTaskBecameDownloadTask(downloadID, WTFMove(download));
-
+ downloadManager.dataTaskBecameDownloadTask(downloadID, WTFMove(download));
networkDataTask->didBecomeDownload();
_session->addDownloadID([downloadTask taskIdentifier], downloadID);
