NetworkSession: Network process crash when converting main resource to download
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 10 Nov 2016 04:49:09 +0000 (04:49 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 10 Nov 2016 04:49:09 +0000 (04:49 +0000)
commit69ba10740646ad41775dda778f6f479632800db9
treea23a8590efa4bb39adc4b1d8ce06023bf82fdaf7
parent2a5ece664d0906b718c24effa3c4758361a38850
NetworkSession: Network process crash when converting main resource to download
https://bugs.webkit.org/show_bug.cgi?id=164220

Patch by Carlos Garcia Campos <cgarcia@igalia.com> on 2016-11-09
Reviewed by Alex Christensen.

Source/WebKit2:

Right after the main resource load is converted to a download, the web process deletes the ResourceLoader which
sends the RemoveLoadIdentifier to the network process and the load is aborted. Sometimes it happens that
NetworkResourceLoader::abort() is called while the NetworkLoad is still deciding the destination of the
download. In such case, NetworkResourceLoader::didConvertToDownload() has already been called, but not
NetworkResourceLoader::didBecomeDownload(). In NetworkResourceLoader::abort() we already handle the case of
having a NetworkLoad after NetworkResourceLoader::didConvertToDownload() has been called, to avoid canceling the
load in such case, however cleanup() is always called unconditionally and the NetworkLoad is deleted before
NetworkResourceLoader::didBecomeDownload() is called. When the NetworkLoad is destroyed the NetworkDataTask
client becomes nullptr, leaving it in a state where both the client is nullptr and the download hasn't been
created yet. That's not expected to happen and when the response completion handler is called in the
NetworkDataTask it tries to use either the client or the download and it crashes.
We need to cleanup and destroy the ResourceLoader as soon as it becomes a download, because that's the expected
behavior, but at the same time we need to keep the NetworkLoad alive until the NetworkDataTask finishes to become
a download. This patch creates a PendingDownload to take the ownership of the NetworkLoad, so that
ResourceLoader can be cleaned up and destroyed. The DownloadManager now will handle the PendingDownload as if it
was created by startDownload(), but ensuring it's deleted right before the final Download object is added to the
downloads map. That way NetworkDataTask will always have a valid client until the final Download is created,
first the ResourceLoader and then the PendingDownload. Since the DownloadManager is the owner of the
PendingDownload we no longer need the didBecomeDownload() callback to delete the NetworkLoad, because the
NetworkLoad will always be owned by the PendingDownload now that will be deleted by the DownloadManager.

* NetworkProcess/Downloads/DownloadManager.cpp:
(WebKit::DownloadManager::dataTaskBecameDownloadTask): Change the ASSERT because at this point we should always
have a PendingDownload, and then remove it from the map here before adding the final Download to the map.
(WebKit::DownloadManager::convertNetworkLoadToDownload): This replaces convertHandleToDownload, but it also now
used for the NetworkSession. It creates a PendingDownload for the given NetworkLoad.
(WebKit::DownloadManager::continueDecidePendingDownloadDestination): Do not take the PendingDownload from the
map here, just check it's present, because it will be removed from dataTaskBecameDownloadTask().
(WebKit::DownloadManager::convertHandleToDownload): Deleted.
* NetworkProcess/Downloads/DownloadManager.h:
* NetworkProcess/Downloads/PendingDownload.cpp:
(WebKit::PendingDownload::PendingDownload): Add a constructor that receives a NetworkLoad.
* NetworkProcess/Downloads/PendingDownload.h:
* NetworkProcess/NetworkConnectionToWebProcess.cpp:
(WebKit::NetworkConnectionToWebProcess::convertMainResourceLoadToDownload): Just ask the ResourceLoader to be
converted to a download.
* NetworkProcess/NetworkDataTask.h:
* NetworkProcess/NetworkDataTaskBlob.cpp:
(WebKit::NetworkDataTaskBlob::download): Do not call didBecomeDownload() and add an assert to ensure the client
has already been removed right after the final Download object is created.
* NetworkProcess/NetworkLoad.cpp:
(WebKit::NetworkLoad::NetworkLoad): Use a pointer for the client instead of a reference because now we need to
change the client when the load is converted to a download. We don't need to null check the client in any case
because the member is only updated internally and always from a passed reference.
(WebKit::NetworkLoad::sharedDidReceiveResponse):
(WebKit::NetworkLoad::sharedWillSendRedirectedRequest):
(WebKit::NetworkLoad::convertTaskToDownload): This now receives a PendingDownload. It updates the client and no
longer sends DidStart, because the PendingDownload sends it now.
(WebKit::NetworkLoad::didReceiveChallenge):
(WebKit::NetworkLoad::didReceiveData):
(WebKit::NetworkLoad::didCompleteWithError):
(WebKit::NetworkLoad::didSendData):
(WebKit::NetworkLoad::wasBlocked):
(WebKit::NetworkLoad::cannotShowURL):
(WebKit::NetworkLoad::didReceiveBuffer):
(WebKit::NetworkLoad::didFinishLoading):
(WebKit::NetworkLoad::didFail):
(WebKit::NetworkLoad::canAuthenticateAgainstProtectionSpaceAsync):
(WebKit::NetworkLoad::didBecomeDownload): Deleted.
* NetworkProcess/NetworkLoad.h:
* NetworkProcess/NetworkLoadClient.h: Remove didBecomeDownload().
* NetworkProcess/NetworkResourceLoader.cpp:
(WebKit::NetworkResourceLoader::convertToDownload): This replaces didBecomeDownload() and
didConvertToDownload(). It transfers the NetworkLoad to the DownloadManager.
(WebKit::NetworkResourceLoader::abort): We don't need to check if the load was converted to a download here,
because m_networkLoad will always be null here in such case.
(WebKit::NetworkResourceLoader::didBecomeDownload): Deleted
(WebKit::NetworkResourceLoader::didConvertToDownload): Deleted
* NetworkProcess/NetworkResourceLoader.h:
* NetworkProcess/PingLoad.h: Remove didBecomeDownload().
* NetworkProcess/cache/NetworkCacheSpeculativeLoad.h: Ditto.
* NetworkProcess/cocoa/NetworkDataTaskCocoa.h: Ditto.
* NetworkProcess/cocoa/NetworkDataTaskCocoa.mm: Ditto.
* NetworkProcess/cocoa/NetworkSessionCocoa.mm:
(-[WKNetworkSessionDelegate URLSession:dataTask:didBecomeDownloadTask:]): Do not call didBecomeDownload().
* NetworkProcess/soup/NetworkDataTaskSoup.cpp:
(WebKit::NetworkDataTaskSoup::download): Do not call didBecomeDownload() and add an assert to ensure the client
has already been removed right after the final Download object is created.

Tools:

Split /webkit2/Downloads/policy-decision-download in two, one to test the full load when main resource is
converted to a download and another one to test the cancellation as the test was doing before. When doing the
full load, delay a bit the decide destination to ensure the load is aborted before the data task has became a
download.

* TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:
(testPolicyResponseDownload):
(testPolicyResponseDownloadCancel):
(beforeAll):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@208521 268f45cc-cd09-0410-ab3c-d52691b4dbfc
21 files changed:
Source/WebKit2/ChangeLog
Source/WebKit2/NetworkProcess/Downloads/DownloadManager.cpp
Source/WebKit2/NetworkProcess/Downloads/DownloadManager.h
Source/WebKit2/NetworkProcess/Downloads/PendingDownload.cpp
Source/WebKit2/NetworkProcess/Downloads/PendingDownload.h
Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp
Source/WebKit2/NetworkProcess/NetworkDataTask.h
Source/WebKit2/NetworkProcess/NetworkDataTaskBlob.cpp
Source/WebKit2/NetworkProcess/NetworkLoad.cpp
Source/WebKit2/NetworkProcess/NetworkLoad.h
Source/WebKit2/NetworkProcess/NetworkLoadClient.h
Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp
Source/WebKit2/NetworkProcess/NetworkResourceLoader.h
Source/WebKit2/NetworkProcess/PingLoad.h
Source/WebKit2/NetworkProcess/cache/NetworkCacheSpeculativeLoad.h
Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.h
Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm
Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm
Source/WebKit2/NetworkProcess/soup/NetworkDataTaskSoup.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp