Bug 256335 - [GLib] Audit and fix incorrect use of (closure) in WebKit
Summary: [GLib] Audit and fix incorrect use of (closure) in WebKit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-05-04 14:49 PDT by Michael Catanzaro
Modified: 2023-05-05 06:37 PDT (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2023-05-04 14:49:33 PDT
Following Emmanuele's guidance in https://www.bassi.io/articles/2023/02/20/bindable-api-2023/ I have audited our use of (closure) annotations in WebKit and found they are all incorrect. Unary (closure) is only allowed on function typedefs; in function declarations an argument is always required, e.g. (closure user_data). But (closure user_data) is actually default, so most of our annotations can actually be removed.

It looks like this was not causing many problems in practice. For example, let's look at the callback parameter to jsc_class_add_constructor generated *before* this patch:

          <parameter name="callback"
                     transfer-ownership="none"
                     scope="notified"
                     closure="2"
                     destroy="3">
            <doc xml:space="preserve"
                 filename="Source/JavaScriptCore/API/glib/JSCClass.cpp"
                 line="569">a #GCallback to be called to create an instance of @jsc_class</doc>
            <type name="GObject.Callback" c:type="GCallback"/>
          </parameter>

This already looks fine: it knows the closure parameter comes right after callback, which is the correct order. So even though Emmanuele says this use of unary closure was incorrect, it wasn't causing problems in practice.

Looking through the generated JavaScriptCore-6.0.gir, this results in gir changes for only jsc_value_new_array_buffer and jsc_value_new_typed_array_with_buffer, where the closure parameter was previously not detected, which is now fixed. I don't think this resulted in any changes to WebKit-6.0.gir.

I've also fixed WPE's webkit_web_view_add_frame_displayed_callback() where the callback parameter defaulted to (scope call), causing it to be valid only for the duration of the function call.
Comment 1 Michael Catanzaro 2023-05-04 14:51:27 PDT
Pull request: https://github.com/WebKit/WebKit/pull/13460
Comment 2 Michael Catanzaro 2023-05-05 06:32:13 PDT
(In reply to Michael Catanzaro from comment #0)
> So even though Emmanuele says this use
> of unary closure was incorrect, it wasn't causing problems in practice.

It will soon, though: Emmanuele is going to make the introspection scanner complain when it sees this, and that will probably cause our builds to fail.
Comment 3 EWS 2023-05-05 06:37:47 PDT
Committed 263714@main (56d72f891945): <https://commits.webkit.org/263714@main>

Reviewed commits have been landed. Closing PR #13460 and removing active labels.