Bug 186276 – [WPE][GTK] webkit_web_resource_get_data_finish can return NULL without setting error

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

[Michael Catanzaro]

This has been confusing me on and off since 2015. webkit_web_resource_get_data_finish can return NULL without setting its error parameter, which is illegal as it is documented to return NULL only on failure. In this case, Epiphany properly tries to use the error and then crashes because it is NULL. We should always set error in this case.

[Michael Catanzaro]

Created attachment 341910 [details]
Patch

[EWS Watchlist]

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

[Carlos Garcia Campos]

Comment on attachment 341910 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitWebResource.cpp:358
> +    if (!wkData->size()) {
> +        g_task_return_new_error(task, G_IO_ERROR, G_IO_ERROR_FAILED, _("Resource data unavailable"));
> +        return;
> +    }

How can this happen? is it an error or simply and empty file? I think this requires a test case.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #3)
> How can this happen? is it an error or simply and empty file? I think this
> requires a test case.

Seems to happen for every media file. See bug #186278. I can try to add a test.

> is it an error or simply and empty file?

Ummm, good point... in this case, it is an error. But with this patch, an empty file would trigger this error, which is probably wrong. My patch should probably check wkData->bytes() instead of wkData->size().

[Michael Catanzaro]

Comment on attachment 341910 [details]
Patch

I need to update this patch.

[Michael Catanzaro]

Can't add any tests because I can't run any tests, bug #191222.

[Michael Catanzaro]

Created attachment 356934 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 356934 [details]
Patch

I don't get why an empty resource is an error, I think we should return an empty string instead of NULL.

[Michael Catanzaro]

It's not a string, it is a byte array. That's why we use guchar*, not char*. So the result is not NULL-terminated and can contain embedded NULLs. So returning an empty string is not really possible. The closest we can get is to return a byte array of length one containing only a NULL. We could do that, but it won't be an empty string, and we'd have to document it carefully so applications know that special care is required.

NULL seems the best way to indicate emptiness, but we have documented that this only returns NULL in case of error. So treating empty as an error seems reasonable. That's what this patch does.

Alternatively, we could change the documentation to indicate that NULL may not always correspond to an error. This would be an API break, but a quick Debian codesearch indicates we would only have to update evolution-data-server and epiphany.

So that's three options... do you have a preference?

(P.S. This reminds me of an unrelated problem. Because the result is not a string, the application has no way to know the encoding it uses. WebCore knows, but our API does not expose it. So there's no way to actually display the result as text without making unsafe assumptions about content type. This was a problem for Eclipse at some point in the past, which we never solved. That is unrelated. I digress.)

[Carlos Garcia Campos]

I see, ideally we would argue that length = 0 + NULL means empty resource, but length is gsize, so we don't have a way to indicate error here using length = -1 + NULL. Maybe we can use G_MAXUINT and document it, of course. Or deprecate this and add webkit_web_resource_get_bytes().

[Michael Catanzaro]

Currently the only possible error is cancellation (e.g. page closed) so my preference is still to use an error for empty resource. G_MAXUINT seems weird and we can avoid a deprecation that way. But I'll implement what you prefer.

[Carlos Garcia Campos]

An empty resource is not an error.

[Michael Catanzaro]

Created attachment 366083 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 366083 [details]
Patch

Clearing flags on attachment: 366083

Committed r243608: <https://trac.webkit.org/changeset/243608>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.