Bug 136322 – [SOUP] WebKitDownload cannot overwrite existing file

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

If the destination of a WebKitDownload is a file that already exists, the download will always fail. This is awkward because some browsers want to allow users to save a downloaded file over an existing file.  For example, in Epiphany if you use the Save Image As feature (right click on an image) and try to save over a file that already exists, we have a dialog box to confirm that the user really wants to overwrite the file -> user confirms he wants to overwrite the file -> download fails with error message "file already exists."

[Michael Catanzaro]

Created attachment 237271 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 237271 [details]
Patch

Not sure we unconditionally want to overwrite an existing file, we should be careful with destructive operations.

[Carlos Garcia Campos]

Comment on attachment 237271 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=237271&action=review

Doing this unconditionally might be dangerous. I think it might make sense to allow overwrite when the destination has been set by the application explicitly, since apps are supposed to handle that situation when deciding the destination name, but we should document that. When the destination is decided automatically by WebKit, I'm not sure sure it's a good idea to overwrite existing file, though. The safest way would be to add API for that something like webkit_download_set_allow_overwrite()

> Source/WebKit2/ChangeLog:3
> +        [GTK] WebKitDownload cannot overwrite existing file

This is not a GTK specific issue, it affects all ports using libsoup as network backend.

> Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp:140
> -        if (!g_file_move(m_intermediateFile.get(), destinationFile.get(), G_FILE_COPY_NONE, nullptr, nullptr, nullptr, &error.outPtr())) {
> +        if (!g_file_move(m_intermediateFile.get(), destinationFile.get(), G_FILE_COPY_OVERWRITE, nullptr, nullptr, nullptr, &error.outPtr())) {

I think this should be done using the allowOverwrite parameter of DecideDestinationWithSuggestedFilename message, so that clients can decide and it won't affect EFL in this case.

[Carlos Garcia Campos]

So, I would split this patch. In this bug I would make sure we use the allowOverwrite parameter that we are currently ignoring, adding G_FILE_COPY_OVERWRITE flag when it's true (it will be false anyway, so there's no change in behaviour). And then I would open a new bug for the GTK port to decide how to handle overwrites in the GTK+ API.

[Michael Catanzaro]

Created attachment 237352 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 237352 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=237352&action=review

Thanks, I have a few minor comments.

> Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp:91
> -        m_destinationURI = m_download->decideDestinationWithSuggestedFilename(suggestedFilename, overwrite);
> +        m_destinationURI = m_download->decideDestinationWithSuggestedFilename(suggestedFilename, m_overwriteDestination);

Nit: I would use m_allowOverwrite for consistency with the IPC message param name.

> Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp:140
> -        if (!g_file_move(m_intermediateFile.get(), destinationFile.get(), G_FILE_COPY_NONE, nullptr, nullptr, nullptr, &error.outPtr())) {
> +        if (!g_file_move(m_intermediateFile.get(), destinationFile.get(), flags, nullptr, nullptr, nullptr, &error.outPtr())) {

I think you can add the if directly here, without the local variable 

if (!g_file_move(m_intermediateFile.get(), destinationFile.get(), m_allowOverwrite ? G_FILE_COPY_OVERWRITE : G_FILE_COPY_NONE, nullptr, nullptr, nullptr, &error.outPtr())) {

> Source/WebKit2/Shared/Downloads/soup/DownloadSoup.cpp:200
> +    bool m_overwriteDestination = false;

Initialize the member in the constructor.

[Michael Catanzaro]

Created attachment 237430 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 237430 [details]
Patch

Clearing flags on attachment: 237430

Committed r173154: <http://trac.webkit.org/changeset/173154>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.