[gjs: 12/16] gtype: Fix error handling in gjs_gtype_get_actual_gtype()

[Philip Chimento <pchimento src gnome org>]

commit 221ffc189ad49b05e36aa38ca6cccee59c7dfd3e
Author: Philip Chimento <philip chimento gmail com>
Date:   Sun Oct 28 20:12:32 2018 -0400

    gtype: Fix error handling in gjs_gtype_get_actual_gtype()
    
    This makes gjs_gtype_get_actual_gtype() conform to the JSAPI return
    convention, so that it is clear when an exception is pending.

 gi/arg.cpp      | 44 ++++++++++++++++++++++++--------------------
 gi/function.cpp |  3 ++-
 gi/gtype.cpp    | 42 ++++++++++++++++++++++--------------------
 gi/gtype.h      |  7 +++----
 gi/private.cpp  | 19 ++++++++++++++-----
 gi/value.cpp    | 54 +++++++++++++++++++++++++++++++-----------------------
 6 files changed, 96 insertions(+), 73 deletions(-)
---
diff --git a/gi/arg.cpp b/gi/arg.cpp
index 170bb913..f1c0dcd9 100644
--- a/gi/arg.cpp
+++ b/gi/arg.cpp
@@ -790,11 +790,11 @@ gjs_gtypearray_to_array(JSContext   *context,
                         unsigned int length,
                         void       **arr_p)
 {
-    GType *result;
     unsigned i;
 
     /* add one so we're always zero terminated */
-    result = (GType *) g_malloc0((length+1) * sizeof(GType));
+    GjsAutoPointer<GType, void, g_free> result =
+        static_cast<GType*>(g_malloc0((length + 1) * sizeof(GType)));
 
     JS::RootedObject elem_obj(context), array(context, array_value.toObjectOrNull());
     JS::RootedValue elem(context);
@@ -802,31 +802,28 @@ gjs_gtypearray_to_array(JSContext   *context,
         GType gtype;
 
         elem = JS::UndefinedValue();
-        if (!JS_GetElement(context, array, i, &elem)) {
-            g_free(result);
-            gjs_throw(context, "Missing array element %u", i);
+        if (!JS_GetElement(context, array, i, &elem))
             return false;
-        }
 
-        if (!elem.isObjectOrNull())
-            goto err;
+        if (!elem.isObject()) {
+            gjs_throw(context, "Invalid element in GType array");
+            return false;
+        }
 
-        elem_obj = elem.toObjectOrNull();
-        gtype = gjs_gtype_get_actual_gtype(context, elem_obj);
-        if (gtype == G_TYPE_INVALID)
-            goto err;
+        elem_obj = &elem.toObject();
+        if (!gjs_gtype_get_actual_gtype(context, elem_obj, &gtype))
+            return false;
+        if (gtype == G_TYPE_INVALID) {
+            gjs_throw(context, "Invalid element in GType array");
+            return false;
+        }
 
         result[i] = gtype;
     }
 
-    *arr_p = result;
+    *arr_p = result.release();
 
     return true;
-
- err:
-    g_free(result);
-    gjs_throw(context, "Invalid element in GType array");
-    return false;
 }
 
 GJS_JSAPI_RETURN_CONVENTION
@@ -1523,7 +1520,10 @@ gjs_value_to_g_argument(JSContext      *context,
         if (value.isObjectOrNull()) {
             GType gtype;
             JS::RootedObject obj(context, value.toObjectOrNull());
-            gtype = gjs_gtype_get_actual_gtype(context, obj);
+            if (!gjs_gtype_get_actual_gtype(context, obj, &gtype)) {
+                wrong = true;
+                break;
+            }
             if (gtype == G_TYPE_INVALID)
                 wrong = true;
             arg->v_ssize = gtype;
@@ -1679,7 +1679,11 @@ gjs_value_to_g_argument(JSContext      *context,
                     GType actual_gtype;
                     gpointer klass;
 
-                    actual_gtype = gjs_gtype_get_actual_gtype(context, obj);
+                    if (!gjs_gtype_get_actual_gtype(context, obj,
+                                                    &actual_gtype)) {
+                        wrong = true;
+                        break;
+                    }
 
                     if (actual_gtype == G_TYPE_NONE) {
                         wrong = true;
diff --git a/gi/function.cpp b/gi/function.cpp
index 82a00d56..2673eaf5 100644
--- a/gi/function.cpp
+++ b/gi/function.cpp
@@ -644,7 +644,8 @@ gjs_fill_method_instance(JSContext       *context,
             GType actual_gtype;
             gpointer klass;
 
-            actual_gtype = gjs_gtype_get_actual_gtype(context, obj);
+            if (!gjs_gtype_get_actual_gtype(context, obj, &actual_gtype))
+                return false;
 
             if (actual_gtype == G_TYPE_NONE) {
                 gjs_throw(context, "Invalid GType class passed for instance parameter");
diff --git a/gi/gtype.cpp b/gi/gtype.cpp
index 2a46cfeb..c54bae55 100644
--- a/gi/gtype.cpp
+++ b/gi/gtype.cpp
@@ -185,41 +185,43 @@ gjs_gtype_create_gtype_wrapper (JSContext *context,
     return *heap_wrapper;
 }
 
-static GType
-_gjs_gtype_get_actual_gtype(JSContext       *context,
-                            JS::HandleObject object,
-                            int              recurse)
-{
-    JSAutoRequest ar(context);
-
-    if (JS_InstanceOf(context, object, &gjs_gtype_class, NULL))
-        return GPOINTER_TO_SIZE(priv_from_js(context, object));
+GJS_JSAPI_RETURN_CONVENTION
+static bool _gjs_gtype_get_actual_gtype(JSContext* context,
+                                        JS::HandleObject object,
+                                        GType* gtype_out, int recurse) {
+    if (JS_InstanceOf(context, object, &gjs_gtype_class, nullptr)) {
+        *gtype_out = GPOINTER_TO_SIZE(priv_from_js(context, object));
+        return true;
+    }
 
     JS::RootedValue gtype_val(context);
 
     /* OK, we don't have a GType wrapper object -- grab the "$gtype"
      * property on that and hope it's a GType wrapper object */
-    if (!JS_GetProperty(context, object, "$gtype", &gtype_val) ||
-        !gtype_val.isObject()) {
-
+    if (!JS_GetProperty(context, object, "$gtype", &gtype_val))
+        return false;
+    if (!gtype_val.isObject()) {
         /* OK, so we're not a class. But maybe we're an instance. Check
            for "constructor" and recurse on that. */
         if (!JS_GetProperty(context, object, "constructor", &gtype_val))
-            return G_TYPE_INVALID;
+            return false;
     }
 
     if (recurse > 0 && gtype_val.isObject()) {
         JS::RootedObject gtype_obj(context, &gtype_val.toObject());
-        return _gjs_gtype_get_actual_gtype(context, gtype_obj, recurse - 1);
+        return _gjs_gtype_get_actual_gtype(context, gtype_obj, gtype_out,
+                                           recurse - 1);
     }
 
-    return G_TYPE_INVALID;
+    *gtype_out = G_TYPE_INVALID;
+    return true;
 }
 
-GType
-gjs_gtype_get_actual_gtype(JSContext       *context,
-                           JS::HandleObject object)
-{
+bool gjs_gtype_get_actual_gtype(JSContext* context, JS::HandleObject object,
+                                GType* gtype_out) {
+    g_assert(gtype_out && "Missing return location");
+    JSAutoRequest ar(context);
+
     /* 2 means: recurse at most three times (including this
        call).
        The levels are calculated considering that, in the
@@ -228,7 +230,7 @@ gjs_gtype_get_actual_gtype(JSContext       *context,
        GType value.
      */
 
-    return _gjs_gtype_get_actual_gtype(context, object, 2);
+    return _gjs_gtype_get_actual_gtype(context, object, gtype_out, 2);
 }
 
 bool
diff --git a/gi/gtype.h b/gi/gtype.h
index 3880b74f..d6f52582 100644
--- a/gi/gtype.h
+++ b/gi/gtype.h
@@ -38,10 +38,9 @@ GJS_JSAPI_RETURN_CONVENTION
 JSObject * gjs_gtype_create_gtype_wrapper (JSContext *context,
                                            GType      gtype);
 
-/* FIXME: This should be GJS_JSAPI_RETURN_CONVENTION, but isn't */
-GJS_USE
-GType       gjs_gtype_get_actual_gtype(JSContext       *context,
-                                       JS::HandleObject object);
+GJS_JSAPI_RETURN_CONVENTION
+bool gjs_gtype_get_actual_gtype(JSContext* context, JS::HandleObject object,
+                                GType* gtype_out);
 
 GJS_USE
 bool        gjs_typecheck_gtype         (JSContext             *context,
diff --git a/gi/private.cpp b/gi/private.cpp
index 6bdf5691..5c938e7d 100644
--- a/gi/private.cpp
+++ b/gi/private.cpp
@@ -51,7 +51,9 @@ static bool gjs_override_property(JSContext* cx, unsigned argc, JS::Value* vp) {
                              "type", &type))
         return false;
 
-    GType gtype = gjs_gtype_get_actual_gtype(cx, type);
+    GType gtype;
+    if (!gjs_gtype_get_actual_gtype(cx, type, &gtype))
+        return false;
     if (gtype == G_TYPE_INVALID) {
         gjs_throw(cx, "Invalid parameter type was not a GType");
         return false;
@@ -163,7 +165,9 @@ static bool get_interface_gtypes(JSContext* cx, JS::HandleObject interfaces,
         }
 
         JS::RootedObject iface(cx, &iface_val.toObject());
-        GType iface_type = gjs_gtype_get_actual_gtype(cx, iface);
+        GType iface_type;
+        if (!gjs_gtype_get_actual_gtype(cx, iface, &iface_type))
+            return false;
         if (iface_type == G_TYPE_INVALID) {
             gjs_throw(
                 cx, "Invalid parameter interfaces (element %d was not a GType)",
@@ -354,7 +358,9 @@ static bool gjs_signal_new(JSContext* cx, unsigned argc, JS::Value* vp) {
             accumulator = NULL;
     }
 
-    GType return_type = gjs_gtype_get_actual_gtype(cx, return_gtype_obj);
+    GType return_type;
+    if (!gjs_gtype_get_actual_gtype(cx, return_gtype_obj, &return_type))
+        return false;
 
     if (accumulator == g_signal_accumulator_true_handled &&
         return_type != G_TYPE_BOOLEAN) {
@@ -378,10 +384,13 @@ static bool gjs_signal_new(JSContext* cx, unsigned argc, JS::Value* vp) {
         }
 
         JS::RootedObject gjs_gtype(cx, &gtype_val.toObject());
-        params[ix] = gjs_gtype_get_actual_gtype(cx, gjs_gtype);
+        if (!gjs_gtype_get_actual_gtype(cx, gjs_gtype, &params[ix]))
+            return false;
     }
 
-    GType gtype = gjs_gtype_get_actual_gtype(cx, gtype_obj);
+    GType gtype;
+    if (!gjs_gtype_get_actual_gtype(cx, gtype_obj, &gtype))
+        return false;
 
     unsigned signal_id = g_signal_newv(
         signal_name.get(), gtype, GSignalFlags(flags),
diff --git a/gi/value.cpp b/gi/value.cpp
index 3467132b..fabecca2 100644
--- a/gi/value.cpp
+++ b/gi/value.cpp
@@ -321,32 +321,38 @@ gjs_closure_new_marshaled (JSContext    *context,
     return closure;
 }
 
-GJS_USE
-static GType
-gjs_value_guess_g_type(JSContext *context,
-                       JS::Value  value)
-{
-    if (value.isNull())
-        return G_TYPE_POINTER;
-
-    if (value.isString())
-        return G_TYPE_STRING;
-
-    if (value.isInt32())
-        return G_TYPE_INT;
-
-    if (value.isDouble())
-        return G_TYPE_DOUBLE;
-
-    if (value.isBoolean())
-        return G_TYPE_BOOLEAN;
+GJS_JSAPI_RETURN_CONVENTION
+static bool gjs_value_guess_g_type(JSContext* context, JS::Value value,
+                                   GType* gtype_out) {
+    g_assert(gtype_out && "Invalid return location");
 
+    if (value.isNull()) {
+        *gtype_out = G_TYPE_POINTER;
+        return true;
+    }
+    if (value.isString()) {
+        *gtype_out = G_TYPE_STRING;
+        return true;
+    }
+    if (value.isInt32()) {
+        *gtype_out = G_TYPE_INT;
+        return true;
+    }
+    if (value.isDouble()) {
+        *gtype_out = G_TYPE_DOUBLE;
+        return true;
+    }
+    if (value.isBoolean()) {
+        *gtype_out = G_TYPE_BOOLEAN;
+        return true;
+    }
     if (value.isObject()) {
         JS::RootedObject obj(context, &value.toObject());
-        return gjs_gtype_get_actual_gtype(context, obj);
+        return gjs_gtype_get_actual_gtype(context, obj, gtype_out);
     }
 
-    return G_TYPE_INVALID;
+    *gtype_out = G_TYPE_INVALID;
+    return true;
 }
 
 static bool
@@ -374,7 +380,8 @@ gjs_value_to_g_value_internal(JSContext      *context,
     gtype = G_VALUE_TYPE(gvalue);
 
     if (gtype == 0) {
-        gtype = gjs_value_guess_g_type(context, value);
+        if (!gjs_value_guess_g_type(context, value, &gtype))
+            return false;
 
         if (gtype == G_TYPE_INVALID) {
             gjs_throw(context, "Could not guess unspecified GValue type");
@@ -658,7 +665,8 @@ gjs_value_to_g_value_internal(JSContext      *context,
             return throw_expect_type(context, value, "GType object");
 
         JS::RootedObject obj(context, &value.toObject());
-        type = gjs_gtype_get_actual_gtype(context, obj);
+        if (!gjs_gtype_get_actual_gtype(context, obj, &type))
+            return false;
         g_value_set_gtype(gvalue, type);
     } else if (g_type_is_a(gtype, G_TYPE_POINTER)) {
         if (value.isNull()) {