- Revision
- 206367
- Author
- [email protected]
- Date
- 2016-09-25 22:35:55 -0700 (Sun, 25 Sep 2016)
Log Message
Regression(r206356): Caused crashes for !NETWORK_SESSION code path
https://bugs.webkit.org/show_bug.cgi?id=162541
Reviewed by Alexey Proskuryakov.
After r206356, if Download is downloading a Blob instead of doing a
network load, then m_resourceHandle / m_downloadClient are used to do
the download. This broke some assumptions on Mac because the
NETWORK_SESSION code path assumed it was relying on m_download and the
!NETWORK_SESSION code path assumed it was using m_nsURLDownload /
m_delegate.
This patch refactors the code so that:
- The Download destructor now takes care of invalidating
m_resourceHandle and m_downloadClient before calling
platformInvalidate(). For the SOUP code path,
platformInvalidate() no longer needs to do anything
because SOUP uses m_resourceHandle / m_downloadClient
only. For the Mac !NETWORK_SESSION code path, we keep
invalidating m_nsURLDownload / m_delegate but we no
longer assume that those are initialized (given that
they are not when downloading a blob). Other
platforms do nothing in platformInvalidate() at the
moment.
- Download::cancel() now takes care of cancelling the
download if we're downloading a blob. Otherwise, we
call the platform specific cancelNetworkLoad(). This
is the same pattern that is used for start() /
startNetworkLoad().
* NetworkProcess/Downloads/BlobDownloadClient.cpp:
(WebKit::BlobDownloadClient::didCancel):
* NetworkProcess/Downloads/BlobDownloadClient.h:
* NetworkProcess/Downloads/Download.cpp:
(WebKit::Download::~Download):
(WebKit::Download::cancel):
* NetworkProcess/Downloads/Download.h:
* NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:
(WebKit::Download::cancelNetworkLoad):
(WebKit::Download::cancel): Deleted.
* NetworkProcess/Downloads/ios/DownloadIOS.mm:
(WebKit::Download::cancelNetworkLoad):
(WebKit::Download::cancel): Deleted.
* NetworkProcess/Downloads/mac/DownloadMac.mm:
(WebKit::Download::cancelNetworkLoad):
(WebKit::Download::platformInvalidate):
(WebKit::Download::cancel): Deleted.
* NetworkProcess/Downloads/soup/DownloadSoup.cpp:
(WebKit::Download::cancelNetworkLoad):
(WebKit::Download::platformInvalidate):
(WebKit::Download::cancel): Deleted.
Modified Paths
Diff
Modified: trunk/Source/WebKit2/ChangeLog (206366 => 206367)
--- trunk/Source/WebKit2/ChangeLog 2016-09-26 03:57:40 UTC (rev 206366)
+++ trunk/Source/WebKit2/ChangeLog 2016-09-26 05:35:55 UTC (rev 206367)
@@ -1,3 +1,57 @@
+2016-09-25 Chris Dumez <[email protected]>
+
+ Regression(r206356): Caused crashes for !NETWORK_SESSION code path
+ https://bugs.webkit.org/show_bug.cgi?id=162541
+
+ Reviewed by Alexey Proskuryakov.
+
+ After r206356, if Download is downloading a Blob instead of doing a
+ network load, then m_resourceHandle / m_downloadClient are used to do
+ the download. This broke some assumptions on Mac because the
+ NETWORK_SESSION code path assumed it was relying on m_download and the
+ !NETWORK_SESSION code path assumed it was using m_nsURLDownload /
+ m_delegate.
+
+ This patch refactors the code so that:
+ - The Download destructor now takes care of invalidating
+ m_resourceHandle and m_downloadClient before calling
+ platformInvalidate(). For the SOUP code path,
+ platformInvalidate() no longer needs to do anything
+ because SOUP uses m_resourceHandle / m_downloadClient
+ only. For the Mac !NETWORK_SESSION code path, we keep
+ invalidating m_nsURLDownload / m_delegate but we no
+ longer assume that those are initialized (given that
+ they are not when downloading a blob). Other
+ platforms do nothing in platformInvalidate() at the
+ moment.
+ - Download::cancel() now takes care of cancelling the
+ download if we're downloading a blob. Otherwise, we
+ call the platform specific cancelNetworkLoad(). This
+ is the same pattern that is used for start() /
+ startNetworkLoad().
+
+ * NetworkProcess/Downloads/BlobDownloadClient.cpp:
+ (WebKit::BlobDownloadClient::didCancel):
+ * NetworkProcess/Downloads/BlobDownloadClient.h:
+ * NetworkProcess/Downloads/Download.cpp:
+ (WebKit::Download::~Download):
+ (WebKit::Download::cancel):
+ * NetworkProcess/Downloads/Download.h:
+ * NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:
+ (WebKit::Download::cancelNetworkLoad):
+ (WebKit::Download::cancel): Deleted.
+ * NetworkProcess/Downloads/ios/DownloadIOS.mm:
+ (WebKit::Download::cancelNetworkLoad):
+ (WebKit::Download::cancel): Deleted.
+ * NetworkProcess/Downloads/mac/DownloadMac.mm:
+ (WebKit::Download::cancelNetworkLoad):
+ (WebKit::Download::platformInvalidate):
+ (WebKit::Download::cancel): Deleted.
+ * NetworkProcess/Downloads/soup/DownloadSoup.cpp:
+ (WebKit::Download::cancelNetworkLoad):
+ (WebKit::Download::platformInvalidate):
+ (WebKit::Download::cancel): Deleted.
+
2016-09-24 Chris Dumez <[email protected]>
[WK2] Enable support for 'download' attribute on anchor elements
Modified: trunk/Source/WebKit2/NetworkProcess/Downloads/BlobDownloadClient.cpp (206366 => 206367)
--- trunk/Source/WebKit2/NetworkProcess/Downloads/BlobDownloadClient.cpp 2016-09-26 03:57:40 UTC (rev 206366)
+++ trunk/Source/WebKit2/NetworkProcess/Downloads/BlobDownloadClient.cpp 2016-09-26 05:35:55 UTC (rev 206367)
@@ -76,6 +76,15 @@
m_download.didFinish();
}
+void BlobDownloadClient::didCancel()
+{
+ closeFile(m_destinationFile);
+ if (!m_destinationPath.isEmpty())
+ deleteFile(m_destinationPath);
+
+ m_download.didCancel(IPC::DataReference());
+}
+
void BlobDownloadClient::didFail(ResourceHandle*, const ResourceError& error)
{
closeFile(m_destinationFile);
Modified: trunk/Source/WebKit2/NetworkProcess/Downloads/BlobDownloadClient.h (206366 => 206367)
--- trunk/Source/WebKit2/NetworkProcess/Downloads/BlobDownloadClient.h 2016-09-26 03:57:40 UTC (rev 206366)
+++ trunk/Source/WebKit2/NetworkProcess/Downloads/BlobDownloadClient.h 2016-09-26 05:35:55 UTC (rev 206367)
@@ -38,6 +38,8 @@
public:
explicit BlobDownloadClient(Download&);
+ void didCancel();
+
private:
// ResourceHandleClient
void didReceiveResponse(WebCore::ResourceHandle*, WebCore::ResourceResponse&&) final;
Modified: trunk/Source/WebKit2/NetworkProcess/Downloads/Download.cpp (206366 => 206367)
--- trunk/Source/WebKit2/NetworkProcess/Downloads/Download.cpp 2016-09-26 03:57:40 UTC (rev 206366)
+++ trunk/Source/WebKit2/NetworkProcess/Downloads/Download.cpp 2016-09-26 05:35:55 UTC (rev 206367)
@@ -69,6 +69,13 @@
Download::~Download()
{
+ if (m_resourceHandle) {
+ m_resourceHandle->clearClient();
+ m_resourceHandle->cancel();
+ m_resourceHandle = nullptr;
+ }
+ m_downloadClient = nullptr;
+
platformInvalidate();
m_downloadManager.didDestroyDownload();
@@ -90,6 +97,17 @@
#endif
}
+void Download::cancel()
+{
+ if (m_request.url().protocolIsBlob()) {
+ auto resourceHandle = WTFMove(m_resourceHandle);
+ resourceHandle->cancel();
+ static_cast<BlobDownloadClient*>(m_downloadClient.get())->didCancel();
+ return;
+ }
+ cancelNetworkLoad();
+}
+
void Download::didStart()
{
send(Messages::DownloadProxy::DidStart(m_request, m_suggestedName));
Modified: trunk/Source/WebKit2/NetworkProcess/Downloads/Download.h (206366 => 206367)
--- trunk/Source/WebKit2/NetworkProcess/Downloads/Download.h 2016-09-26 03:57:40 UTC (rev 206366)
+++ trunk/Source/WebKit2/NetworkProcess/Downloads/Download.h 2016-09-26 05:35:55 UTC (rev 206367)
@@ -115,6 +115,7 @@
#if !USE(NETWORK_SESSION)
void startNetworkLoad();
#endif
+ void cancelNetworkLoad();
void platformInvalidate();
Modified: trunk/Source/WebKit2/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm (206366 => 206367)
--- trunk/Source/WebKit2/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm 2016-09-26 03:57:40 UTC (rev 206366)
+++ trunk/Source/WebKit2/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm 2016-09-26 05:35:55 UTC (rev 206367)
@@ -38,7 +38,7 @@
notImplemented();
}
-void Download::cancel()
+void Download::cancelNetworkLoad()
{
ASSERT(m_download);
[m_download cancelByProducingResumeData: ^(NSData * _Nullable resumeData)
Modified: trunk/Source/WebKit2/NetworkProcess/Downloads/ios/DownloadIOS.mm (206366 => 206367)
--- trunk/Source/WebKit2/NetworkProcess/Downloads/ios/DownloadIOS.mm 2016-09-26 03:57:40 UTC (rev 206366)
+++ trunk/Source/WebKit2/NetworkProcess/Downloads/ios/DownloadIOS.mm 2016-09-26 05:35:55 UTC (rev 206367)
@@ -142,7 +142,7 @@
#pragma clang diagnostic pop
}
-void Download::cancel()
+void Download::cancelNetworkLoad()
{
notImplemented();
}
Modified: trunk/Source/WebKit2/NetworkProcess/Downloads/mac/DownloadMac.mm (206366 => 206367)
--- trunk/Source/WebKit2/NetworkProcess/Downloads/mac/DownloadMac.mm 2016-09-26 03:57:40 UTC (rev 206366)
+++ trunk/Source/WebKit2/NetworkProcess/Downloads/mac/DownloadMac.mm 2016-09-26 05:35:55 UTC (rev 206367)
@@ -109,7 +109,7 @@
[m_nsURLDownload setDeletesFileUponFailure:NO];
}
-void Download::cancel()
+void Download::cancelNetworkLoad()
{
[m_nsURLDownload cancel];
@@ -119,11 +119,10 @@
void Download::platformInvalidate()
{
- ASSERT(m_nsURLDownload);
- ASSERT(m_delegate);
-
- [m_delegate invalidate];
- m_delegate = nullptr;
+ if (m_delegate) {
+ [m_delegate invalidate];
+ m_delegate = nullptr;
+ }
m_nsURLDownload = nullptr;
}
Modified: trunk/Source/WebKit2/NetworkProcess/Downloads/soup/DownloadSoup.cpp (206366 => 206367)
--- trunk/Source/WebKit2/NetworkProcess/Downloads/soup/DownloadSoup.cpp 2016-09-26 03:57:40 UTC (rev 206366)
+++ trunk/Source/WebKit2/NetworkProcess/Downloads/soup/DownloadSoup.cpp 2016-09-26 05:35:55 UTC (rev 206367)
@@ -231,7 +231,7 @@
notImplemented();
}
-void Download::cancel()
+void Download::cancelNetworkLoad()
{
if (!m_resourceHandle)
return;
@@ -245,13 +245,6 @@
void Download::platformInvalidate()
{
- if (m_resourceHandle) {
- m_resourceHandle->clearClient();
- m_resourceHandle->cancel();
- m_resourceHandle = nullptr;
- }
-
- m_downloadClient = nullptr;
}
void Download::platformDidFinish()
