Changeset 242693 in webkit

Timestamp:

Mar 10, 2019, 5:27:22 PM (7 years ago)

Author:

david_quesada@apple.com

Message:

ASSERT(m_downloads.isEmpty()) fails in DownloadProxyMap::~DownloadProxyMap()
​https://bugs.webkit.org/show_bug.cgi?id=152480

Reviewed by Chris Dumez.

Source/WebKit:

• UIProcess/Downloads/DownloadProxyMap.cpp:

(WebKit::DownloadProxyMap::downloadFinished):

If the DownloadProxy is holding the last reference to the process pool, then
invalidating the proxy will cause the process pool, the network process proxy,
and this DownloadProxyMap to deallocate. Ensure that doesn't happen until this
method has done everything it wants to do to clean up.

Tools:

Add a unit test based on Daniel Bates's test case that starts a download, ensures
there are no additional references to the process pool besides the one held by
the download, waits for the download to finish (in the sense that the
DownloadProxyMap is done tracking the DownloadProxy), and doesn't crash. For good
measure, also check that the process pool has been deallocated at the end of the
test. The test wouldn't be meaningful if the process pool were still alive.

• TestWebKitAPI/Tests/WebKitCocoa/Download.mm:

(-[WaitUntilDownloadCanceledDelegate _downloadDidStart:]):
(-[WaitUntilDownloadCanceledDelegate _downloadDidCancel:]):

The download will be canceled because the delegate does not implement the
method to decide the download's destination, so this is where we know the
DownloadProxyMap is done with the DownloadProxy.

(TEST):

Location:

trunk

Files:

• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm (modified) (2 diffs)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-03-10  David Quesada  <david_quesada@apple.com>
+
+        ASSERT(m_downloads.isEmpty()) fails in DownloadProxyMap::~DownloadProxyMap()
+        https://bugs.webkit.org/show_bug.cgi?id=152480
+
+        Reviewed by Chris Dumez.
+
+        * UIProcess/Downloads/DownloadProxyMap.cpp:
+        (WebKit::DownloadProxyMap::downloadFinished):
+            If the DownloadProxy is holding the last reference to the process pool, then
+            invalidating the proxy will cause the process pool, the network process proxy,
+            and this DownloadProxyMap to deallocate. Ensure that doesn't happen until this
+            method has done everything it wants to do to clean up.
+
 2019-03-10  Wenson Hsieh  <wenson_hsieh@apple.com>
 

trunk/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp

@@ -72 +72 @@
     auto downloadID = downloadProxy->downloadID();
 
+    // The DownloadProxy may be holding the last reference to the process pool.
+    auto protectedProcessPool = makeRefPtr(m_process->processPool());
+
     ASSERT(m_downloads.contains(downloadID));
 

trunk/Tools/ChangeLog

@@ +1 @@
+2019-03-10  David Quesada  <david_quesada@apple.com>
+
+        ASSERT(m_downloads.isEmpty()) fails in DownloadProxyMap::~DownloadProxyMap()
+        https://bugs.webkit.org/show_bug.cgi?id=152480
+
+        Reviewed by Chris Dumez.
+
+        Add a unit test based on Daniel Bates's test case that starts a download, ensures
+        there are no additional references to the process pool besides the one held by
+        the download, waits for the download to finish (in the sense that the
+        DownloadProxyMap is done tracking the DownloadProxy), and doesn't crash. For good
+        measure, also check that the process pool has been deallocated at the end of the
+        test. The test wouldn't be meaningful if the process pool were still alive.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/Download.mm:
+        (-[WaitUntilDownloadCanceledDelegate _downloadDidStart:]):
+        (-[WaitUntilDownloadCanceledDelegate _downloadDidCancel:]):
+            The download will be canceled because the delegate does not implement the
+            method to decide the download's destination, so this is where we know the
+            DownloadProxyMap is done with the DownloadProxy.
+        (TEST):
+
 2019-03-08  Chris Dumez  <cdumez@apple.com>
 

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

@@ -43 +43 @@
 #import <wtf/MainThread.h>
 #import <wtf/RetainPtr.h>
+#import <wtf/WeakObjCPtr.h>
 #import <wtf/text/WTFString.h>
 
@@ -736 +737 @@
 }
 
+static bool didDownloadStart;
+
+@interface WaitUntilDownloadCanceledDelegate : NSObject <_WKDownloadDelegate>
+@end
+
+@implementation WaitUntilDownloadCanceledDelegate
+
+- (void)_downloadDidStart:(_WKDownload *)download
+{
+    didDownloadStart = true;
+}
+
+- (void)_downloadDidCancel:(_WKDownload *)download
+{
+    isDone = true;
+}
+
+@end
+
+TEST(_WKDownload, CrashAfterDownloadDidFinishWhenDownloadProxyHoldsTheLastRefOnWebProcessPool)
+{
+    auto navigationDelegate = adoptNS([[DownloadNavigationDelegate alloc] init]);
+    auto downloadDelegate = adoptNS([[WaitUntilDownloadCanceledDelegate alloc] init]);
+    WeakObjCPtr<WKProcessPool> processPool;
+    @autoreleasepool {
+        RetainPtr<WKWebView> webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600)]);
+        [webView setNavigationDelegate:navigationDelegate.get()];
+        processPool = [webView configuration].processPool;
+        [webView configuration].processPool._downloadDelegate = downloadDelegate.get();
+        [webView loadRequest:[NSURLRequest requestWithURL:sourceURL]];
+
+        didDownloadStart = false;
+        TestWebKitAPI::Util::run(&didDownloadStart);
+    }
+
+    isDone = false;
+    TestWebKitAPI::Util::run(&isDone);
+    EXPECT_NULL(processPool.get());
+}
+
 #endif