Bug 136372 – [GTK] allow overwriting destination of download

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

[Michael Catanzaro]

When bug #136322 is fixed it will be possible for a download to overwrite an existing file on this. We should figure out if or how we want to expose this in the API. One possibility would be to implicitly allow overwrites if webkit_download_set_destination() is called. Another would be to add webkit_download_get_allow_overwrite() and webkit_download_set_allow_overwrite() or similarly-named functions.

[Michael Catanzaro]

Created attachment 237353 [details]
Patch

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[WebKit Commit Bot]

Attachment 237353 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitDownload.h"
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:207:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:209:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:210:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:211:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:212:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 237353 [details]
Patch

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

We need a test case in the unit tests for this new API, and new symbols in the docs, see http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

> Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:205
> +     */

Since: 2.6

> Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:213
> +    g_object_class_install_property(objectClass,
> +                                    PROP_ALLOW_OVERWRITE,
> +                                    g_param_spec_boolean("allow-overwrite",
> +                                                         _("Allow Overwrite"),
> +                                                         _("Whether the destination may be overwritten"),
> +                                                         FALSE,
> +                                                         WEBKIT_PARAM_READWRITE));
> +

You need to follow the coding style here regarding indentation.

> Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:406
> -CString webkitDownloadDecideDestinationWithSuggestedFilename(WebKitDownload* download, const CString& suggestedFilename)
> +CString webkitDownloadDecideDestinationWithSuggestedFilename(WebKitDownload* download, const CString& suggestedFilename, bool* allowOverwrite)

The public C API uses a pointer for the out parameter but here we can use a reference instead. We should move this to use API::DownloadClient at some point instead of the C API.

> Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:413
> +    if (allowOverwrite)
> +        *allowOverwrite = download->priv->allowOverwrite;

We should always receive a reference.

> Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:611
> + * Returns the current value of the #WebKitDownload::allow-overwrite property,

:: is for signals, for properties use a single :

> Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:615
> + * Returns: the current value of the #WebKitDownload::allow-overwrite property

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:629
> + * @allowed: the new value for the #WebKitDownload::allow-overwrite property

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:631
> + * Sets the #WebKitDownload::allow-overwrite property, which determines whether

Ditto.

[Michael Catanzaro]

Created attachment 237436 [details]
Patch

[Michael Catanzaro]

I'm concerned that we emit the "created-destination" signal after the temporary destination has been created, rather than after the final destination has been created. There doesn't seem to be any way to fix this without violating the documented behavior that it's emitted before "received-data" :/

[Michael Catanzaro]

(In reply to comment #6)
> I'm concerned that we emit the "created-destination" signal after the temporary destination has been created, rather than after the final destination has been created. There doesn't seem to be any way to fix this without violating the documented behavior that it's emitted before "received-data" :/

I guess we could create the destination early, potentially failing if it already exists, leave it empty, and copy over it later. Not sure why that didn't seem obvious to me earlier....

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #6)
> > I'm concerned that we emit the "created-destination" signal after the temporary destination has been created, rather than after the final destination has been created. There doesn't seem to be any way to fix this without violating the documented behavior that it's emitted before "received-data" :/
> 
> I guess we could create the destination early, potentially failing if it already exists, leave it empty, and copy over it later. Not sure why that didn't seem obvious to me earlier....

hmm, you are right, but in that case we would always use G_FILE_COPY_OVERWRITE for g_file_move() and when creating the file in didReceiveResponse() only when allowOverwrite is true. If that fails we would not even create the temporary file. That would also save the race condition of the destination doesn't exist when the download start, but exists when it finishes, and might be overwritten.

[Carlos Garcia Campos]

Comment on attachment 237436 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:159
> +        webkit_download_set_allow_overwrite(download, m_allowOverwrite);

Add an assert here to check that false is always the default and another assert after the set() to check that the get() now returns true.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:222
> +    const char* filename = "test.pdf";

static const char*

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:229
> +    const char* filename = "test.pdf";

static const char*

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:240
> +    const char* filename = "test.pdf";

static const char*

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:258
> +    Vector<DownloadTest::DownloadEvent>& events = test->m_downloadEvents;
> +    g_assert_cmpint(events.size(), ==, 6);
> +    g_assert_cmpint(events[0], ==, DownloadTest::Started);
> +    g_assert_cmpint(events[1], ==, DownloadTest::ReceivedResponse);
> +    g_assert_cmpint(events[2], ==, DownloadTest::CreatedDestination);
> +    g_assert_cmpint(events[3], ==, DownloadTest::ReceivedData);
> +    g_assert_cmpint(events[4], ==, DownloadTest::Failed);
> +    g_assert_cmpint(events[5], ==, DownloadTest::Finished);
> +    g_assert_cmpfloat(webkit_download_get_estimated_progress(additionalDownload.get()), ==, 1);

You should use a DownloadErrorTest to check the actual error is the expected one.

[Carlos Garcia Campos]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > I'm concerned that we emit the "created-destination" signal after the temporary destination has been created, rather than after the final destination has been created. There doesn't seem to be any way to fix this without violating the documented behavior that it's emitted before "received-data" :/
> > 
> > I guess we could create the destination early, potentially failing if it already exists, leave it empty, and copy over it later. Not sure why that didn't seem obvious to me earlier....
> 
> hmm, you are right, but in that case we would always use G_FILE_COPY_OVERWRITE for g_file_move() and when creating the file in didReceiveResponse() only when allowOverwrite is true. If that fails we would not even create the temporary file. That would also save the race condition of the destination doesn't exist when the download start, but exists when it finishes, and might be overwritten.

See https://bugs.webkit.org/show_bug.cgi?id=136423

[Michael Catanzaro]

Created attachment 237460 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 237460 [details]
Patch

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

Once patch in bug #136423 lands we should probably check also that the destination file exists when decided destination callback is called.

> Source/WebKit2/UIProcess/API/gtk/WebKitDownload.cpp:124
> +    case PROP_ALLOW_OVERWRITE:
> +        g_value_set_boolean(value, webkit_download_get_allow_overwrite(download));
>      default:

break;

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:162
> +        ASSERT(!webkit_download_get_allow_overwrite(download));
> +        webkit_download_set_allow_overwrite(download, m_allowOverwrite);
> +        ASSERT(webkit_download_get_allow_overwrite(download) == m_allowOverwrite);

Use g_assert() instead of ASSERT(), because ASSERT() is noop in release builds but we want unit tests to fail also in release builds.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:234
> +    static const char* filename = "test.pdf";
> +    GRefPtr<WebKitDownload> originalDownload = downloadLocalFileSuccessfully(test, filename);
> +    test->checkDestination(originalDownload.get(), filename);

I guess we could just create the file to ensure it exists instead of doing the download. That way we don't need to change the checkDestinationAndDeleteFile()

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:238
> +    test->m_allowOverwrite = true;
> +    GRefPtr<WebKitDownload> additionalDownload = downloadLocalFileSuccessfully(test, filename);
> +    test->checkDestinationAndDeleteFile(additionalDownload.get(), filename);

This should work when the destination file exists no matter if it was a previous download or created by us manually. I think it would simply the test

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:247
> +        , m_permitSuccess(false)

I would use m_expectedToFail, initialized to true

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:-222
> -    void receivedResponse(WebKitDownload* download)
> -    {
> -        DownloadTest::receivedResponse(download);
> -    }

Why did you delete this?

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:269
> -        if (m_expectedError != WEBKIT_DOWNLOAD_ERROR_DESTINATION) {
> +        if (m_permitSuccess || m_expectedError != WEBKIT_DOWNLOAD_ERROR_DESTINATION) {

Since now we are going to handle several cases of destination failure, I think we could use our own internal enum, for errors, possible including None (if we really need to handle of not failure). Then we could have InvalidDestination to set here and invalid dest, and DestinationExists, to set the right destination here

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:285
> +    static const char* filename = "test.pdf";
> +    test->m_permitSuccess = true;
> +    GRefPtr<WebKitDownload> originalDownload = downloadLocalFileSuccessfully(test, filename);
> +    test->checkDestination(originalDownload.get(), filename);

Same here about the download, just create the file and then you don't need the special case of a DownloadErrorTest that doesn't fail.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:288
> +    test->m_permitSuccess = false;

I don't think this is working, this combination makes the test set an invalid destination in decideDestination(), so this will fail because of the invalid destination and not because of the destination file already exists.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:303
> +    test->checkDestinationAndDeleteFile(originalDownload.get(), filename);

I've noticed we are using this in all download error tests, but I guess we should check the files don't exist at this point, since they are supposed to be deleted by the web process when the download fails or is cancelled.

[Michael Catanzaro]

(In reply to comment #12)
> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:-222
> > -    void receivedResponse(WebKitDownload* download)
> > -    {
> > -        DownloadTest::receivedResponse(download);
> > -    }
> 
> Why did you delete this?

I guess I should not have in this patch, since it's not related. I removed it because it wasn't doing anything (it's an override of a virtual function that just calls the parent class's version of that function).

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestDownloads.cpp:288
> > +    test->m_permitSuccess = false;
> 
> I don't think this is working, this combination makes the test set an invalid destination in decideDestination(), so this will fail because of the invalid destination and not because of the destination file already exists.

Yes, this test was really broken, sorry.

> I've noticed we are using this in all download error tests, but I guess we should check the files don't exist at this
> point, since they are supposed to be deleted by the web process when the download fails or is cancelled.

Well, if the download fails because the file already exists, as would be the case here, that wouldn't make sense.  But it would be good for the other test cases, yes.

[Michael Catanzaro]

Created attachment 237747 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 237747 [details]
Patch

LGTM

[WebKit Commit Bot]

Comment on attachment 237747 [details]
Patch

Clearing flags on attachment: 237747

Committed r173456: <http://trac.webkit.org/changeset/173456>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.