Modified: trunk/Source/WebKit2/ChangeLog (208292 => 208293)
--- trunk/Source/WebKit2/ChangeLog 2016-11-02 18:11:13 UTC (rev 208292)
+++ trunk/Source/WebKit2/ChangeLog 2016-11-02 18:24:18 UTC (rev 208293)
@@ -1,3 +1,33 @@
+2016-11-02 Carlos Garcia Campos <[email protected]>
+
+ 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/NetworkConnectionToWebProcess.cpp:
+ (WebKit::NetworkConnectionToWebProcess::removeLoadIdentifier): Update ASSERT, because abort doesn't cleanup the
+ resource loader in case it's becoming a download.
+ * NetworkProcess/NetworkResourceLoader.cpp:
+ (WebKit::NetworkResourceLoader::didBecomeDownload): Call cleanup() instead of just deleting the network load.
+ (WebKit::NetworkResourceLoader::isBecomingDownload): Helper method to check if the resource load was converted to a
+ download, but didBecomeDownload() hasn't been called yet.
+ (WebKit::NetworkResourceLoader::abort): If the resource load is becoming a download do not call cleanup()
+ because it will be called by didBecomeDownload() later.
+ * NetworkProcess/NetworkResourceLoader.h:
+
2016-11-02 David Kilzer <[email protected]>
Add logging for "WebKit encountered an internal error" messages
Modified: trunk/Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp (208292 => 208293)
--- trunk/Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp 2016-11-02 18:11:13 UTC (rev 208292)
+++ trunk/Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp 2016-11-02 18:24:18 UTC (rev 208293)
@@ -158,7 +158,7 @@
// Abort the load now, as the WebProcess won't be able to respond to messages any more which might lead
// to leaked loader resources (connections, threads, etc).
loader->abort();
- ASSERT(!m_networkResourceLoaders.contains(identifier));
+ ASSERT(!m_networkResourceLoaders.contains(identifier) || loader->isBecomingDownload());
}
void NetworkConnectionToWebProcess::setDefersLoading(ResourceLoadIdentifier identifier, bool defers)
Modified: trunk/Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp (208292 => 208293)
--- trunk/Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp 2016-11-02 18:11:13 UTC (rev 208292)
+++ trunk/Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp 2016-11-02 18:24:18 UTC (rev 208293)
@@ -286,10 +286,19 @@
{
ASSERT(m_didConvertToDownload);
ASSERT(m_networkLoad);
- m_networkLoad = nullptr;
+ cleanup();
}
#endif
+bool NetworkResourceLoader::isBecomingDownload() const
+{
+#if USE(NETWORK_SESSION)
+ return m_networkLoad && m_didConvertToDownload;
+#else
+ return false;
+#endif
+}
+
void NetworkResourceLoader::abort()
{
ASSERT(RunLoop::isMain());
@@ -308,6 +317,9 @@
m_networkLoad->cancel();
}
+ if (isBecomingDownload())
+ return;
+
cleanup();
}
Modified: trunk/Tools/ChangeLog (208292 => 208293)
--- trunk/Tools/ChangeLog 2016-11-02 18:11:13 UTC (rev 208292)
+++ trunk/Tools/ChangeLog 2016-11-02 18:24:18 UTC (rev 208293)
@@ -1,3 +1,20 @@
+2016-11-02 Carlos Garcia Campos <[email protected]>
+
+ 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):
+
2016-11-01 Alex Christensen <[email protected]>
Remove PassRefPtr from DumpRenderTree
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp (208292 => 208293)
--- trunk/Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp 2016-11-02 18:11:13 UTC (rev 208292)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp 2016-11-02 18:24:18 UTC (rev 208293)
@@ -515,6 +515,8 @@
{
test->m_download = download;
test->assertObjectIsDeletedWhenTestFinishes(G_OBJECT(download));
+ g_signal_connect(download, "decide-destination", G_CALLBACK(downloadDecideDestinationCallback), test);
+ g_signal_connect(download, "finished", G_CALLBACK(downloadFinishedCallback), test);
test->quitMainLoop();
}
@@ -539,6 +541,8 @@
{
GUniquePtr<char> destination(g_build_filename(Test::dataDirectory(), suggestedFilename, nullptr));
GUniquePtr<char> destinationURI(g_filename_to_uri(destination.get(), 0, 0));
+ if (test->m_shouldDelayDecideDestination)
+ g_usleep(0.2 * G_USEC_PER_SEC);
webkit_download_set_destination(download, destinationURI.get());
return TRUE;
}
@@ -550,12 +554,11 @@
void waitUntilDownloadFinished()
{
- g_signal_connect(m_download.get(), "decide-destination", G_CALLBACK(downloadDecideDestinationCallback), this);
- g_signal_connect(m_download.get(), "finished", G_CALLBACK(downloadFinishedCallback), this);
g_main_loop_run(m_mainLoop);
}
GRefPtr<WebKitDownload> m_download;
+ bool m_shouldDelayDecideDestination { false };
};
static void testWebViewDownloadURI(WebViewDownloadTest* test, gconstpointer)
@@ -604,8 +607,10 @@
static void testPolicyResponseDownload(PolicyResponseDownloadTest* test, gconstpointer)
{
- // Test that a download started by the the policy checker contains the web view.
CString requestURI = kServer->getURIForPath("/test.pdf").data();
+ // 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;
test->loadURI(requestURI.data());
test->waitUntilDownloadStarted();
@@ -614,6 +619,25 @@
ASSERT_CMP_CSTRING(webkit_uri_request_get_uri(request), ==, requestURI);
g_assert(test->m_webView == webkit_download_get_web_view(test->m_download.get()));
+ 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()));
test->cancelDownloadAndWaitUntilFinished();
}
@@ -657,6 +681,7 @@
DownloadErrorTest::add("Downloads", "remote-file-error", testDownloadRemoteFileError);
WebViewDownloadTest::add("WebKitWebView", "download-uri", testWebViewDownloadURI);
PolicyResponseDownloadTest::add("Downloads", "policy-decision-download", testPolicyResponseDownload);
+ PolicyResponseDownloadTest::add("Downloads", "policy-decision-download-cancel", testPolicyResponseDownloadCancel);
DownloadTest::add("Downloads", "mime-type", testDownloadMIMEType);
}
