- Revision
- 245901
- Author
- [email protected]
- Date
- 2019-05-30 12:52:57 -0700 (Thu, 30 May 2019)
Log Message
REGRESSION (r245756) [Mac] 2 TestWebKitAPI.DownloadProgress* and TestWebKitAPI._WKDownload.DownloadMonitorCancel are flaky timeouts
https://bugs.webkit.org/show_bug.cgi?id=198298
rdar://problem/51182393
Reviewed by Alexey Proskuryakov.
Source/WebKit:
When canceling a download, there has always been a race condition between:
(A) the execution of Download::didCancel() within the block passed to
-[NSURLSessionDownloadTask cancelByProducingResumeData:] within
Download::platformCancelNetworkLoad(), and
(B) the invocation of -[WKNetworkSessionDelegate URLSession:task:didCompleteWithError:]
If A happens before B, the block calls didCancel() on the download, which reports the
cancellation to the UI process and tears down the download. When B happens, WKNetworkSessionDelegate
gracefully handles the fact that the Download has been removed from the map, and nothing
else happens. Life is good.
If B happens before A, -URLSession:task:didCompleteWithError: invokes Download::didFail(),
which reports a download failure (*not* a cancellation) to the UI process and tears down
the Download and DownloadProxy. On release builds, this can leave the tests waiting for a
cancellation until they time out. When A happens, the block calls Download::didCancel().
This messages the UI process, which results in a debug assertion failure from an unhandled
message since the DownloadProxy was torn down when the failure was reported. Meanwhile,
the network process hits a debug assertion in DownloadManager::downloadFinished() when
trying to remove the Download *again*.
r245756 made the bad case (B before A) more likely by adding a delay before didCancel()
is called.
Make this race condition impossible by eliminating the didCancel() from the cancellation
block, and instead relying on -URLSession:task:didCompleteWithError: to report the
download as canceled. This also effectively coalesces calls to platformCancelNetworkLoad(),
which, if called multiple times before CFNetwork reports that the download was canceled,
could cause multiple calls to didCancel(), resulting in the same assertion failures seen
in the B-before-A case.
No new tests, as recreating this race condition in the test scenario would require
additional machinery, and is no longer even possible since we don't depend on the calling
of the cancellation handler in order to report the Download as canceled.
* NetworkProcess/Downloads/Download.cpp:
(WebKit::Download::cancel):
* NetworkProcess/Downloads/Download.h:
(WebKit::Download::wasCanceled const):
* NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:
(WebKit::Download::platformCancelNetworkLoad):
* NetworkProcess/cocoa/NetworkSessionCocoa.mm:
(-[WKNetworkSessionDelegate URLSession:task:didCompleteWithError:]):
Tools:
Re-enable _WKDownload.DownloadMonitorCancel, which should no longer time out with this fix.
* TestWebKitAPI/Tests/WebKitCocoa/Download.mm:
(TestWebKitAPI::TEST):
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (245900 => 245901)
--- trunk/Source/WebKit/ChangeLog 2019-05-30 19:41:53 UTC (rev 245900)
+++ trunk/Source/WebKit/ChangeLog 2019-05-30 19:52:57 UTC (rev 245901)
@@ -1,3 +1,56 @@
+2019-05-30 David Quesada <[email protected]>
+
+ REGRESSION (r245756) [Mac] 2 TestWebKitAPI.DownloadProgress* and TestWebKitAPI._WKDownload.DownloadMonitorCancel are flaky timeouts
+ https://bugs.webkit.org/show_bug.cgi?id=198298
+ rdar://problem/51182393
+
+ Reviewed by Alexey Proskuryakov.
+
+ When canceling a download, there has always been a race condition between:
+
+ (A) the execution of Download::didCancel() within the block passed to
+ -[NSURLSessionDownloadTask cancelByProducingResumeData:] within
+ Download::platformCancelNetworkLoad(), and
+
+ (B) the invocation of -[WKNetworkSessionDelegate URLSession:task:didCompleteWithError:]
+
+ If A happens before B, the block calls didCancel() on the download, which reports the
+ cancellation to the UI process and tears down the download. When B happens, WKNetworkSessionDelegate
+ gracefully handles the fact that the Download has been removed from the map, and nothing
+ else happens. Life is good.
+
+ If B happens before A, -URLSession:task:didCompleteWithError: invokes Download::didFail(),
+ which reports a download failure (*not* a cancellation) to the UI process and tears down
+ the Download and DownloadProxy. On release builds, this can leave the tests waiting for a
+ cancellation until they time out. When A happens, the block calls Download::didCancel().
+ This messages the UI process, which results in a debug assertion failure from an unhandled
+ message since the DownloadProxy was torn down when the failure was reported. Meanwhile,
+ the network process hits a debug assertion in DownloadManager::downloadFinished() when
+ trying to remove the Download *again*.
+
+ r245756 made the bad case (B before A) more likely by adding a delay before didCancel()
+ is called.
+
+ Make this race condition impossible by eliminating the didCancel() from the cancellation
+ block, and instead relying on -URLSession:task:didCompleteWithError: to report the
+ download as canceled. This also effectively coalesces calls to platformCancelNetworkLoad(),
+ which, if called multiple times before CFNetwork reports that the download was canceled,
+ could cause multiple calls to didCancel(), resulting in the same assertion failures seen
+ in the B-before-A case.
+
+ No new tests, as recreating this race condition in the test scenario would require
+ additional machinery, and is no longer even possible since we don't depend on the calling
+ of the cancellation handler in order to report the Download as canceled.
+
+ * NetworkProcess/Downloads/Download.cpp:
+ (WebKit::Download::cancel):
+ * NetworkProcess/Downloads/Download.h:
+ (WebKit::Download::wasCanceled const):
+ * NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:
+ (WebKit::Download::platformCancelNetworkLoad):
+ * NetworkProcess/cocoa/NetworkSessionCocoa.mm:
+ (-[WKNetworkSessionDelegate URLSession:task:didCompleteWithError:]):
+
2019-05-30 Chris Dumez <[email protected]>
[iOS] UIProcess' background task expiration handler may get called after the app is foreground again
Modified: trunk/Source/WebKit/NetworkProcess/Downloads/Download.cpp (245900 => 245901)
--- trunk/Source/WebKit/NetworkProcess/Downloads/Download.cpp 2019-05-30 19:41:53 UTC (rev 245900)
+++ trunk/Source/WebKit/NetworkProcess/Downloads/Download.cpp 2019-05-30 19:52:57 UTC (rev 245901)
@@ -86,6 +86,7 @@
void Download::cancel()
{
+ m_wasCanceled = true;
if (m_download) {
m_download->cancel();
didCancel({ });
Modified: trunk/Source/WebKit/NetworkProcess/Downloads/Download.h (245900 => 245901)
--- trunk/Source/WebKit/NetworkProcess/Downloads/Download.h 2019-05-30 19:41:53 UTC (rev 245900)
+++ trunk/Source/WebKit/NetworkProcess/Downloads/Download.h 2019-05-30 19:52:57 UTC (rev 245901)
@@ -92,6 +92,7 @@
void didCancel(const IPC::DataReference& resumeData);
bool isAlwaysOnLoggingAllowed() const;
+ bool wasCanceled() const { return m_wasCanceled; }
void applicationDidEnterBackground() { m_monitor.applicationDidEnterBackground(); }
void applicationWillEnterForeground() { m_monitor.applicationWillEnterForeground(); }
@@ -119,6 +120,7 @@
#endif
PAL::SessionID m_sessionID;
String m_suggestedName;
+ bool m_wasCanceled { false };
bool m_hasReceivedData { false };
DownloadMonitor m_monitor { *this };
};
Modified: trunk/Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm (245900 => 245901)
--- trunk/Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm 2019-05-30 19:41:53 UTC (rev 245900)
+++ trunk/Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm 2019-05-30 19:52:57 UTC (rev 245901)
@@ -80,13 +80,13 @@
void Download::platformCancelNetworkLoad()
{
ASSERT(m_downloadTask);
+
+ // The download's resume data is accessed in the network session delegate
+ // method -URLSession:task:didCompleteWithError: instead of inside this block,
+ // to avoid race conditions between the two. Calling -cancel is not sufficient
+ // here because CFNetwork won't provide the resume data unless we ask for it.
[m_downloadTask cancelByProducingResumeData:^(NSData *resumeData) {
- callOnMainThread([this, resumeData = retainPtr(resumeData)] {
- if (resumeData && resumeData.get().bytes && resumeData.get().length)
- didCancel(IPC::DataReference(reinterpret_cast<const uint8_t*>(resumeData.get().bytes), resumeData.get().length));
- else
- didCancel({ });
- });
+ UNUSED_PARAM(resumeData);
}];
}
Modified: trunk/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm (245900 => 245901)
--- trunk/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm 2019-05-30 19:41:53 UTC (rev 245900)
+++ trunk/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm 2019-05-30 19:52:57 UTC (rev 245901)
@@ -623,14 +623,22 @@
if (auto* download = _session->networkProcess().downloadManager().download(downloadID)) {
NSData *resumeData = nil;
if (id userInfo = error.userInfo) {
- if ([userInfo isKindOfClass:[NSDictionary class]])
+ if ([userInfo isKindOfClass:[NSDictionary class]]) {
resumeData = userInfo[@"NSURLSessionDownloadTaskResumeData"];
+ if (resumeData && ![resumeData isKindOfClass:[NSData class]]) {
+ RELEASE_LOG(NetworkSession, "Download task %llu finished with resume data of wrong class: %s", (unsigned long long)task.taskIdentifier, NSStringFromClass([resumeData class]).UTF8String);
+ ASSERT_NOT_REACHED();
+ resumeData = nil;
+ }
+ }
}
-
- if (resumeData && [resumeData isKindOfClass:[NSData class]])
- download->didFail(error, { static_cast<const uint8_t*>(resumeData.bytes), resumeData.length });
+
+ auto resumeDataReference = resumeData ? IPC::DataReference { static_cast<const uint8_t*>(resumeData.bytes), resumeData.length } : IPC::DataReference { };
+
+ if (download->wasCanceled())
+ download->didCancel(resumeDataReference);
else
- download->didFail(error, { });
+ download->didFail(error, resumeDataReference);
}
}
}
Modified: trunk/Tools/ChangeLog (245900 => 245901)
--- trunk/Tools/ChangeLog 2019-05-30 19:41:53 UTC (rev 245900)
+++ trunk/Tools/ChangeLog 2019-05-30 19:52:57 UTC (rev 245901)
@@ -1,3 +1,16 @@
+2019-05-30 David Quesada <[email protected]>
+
+ REGRESSION (r245756) [Mac] 2 TestWebKitAPI.DownloadProgress* and TestWebKitAPI._WKDownload.DownloadMonitorCancel are flaky timeouts
+ https://bugs.webkit.org/show_bug.cgi?id=198298
+ rdar://problem/51182393
+
+ Reviewed by Alexey Proskuryakov.
+
+ Re-enable _WKDownload.DownloadMonitorCancel, which should no longer time out with this fix.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/Download.mm:
+ (TestWebKitAPI::TEST):
+
2019-05-30 Truitt Savell <[email protected]>
Unreviewed, rolling out r245881.
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm (245900 => 245901)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm 2019-05-30 19:41:53 UTC (rev 245900)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm 2019-05-30 19:52:57 UTC (rev 245901)
@@ -879,7 +879,7 @@
[[NSFileManager defaultManager] removeItemAtURL:[NSURL fileURLWithPath:destination.get() isDirectory:NO] error:nil];
}
-TEST(_WKDownload, DISABLED_DownloadMonitorCancel)
+TEST(_WKDownload, DownloadMonitorCancel)
{
downloadAtRate(0.5, 120); // Should cancel in ~0.5 seconds
downloadAtRate(1.5, 120); // Should cancel in ~2.5 seconds
