[gjs: 7/8] js: Fix return values of not-really-infallible functions

[Cosimo Cecchi <cosimoc src gnome org>]

commit fb100c9a5cb74d2d21cc0d845a2602f77e223d9d
Author: Philip Chimento <philip chimento gmail com>
Date:   Tue Jun 12 00:45:57 2018 -0700

    js: Fix return values of not-really-infallible functions
    
    While refactoring object.cpp I noticed that gjs_define_object_class()
    doesn't do proper error checking; the JSAPI functions used within it can
    throw, but it doesn't check that.
    
    Fixing this also had a cascade effect on
    gjs_object_define_static_methods() and gjs_define_param_class(), as well
    as some code in fundamental.cpp and repo.cpp.
    
    Similarly, find_vfunc_info() in object.cpp could also throw but the
    return value was never checked.

 gi/fundamental.cpp | 24 +++++++++------------
 gi/object.cpp      | 63 +++++++++++++++++++++++++++++-------------------------
 gi/object.h        |  4 ++--
 gi/param.cpp       | 21 ++++++++++--------
 gi/param.h         |  2 +-
 gi/repo.cpp        |  8 ++++---
 6 files changed, 64 insertions(+), 58 deletions(-)
---
diff --git a/gi/fundamental.cpp b/gi/fundamental.cpp
index 44502897..e3475aa1 100644
--- a/gi/fundamental.cpp
+++ b/gi/fundamental.cpp
@@ -614,14 +614,12 @@ gjs_lookup_fundamental_prototype(JSContext    *context,
 
     g_assert(constructor);
 
-    if (!gjs_object_get_property(context, constructor,
-                                 GJS_STRING_PROTOTYPE, &value))
+    JS::RootedObject prototype(context);
+    if (!gjs_object_require_property(context, constructor, "constructor object",
+                                     GJS_STRING_PROTOTYPE, &prototype))
         return NULL;
 
-    if (G_UNLIKELY (!value.isObjectOrNull()))
-        return NULL;
-
-    return value.toObjectOrNull();
+    return prototype;
 }
 
 static JSObject*
@@ -697,10 +695,8 @@ gjs_define_fundamental_class(JSContext              *context,
                                 /* funcs of constructor, MyConstructor.myfunc() */
                                 NULL,
                                 prototype,
-                                constructor)) {
-        gjs_log_exception(context);
-        g_error("Can't init class %s", constructor_name);
-    }
+                                constructor))
+        return false;
 
     /* Put the info in the prototype */
     priv = g_slice_new0(Fundamental);
@@ -737,13 +733,13 @@ gjs_define_fundamental_class(JSContext              *context,
                   g_base_info_get_name ((GIBaseInfo *)priv->info));
     }
 
-    gjs_object_define_static_methods(context, constructor, gtype, info);
+    if (!gjs_object_define_static_methods(context, constructor, gtype, info))
+        return false;
 
     JS::RootedObject gtype_obj(context,
         gjs_gtype_create_gtype_wrapper(context, gtype));
-    JS_DefineProperty(context, constructor, "$gtype", gtype_obj, JSPROP_PERMANENT);
-
-    return true;
+    return JS_DefineProperty(context, constructor, "$gtype", gtype_obj,
+                             JSPROP_PERMANENT);
 }
 
 JSObject*
diff --git a/gi/object.cpp b/gi/object.cpp
index 9e80804b..2ef5dede 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -2002,8 +2002,9 @@ gjs_lookup_object_constructor_from_info(JSContext    *context,
            we need to define it first.
         */
         JS::RootedObject ignored(context);
-        gjs_define_object_class(context, in_object, NULL, gtype, &constructor,
-                                &ignored);
+        if (!gjs_define_object_class(context, in_object, nullptr, gtype,
+                                     &constructor, &ignored))
+            return nullptr;
     } else {
         if (G_UNLIKELY (!value.isObject()))
             return NULL;
@@ -2027,15 +2028,12 @@ gjs_lookup_object_prototype_from_info(JSContext    *context,
     if (G_UNLIKELY(!constructor))
         return NULL;
 
-    JS::RootedValue value(context);
-    if (!gjs_object_get_property(context, constructor,
-                                 GJS_STRING_PROTOTYPE, &value))
-        return NULL;
-
-    if (G_UNLIKELY (!value.isObjectOrNull()))
+    JS::RootedObject prototype(context);
+    if (!gjs_object_require_property(context, constructor, "constructor object",
+                                     GJS_STRING_PROTOTYPE, &prototype))
         return NULL;
 
-    return value.toObjectOrNull();
+    return prototype;
 }
 
 static JSObject *
@@ -2310,7 +2308,7 @@ JSFunctionSpec gjs_object_instance_proto_funcs[] = {
     JS_FS_END
 };
 
-void
+bool
 gjs_object_define_static_methods(JSContext       *context,
                                  JS::HandleObject constructor,
                                  GType            gtype,
@@ -2337,14 +2335,14 @@ gjs_object_define_static_methods(JSContext       *context,
          */
         if (!(flags & GI_FUNCTION_IS_METHOD)) {
             if (!gjs_define_function(context, constructor, gtype, meth_info))
-                gjs_log_exception(context);
+                return false;
         }
     }
 
     GjsAutoInfo<GIStructInfo> gtype_struct =
         g_object_info_get_class_struct(object_info);
     if (gtype_struct == NULL)
-        return;
+        return true;  /* not an error? */
 
     n_methods = g_struct_info_get_n_methods(gtype_struct);
 
@@ -2353,8 +2351,10 @@ gjs_object_define_static_methods(JSContext       *context,
             g_struct_info_get_method(gtype_struct, i);
 
         if (!gjs_define_function(context, constructor, gtype, meth_info))
-            gjs_log_exception(context);
+            return false;
     }
+
+    return true;
 }
 
 JS::Symbol*
@@ -2370,7 +2370,7 @@ ObjectInstance::hook_up_vfunc_symbol(JSContext *cx)
     return hook_up_vfunc_root;
 }
 
-void
+bool
 gjs_define_object_class(JSContext              *context,
                         JS::HandleObject        in_object,
                         GIObjectInfo           *info,
@@ -2430,7 +2430,7 @@ gjs_define_object_class(JSContext              *context,
 
     parent_type = g_type_parent(gtype);
     if (parent_type != G_TYPE_INVALID)
-       parent_proto = gjs_lookup_object_prototype(context, parent_type);
+        parent_proto = gjs_lookup_object_prototype(context, parent_type);
 
     ns = gjs_get_names_from_gtype_and_gi_info(gtype, (GIBaseInfo *) info,
                                               &constructor_name);
@@ -2449,9 +2449,8 @@ gjs_define_object_class(JSContext              *context,
                                 /* funcs of constructor, MyConstructor.myfunc() */
                                 NULL,
                                 prototype,
-                                constructor)) {
-        g_error("Can't init class %s", constructor_name);
-    }
+                                constructor))
+        return false;
 
     /* Hook_up_vfunc can't be included in gjs_object_instance_proto_funcs
      * because it's a custom symbol. */
@@ -2460,7 +2459,7 @@ gjs_define_object_class(JSContext              *context,
     if (!JS_DefineFunctionById(context, prototype, hook_up_vfunc,
                                &ObjectInstance::hook_up_vfunc, 3,
                                GJS_MODULE_PROP_FLAGS))
-        return;
+        return false;
 
     GJS_INC_COUNTER(object);
     priv = g_slice_new0(ObjectInstance);
@@ -2471,12 +2470,13 @@ gjs_define_object_class(JSContext              *context,
               prototype.get(), JS_GetClass(prototype), in_object.get());
 
     if (info)
-        gjs_object_define_static_methods(context, constructor, gtype, info);
+        if (!gjs_object_define_static_methods(context, constructor, gtype, info))
+            return false;
 
     JS::RootedObject gtype_obj(context,
         gjs_gtype_create_gtype_wrapper(context, gtype));
-    JS_DefineProperty(context, constructor, "$gtype", gtype_obj,
-                      JSPROP_PERMANENT);
+    return JS_DefineProperty(context, constructor, "$gtype", gtype_obj,
+                             JSPROP_PERMANENT);
 }
 
 JSObject*
@@ -2600,7 +2600,7 @@ ObjectInstance::typecheck_object(JSContext *context,
 }
 
 
-static void
+static bool
 find_vfunc_info (JSContext *context,
                  GType implementor_gtype,
                  GIBaseInfo *vfunc_info,
@@ -2632,7 +2632,7 @@ find_vfunc_info (JSContext *context,
             g_type_class_unref(implementor_class);
             gjs_throw (context, "Couldn't find GType of implementor of interface %s.",
                        g_type_name(ancestor_gtype));
-            return;
+            return false;
         }
 
         *implementor_vtable_ret = implementor_iface_class;
@@ -2657,12 +2657,13 @@ find_vfunc_info (JSContext *context,
             /* We have a field with the same name, but it's not a callback.
              * There's no hope of being another field with a correct name,
              * so just abort early. */
-            return;
+            return true;
         } else {
             *field_info_ret = std::move(field_info);
-            return;
+            return true;
         }
     }
+    return true;
 }
 
 bool
@@ -2738,7 +2739,10 @@ ObjectInstance::hook_up_vfunc_impl(JSContext          *cx,
 
     void *implementor_vtable;
     GjsAutoInfo<GIFieldInfo> field_info;
-    find_vfunc_info(cx, m_gtype, vfunc, name, &implementor_vtable, &field_info);
+    if (!find_vfunc_info(cx, m_gtype, vfunc, name, &implementor_vtable,
+                         &field_info))
+        return false;
+
     if (field_info != NULL) {
         gint offset;
         gpointer method_ptr;
@@ -3324,8 +3328,9 @@ gjs_register_type(JSContext *cx,
     /* create a custom JSClass */
     JS::RootedObject module(cx, gjs_lookup_private_namespace(cx));
     JS::RootedObject constructor(cx), prototype(cx);
-    gjs_define_object_class(cx, module, nullptr, instance_type, &constructor,
-                            &prototype);
+    if (!gjs_define_object_class(cx, module, nullptr, instance_type, &constructor,
+                                 &prototype))
+        return false;
 
     ObjectInstance *priv = priv_from_js(cx, prototype);
     g_type_set_qdata(instance_type, gjs_object_priv_quark(), priv);
diff --git a/gi/object.h b/gi/object.h
index 1f1dce8c..a7e855b0 100644
--- a/gi/object.h
+++ b/gi/object.h
@@ -32,7 +32,7 @@
 
 G_BEGIN_DECLS
 
-void gjs_define_object_class(JSContext              *context,
+bool gjs_define_object_class(JSContext              *cx,
                              JS::HandleObject        in_object,
                              GIObjectInfo           *info,
                              GType                   gtype,
@@ -64,7 +64,7 @@ void gjs_object_shutdown_toggle_queue(void);
 void gjs_object_context_dispose_notify(void    *data,
                                        GObject *where_the_object_was);
 
-void gjs_object_define_static_methods(JSContext       *context,
+bool gjs_object_define_static_methods(JSContext       *context,
                                       JS::HandleObject constructor,
                                       GType            gtype,
                                       GIObjectInfo    *object_info);
diff --git a/gi/param.cpp b/gi/param.cpp
index 92a319a4..5cbf17cb 100644
--- a/gi/param.cpp
+++ b/gi/param.cpp
@@ -202,13 +202,12 @@ gjs_lookup_param_prototype(JSContext    *context)
     return value.toObjectOrNull();
 }
 
-void
+bool
 gjs_define_param_class(JSContext       *context,
                        JS::HandleObject in_object)
 {
     const char *constructor_name;
     JS::RootedObject prototype(context), constructor(context);
-    GIObjectInfo *info;
 
     constructor_name = "ParamSpec";
 
@@ -225,21 +224,25 @@ gjs_define_param_class(JSContext       *context,
                                 /* funcs of constructor, MyConstructor.myfunc() */
                                 gjs_param_constructor_funcs,
                                 &prototype,
-                                &constructor)) {
-        g_error("Can't init class %s", constructor_name);
-    }
+                                &constructor))
+        return false;
 
     JS::RootedObject gtype_obj(context,
         gjs_gtype_create_gtype_wrapper(context, G_TYPE_PARAM));
-    JS_DefineProperty(context, constructor, "$gtype", gtype_obj, JSPROP_PERMANENT);
+    if (!gtype_obj ||
+        !JS_DefineProperty(context, constructor, "$gtype", gtype_obj,
+                           JSPROP_PERMANENT))
+        return false;
 
-    info = (GIObjectInfo*)g_irepository_find_by_gtype(g_irepository_get_default(), G_TYPE_PARAM);
-    gjs_object_define_static_methods(context, constructor, G_TYPE_PARAM, info);
-    g_base_info_unref( (GIBaseInfo*) info);
+    GjsAutoInfo<GIObjectInfo> info =
+        g_irepository_find_by_gtype(g_irepository_get_default(), G_TYPE_PARAM);
+    if (!gjs_object_define_static_methods(context, constructor, G_TYPE_PARAM, info))
+        return false;
 
     gjs_debug(GJS_DEBUG_GPARAM, "Defined class %s prototype is %p class %p in object %p",
               constructor_name, prototype.get(), JS_GetClass(prototype),
               in_object.get());
+    return true;
 }
 
 JSObject*
diff --git a/gi/param.h b/gi/param.h
index 10a1de71..f5ed5467 100644
--- a/gi/param.h
+++ b/gi/param.h
@@ -31,7 +31,7 @@
 
 G_BEGIN_DECLS
 
-void gjs_define_param_class(JSContext       *context,
+bool gjs_define_param_class(JSContext       *context,
                             JS::HandleObject in_object);
 
 GParamSpec *gjs_g_param_from_param (JSContext       *context,
diff --git a/gi/repo.cpp b/gi/repo.cpp
index d819dcaa..bb1e5422 100644
--- a/gi/repo.cpp
+++ b/gi/repo.cpp
@@ -425,11 +425,13 @@ gjs_define_info(JSContext       *context,
             gtype = g_registered_type_info_get_g_type((GIRegisteredTypeInfo*)info);
 
             if (g_type_is_a (gtype, G_TYPE_PARAM)) {
-                gjs_define_param_class(context, in_object);
+                if (!gjs_define_param_class(context, in_object))
+                    return false;
             } else if (g_type_is_a (gtype, G_TYPE_OBJECT)) {
                 JS::RootedObject ignored1(context), ignored2(context);
-                gjs_define_object_class(context, in_object, info, gtype,
-                                        &ignored1, &ignored2);
+                if (!gjs_define_object_class(context, in_object, info, gtype,
+                                             &ignored1, &ignored2))
+                    return false;
             } else if (G_TYPE_IS_INSTANTIATABLE(gtype)) {
                 JS::RootedObject ignored1(context), ignored2(context);
                 if (!gjs_define_fundamental_class(context, in_object,