<jdahlin> hp: do you have any ideas on how I could properly free up callback resources as part of #563025 ?
<jdahlin> http://bugzilla.gnome.org/attachment.cgi?id=127978&action=view
<jdahlin> gjs_g_argument_release_in_arg only gets the GArgument which doesn't contain a pointer to the allocated closure structure
<jdahlin> hmm, maybe a pointer can be stored in the jsval of the function itself
<hp> looking
<jdahlin> thanks, I'm slightly lost
<jdahlin> what you send in to libffi is a ffi_closure*
<hp> the patch is a little confusing, but I would expect generally how it works is that the GArgument just has the C function, and you would alloc/free the data that goes with the C function either with a DestroyNotify for APIs that have one, or just at the end of gjs_invoke_c_function() when the C function is only used transiently
<hp> too many things called closure in this file ;-)
<jdahlin> remember that g_callable_info_prepare_closure is overwriting the real C callback
<jdahlin> yeah, definitely
<hp> overwriting?
<hp> is ffi_closure* really a C function pointer?
<jdahlin> yeah, it replaces it with a trampoline where you get the arguments as a list of void*
<hp> I think you should use closure.c here to store the jsval function, otherwise you'll be in memory management hell
<jdahlin> so some_callback(a, b, c) turns into something like this (closure, [a, b, c], retval_p, user_data)
<jdahlin> oh, we're already ending up in a pretty warm place, there's no way around that
<hp> _gjs_callback_to_value() should likely take a GClosure rather than a jsval, then there's existing code to create the GClosure from jsval
<hp> anyway that's a side issue ... 
<jdahlin> I'm not quite I understand how closure.c helps here
<hp> right now the "jsval function" can be garbage collected before you invoke it
<hp> then it will segfault
<jdahlin> I need to JS_AddRoot it right?
<hp> it's more complicated than that, you need to JS_AddRoot it in a way that you unroot it when the runtime is destroyed
<hp> if you just wrap it in GClosure from closure.c that will deal with it ;-)
<jdahlin> okay, that'll solve one problem I hadn't anticipated
<hp> so where is g_callable_info_prepare_closure ?
<jdahlin> in gobject-introspection
<jdahlin> in here http://git.gnome.org/cgit/gobject-introspection/tree/girepository/girffi.c
<hp> oh, hmm, I have the 'big' one and the svn one, let me get that one
<jdahlin> time to rebase soon
<hp> hmm, not a cheap function ;-)
<jdahlin> unfortunately not
<jdahlin> but it'll only be called once per callback, not per invocation
<jdahlin> it's less important for it to run fast, than say g_function_info_invoke
<hp> I think basically gjs_value_to_arg() isn't a good enough interface for this
<hp> this has to be sorted out looking at the whole list of args and whole invocation
<hp> because you want to free the GjsCallbackClosure at the end of invoking the function sometimes (like gtk_container_foreach()), but lots of the time you want to free it in a destroy notifier ... 
<jdahlin> right, there are a couple of different situations
<jdahlin> atexit handlers is another fun case
<hp> so I think callbacks should be special-cased in gjs_invoke_c_function
<hp> well atexit() is always broken to use so we can just ignore that ;-)
<jdahlin> there are others iirc, which never exists
<jdahlin> right, for GDestroyNotify looking at other arguments is needed
<hp> you could make gjs_value_to_arg() just return some kind of additional stuff which would be the destroy notifier and the user_data to go with the callback, and NULL if the value was not a callback
<hp> then gjs_invoke_c_function could either immediately call the destroy notifier on the user data, or it could provide the destroy notifier to the invoked function rather than calling it itself
<jdahlin> but I haven't even started to look at that case, trying to focus on the simplest one, which is no destroy notifier at all
<hp> gjs_value_to_arg(context, argv[argv_pos], arg_info, &in_value, &callback_data, &callback_dnotifier);
<hp> then for your simple case, you just do (* callback_dnotifier) (callback_data) after invoking
<jdahlin> oh, right, so then gjs_invoke_c_function() needs to know if the related C call is going to call any closures inside
<jdahlin> that makes a lot of sense yeah
<hp> I think there has to be an annotation for whether the invoked function keeps the closure around
<hp> if it does, there has to be a GDestroyNotify or the API is broken and can't be used
<jdahlin> there is annotation for that already
<jdahlin> it should work even without GDestroyNotify, we'll just leak it a bit I guess
<hp> well I'd like at least a "throw error instead of leaking" mode, personally ;-)
<jdahlin> gdk_window_add_filter() is an example of something tricky to wrap
<jdahlin> which arguable is as broken as atexit()
<jdahlin> it should keep the structure around until gdk_window_remove_filter() is called
<hp> yeah, the "remove callback by callback pointer" type of APIs can't be used without leaking since our callback pointer won't be the same ... 
<hp> unless we take really extreme measures like keeping a hash table of jsval to ffi closures
<jdahlin> that's what I was thinking
<hp> I guess we could annotate for this though I don't know what the annotation is called
<jdahlin> we call the annotation scope
<jdahlin> but I don't think we figured out all use cases yet, just simple + gdestroynotify properly
<hp> gdk_window_add_filter() is pretty useless outside C anyway, you need to mess with the XEvent, would need Xlib bindings, and it needs to be very fast...
<jdahlin> racarr has implemented a few of them, but I'm not sure how seed releases the resources
<hp> I think if you get most of the useful APIs that's pretty good
<jdahlin> it's a very useful api
<jdahlin> I've used it to write a testing framework in python
<hp> so if there's no dnotify, you could hash the function and hook it up to be destroyed along with the runtime (most easily by pulling it out of your hash when the associated GClosure gets invalidated)
<jdahlin> right, that's probably the only way

