Bug 186276

Summary: [WPE][GTK] webkit_web_resource_get_data_finish can return NULL without setting error
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: Michael Catanzaro <mcatanzaro>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, bugs-noreply, calvaris, cgarcia, commit-queue, ews-watchlist, gns, mcatanzaro
Priority: P2    
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
See Also: https://bugzilla.gnome.org/show_bug.cgi?id=746737
https://bugs.webkit.org/show_bug.cgi?id=186278
https://bugs.webkit.org/show_bug.cgi?id=177107
https://bugs.webkit.org/show_bug.cgi?id=221672
Bug Depends on: 191222    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Michael Catanzaro 2018-06-04 12:30:45 PDT
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.
Comment 1 Michael Catanzaro 2018-06-04 12:52:20 PDT
Created attachment 341910 [details]
Patch
Comment 2 EWS Watchlist 2018-06-04 12:54:04 PDT
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
Comment 3 Carlos Garcia Campos 2018-06-04 22:29:56 PDT
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.
Comment 4 Michael Catanzaro 2018-06-04 22:55:42 PDT
(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().
Comment 5 Michael Catanzaro 2018-10-22 22:50:14 PDT
Comment on attachment 341910 [details]
Patch

I need to update this patch.
Comment 6 Michael Catanzaro 2018-11-23 11:51:44 PST
Can't add any tests because I can't run any tests, bug #191222.
Comment 7 Michael Catanzaro 2018-12-09 17:15:36 PST
Created attachment 356934 [details]
Patch
Comment 8 Carlos Garcia Campos 2018-12-12 00:44:14 PST
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.
Comment 9 Michael Catanzaro 2018-12-12 19:34:32 PST
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.)
Comment 10 Carlos Garcia Campos 2018-12-12 23:18:43 PST
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().
Comment 11 Michael Catanzaro 2018-12-14 07:07:37 PST
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.
Comment 12 Carlos Garcia Campos 2018-12-14 07:12:21 PST
An empty resource is not an error.
Comment 13 Michael Catanzaro 2019-03-27 10:48:45 PDT
Created attachment 366083 [details]
Patch
Comment 14 WebKit Commit Bot 2019-03-28 10:50:24 PDT
Comment on attachment 366083 [details]
Patch

Clearing flags on attachment: 366083

Committed r243608: <https://trac.webkit.org/changeset/243608>
Comment 15 WebKit Commit Bot 2019-03-28 10:50:26 PDT
All reviewed patches have been landed.  Closing bug.