Changeset 245901 in webkit


Ignore:
Timestamp:
May 30, 2019 12:52:57 PM (5 years ago)
Author:
david_quesada@apple.com
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):

Location:
trunk
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r245899 r245901  
     12019-05-30  David Quesada  <david_quesada@apple.com>
     2
     3        REGRESSION (r245756) [Mac] 2 TestWebKitAPI.DownloadProgress* and TestWebKitAPI._WKDownload.DownloadMonitorCancel are flaky timeouts
     4        https://bugs.webkit.org/show_bug.cgi?id=198298
     5        rdar://problem/51182393
     6
     7        Reviewed by Alexey Proskuryakov.
     8
     9        When canceling a download, there has always been a race condition between:
     10
     11         (A) the execution of Download::didCancel() within the block passed to
     12             -[NSURLSessionDownloadTask cancelByProducingResumeData:] within
     13             Download::platformCancelNetworkLoad(), and
     14
     15         (B) the invocation of -[WKNetworkSessionDelegate URLSession:task:didCompleteWithError:]
     16
     17        If A happens before B, the block calls didCancel() on the download, which reports the
     18        cancellation to the UI process and tears down the download. When B happens, WKNetworkSessionDelegate
     19        gracefully handles the fact that the Download has been removed from the map, and nothing
     20        else happens. Life is good.
     21
     22        If B happens before A, -URLSession:task:didCompleteWithError: invokes Download::didFail(),
     23        which reports a download failure (*not* a cancellation) to the UI process and tears down
     24        the Download and DownloadProxy. On release builds, this can leave the tests waiting for a
     25        cancellation until they time out. When A happens, the block calls Download::didCancel().
     26        This messages the UI process, which results in a debug assertion failure from an unhandled
     27        message since the DownloadProxy was torn down when the failure was reported. Meanwhile,
     28        the network process hits a debug assertion in DownloadManager::downloadFinished() when
     29        trying to remove the Download *again*.
     30
     31        r245756 made the bad case (B before A) more likely by adding a delay before didCancel()
     32        is called.
     33
     34        Make this race condition impossible by eliminating the didCancel() from the cancellation
     35        block, and instead relying on -URLSession:task:didCompleteWithError: to report the
     36        download as canceled. This also effectively coalesces calls to platformCancelNetworkLoad(),
     37        which, if called multiple times before CFNetwork reports that the download was canceled,
     38        could cause multiple calls to didCancel(), resulting in the same assertion failures seen
     39        in the B-before-A case.
     40
     41        No new tests, as recreating this race condition in the test scenario would require
     42        additional machinery, and is no longer even possible since we don't depend on the calling
     43        of the cancellation handler in order to report the Download as canceled.
     44
     45        * NetworkProcess/Downloads/Download.cpp:
     46        (WebKit::Download::cancel):
     47        * NetworkProcess/Downloads/Download.h:
     48        (WebKit::Download::wasCanceled const):
     49        * NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:
     50        (WebKit::Download::platformCancelNetworkLoad):
     51        * NetworkProcess/cocoa/NetworkSessionCocoa.mm:
     52        (-[WKNetworkSessionDelegate URLSession:task:didCompleteWithError:]):
     53
    1542019-05-30  Chris Dumez  <cdumez@apple.com>
    255

  • trunk/Source/WebKit/NetworkProcess/Downloads/Download.cpp

    r243110 r245901  
    8787void Download::cancel()
    8888{
     89    m_wasCanceled = true;
    8990    if (m_download) {
    9091        m_download->cancel();

  • trunk/Source/WebKit/NetworkProcess/Downloads/Download.h

    r243110 r245901  
    9393
    9494    bool isAlwaysOnLoggingAllowed() const;
     95    bool wasCanceled() const { return m_wasCanceled; }
    9596
    9697    void applicationDidEnterBackground() { m_monitor.applicationDidEnterBackground(); }
    … …  
    120121    PAL::SessionID m_sessionID;
    121122    String m_suggestedName;
     123    bool m_wasCanceled { false };
    122124    bool m_hasReceivedData { false };
    123125    DownloadMonitor m_monitor { *this };

  • trunk/Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm

    r245756 r245901  
    8181{
    8282    ASSERT(m_downloadTask);
     83
     84    // The download's resume data is accessed in the network session delegate
     85    // method -URLSession:task:didCompleteWithError: instead of inside this block,
     86    // to avoid race conditions between the two. Calling -cancel is not sufficient
     87    // here because CFNetwork won't provide the resume data unless we ask for it.
    8388    [m_downloadTask cancelByProducingResumeData:^(NSData *resumeData) {
    84         callOnMainThread([this, resumeData = retainPtr(resumeData)] {
    85             if (resumeData && resumeData.get().bytes && resumeData.get().length)
    86                 didCancel(IPC::DataReference(reinterpret_cast<const uint8_t*>(resumeData.get().bytes), resumeData.get().length));
    87             else
    88                 didCancel({ });
    89         });
     89        UNUSED_PARAM(resumeData);
    9090    }];
    9191}

  • trunk/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm

    r245540 r245901  
    624624                NSData *resumeData = nil;
    625625                if (id userInfo = error.userInfo) {
    626                     if ([userInfo isKindOfClass:[NSDictionary class]])
     626                    if ([userInfo isKindOfClass:[NSDictionary class]]) {
    627627                        resumeData = userInfo[@"NSURLSessionDownloadTaskResumeData"];
     628                        if (resumeData && ![resumeData isKindOfClass:[NSData class]]) {
     629                            RELEASE_LOG(NetworkSession, "Download task %llu finished with resume data of wrong class: %s", (unsigned long long)task.taskIdentifier, NSStringFromClass([resumeData class]).UTF8String);
     630                            ASSERT_NOT_REACHED();
     631                            resumeData = nil;
     632                        }
     633                    }
    628634                }
    629                
    630                 if (resumeData && [resumeData isKindOfClass:[NSData class]])
    631                     download->didFail(error, { static_cast<const uint8_t*>(resumeData.bytes), resumeData.length });
     635
     636                auto resumeDataReference = resumeData ? IPC::DataReference { static_cast<const uint8_t*>(resumeData.bytes), resumeData.length } : IPC::DataReference { };
     637
     638                if (download->wasCanceled())
     639                    download->didCancel(resumeDataReference);
    632640                else
    633                     download->didFail(error, { });
     641                    download->didFail(error, resumeDataReference);
    634642            }
    635643        }

  • trunk/Tools/ChangeLog

    r245891 r245901  
     12019-05-30  David Quesada  <david_quesada@apple.com>
     2
     3        REGRESSION (r245756) [Mac] 2 TestWebKitAPI.DownloadProgress* and TestWebKitAPI._WKDownload.DownloadMonitorCancel are flaky timeouts
     4        https://bugs.webkit.org/show_bug.cgi?id=198298
     5        rdar://problem/51182393
     6
     7        Reviewed by Alexey Proskuryakov.
     8
     9        Re-enable _WKDownload.DownloadMonitorCancel, which should no longer time out with this fix.
     10
     11        * TestWebKitAPI/Tests/WebKitCocoa/Download.mm:
     12        (TestWebKitAPI::TEST):
     13
    1142019-05-30  Truitt Savell  <tsavell@apple.com>
    215

  • trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm

    r245851 r245901  
    880880}
    881881
    882 TEST(_WKDownload, DISABLED_DownloadMonitorCancel)
     882TEST(_WKDownload, DownloadMonitorCancel)
    883883{
    884884    downloadAtRate(0.5, 120); // Should cancel in ~0.5 seconds
Note: See TracChangeset for help on using the changeset viewer.