Title: [242693] trunk
- Revision
- 242693
- Author
- [email protected]
- Date
- 2019-03-10 17:27:22 -0700 (Sun, 10 Mar 2019)
Log 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):
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (242692 => 242693)
--- trunk/Source/WebKit/ChangeLog 2019-03-11 00:13:52 UTC (rev 242692)
+++ trunk/Source/WebKit/ChangeLog 2019-03-11 00:27:22 UTC (rev 242693)
@@ -1,3 +1,17 @@
+2019-03-10 David Quesada <[email protected]>
+
+ 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 <[email protected]>
Fix some misleading function and variable names in WKContentViewInteraction.mm
Modified: trunk/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp (242692 => 242693)
--- trunk/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp 2019-03-11 00:13:52 UTC (rev 242692)
+++ trunk/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp 2019-03-11 00:27:22 UTC (rev 242693)
@@ -71,6 +71,9 @@
{
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));
m_process->removeMessageReceiver(Messages::DownloadProxy::messageReceiverName(), downloadID.downloadID());
Modified: trunk/Tools/ChangeLog (242692 => 242693)
--- trunk/Tools/ChangeLog 2019-03-11 00:13:52 UTC (rev 242692)
+++ trunk/Tools/ChangeLog 2019-03-11 00:27:22 UTC (rev 242693)
@@ -1,3 +1,25 @@
+2019-03-10 David Quesada <[email protected]>
+
+ 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 <[email protected]>
Add support for Device Orientation / Motion permission API
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm (242692 => 242693)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm 2019-03-11 00:13:52 UTC (rev 242692)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm 2019-03-11 00:27:22 UTC (rev 242693)
@@ -42,6 +42,7 @@
#import <wtf/FileSystem.h>
#import <wtf/MainThread.h>
#import <wtf/RetainPtr.h>
+#import <wtf/WeakObjCPtr.h>
#import <wtf/text/WTFString.h>
static bool isDone;
@@ -735,4 +736,44 @@
EXPECT_FALSE([delegate didStartProvisionalNavigation]);
}
+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
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
