Bug 167495 – [GTK] Confusing webkit_uri_scheme_request_finish_error() description

• Status: NEW
• Component: WebKitGTK

[Milan Crha]

I just found out in one of my valgrind logs this:

==8106== 52 (32 direct, 20 indirect) bytes in 2 blocks are definitely lost in loss record 22,849 of 44,246
==8106==    at 0x4C2DB9D: malloc (vg_replace_malloc.c:299)
==8106==    by 0x6AC229E: g_malloc (gmem.c:94)
==8106==    by 0x6ADC406: g_slice_alloc (gslice.c:1025)
==8106==    by 0x6AA2767: g_error_new_literal (gerror.c:471)
==8106==    by 0x969D47F: web_view_process_uri_request_cb (e-web-view.c:1103)
==8106==    by 0x751FB15: ??? (in /usr/lib64/libwebkit2gtk-4.0.so.37.14.9)
==8106==    by 0x753BB54: ??? (in /usr/lib64/libwebkit2gtk-4.0.so.37.14.9)
==8106==    by 0x753B7D4: ??? (in /usr/lib64/libwebkit2gtk-4.0.so.37.14.9)
==8106==    by 0x75B4B51: ??? (in /usr/lib64/libwebkit2gtk-4.0.so.37.14.9)
==8106==    by 0x75B4778: ??? (in /usr/lib64/libwebkit2gtk-4.0.so.37.14.9)
==8106==    by 0x72B4168: ??? (in /usr/lib64/libwebkit2gtk-4.0.so.37.14.9)
==8106==    by 0x73AB521: ??? (in /usr/lib64/libwebkit2gtk-4.0.so.37.14.9)
==8106==    by 0x72B02C5: ??? (in /usr/lib64/libwebkit2gtk-4.0.so.37.14.9)
==8106==    by 0x72B0F57: ??? (in /usr/lib64/libwebkit2gtk-4.0.so.37.14.9)
==8106==    by 0x1120EB9C: WTF::RunLoop::performWork() (in /usr/lib64/libjavascriptcoregtk-4.0.so.18.4.9)
==8106==    by 0x11235588: ??? (in /usr/lib64/libjavascriptcoregtk-4.0.so.18.4.9)
==8106==    by 0x6AB9C0B: g_main_dispatch (gmain.c:3203)
==8106==    by 0x6ABAABC: g_main_context_dispatch (gmain.c:3856)
==8106==    by 0x6ABACA1: g_main_context_iterate (gmain.c:3929)
==8106==    by 0x6ABB0C8: g_main_loop_run (gmain.c:4125)
==8106==    by 0x5D577E4: gtk_main (gtkmain.c:1301)
==8106==    by 0x404CB7: main (main.c:667)

This newly created GError instance is passed to the webkit_uri_scheme_request_finish_error(), which takes a non-const GError structure pointer and claims in the documentation:
/**
 * webkit_uri_scheme_request_finish_error:
 * @request: a #WebKitURISchemeRequest
 * @error: a #GError that will be passed to the #WebKitWebView
 *
 * Finish a #WebKitURISchemeRequest with a #GError.
 *
 * Since: 2.2
 */

that the GError will be passed somewhere. I'd understand from it that the function takes ownership of the @error, the parameter type suggests the same (as being non-const), but when I checked the implementation the @error is only dereferenced and its values are copied to some other structures, and even that not always.

I suggest to make the @error a 'const GError *error' parameter of the function and eventually also write a note into the documentation that the 'error' is not taken by the function (which would be understood by the 'const' on its own).

Opinions?

[Michael Catanzaro]

Hm, I agree that's kind of confusing for a C developer.

Is it normal to use const to indicate transfer for GError parameters? The default annotation is transfer none, so it's unambiguous that all language bindings will maintain ownership and that your code should too, but I do understand why you expected it to be transfer full.

[Milan Crha]

I do not know how "normal" it is, I only try to keep APIs clean as much as possible, even in C, where the 'const' doesn't propagate to structure members. The hint with 'const' is like to mark a function 'const' in C++. Of course, there are some "issues" sometimes, when the corresponding API doesn't do the same, thus you can end with a silly typecast from const to non-const on an object where its get-function doesn't declare the object as const (and example can be a GSList/GList), though this is hidden to the caller.

I'm fine if you do not want to change it to the 'const', just make it explicit in the documentation that the 'error' is not taken by the function.

I consider confusing also [1], but maybe it's more about no explicit mention that the 'model' is referenced by the GtkTreeView. I understand that it's obvious when one knows about "default transfer method", but I confess I didn't know that.

The closest to this webkit function might be [2], and even it's all deprecated, see the [3], which takes ownership of the 'error', but it's not properly annotated. GTask has this done properly, at least where I looked.

[1] https://developer.gnome.org/gtk3/stable/GtkTreeView.html#gtk-tree-view-set-model
[2] https://developer.gnome.org/gio/stable/GSimpleAsyncResult.html#g-simple-async-result-set-from-error
[3] https://developer.gnome.org/gio/stable/GSimpleAsyncResult.html#g-simple-async-result-take-error

[Michael Catanzaro]

I don't think it's desirable to mention in freeform documentation whether you have to free a variable when we already use a structured unambiguous annotation language to describe that. I agree that it is confusing if you're not familiar with the rules for defaults; I have to look them up semi-regularly myself. gtk-doc could be improved to explicitly indicate default annotations in order to avoid such confusion.

As for const, we normally only use const to indicate ownership of char* strings.