Created attachment 123823 [details] [review]
Only do GType checks if applicable and display a message if a function with a callback is called.

Thanks to walters for the improvements.

The patch generally looks good, I'd like to see the following done though:
- A test case against the Everything module in g-i which fails
  You might need to add a callback test to Everything first.
- Ensurance that make check passes
- Ensurance that the patch does not introduce any new memory leaks, eg, run make valgrind-check before and after applying the patch.

It's really important to avoid introducing new leaks and keeping current functionality unittested.

Created attachment 125095 [details] [review]
Adds callback tests to Everything in gobject-introspection

This patch makes Everything store a user-defined callback function, and lets the user call it. I didn't test it with working implementations, but gjs segfaults on it indeed. (The next post will be a patch to gjs to test this.)

Created attachment 125096 [details] [review]
Adds testCallbacks() to testEverythingBasic.js in gjs

Note that the set_callback function is called as follows:
  Everything.test_set_callback_function(someCallback, null);
The 'null' should probably not be needed, but omitting it causes an argument number mismatch error right now.

Attachment #123823 [details], the first patch submitted in this bug, causes `make check' to fail at testClosure on my system.  The errors are at:
  http://fpaste.org/paste/30
I don't know what exactly is wrong, but it seems the `if (g_type_is_a(gtype, G_TYPE_CLOSURE))' test is never reached.

Created attachment 127978 [details] [review]
v1

Work in progress. Depends on g-i head, can be used to call the simplest test from Everything.

Created attachment 129533 [details]
irc discussion log from 2009-02-25

Comment on attachment 127978 [details] [review]
v1

Marking work-in-progress patch as needs-work to take it out of queries

Created attachment 148603 [details] [review]
Patch for this bug

Based on previous patch. Added correct memory management

Created attachment 148791 [details] [review]
Add callback support for GJS

Incorporated memory manager changes from Maxim Ermilov,
still needs to do the following:
- GDestroyNotify should not be callable from Javascript code,
  passing in an argument where a GDestroyNotify should be an error
  and an exception raised (too many arguments)
- GJS should send in a custom callback to all GDestroyNotify functions
  and free up the data in there instead of directly after the
  callback invokation
- All arguments added with JS_AddRoot needs to be unrooted.

(In reply to comment #9)
> - GJS should send in a custom callback to all GDestroyNotify functions
>   and free up the data in there instead of directly after the
>   callback invokation

This should depend on the 'scope', as I recall there are three options:

 call:     free after calling the function that takes the callback as an argument
 async:    call immediately after calling the callback - callback will be called exactly once
 notified: use the GDestroyNotify

Owen: yes, the patch is actually using the scope annotation. My point was just
in the notified case the GDestroyNotify argument should be an implementation
detail, it shouldn't be visible in the JS api. Freeing the structures needed
for the callback invokation obviously depends on the scope. 

call / async should be pretty much correct with the patch above, but notified
will as far as I understand a bit of extra work.

Review of attachment 148791 [details] [review]:

Okay, I'm reviewing my own patch as Maxim told me today that he's going to do further work on this.
Hopefully it can be helpful. If he doesn't get around to do it I'll try to tackle it tomorrow evening or so.

::: gi/function.c
@@ -86,0 +102,24 @@
+static void
+invoke_info_for_js_callback(ffi_cif *cif,
+                            void *result,
... 21 more ...

Should these two be freed after usage?

@@ -86,0 +102,49 @@
+static void
+invoke_info_for_js_callback(ffi_cif *cif,
+                            void *result,
... 46 more ...

This could perhaps be done in a separate function, gjs_callback_info_free() and use g_slist_foreach for consistency.

@@ -86,0 +102,64 @@
+static void
+invoke_info_for_js_callback(ffi_cif *cif,
+                            void *result,
... 61 more ...

g_free is not right here, should use gjs_callback_invoke_info_free instead

@@ -86,0 +102,96 @@
+static void
+invoke_info_for_js_callback(ffi_cif *cif,
+                            void *result,
... 93 more ...

invoke_info->closure and invoke_info->cif should be freed as well.

Actually, I'm unsure of the life cycle of closure, as it is referenced in in_value->v_pointer. But I guess it needs to be freed somewhere.

@@ -86,0 +102,181 @@
+static void
+invoke_info_for_js_callback(ffi_cif *cif,
+                            void *result,
... 178 more ...

invoke_info leaks here

@@ -206,0 +415,8 @@
+
+            type_tag = g_type_info_get_tag(info);
+            if (g_slist_find(callback_arg_indices, GINT_TO_POINTER(i)) != NULL) {
... 5 more ...

This is kind of ugly, saving ints on a linked list. Do we have any better alternatives though?
Maybe creating a fixed list of the same length as the number of arguments using g_alloca()?

@@ +503,3 @@
+    g_slist_foreach(notify_free_list, (GFunc)gjs_callback_info_free, NULL);
+    g_slist_free(call_free_list);
+    g_slist_foreach(call_free_list, (GFunc)gjs_callback_invoke_info_free, NULL);

As mentioned in a comment above, notify arguments should not be freed here, they should be freed in the destroy notify function

::: test/js/testEverythingBasic.js
@@ -188,0 +188,8 @@
+                   };
+}
+/*
... 5 more ...

Better just kill this for now until infinite callbacks are better handled/considered

@@ -188,0 +188,41 @@
+function testCallback() {
+    let callback = function() {
+                       return 42;
... 38 more ...

This should be merged with testCallbackDestroyNotify, it should be called after test_callback_destroy_notify.

Actually, the test should be something like this:

let called = 0;
let callback = function(userData) {
   called++;
   return userData === 42;
}

Everything.test_callback_destroy_notify(test, 42);
assert called == 0
retval Everything.test_callback_thaw_notifications();
assert called == 1
assert retval == 42

Review of attachment 148791 [details] [review]:

A few more things that I noticed after taking another closer look.

::: gi/function.c
@@ +117,3 @@
+                            void *result,
+invoke_info_for_js_callback(ffi_cif *cif,
+static void

Allocate this on the stack, eg g_newa instead?

@@ -86,0 +102,45 @@
+static void
+invoke_info_for_js_callback(ffi_cif *cif,
+                            void *result,
... 42 more ...

jsargs should be freed here as well.

@@ -86,0 +102,48 @@
+static void
+invoke_info_for_js_callback(ffi_cif *cif,
+                            void *result,
... 45 more ...

l = l->next for consistency?

@@ -86,0 +102,111 @@
+static void
+invoke_info_for_js_callback(ffi_cif *cif,
+                            void *result,
... 108 more ...

Why is it necessary to copy the jsval here, isn't it enough to use the original value?

@@ -207,3 +432,23 @@
-                                  &in_value)) {
-                failed = TRUE;
-            if (!gjs_value_to_arg(context, argv[argv_pos], arg_info,
+                GIBaseInfo* interface_info;
+            if (type_tag == GI_TYPE_TAG_INTERFACE) {
+                GIInfoType interface_type;
... 20 more ...

This should probably be moved and done in gjs_callback_invoke_info_free, eg, let gjs_callback_from_arguments take ownership.

Unless gjs_callback_from_arguments() return FALSE of course, in that case it should be unreffed right away.

Would be nice if more error paths could be tested so we can catch these situations earlier.

Created attachment 148859 [details] [review]
Corrected patch

Review of attachment 148859 [details] [review]:

Thanks, much better, still a few more things that I'd like to fix.

::: gi/function.c
@@ -45,1 +50,4 @@
 static struct JSClass gjs_function_class;
+    GSList *invoke_infos; /* g_free for each*/
+    GSList *arguments; /* List of jsval, that need to be unrooted after call*/
+typedef struct {

The comment is wrong, should be gjs_callback_invoke_info_free instead of g_free

@@ -46,0 +51,12 @@
+    gint arg_index;
+
+    JSContext *context;
... 9 more ...

This seems useless, as it's always TRUE.

@@ +63,3 @@
+    GSList *invoke_infos; /* g_free for each*/
+    GSList *arguments; /* List of jsval, that need to be unrooted after call*/
+typedef struct {

Instead of marking when this should be freed do the following instead:
1) add a scope field to the struct indicating what kind of callback it is
2) add a scope argument to gjs_callback_invoke_info_free_content() to indicate when it's called
3) compare the two scopes to figure out if it should be freed

@@ -86,0 +108,110 @@
+static void gjs_callback_invoke_info_free_content(GjsCallbackInvokeInfo *invoke_info);
+
+static void
... 107 more ...

Not sure it makes sense to put defines for this, I'd just inline these three to the call to ffi_prep_cif

@@ -86,0 +108,128 @@
+static void gjs_callback_invoke_info_free_content(GjsCallbackInvokeInfo *invoke_info);
+
+static void
... 125 more ...

gjs_destroy_notify_create I guess

@@ -86,0 +108,162 @@
+static void gjs_callback_invoke_info_free_content(GjsCallbackInvokeInfo *invoke_info);
+
+static void
... 159 more ...

mprotect should be setting errno when it fails, you should include strerror in the exception message.

@@ -115,0 +444,7 @@
+    static GList *invoke_infos = NULL;
+
+    for (k = invoke_infos; k; k = g_list_next(k)) {
... 4 more ...

Shouldn't this be done after calling the function instead of before?

This for could be a separate function as well, I think that's clearer.

@@ -115,0 +444,20 @@
+    static GList *invoke_infos = NULL;
+
+        GList *t;
... 17 more ...

This is a bit messy, there must be a better way of accomplishing the same thing

@@ -128,1 +480,5 @@
         direction = g_arg_info_get_direction(arg_info);
+            expected_in_argc --;
+            destroynotify_count++;
+        if (destroy > 0 && destroy < n_args) {
+

destroy_notify_argc might be a better variable name no?

@@ -250,1 +652,4 @@
     failed = FALSE;
+    g_slist_foreach(notify_free_list, (GFunc)gjs_callback_info_free, NULL);
+    g_slist_free(call_free_list);
+    g_slist_foreach(call_free_list, (GFunc)gjs_callback_invoke_info_free, NULL);

shouldn't this list be freed in the destroy_notify function?

::: test/js/testEverythingBasic.js
@@ -188,0 +188,29 @@
+function testCallback() {
+    let callback = function() {
+                       return 42;
... 26 more ...

You shouldn't call these two separately, please merge testCallbackDestroyNotify and testCallbackDestroyFinish into *one* test

@@ -188,0 +188,32 @@
+function testCallback() {
+    let callback = function() {
+                       return 42;
... 29 more ...

Ditto, these tests belongs together.

We need to add this patch to gobject-repository before committing this to gjs.

@@ -188,0 +188,45 @@
+}
+                       return 42;
+                   };
... 42 more ...

I don't think this should be necessary, why is this done? to catch leaks? valgrind should be able to do the same thing.

Created attachment 148872 [details] [review]
Corrected patch

Comment on attachment 148872 [details] [review]
Corrected patch

>From 66f3aa6e710bd1981e00e04d27e6536b99702cac Mon Sep 17 00:00:00 2001
>From: Maxim Ermilov <zaspire@rambler.ru>
>Date: Wed, 2 Dec 2009 03:26:09 +0300
>Subject: [PATCH] Support for callback functions

>+static GList *invoke_infos = NULL;

Globals should be avoided for various reasons (thread-safety etc).

Cleaning up old data when you invoke the next function seems wrong, it seems
we'll always leak a bit. Instead you need to clean all this data up earlier,
depending on the scope type.

>+function testAllCallback() {
>+/* This need for catching unrooted JSObject.
>+ * When there is small amount of unrooted JSObject,
>+ * SpiderMonkey some times does not show error.*/
>+    for (let i = 0; i < 10000; i++) {
>+        testCallbackAsync();
>+        testCallback();
>+        testCallbackDestroyNotify();
>+        testCallbackUserData();
>+    }
>+}

I think this test should still be here, but commented out by default as it is only 
necessary for checking memory, no need to run it during a normal check.

(In reply to comment #17)

> I think this test should still be here, but commented out by default as it is
> only 
> necessary for checking memory, no need to run it during a normal check.

To catch rooting problems we should run the tests with

  JS_SetGCZeal(cx, 2);

and you should only need single run to catch any rooting issues.

the tests would already use JS_SetGCZeal except for 
https://bugzilla.mozilla.org/show_bug.cgi?id=437865

(In reply to comment #18)
[..]
> To catch rooting problems we should run the tests with
> 
>   JS_SetGCZeal(cx, 2);

I think it was more for memory usage which is normally caught by valgrind, but for some reason valgrind is not really helpful here, at least not when I tried.

Created attachment 148930 [details] [review]
Updated patch

 1. make invoke_infos static inside function
 2. remove test with alot of calls.

Created attachment 148954 [details] [review]
Patch that got pushed

I pushed this to git.

Thanks a lot to everybody who helped out.