[gjs] gi: Marshal variable array-typed signal arguments

[Philip Chimento <pchimento src gnome org>]

commit 67d7d2d550e81d74cdc392e360a7be5de706053f
Author: Philip Chimento <philip chimento gmail com>
Date:   Sat Feb 6 01:36:04 2016 -0800

    gi: Marshal variable array-typed signal arguments
    
    It can happen that we need to marshal a closure parameter which is a
    GValue holding a pointer to an array, with the array length passed in as
    a separate parameter. For example, GApplication::open is such a signal.
    
    Previously, the pointer GValue would be sent to
    gjs_value_from_g_value_internal() which sends it on to
    gjs_value_from_g_argument(), which states in its code that arrays with
    explicit length parameters should be handled elsewhere. Normally they are
    indeed handled elsewhere (see e.g. gjs_invoke_c_function() in
    function.cpp) but in closure_marshal() they are not.
    
    This adds code to closure_marshal() that handles that case, also removing
    the length parameter from the arguments passed to the closure.
    
    It also adds some tests for this case (requires update to
    gobject-introspection) and a similar case that was until now untested,
    namely passing an array with length parameter out of an introspected
    function.
    
    Lastly, it adds some asserts to gjs_value_from_g_value_internal() and
    gjs_value_from_g_argument() which try to give a hint what's going wrong
    when you call those functions without having checked for this case.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=761659

 gi/arg.cpp                                |    2 +
 gi/value.cpp                              |  129 ++++++++++++++++++++++++++++-
 installed-tests/js/testEverythingBasic.js |   25 ++++++
 3 files changed, 154 insertions(+), 2 deletions(-)
---
diff --git a/gi/arg.cpp b/gi/arg.cpp
index 7ce45bf..72ac37a 100644
--- a/gi/arg.cpp
+++ b/gi/arg.cpp
@@ -2798,6 +2798,8 @@ gjs_value_from_g_argument (JSContext  *context,
                 return result;
             } else {
                 /* arrays with length are handled outside of this function */
+                g_assert(("Use gjs_value_from_explicit_array() for arrays with length param",
+                          g_type_info_get_array_length(type_info) == -1));
                 return gjs_array_from_fixed_size_array(context, value_p, type_info, arg->v_pointer);
             }
         } else if (g_type_info_get_array_type(type_info) == GI_ARRAY_TYPE_BYTE_ARRAY) {
diff --git a/gi/value.cpp b/gi/value.cpp
index ca50cdd..aad2cd0 100644
--- a/gi/value.cpp
+++ b/gi/value.cpp
@@ -48,6 +48,62 @@ static JSBool gjs_value_from_g_value_internal(JSContext    *context,
                                               GSignalQuery *signal_query,
                                               gint          arg_n);
 
+/*
+ * Gets signal introspection info about closure, or NULL if not found. Currently
+ * only works for signals on introspected GObjects, not signals on GJS-defined
+ * GObjects nor standalone closures. The return value must be unreffed.
+ */
+static GISignalInfo *
+get_signal_info_if_available(GSignalQuery *signal_query)
+{
+    GIBaseInfo *obj;
+    GISignalInfo *signal_info;
+
+    if (!signal_query->itype)
+        return NULL;
+
+    obj = g_irepository_find_by_gtype(NULL, signal_query->itype);
+    if (!obj)
+        return NULL;
+
+    signal_info = g_object_info_find_signal((GIObjectInfo*)obj,
+                                            signal_query->signal_name);
+    g_base_info_unref((GIBaseInfo*)obj);
+
+    return signal_info;
+}
+
+/*
+ * Fill in value_p with a JS array, converted from a C array stored as a pointer
+ * in array_value, with its length stored in array_length_value.
+ */
+static JSBool
+gjs_value_from_array_and_length_values(JSContext    *context,
+                                       jsval        *value_p,
+                                       GITypeInfo   *array_type_info,
+                                       const GValue *array_value,
+                                       const GValue *array_length_value,
+                                       gboolean      no_copy,
+                                       GSignalQuery *signal_query,
+                                       int           array_length_arg_n)
+{
+    jsval array_length;
+    GArgument array_arg;
+
+    g_assert(G_VALUE_HOLDS_POINTER(array_value));
+    g_assert(G_VALUE_HOLDS_INT(array_length_value));
+
+    if (!gjs_value_from_g_value_internal(context, &array_length,
+                                         array_length_value, no_copy,
+                                         signal_query, array_length_arg_n))
+        return JS_FALSE;
+
+    array_arg.v_pointer = g_value_get_pointer(array_value);
+
+    return gjs_value_from_explicit_array(context, value_p, array_type_info,
+                                         &array_arg, JSVAL_TO_INT(array_length));
+}
+
 static void
 closure_marshal(GClosure        *closure,
                 GValue          *return_value,
@@ -64,6 +120,11 @@ closure_marshal(GClosure        *closure,
     jsval rval;
     int i;
     GSignalQuery signal_query = { 0, };
+    GISignalInfo *signal_info;
+    gboolean *skip;
+    int *array_len_indices_for;
+    GITypeInfo **type_info_for;
+    int argv_index;
 
     gjs_debug_marshal(GJS_DEBUG_GCLOSURE,
                       "Marshal closure %p",
@@ -138,9 +199,48 @@ closure_marshal(GClosure        *closure,
         }
     }
 
+    /* Check if any parameters, such as array lengths, need to be eliminated
+     * before we invoke the closure.
+     */
+    skip = g_newa(gboolean, argc);
+    memset(skip, 0, sizeof (gboolean) * argc);
+    array_len_indices_for = g_newa(int, argc);
+    for(i = 0; i < argc; i++)
+        array_len_indices_for[i] = -1;
+    type_info_for = g_newa(GITypeInfo *, argc);
+    memset(type_info_for, 0, sizeof (gpointer) * argc);
+
+    signal_info = get_signal_info_if_available(&signal_query);
+    if (signal_info) {
+        /* Start at argument 1, skip the instance parameter */
+        for (i = 1; i < argc; ++i) {
+            GIArgInfo *arg_info;
+            int array_len_pos;
+
+            arg_info = g_callable_info_get_arg(signal_info, i - 1);
+            type_info_for[i] = g_arg_info_get_type(arg_info);
+
+            array_len_pos = g_type_info_get_array_length(type_info_for[i]);
+            if (array_len_pos != -1) {
+                skip[array_len_pos + 1] = TRUE;
+                array_len_indices_for[i] = array_len_pos + 1;
+            }
+
+            g_base_info_unref((GIBaseInfo *)arg_info);
+        }
+
+        g_base_info_unref((GIBaseInfo *)signal_info);
+    }
+
+    argv_index = 0;
     for (i = 0; i < argc; ++i) {
         const GValue *gval = &param_values[i];
         gboolean no_copy;
+        int array_len_index;
+        JSBool res;
+
+        if (skip[i])
+            continue;
 
         no_copy = FALSE;
 
@@ -148,16 +248,37 @@ closure_marshal(GClosure        *closure,
             no_copy = (signal_query.param_types[i - 1] & G_SIGNAL_TYPE_STATIC_SCOPE) != 0;
         }
 
-        if (!gjs_value_from_g_value_internal(context, &argv[i], gval, no_copy, &signal_query, i)) {
+        array_len_index = array_len_indices_for[i];
+        if (array_len_index != -1) {
+            const GValue *array_len_gval = &param_values[array_len_index];
+            res = gjs_value_from_array_and_length_values(context,
+                                                         &argv[argv_index],
+                                                         type_info_for[i],
+                                                         gval, array_len_gval,
+                                                         no_copy, &signal_query,
+                                                         array_len_index);
+        } else {
+            res = gjs_value_from_g_value_internal(context, &argv[argv_index],
+                                                  gval, no_copy, &signal_query,
+                                                  i);
+        }
+
+        if (!res) {
             gjs_debug(GJS_DEBUG_GCLOSURE,
                       "Unable to convert arg %d in order to invoke closure",
                       i);
             gjs_log_exception(context);
             goto cleanup;
         }
+
+        argv_index++;
     }
 
-    gjs_closure_invoke(closure, argc, argv, &rval);
+    for (i = 1; i < argc; i++)
+        if (type_info_for[i])
+            g_base_info_unref((GIBaseInfo *)type_info_for[i]);
+
+    gjs_closure_invoke(closure, argv_index, argv, &rval);
 
     if (return_value != NULL) {
         if (JSVAL_IS_VOID(rval)) {
@@ -850,6 +971,10 @@ gjs_value_from_g_value_internal(JSContext    *context,
         arg_info = g_callable_info_get_arg(signal_info, arg_n - 1);
         g_arg_info_load_type(arg_info, &type_info);
 
+        g_assert(("Check gjs_value_from_array_and_length_values() before calling"
+                  " gjs_value_from_g_value_internal()",
+                  g_type_info_get_array_length(&type_info) == -1));
+
         arg.v_pointer = g_value_get_pointer(gvalue);
 
         res = gjs_value_from_g_argument(context, value_p, &type_info, &arg, TRUE);
diff --git a/installed-tests/js/testEverythingBasic.js b/installed-tests/js/testEverythingBasic.js
index 3291dd8..edd50dc 100644
--- a/installed-tests/js/testEverythingBasic.js
+++ b/installed-tests/js/testEverythingBasic.js
@@ -342,6 +342,9 @@ function testArrayOut() {
             JSUnit.assertEquals(ref[i], res[i]);
     }
 
+    let array = Everything.test_array_int_out();
+    arrayEqual([0, 1, 2, 3, 4], array);
+
     let array =  Everything.test_array_fixed_size_int_out();
     JSUnit.assertEquals(0, array[0]);
     JSUnit.assertEquals(4, array[4]);
@@ -473,6 +476,28 @@ function testSignalWithStaticScopeArg() {
     JSUnit.assertEquals('signal handler was passed arg as reference', 44, b.some_int);
 }
 
+function testSignalWithArrayLenParam() {
+    let o = new Everything.TestObj();
+    let array;
+    o.connect('sig-with-array-len-prop', function(signalObj, signalArray, shouldBeUndefined) {
+        array = signalArray;
+        JSUnit.assertUndefined('no extra length arg', shouldBeUndefined);
+    });
+
+    o.emit_sig_with_array_len_prop();
+    JSUnit.assertEquals('handler was passed array with length', array.length, 5);
+    for (let i = 0; i < 5; i++)
+        JSUnit.assertEquals('handler was passed correct array', array[i], i);
+
+    // FIXME not yet implemented:
+    // o.emit('sig-with-array-len-prop', [0, 1, 2, 3, 4]);
+    // JSUnit.assertEquals('handler was passed array with length', array.length, 5);
+    // for (let i = 0; i < 5; i++)
+    //     JSUnit.assertEquals('handler was passed correct array', array[i], i);
+    // o.emit('sig-with-array-len-prop', null);
+    // JSUnit.assertNull('handler was passed null array', array);
+}
+
 function testTortureSignature0() {
     let [y, z, q] = Everything.test_torture_signature_0(42, 'foo', 7);
     JSUnit.assertEquals(Math.floor(y), 42);