[SOUP] Partial file left on disk after a download fails or is cancelled in WebKit2
authorcarlosgc@webkit.org <carlosgc@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 9 Jan 2014 16:08:48 +0000 (16:08 +0000)
committercarlosgc@webkit.org <carlosgc@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 9 Jan 2014 16:08:48 +0000 (16:08 +0000)
https://bugs.webkit.org/show_bug.cgi?id=126686

Reviewed by Martin Robinson.

Source/WebKit2:

We are currently writing the downloads directly into the
destination, and when a download fails or is cancelled after the
destination has been decided, the partial file is left on the
disk. Deleting the final file is not safe because there might be a
race condition, so we can use an intermediate file like other
browsers do, a file in the same directory than the target
destination but with .wkdownload suffix, that is removed when the
download fails or is cancelled. If the download finishes
successfully the intermediate file is renamed to the final
destination.

* Shared/Downloads/soup/DownloadSoup.cpp:
(WebKit::DownloadClient::deleteIntermediateFileInNeeded): Delete
the intermdiate file if it's been created already.
(WebKit::DownloadClient::downloadFailed): Call deleteIntermediateFileInNeeded.
(WebKit::DownloadClient::didReceiveResponse): Do not create a
SoupMessage for the given ResourceResponse that is not used, cache
the ResourceResponse instead. Create the intermediate file and use
it instead of the final destination.
(WebKit::DownloadClient::didReceiveData): Use the cached
ResourceResponse directly.
(WebKit::DownloadClient::didFinishLoading): Rename the
intermediate file to the final destination and write the metadata
in the final target destination.
(WebKit::DownloadClient::cancel): Handle the download cancellation
here, removing the intermediate file is needed and cancelling the
ResourceHandle and the download.
(WebKit::DownloadClient::handleResponseLater):
(WebKit::Download::cancel): Let the client handle the cancellation.

Tools:

Test that partial files are not left on disk after a download has
been cancelled after the destination has been decided. To make
sure the download is cancelled after the destination has been
decided and before the operation finishes, we cancel the download
in the destination decided callback, and we use an infinite
resource that writes chunks to the response body and never
completes the body.

* TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:
(addContentDispositionHTTPHeaderToResponse): Helper function to
add the Content-Disposition to the response headers.
(writeNextChunkIdle): Write next chunk to response body.
(writeNextChunk): Write next chunk in an idle to avoid flooding
the network with the inifnite resource.
(serverCallback): Add an inifinite resource.
(testDownloadRemoteFileError): Check that partial file is not
present after the download has been cancelled.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@161555 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit2/ChangeLog
Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp

index 9e1fef8..aba21ca 100644 (file)
@@ -1,3 +1,40 @@
+2014-01-09  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        [SOUP] Partial file left on disk after a download fails or is cancelled in WebKit2
+        https://bugs.webkit.org/show_bug.cgi?id=126686
+
+        Reviewed by Martin Robinson.
+
+        We are currently writing the downloads directly into the
+        destination, and when a download fails or is cancelled after the
+        destination has been decided, the partial file is left on the
+        disk. Deleting the final file is not safe because there might be a
+        race condition, so we can use an intermediate file like other
+        browsers do, a file in the same directory than the target
+        destination but with .wkdownload suffix, that is removed when the
+        download fails or is cancelled. If the download finishes
+        successfully the intermediate file is renamed to the final
+        destination.
+
+        * Shared/Downloads/soup/DownloadSoup.cpp:
+        (WebKit::DownloadClient::deleteIntermediateFileInNeeded): Delete
+        the intermdiate file if it's been created already.
+        (WebKit::DownloadClient::downloadFailed): Call deleteIntermediateFileInNeeded.
+        (WebKit::DownloadClient::didReceiveResponse): Do not create a
+        SoupMessage for the given ResourceResponse that is not used, cache
+        the ResourceResponse instead. Create the intermediate file and use
+        it instead of the final destination.
+        (WebKit::DownloadClient::didReceiveData): Use the cached
+        ResourceResponse directly.
+        (WebKit::DownloadClient::didFinishLoading): Rename the
+        intermediate file to the final destination and write the metadata
+        in the final target destination.
+        (WebKit::DownloadClient::cancel): Handle the download cancellation
+        here, removing the intermediate file is needed and cancelling the
+        ResourceHandle and the download.
+        (WebKit::DownloadClient::handleResponseLater):
+        (WebKit::Download::cancel): Let the client handle the cancellation.
+
 2014-01-08  Jinwoo Song  <jinwoo7.song@samsung.com>
 
         WebKit2 EFL build fix after r161530
index 202c1cb..28b1dcd 100644 (file)
@@ -59,14 +59,22 @@ public:
             g_source_remove(m_handleResponseLaterID);
     }
 
+    void deleteIntermediateFileInNeeded()
+    {
+        if (!m_intermediateFile)
+            return;
+        g_file_delete(m_intermediateFile.get(), nullptr, nullptr);
+    }
+
     void downloadFailed(const ResourceError& error)
     {
+        deleteIntermediateFileInNeeded();
         m_download->didFail(error, IPC::DataReference());
     }
 
     void didReceiveResponse(ResourceHandle*, const ResourceResponse& response)
     {
-        m_response = adoptGRef(response.toSoupMessage());
+        m_response = response;
         m_download->didReceiveResponse(response);
 
         if (response.httpStatusCode() >= 400) {
@@ -83,8 +91,8 @@ public:
         }
 
         bool overwrite;
-        String destinationURI = m_download->decideDestinationWithSuggestedFilename(suggestedFilename, overwrite);
-        if (destinationURI.isEmpty()) {
+        m_destinationURI = m_download->decideDestinationWithSuggestedFilename(suggestedFilename, overwrite);
+        if (m_destinationURI.isEmpty()) {
 #if PLATFORM(GTK)
             GOwnPtr<char> buffer(g_strdup_printf(_("Cannot determine destination URI for download with suggested filename %s"), suggestedFilename.utf8().data()));
             String errorMessage = String::fromUTF8(buffer.get());
@@ -95,21 +103,16 @@ public:
             return;
         }
 
-        GRefPtr<GFile> file = adoptGRef(g_file_new_for_uri(destinationURI.utf8().data()));
+        String intermediateURI = m_destinationURI + ".wkdownload";
+        m_intermediateFile = adoptGRef(g_file_new_for_uri(intermediateURI.utf8().data()));
         GOwnPtr<GError> error;
-        m_outputStream = adoptGRef(g_file_replace(file.get(), 0, TRUE, G_FILE_CREATE_NONE, 0, &error.outPtr()));
+        m_outputStream = adoptGRef(g_file_replace(m_intermediateFile.get(), 0, TRUE, G_FILE_CREATE_NONE, 0, &error.outPtr()));
         if (!m_outputStream) {
             downloadFailed(platformDownloadDestinationError(response, error->message));
             return;
         }
 
-        GRefPtr<GFileInfo> info = adoptGRef(g_file_info_new());
-        const char* uri = response.url().string().utf8().data();
-        g_file_info_set_attribute_string(info.get(), "metadata::download-uri", uri);
-        g_file_info_set_attribute_string(info.get(), "xattr::xdg.origin.url", uri);
-        g_file_set_attributes_async(file.get(), info.get(), G_FILE_QUERY_INFO_NONE, G_PRIORITY_DEFAULT, 0, 0, 0);
-
-        m_download->didCreateDestination(destinationURI);
+        m_download->didCreateDestination(m_destinationURI);
     }
 
     void didReceiveData(ResourceHandle*, const char* data, unsigned length, int /*encodedDataLength*/)
@@ -123,7 +126,7 @@ public:
         GOwnPtr<GError> error;
         g_output_stream_write_all(G_OUTPUT_STREAM(m_outputStream.get()), data, length, &bytesWritten, 0, &error.outPtr());
         if (error) {
-            downloadFailed(platformDownloadDestinationError(ResourceResponse(m_response.get()), error->message));
+            downloadFailed(platformDownloadDestinationError(m_response, error->message));
             return;
         }
         m_download->didReceiveData(bytesWritten);
@@ -132,6 +135,21 @@ public:
     void didFinishLoading(ResourceHandle*, double)
     {
         m_outputStream = 0;
+
+        ASSERT(m_intermediateFile);
+        GRefPtr<GFile> destinationFile = adoptGRef(g_file_new_for_uri(m_destinationURI.utf8().data()));
+        GOwnPtr<GError> error;
+        if (!g_file_move(m_intermediateFile.get(), destinationFile.get(), G_FILE_COPY_NONE, nullptr, nullptr, nullptr, &error.outPtr())) {
+            downloadFailed(platformDownloadDestinationError(m_response, error->message));
+            return;
+        }
+
+        GRefPtr<GFileInfo> info = adoptGRef(g_file_info_new());
+        CString uri = m_response.url().string().utf8();
+        g_file_info_set_attribute_string(info.get(), "metadata::download-uri", uri.data());
+        g_file_info_set_attribute_string(info.get(), "xattr::xdg.origin.url", uri.data());
+        g_file_set_attributes_async(destinationFile.get(), info.get(), G_FILE_QUERY_INFO_NONE, G_PRIORITY_DEFAULT, nullptr, nullptr, nullptr);
+
         m_download->didFinish();
     }
 
@@ -150,6 +168,13 @@ public:
         notImplemented();
     }
 
+    void cancel(ResourceHandle* handle)
+    {
+        handle->cancel();
+        deleteIntermediateFileInNeeded();
+        m_download->didCancel(IPC::DataReference());
+    }
+
     void handleResponse()
     {
         m_handleResponseLaterID = 0;
@@ -164,7 +189,7 @@ public:
 
     void handleResponseLater(const ResourceResponse& response)
     {
-        ASSERT(!m_response);
+        ASSERT(m_response.isNull());
         ASSERT(!m_handleResponseLaterID);
 
         m_delayedResponse = response;
@@ -176,7 +201,9 @@ public:
 
     Download* m_download;
     GRefPtr<GFileOutputStream> m_outputStream;
-    GRefPtr<SoupMessage> m_response;
+    ResourceResponse m_response;
+    String m_destinationURI;
+    GRefPtr<GFile> m_intermediateFile;
     ResourceResponse m_delayedResponse;
     unsigned m_handleResponseLaterID;
 };
@@ -205,8 +232,7 @@ void Download::cancel()
 {
     if (!m_resourceHandle)
         return;
-    m_resourceHandle->cancel();
-    didCancel(IPC::DataReference());
+    static_cast<DownloadClient*>(m_downloadClient.get())->cancel(m_resourceHandle.get());
     m_resourceHandle = 0;
 }
 
index e560e7d..d06f900 100644 (file)
@@ -1,3 +1,28 @@
+2014-01-09  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        [SOUP] Partial file left on disk after a download fails or is cancelled in WebKit2
+        https://bugs.webkit.org/show_bug.cgi?id=126686
+
+        Reviewed by Martin Robinson.
+
+        Test that partial files are not left on disk after a download has
+        been cancelled after the destination has been decided. To make
+        sure the download is cancelled after the destination has been
+        decided and before the operation finishes, we cancel the download
+        in the destination decided callback, and we use an infinite
+        resource that writes chunks to the response body and never
+        completes the body.
+
+        * TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:
+        (addContentDispositionHTTPHeaderToResponse): Helper function to
+        add the Content-Disposition to the response headers.
+        (writeNextChunkIdle): Write next chunk to response body.
+        (writeNextChunk): Write next chunk in an idle to avoid flooding
+        the network with the inifnite resource.
+        (serverCallback): Add an inifinite resource.
+        (testDownloadRemoteFileError): Check that partial file is not
+        present after the download has been cancelled.
+
 2014-01-09  Roland Takacs  <rtakacs@inf.u-szeged.hu>
 
         Move myself to the committers list.
index ce15990..867824f 100644 (file)
@@ -223,13 +223,14 @@ public:
     void receivedResponse(WebKitDownload* download)
     {
         DownloadTest::receivedResponse(download);
-        if (m_expectedError == WEBKIT_DOWNLOAD_ERROR_CANCELLED_BY_USER)
-            webkit_download_cancel(download);
     }
 
     void createdDestination(WebKitDownload* download, const char* destination)
     {
-        g_assert_not_reached();
+        if (m_expectedError == WEBKIT_DOWNLOAD_ERROR_CANCELLED_BY_USER)
+            webkit_download_cancel(download);
+        else
+            g_assert_not_reached();
     }
 
     void failed(WebKitDownload* download, GError* error)
@@ -297,6 +298,23 @@ static void testDownloadLocalFileError(DownloadErrorTest* test, gconstpointer)
 static WebKitTestServer* kServer;
 static const char* kServerSuggestedFilename = "webkit-downloaded-file";
 
+static void addContentDispositionHTTPHeaderToResponse(SoupMessage* message)
+{
+    GOwnPtr<char> contentDisposition(g_strdup_printf("filename=%s", kServerSuggestedFilename));
+    soup_message_headers_append(message->response_headers, "Content-Disposition", contentDisposition.get());
+}
+
+static gboolean writeNextChunkIdle(SoupMessage* message)
+{
+    soup_message_body_append(message->response_body, SOUP_MEMORY_STATIC, "chunk", 5);
+    return FALSE;
+}
+
+static void writeNextChunk(SoupMessage* message)
+{
+    g_timeout_add(100, reinterpret_cast<GSourceFunc>(writeNextChunkIdle), message);
+}
+
 static void serverCallback(SoupServer* server, SoupMessage* message, const char* path, GHashTable*, SoupClientContext*, gpointer)
 {
     if (message->method != SOUP_METHOD_GET) {
@@ -304,6 +322,17 @@ static void serverCallback(SoupServer* server, SoupMessage* message, const char*
         return;
     }
 
+    soup_message_set_status(message, SOUP_STATUS_OK);
+
+    if (g_str_equal(path, "/cancel-after-destination")) {
+        // Use an infinite message to make sure it's cancelled before it finishes.
+        soup_message_headers_set_encoding(message->response_headers, SOUP_ENCODING_CHUNKED);
+        addContentDispositionHTTPHeaderToResponse(message);
+        g_signal_connect(message, "wrote_headers", G_CALLBACK(writeNextChunk), nullptr);
+        g_signal_connect(message, "wrote_chunk", G_CALLBACK(writeNextChunk), nullptr);
+        return;
+    }
+
     GOwnPtr<char> filePath(g_build_filename(Test::getWebKit1TestResoucesDir().data(), path, NULL));
     char* contents;
     gsize contentsLength;
@@ -313,10 +342,7 @@ static void serverCallback(SoupServer* server, SoupMessage* message, const char*
         return;
     }
 
-    soup_message_set_status(message, SOUP_STATUS_OK);
-
-    GOwnPtr<char> contentDisposition(g_strdup_printf("filename=%s", kServerSuggestedFilename));
-    soup_message_headers_append(message->response_headers, "Content-Disposition", contentDisposition.get());
+    addContentDispositionHTTPHeaderToResponse(message);
     soup_message_body_append(message->response_body, SOUP_MEMORY_TAKE, contents, contentsLength);
 
     soup_message_body_complete(message->response_body);
@@ -376,7 +402,7 @@ static void testDownloadRemoteFileError(DownloadErrorTest* test, gconstpointer)
     test->checkDestinationAndDeleteFile(download.get(), "bar");
 
     test->m_expectedError = WEBKIT_DOWNLOAD_ERROR_CANCELLED_BY_USER;
-    download = adoptGRef(test->downloadURIAndWaitUntilFinishes(kServer->getURIForPath("/test.pdf")));
+    download = adoptGRef(test->downloadURIAndWaitUntilFinishes(kServer->getURIForPath("/cancel-after-destination")));
     g_assert(!webkit_download_get_web_view(download.get()));
 
     g_assert_cmpint(events.size(), ==, 4);
@@ -386,7 +412,10 @@ static void testDownloadRemoteFileError(DownloadErrorTest* test, gconstpointer)
     g_assert_cmpint(events[3], ==, DownloadTest::Finished);
     events.clear();
     g_assert_cmpfloat(webkit_download_get_estimated_progress(download.get()), <, 1);
-    test->checkDestinationAndDeleteFile(download.get(), kServerSuggestedFilename);
+    // Check the intermediate file is deleted when the download is cancelled.
+    GOwnPtr<char> intermediateURI(g_strdup_printf("%s.wkdownload", webkit_download_get_destination(download.get())));
+    GRefPtr<GFile> intermediateFile = adoptGRef(g_file_new_for_uri(intermediateURI.get()));
+    g_assert(!g_file_query_exists(intermediateFile.get(), nullptr));
 }
 
 class WebViewDownloadTest: public WebViewTest {