<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[242693] trunk</title>
</head>
<body>

<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt;  }
#msg dl a { font-weight: bold}
#msg dl a:link    { color:#fc3; }
#msg dl a:active  { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff  {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/242693">242693</a></dd>
<dt>Author</dt> <dd>david_quesada@apple.com</dd>
<dt>Date</dt> <dd>2019-03-10 17:27:22 -0700 (Sun, 10 Mar 2019)</dd>
</dl>

<h3>Log Message</h3>
<pre>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):</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWebKitChangeLog">trunk/Source/WebKit/ChangeLog</a></li>
<li><a href="#trunkSourceWebKitUIProcessDownloadsDownloadProxyMapcpp">trunk/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp</a></li>
<li><a href="#trunkToolsChangeLog">trunk/Tools/ChangeLog</a></li>
<li><a href="#trunkToolsTestWebKitAPITestsWebKitCocoaDownloadmm">trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWebKitChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit/ChangeLog (242692 => 242693)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -1,3 +1,17 @@
</span><ins>+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.
+
</ins><span class="cx"> 2019-03-10  Wenson Hsieh  <wenson_hsieh@apple.com>
</span><span class="cx"> 
</span><span class="cx">         Fix some misleading function and variable names in WKContentViewInteraction.mm
</span></span></pre></div>
<a id="trunkSourceWebKitUIProcessDownloadsDownloadProxyMapcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit/UIProcess/Downloads/DownloadProxyMap.cpp (242692 => 242693)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -71,6 +71,9 @@
</span><span class="cx"> {
</span><span class="cx">     auto downloadID = downloadProxy->downloadID();
</span><span class="cx"> 
</span><ins>+    // The DownloadProxy may be holding the last reference to the process pool.
+    auto protectedProcessPool = makeRefPtr(m_process->processPool());
+
</ins><span class="cx">     ASSERT(m_downloads.contains(downloadID));
</span><span class="cx"> 
</span><span class="cx">     m_process->removeMessageReceiver(Messages::DownloadProxy::messageReceiverName(), downloadID.downloadID());
</span></span></pre></div>
<a id="trunkToolsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Tools/ChangeLog (242692 => 242693)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Tools/ChangeLog    2019-03-11 00:13:52 UTC (rev 242692)
+++ trunk/Tools/ChangeLog       2019-03-11 00:27:22 UTC (rev 242693)
</span><span class="lines">@@ -1,3 +1,25 @@
</span><ins>+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):
+
</ins><span class="cx"> 2019-03-08  Chris Dumez  <cdumez@apple.com>
</span><span class="cx"> 
</span><span class="cx">         Add support for Device Orientation / Motion permission API
</span></span></pre></div>
<a id="trunkToolsTestWebKitAPITestsWebKitCocoaDownloadmm"></a>
<div class="modfile"><h4>Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/Download.mm (242692 => 242693)</h4>
<pre class="diff"><span>
<span class="info">--- 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)
</span><span class="lines">@@ -42,6 +42,7 @@
</span><span class="cx"> #import <wtf/FileSystem.h>
</span><span class="cx"> #import <wtf/MainThread.h>
</span><span class="cx"> #import <wtf/RetainPtr.h>
</span><ins>+#import <wtf/WeakObjCPtr.h>
</ins><span class="cx"> #import <wtf/text/WTFString.h>
</span><span class="cx"> 
</span><span class="cx"> static bool isDone;
</span><span class="lines">@@ -735,4 +736,44 @@
</span><span class="cx">     EXPECT_FALSE([delegate didStartProvisionalNavigation]);
</span><span class="cx"> }
</span><span class="cx"> 
</span><ins>+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());
+}
+
</ins><span class="cx"> #endif
</span></span></pre>
</div>
</div>

</body>
</html>