WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
NEW
167495
[GTK] Confusing webkit_uri_scheme_request_finish_error() description
https://bugs.webkit.org/show_bug.cgi?id=167495
Summary
[GTK] Confusing webkit_uri_scheme_request_finish_error() description
Milan Crha
Reported
2017-01-27 02:47:10 PST
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?
Attachments
Add attachment
proposed patch, testcase, etc.
Michael Catanzaro
Comment 1
2017-01-27 08:02:43 PST
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
Comment 2
2017-01-30 00:16:59 PST
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
Comment 3
2017-01-30 06:40:55 PST
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug