Attachment 372366 Details for Bug 198945 – Patch

[patch] Patch

bug-198945-20190618125933.patch (text/plain), 4.21 KB, created by Brady Eidson on 2019-06-18 12:59:34 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Brady Eidson

Created: 2019-06-18 12:59:34 PDT

Size: 4.21 KB

patch

obsolete

>Subversion Revision: 246526
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index ce34490edf06eec0e15f426e0fa45d76739d363a..be8d3e77eb8eb725fa9d8367772d51997079650d 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,28 @@
>+2019-06-18  Brady Eidson  <beidson@apple.com>
>+
>+        Handle NSProgress calling our cancellation handler on background threads (and calling it more than once).
>+        <rdar://problem/51392926> and https://bugs.webkit.org/show_bug.cgi?id=198945
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        If you have a download in progress and quickly tap the button to cancel it multiple times, then:
>+            - NSProgress calls our cancellation handler on a non-main thread, which we can't handle.
>+            - They do it more than once, which is also bad.
>+            - They might even do it multiple times concurrently (on different background dispatch queues)
>+        
>+        Let's work around these.
>+
>+        * NetworkProcess/Downloads/Download.cpp:
>+        (WebKit::Download::cancel): Double check we're on the main thread, and handle being called twice.
>+
>+        * NetworkProcess/Downloads/cocoa/WKDownloadProgress.mm:
>+        (-[WKDownloadProgress performCancel]): Actually cancel the WebKit::Download if we still have one.
>+        (-[WKDownloadProgress progressCancelled]): Called when NSProgress calls the cancellation handler, no matter
>+          which thread it does it on. By leveraging std::call_once we handle multiple calls as well as being called
>+          concurrently from different threads. call_once punts the *actual* cancel operation off to the main thread.
>+        (-[WKDownloadProgress initWithDownloadTask:download:URL:sandboxExtension:]): The cancellation handler is
>+          now simply calling 'progressCancelled' on self, assuming the weak pointer for self is still valid.
>+
> 2019-06-17  Alex Christensen  <achristensen@webkit.org>
> 
>         Protect StorageManager::m_localStorageNamespaces with a Lock
>diff --git a/Source/WebKit/NetworkProcess/Downloads/Download.cpp b/Source/WebKit/NetworkProcess/Downloads/Download.cpp
>index e210d0dcabf084455dd46a27c0fbcf2da3bd5247..bf051bf86b22bb6bd894e301247926b75b61d985 100644
>--- a/Source/WebKit/NetworkProcess/Downloads/Download.cpp
>+++ b/Source/WebKit/NetworkProcess/Downloads/Download.cpp
>@@ -86,7 +86,12 @@ Download::~Download()
> 
> void Download::cancel()
> {
>+    RELEASE_ASSERT(isMainThread());
>+
>+    if (m_wasCanceled)
>+        return;
>     m_wasCanceled = true;
>+
>     if (m_download) {
>         m_download->cancel();
>         didCancel({ });
>diff --git a/Source/WebKit/NetworkProcess/Downloads/cocoa/WKDownloadProgress.mm b/Source/WebKit/NetworkProcess/Downloads/cocoa/WKDownloadProgress.mm
>index 81b5937161895bb1d57bb870ca2ea421ef073fca..1b93b6698a943e0cc2b56982965f181866cffd94 100644
>--- a/Source/WebKit/NetworkProcess/Downloads/cocoa/WKDownloadProgress.mm
>+++ b/Source/WebKit/NetworkProcess/Downloads/cocoa/WKDownloadProgress.mm
>@@ -41,6 +41,25 @@ @implementation WKDownloadProgress {
>     RetainPtr<NSURLSessionDownloadTask> m_task;
>     WebKit::Download* m_download;
>     RefPtr<WebKit::SandboxExtension> m_sandboxExtension;
>+    std::once_flag m_progressCancelOnce;
>+}
>+
>+- (void)performCancel
>+{
>+    if (m_download)
>+        m_download->cancel();
>+}
>+
>+- (void)progressCancelled
>+{
>+    std::call_once(m_progressCancelOnce, [self] {
>+        if (!isMainThread()) {
>+            [self performSelectorOnMainThread:@selector(performCancel) withObject:nil waitUntilDone:NO];
>+            return;
>+        }
>+
>+        [self performCancel];
>+    });
> }
> 
> - (instancetype)initWithDownloadTask:(NSURLSessionDownloadTask *)task download:(WebKit::Download*)download URL:(NSURL *)fileURL sandboxExtension:(RefPtr<WebKit::SandboxExtension>)sandboxExtension
>@@ -66,12 +85,7 @@ - (instancetype)initWithDownloadTask:(NSURLSessionDownloadTask *)task download:(
> 
>     self.cancellable = YES;
>     self.cancellationHandler = makeBlockPtr([weakSelf = WeakObjCPtr<WKDownloadProgress> { self }] {
>-        auto strongSelf = weakSelf.get();
>-        if (!strongSelf)
>-            return;
>-
>-        if (auto* download = strongSelf.get()->m_download)
>-            download->cancel();
>+        [weakSelf.get() progressCancelled];
>     }).get();
> 
>     return self;

Actions: View | Formatted Diff | Diff

Attachments on bug 198945: 372300 | 372366