Changeset 161555 in webkit


Ignore:
Timestamp:
Jan 9, 2014 8:08:48 AM (10 years ago)
Author:
Carlos Garcia Campos
Message:

[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.

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.

Location:
trunk
Files:
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit2/ChangeLog

    r161548 r161555  
     12014-01-09  Carlos Garcia Campos  <cgarcia@igalia.com>
     2
     3        [SOUP] Partial file left on disk after a download fails or is cancelled in WebKit2
     4        https://bugs.webkit.org/show_bug.cgi?id=126686
     5
     6        Reviewed by Martin Robinson.
     7
     8        We are currently writing the downloads directly into the
     9        destination, and when a download fails or is cancelled after the
     10        destination has been decided, the partial file is left on the
     11        disk. Deleting the final file is not safe because there might be a
     12        race condition, so we can use an intermediate file like other
     13        browsers do, a file in the same directory than the target
     14        destination but with .wkdownload suffix, that is removed when the
     15        download fails or is cancelled. If the download finishes
     16        successfully the intermediate file is renamed to the final
     17        destination.
     18
     19        * Shared/Downloads/soup/DownloadSoup.cpp:
     20        (WebKit::DownloadClient::deleteIntermediateFileInNeeded): Delete
     21        the intermdiate file if it's been created already.
     22        (WebKit::DownloadClient::downloadFailed): Call deleteIntermediateFileInNeeded.
     23        (WebKit::DownloadClient::didReceiveResponse): Do not create a
     24        SoupMessage for the given ResourceResponse that is not used, cache
     25        the ResourceResponse instead. Create the intermediate file and use
     26        it instead of the final destination.
     27        (WebKit::DownloadClient::didReceiveData): Use the cached
     28        ResourceResponse directly.
     29        (WebKit::DownloadClient::didFinishLoading): Rename the
     30        intermediate file to the final destination and write the metadata
     31        in the final target destination.
     32        (WebKit::DownloadClient::cancel): Handle the download cancellation
     33        here, removing the intermediate file is needed and cancelling the
     34        ResourceHandle and the download.
     35        (WebKit::DownloadClient::handleResponseLater):
     36        (WebKit::Download::cancel): Let the client handle the cancellation.
     37
    1382014-01-08  Jinwoo Song  <jinwoo7.song@samsung.com>
    239

  • trunk/Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp

    r161536 r161555  
    6060    }
    6161
     62    void deleteIntermediateFileInNeeded()
     63    {
     64        if (!m_intermediateFile)
     65            return;
     66        g_file_delete(m_intermediateFile.get(), nullptr, nullptr);
     67    }
     68
    6269    void downloadFailed(const ResourceError& error)
    6370    {
     71        deleteIntermediateFileInNeeded();
    6472        m_download->didFail(error, IPC::DataReference());
    6573    }
    … …  
    6775    void didReceiveResponse(ResourceHandle*, const ResourceResponse& response)
    6876    {
    69         m_response = adoptGRef(response.toSoupMessage());
     77        m_response = response;
    7078        m_download->didReceiveResponse(response);
    7179
    … …  
    8492
    8593        bool overwrite;
    86         String destinationURI = m_download->decideDestinationWithSuggestedFilename(suggestedFilename, overwrite);
    87         if (destinationURI.isEmpty()) {
     94        m_destinationURI = m_download->decideDestinationWithSuggestedFilename(suggestedFilename, overwrite);
     95        if (m_destinationURI.isEmpty()) {
    8896#if PLATFORM(GTK)
    8997            GOwnPtr<char> buffer(g_strdup_printf(_("Cannot determine destination URI for download with suggested filename %s"), suggestedFilename.utf8().data()));
    … …  
    96104        }
    97105
    98         GRefPtr<GFile> file = adoptGRef(g_file_new_for_uri(destinationURI.utf8().data()));
     106        String intermediateURI = m_destinationURI + ".wkdownload";
     107        m_intermediateFile = adoptGRef(g_file_new_for_uri(intermediateURI.utf8().data()));
    99108        GOwnPtr<GError> error;
    100         m_outputStream = adoptGRef(g_file_replace(file.get(), 0, TRUE, G_FILE_CREATE_NONE, 0, &error.outPtr()));
     109        m_outputStream = adoptGRef(g_file_replace(m_intermediateFile.get(), 0, TRUE, G_FILE_CREATE_NONE, 0, &error.outPtr()));
    101110        if (!m_outputStream) {
    102111            downloadFailed(platformDownloadDestinationError(response, error->message));
    … …  
    104113        }
    105114
    106         GRefPtr<GFileInfo> info = adoptGRef(g_file_info_new());
    107         const char* uri = response.url().string().utf8().data();
    108         g_file_info_set_attribute_string(info.get(), "metadata::download-uri", uri);
    109         g_file_info_set_attribute_string(info.get(), "xattr::xdg.origin.url", uri);
    110         g_file_set_attributes_async(file.get(), info.get(), G_FILE_QUERY_INFO_NONE, G_PRIORITY_DEFAULT, 0, 0, 0);
    111 
    112         m_download->didCreateDestination(destinationURI);
     115        m_download->didCreateDestination(m_destinationURI);
    113116    }
    114117
    … …  
    124127        g_output_stream_write_all(G_OUTPUT_STREAM(m_outputStream.get()), data, length, &bytesWritten, 0, &error.outPtr());
    125128        if (error) {
    126             downloadFailed(platformDownloadDestinationError(ResourceResponse(m_response.get()), error->message));
     129            downloadFailed(platformDownloadDestinationError(m_response, error->message));
    127130            return;
    128131        }
    … …  
    133136    {
    134137        m_outputStream = 0;
     138
     139        ASSERT(m_intermediateFile);
     140        GRefPtr<GFile> destinationFile = adoptGRef(g_file_new_for_uri(m_destinationURI.utf8().data()));
     141        GOwnPtr<GError> error;
     142        if (!g_file_move(m_intermediateFile.get(), destinationFile.get(), G_FILE_COPY_NONE, nullptr, nullptr, nullptr, &error.outPtr())) {
     143            downloadFailed(platformDownloadDestinationError(m_response, error->message));
     144            return;
     145        }
     146
     147        GRefPtr<GFileInfo> info = adoptGRef(g_file_info_new());
     148        CString uri = m_response.url().string().utf8();
     149        g_file_info_set_attribute_string(info.get(), "metadata::download-uri", uri.data());
     150        g_file_info_set_attribute_string(info.get(), "xattr::xdg.origin.url", uri.data());
     151        g_file_set_attributes_async(destinationFile.get(), info.get(), G_FILE_QUERY_INFO_NONE, G_PRIORITY_DEFAULT, nullptr, nullptr, nullptr);
     152
    135153        m_download->didFinish();
    136154    }
    … …  
    149167    {
    150168        notImplemented();
     169    }
     170
     171    void cancel(ResourceHandle* handle)
     172    {
     173        handle->cancel();
     174        deleteIntermediateFileInNeeded();
     175        m_download->didCancel(IPC::DataReference());
    151176    }
    152177
    … …  
    165190    void handleResponseLater(const ResourceResponse& response)
    166191    {
    167         ASSERT(!m_response);
     192        ASSERT(m_response.isNull());
    168193        ASSERT(!m_handleResponseLaterID);
    169194
    … …  
    177202    Download* m_download;
    178203    GRefPtr<GFileOutputStream> m_outputStream;
    179     GRefPtr<SoupMessage> m_response;
     204    ResourceResponse m_response;
     205    String m_destinationURI;
     206    GRefPtr<GFile> m_intermediateFile;
    180207    ResourceResponse m_delayedResponse;
    181208    unsigned m_handleResponseLaterID;
    … …  
    206233    if (!m_resourceHandle)
    207234        return;
    208     m_resourceHandle->cancel();
    209     didCancel(IPC::DataReference());
     235    static_cast<DownloadClient*>(m_downloadClient.get())->cancel(m_resourceHandle.get());
    210236    m_resourceHandle = 0;
    211237}

  • trunk/Tools/ChangeLog

    r161552 r161555  
     12014-01-09  Carlos Garcia Campos  <cgarcia@igalia.com>
     2
     3        [SOUP] Partial file left on disk after a download fails or is cancelled in WebKit2
     4        https://bugs.webkit.org/show_bug.cgi?id=126686
     5
     6        Reviewed by Martin Robinson.
     7
     8        Test that partial files are not left on disk after a download has
     9        been cancelled after the destination has been decided. To make
     10        sure the download is cancelled after the destination has been
     11        decided and before the operation finishes, we cancel the download
     12        in the destination decided callback, and we use an infinite
     13        resource that writes chunks to the response body and never
     14        completes the body.
     15
     16        * TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:
     17        (addContentDispositionHTTPHeaderToResponse): Helper function to
     18        add the Content-Disposition to the response headers.
     19        (writeNextChunkIdle): Write next chunk to response body.
     20        (writeNextChunk): Write next chunk in an idle to avoid flooding
     21        the network with the inifnite resource.
     22        (serverCallback): Add an inifinite resource.
     23        (testDownloadRemoteFileError): Check that partial file is not
     24        present after the download has been cancelled.
     25
    1262014-01-09  Roland Takacs  <rtakacs@inf.u-szeged.hu>
    227

  • trunk/Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp

    r161366 r161555  
    224224    {
    225225        DownloadTest::receivedResponse(download);
     226    }
     227
     228    void createdDestination(WebKitDownload* download, const char* destination)
     229    {
    226230        if (m_expectedError == WEBKIT_DOWNLOAD_ERROR_CANCELLED_BY_USER)
    227231            webkit_download_cancel(download);
    228     }
    229 
    230     void createdDestination(WebKitDownload* download, const char* destination)
    231     {
    232         g_assert_not_reached();
     232        else
     233            g_assert_not_reached();
    233234    }
    234235
    … …  
    298299static const char* kServerSuggestedFilename = "webkit-downloaded-file";
    299300
     301static void addContentDispositionHTTPHeaderToResponse(SoupMessage* message)
     302{
     303    GOwnPtr<char> contentDisposition(g_strdup_printf("filename=%s", kServerSuggestedFilename));
     304    soup_message_headers_append(message->response_headers, "Content-Disposition", contentDisposition.get());
     305}
     306
     307static gboolean writeNextChunkIdle(SoupMessage* message)
     308{
     309    soup_message_body_append(message->response_body, SOUP_MEMORY_STATIC, "chunk", 5);
     310    return FALSE;
     311}
     312
     313static void writeNextChunk(SoupMessage* message)
     314{
     315    g_timeout_add(100, reinterpret_cast<GSourceFunc>(writeNextChunkIdle), message);
     316}
     317
    300318static void serverCallback(SoupServer* server, SoupMessage* message, const char* path, GHashTable*, SoupClientContext*, gpointer)
    301319{
    302320    if (message->method != SOUP_METHOD_GET) {
    303321        soup_message_set_status(message, SOUP_STATUS_NOT_IMPLEMENTED);
     322        return;
     323    }
     324
     325    soup_message_set_status(message, SOUP_STATUS_OK);
     326
     327    if (g_str_equal(path, "/cancel-after-destination")) {
     328        // Use an infinite message to make sure it's cancelled before it finishes.
     329        soup_message_headers_set_encoding(message->response_headers, SOUP_ENCODING_CHUNKED);
     330        addContentDispositionHTTPHeaderToResponse(message);
     331        g_signal_connect(message, "wrote_headers", G_CALLBACK(writeNextChunk), nullptr);
     332        g_signal_connect(message, "wrote_chunk", G_CALLBACK(writeNextChunk), nullptr);
    304333        return;
    305334    }
    … …  
    314343    }
    315344
    316     soup_message_set_status(message, SOUP_STATUS_OK);
    317 
    318     GOwnPtr<char> contentDisposition(g_strdup_printf("filename=%s", kServerSuggestedFilename));
    319     soup_message_headers_append(message->response_headers, "Content-Disposition", contentDisposition.get());
     345    addContentDispositionHTTPHeaderToResponse(message);
    320346    soup_message_body_append(message->response_body, SOUP_MEMORY_TAKE, contents, contentsLength);
    321347
    … …  
    377403
    378404    test->m_expectedError = WEBKIT_DOWNLOAD_ERROR_CANCELLED_BY_USER;
    379     download = adoptGRef(test->downloadURIAndWaitUntilFinishes(kServer->getURIForPath("/test.pdf")));
     405    download = adoptGRef(test->downloadURIAndWaitUntilFinishes(kServer->getURIForPath("/cancel-after-destination")));
    380406    g_assert(!webkit_download_get_web_view(download.get()));
    381407
    … …  
    387413    events.clear();
    388414    g_assert_cmpfloat(webkit_download_get_estimated_progress(download.get()), <, 1);
    389     test->checkDestinationAndDeleteFile(download.get(), kServerSuggestedFilename);
     415    // Check the intermediate file is deleted when the download is cancelled.
     416    GOwnPtr<char> intermediateURI(g_strdup_printf("%s.wkdownload", webkit_download_get_destination(download.get())));
     417    GRefPtr<GFile> intermediateFile = adoptGRef(g_file_new_for_uri(intermediateURI.get()));
     418    g_assert(!g_file_query_exists(intermediateFile.get(), nullptr));
    390419}
    391420
Note: See TracChangeset for help on using the changeset viewer.