Attachment 370960 Details for Bug 198298 – Patch for landing, v2

[patch] Patch for landing, v2

file_198298.txt (text/plain), 12.73 KB, created by David Quesada on 2019-05-30 11:35:54 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: David Quesada

Created: 2019-05-30 11:35:54 PDT

Size: 12.73 KB

patch

obsolete

>commit 6f966b7956bfab04706488badeb3c53a6b7cc77e
>Author: David Quesada <david_quesada@apple.com>
>Date:   Thu May 30 10:46:17 2019 -0700
>
>    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):
>
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 9eb299bb2e4..ea8f25c9d65 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,56 @@
>+2019-05-30  David Quesada  <david_quesada@apple.com>
>+
>+        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  Truitt Savell  <tsavell@apple.com>
> 
>         Unreviewed, rolling out r245881.
>diff --git a/Source/WebKit/NetworkProcess/Downloads/Download.cpp b/Source/WebKit/NetworkProcess/Downloads/Download.cpp
>index 7d8b4d447a2..e210d0dcabf 100644
>--- a/Source/WebKit/NetworkProcess/Downloads/Download.cpp
>+++ b/Source/WebKit/NetworkProcess/Downloads/Download.cpp
>@@ -86,6 +86,7 @@ Download::~Download()
> 
> void Download::cancel()
> {
>+    m_wasCanceled = true;
>     if (m_download) {
>         m_download->cancel();
>         didCancel({ });
>diff --git a/Source/WebKit/NetworkProcess/Downloads/Download.h b/Source/WebKit/NetworkProcess/Downloads/Download.h
>index 772dd0227f5..f720c012b39 100644
>--- a/Source/WebKit/NetworkProcess/Downloads/Download.h
>+++ b/Source/WebKit/NetworkProcess/Downloads/Download.h
>@@ -92,6 +92,7 @@ public:
>     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 @@ private:
> #endif
>     PAL::SessionID m_sessionID;
>     String m_suggestedName;
>+    bool m_wasCanceled { false };
>     bool m_hasReceivedData { false };
>     DownloadMonitor m_monitor { *this };
> };
>diff --git a/Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm b/Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm
>index f10467fee7f..9bcf128b8be 100644
>--- a/Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm
>+++ b/Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm
>@@ -80,13 +80,13 @@ void Download::resume(const IPC::DataReference& resumeData, const String& path,
> 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);
>     }];
> }
> 
>diff --git a/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm b/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm
>index c4ca391df7e..80179ead3a4 100644
>--- a/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm
>+++ b/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm
>@@ -623,14 +623,22 @@ - (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didComp
>             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);
>             }
>         }
>     }
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 3f09f1970c9..1c612b97f06 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,16 @@
>+2019-05-30  David Quesada  <david_quesada@apple.com>
>+
>+        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  <tsavell@apple.com>
> 
>         Unreviewed, rolling out r245881.
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm
>index 29ffe13c7d4..c6c553716ac 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm
>@@ -879,7 +879,7 @@ void downloadAtRate(double desiredKbps, unsigned speedMultiplier, AppReturnsToFo
>     [[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

Actions: View | Formatted Diff | Diff

Attachments on bug 198298: 370906 | 370955 | 370960