<!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>[208154] 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/208154">208154</a></dd>
<dt>Author</dt> <dd>carlosgc@webkit.org</dd>
<dt>Date</dt> <dd>2016-10-31 09:57:03 -0700 (Mon, 31 Oct 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>NetworkSession: Network process crash when converting main resource to download
https://bugs.webkit.org/show_bug.cgi?id=164220
Reviewed by Alex Christensen.
Source/WebKit2:
Right after the main resource load is converted to a download, the web process deletes the ResourceLoader which
sends the RemoveLoadIdentifier to the network process and the load is aborted. Sometimes it happens that
NetworkResourceLoader::abort() is called while the NetworkLoad is still deciding the destination of the
download. In such case, NetworkResourceLoader::didConvertToDownload() has already been called, but not
NetworkResourceLoader::didBecomeDownload(). In NetworkResourceLoader::abort() we already handle the case of
having a NetworkLoad after NetworkResourceLoader::didConvertToDownload() has been called, to avoid canceling the
load in such case, however cleanup() is always called unconditionally and the NetworkLoad is deleted before
NetworkResourceLoader::didBecomeDownload() is called. When the NetworkLoad is destroyed the NetworkDataTask
client becomes nullptr, leaving it in a state where both the client is nullptr and the download hasn't been
created yet. That's not expected to happen and when the response completion handler is called in the
NetworkDataTask it tries to use either the client or the download and it crashes.
* NetworkProcess/NetworkResourceLoader.cpp:
(WebKit::NetworkResourceLoader::didBecomeDownload): Call cleanup() instead of just deleting the network load.
(WebKit::NetworkResourceLoader::abort): If we still have a network load that was converted to a download, do not
call cleanup() because it will be called by didBecomeDownload() later.
Tools:
Split /webkit2/Downloads/policy-decision-download in two, one to test the full load when main resource is
converted to a download and another one to test the cancellation as the test was doing before. When doing the
full load, delay a bit the decide destination to ensure the load is aborted before the data task has became a
download.
* TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:
(testPolicyResponseDownload):
(testPolicyResponseDownloadCancel):
(beforeAll):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWebKit2ChangeLog">trunk/Source/WebKit2/ChangeLog</a></li>
<li><a href="#trunkSourceWebKit2NetworkProcessNetworkResourceLoadercpp">trunk/Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp</a></li>
<li><a href="#trunkToolsChangeLog">trunk/Tools/ChangeLog</a></li>
<li><a href="#trunkToolsTestWebKitAPITestsWebKit2GtkTestDownloadscpp">trunk/Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWebKit2ChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit2/ChangeLog (208153 => 208154)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit2/ChangeLog 2016-10-31 16:51:40 UTC (rev 208153)
+++ trunk/Source/WebKit2/ChangeLog 2016-10-31 16:57:03 UTC (rev 208154)
</span><span class="lines">@@ -1,3 +1,27 @@
</span><ins>+2016-10-31 Carlos Garcia Campos <cgarcia@igalia.com>
+
+ NetworkSession: Network process crash when converting main resource to download
+ https://bugs.webkit.org/show_bug.cgi?id=164220
+
+ Reviewed by Alex Christensen.
+
+ Right after the main resource load is converted to a download, the web process deletes the ResourceLoader which
+ sends the RemoveLoadIdentifier to the network process and the load is aborted. Sometimes it happens that
+ NetworkResourceLoader::abort() is called while the NetworkLoad is still deciding the destination of the
+ download. In such case, NetworkResourceLoader::didConvertToDownload() has already been called, but not
+ NetworkResourceLoader::didBecomeDownload(). In NetworkResourceLoader::abort() we already handle the case of
+ having a NetworkLoad after NetworkResourceLoader::didConvertToDownload() has been called, to avoid canceling the
+ load in such case, however cleanup() is always called unconditionally and the NetworkLoad is deleted before
+ NetworkResourceLoader::didBecomeDownload() is called. When the NetworkLoad is destroyed the NetworkDataTask
+ client becomes nullptr, leaving it in a state where both the client is nullptr and the download hasn't been
+ created yet. That's not expected to happen and when the response completion handler is called in the
+ NetworkDataTask it tries to use either the client or the download and it crashes.
+
+ * NetworkProcess/NetworkResourceLoader.cpp:
+ (WebKit::NetworkResourceLoader::didBecomeDownload): Call cleanup() instead of just deleting the network load.
+ (WebKit::NetworkResourceLoader::abort): If we still have a network load that was converted to a download, do not
+ call cleanup() because it will be called by didBecomeDownload() later.
+
</ins><span class="cx"> 2016-10-31 Darin Adler <darin@apple.com>
</span><span class="cx">
</span><span class="cx"> Move ChildNode and ParentNode from ExceptionCode to Exception, add support for ExceptionOr<T&>
</span></span></pre></div>
<a id="trunkSourceWebKit2NetworkProcessNetworkResourceLoadercpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp (208153 => 208154)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp 2016-10-31 16:51:40 UTC (rev 208153)
+++ trunk/Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp 2016-10-31 16:57:03 UTC (rev 208154)
</span><span class="lines">@@ -284,7 +284,7 @@
</span><span class="cx"> {
</span><span class="cx"> ASSERT(m_didConvertToDownload);
</span><span class="cx"> ASSERT(m_networkLoad);
</span><del>- m_networkLoad = nullptr;
</del><ins>+ cleanup();
</ins><span class="cx"> }
</span><span class="cx"> #endif
</span><span class="cx">
</span><span class="lines">@@ -306,6 +306,11 @@
</span><span class="cx"> m_networkLoad->cancel();
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+#if USE(NETWORK_SESSION)
+ if (m_networkLoad && m_didConvertToDownload)
+ return;
+#endif
+
</ins><span class="cx"> cleanup();
</span><span class="cx"> }
</span><span class="cx">
</span></span></pre></div>
<a id="trunkToolsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Tools/ChangeLog (208153 => 208154)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Tools/ChangeLog 2016-10-31 16:51:40 UTC (rev 208153)
+++ trunk/Tools/ChangeLog 2016-10-31 16:57:03 UTC (rev 208154)
</span><span class="lines">@@ -1,3 +1,20 @@
</span><ins>+2016-10-31 Carlos Garcia Campos <cgarcia@igalia.com>
+
+ NetworkSession: Network process crash when converting main resource to download
+ https://bugs.webkit.org/show_bug.cgi?id=164220
+
+ Reviewed by Alex Christensen.
+
+ Split /webkit2/Downloads/policy-decision-download in two, one to test the full load when main resource is
+ converted to a download and another one to test the cancellation as the test was doing before. When doing the
+ full load, delay a bit the decide destination to ensure the load is aborted before the data task has became a
+ download.
+
+ * TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:
+ (testPolicyResponseDownload):
+ (testPolicyResponseDownloadCancel):
+ (beforeAll):
+
</ins><span class="cx"> 2016-10-31 Keith Miller <keith_miller@apple.com>
</span><span class="cx">
</span><span class="cx"> Unreviewed, fix watchlist regexp for wasm.
</span></span></pre></div>
<a id="trunkToolsTestWebKitAPITestsWebKit2GtkTestDownloadscpp"></a>
<div class="modfile"><h4>Modified: trunk/Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp (208153 => 208154)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp 2016-10-31 16:51:40 UTC (rev 208153)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp 2016-10-31 16:57:03 UTC (rev 208154)
</span><span class="lines">@@ -515,6 +515,8 @@
</span><span class="cx"> {
</span><span class="cx"> test->m_download = download;
</span><span class="cx"> test->assertObjectIsDeletedWhenTestFinishes(G_OBJECT(download));
</span><ins>+ g_signal_connect(download, "decide-destination", G_CALLBACK(downloadDecideDestinationCallback), test);
+ g_signal_connect(download, "finished", G_CALLBACK(downloadFinishedCallback), test);
</ins><span class="cx"> test->quitMainLoop();
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -539,6 +541,8 @@
</span><span class="cx"> {
</span><span class="cx"> GUniquePtr<char> destination(g_build_filename(Test::dataDirectory(), suggestedFilename, nullptr));
</span><span class="cx"> GUniquePtr<char> destinationURI(g_filename_to_uri(destination.get(), 0, 0));
</span><ins>+ if (test->m_shouldDelayDecideDestination)
+ g_usleep(0.2 * G_USEC_PER_SEC);
</ins><span class="cx"> webkit_download_set_destination(download, destinationURI.get());
</span><span class="cx"> return TRUE;
</span><span class="cx"> }
</span><span class="lines">@@ -550,12 +554,11 @@
</span><span class="cx">
</span><span class="cx"> void waitUntilDownloadFinished()
</span><span class="cx"> {
</span><del>- g_signal_connect(m_download.get(), "decide-destination", G_CALLBACK(downloadDecideDestinationCallback), this);
- g_signal_connect(m_download.get(), "finished", G_CALLBACK(downloadFinishedCallback), this);
</del><span class="cx"> g_main_loop_run(m_mainLoop);
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> GRefPtr<WebKitDownload> m_download;
</span><ins>+ bool m_shouldDelayDecideDestination { false };
</ins><span class="cx"> };
</span><span class="cx">
</span><span class="cx"> static void testWebViewDownloadURI(WebViewDownloadTest* test, gconstpointer)
</span><span class="lines">@@ -604,8 +607,10 @@
</span><span class="cx">
</span><span class="cx"> static void testPolicyResponseDownload(PolicyResponseDownloadTest* test, gconstpointer)
</span><span class="cx"> {
</span><del>- // Test that a download started by the the policy checker contains the web view.
</del><span class="cx"> CString requestURI = kServer->getURIForPath("/test.pdf").data();
</span><ins>+ // Delay the DecideDestination to ensure that the load is aborted before the network task has became a download.
+ // See https://bugs.webkit.org/show_bug.cgi?id=164220.
+ test->m_shouldDelayDecideDestination = true;
</ins><span class="cx"> test->loadURI(requestURI.data());
</span><span class="cx"> test->waitUntilDownloadStarted();
</span><span class="cx">
</span><span class="lines">@@ -614,6 +619,25 @@
</span><span class="cx"> ASSERT_CMP_CSTRING(webkit_uri_request_get_uri(request), ==, requestURI);
</span><span class="cx">
</span><span class="cx"> g_assert(test->m_webView == webkit_download_get_web_view(test->m_download.get()));
</span><ins>+ test->waitUntilDownloadFinished();
+
+ GRefPtr<GFile> downloadFile = adoptGRef(g_file_new_for_uri(webkit_download_get_destination(test->m_download.get())));
+ GRefPtr<GFileInfo> downloadFileInfo = adoptGRef(g_file_query_info(downloadFile.get(), G_FILE_ATTRIBUTE_STANDARD_SIZE, static_cast<GFileQueryInfoFlags>(0), nullptr, nullptr));
+ g_assert_cmpint(g_file_info_get_size(downloadFileInfo.get()), >, 0);
+ g_file_delete(downloadFile.get(), nullptr, nullptr);
+}
+
+static void testPolicyResponseDownloadCancel(PolicyResponseDownloadTest* test, gconstpointer)
+{
+ CString requestURI = kServer->getURIForPath("/test.pdf").data();
+ test->loadURI(requestURI.data());
+ test->waitUntilDownloadStarted();
+
+ WebKitURIRequest* request = webkit_download_get_request(test->m_download.get());
+ g_assert(request);
+ ASSERT_CMP_CSTRING(webkit_uri_request_get_uri(request), ==, requestURI);
+
+ g_assert(test->m_webView == webkit_download_get_web_view(test->m_download.get()));
</ins><span class="cx"> test->cancelDownloadAndWaitUntilFinished();
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -657,6 +681,7 @@
</span><span class="cx"> DownloadErrorTest::add("Downloads", "remote-file-error", testDownloadRemoteFileError);
</span><span class="cx"> WebViewDownloadTest::add("WebKitWebView", "download-uri", testWebViewDownloadURI);
</span><span class="cx"> PolicyResponseDownloadTest::add("Downloads", "policy-decision-download", testPolicyResponseDownload);
</span><ins>+ PolicyResponseDownloadTest::add("Downloads", "policy-decision-download-cancel", testPolicyResponseDownloadCancel);
</ins><span class="cx"> DownloadTest::add("Downloads", "mime-type", testDownloadMIMEType);
</span><span class="cx"> }
</span><span class="cx">
</span></span></pre>
</div>
</div>
</body>
</html>