[gjs: 1/4] boxed: Move special GError toString() to prototype

[Cosimo Cecchi <cosimoc src gnome org>]

commit cfe79967e8c748edec0cc60f75d690ebd70aabff
Author: Philip Chimento <philip chimento gmail com>
Date:   Fri Nov 2 23:36:32 2018 -0400

    boxed: Move special GError toString() to prototype
    
    In commit 8da83b72 we moved a special case from gjs_log_exception() to
    the GError toString() function, for pretty-printing GErrors. We also
    defined the GError toString() function on GBoxed objects if they were of
    type G_TYPE_ERROR, which could happen when creating them via the
    constructor.
    
    This caused a regression in GBoxed error objects belonging to error
    domains for which introspection information was missing. They got the
    standard GBoxed "[boxed intance proxy ...]" toString(). (Unfortunately,
    as you might guess, we can't test that case in the test suite.)
    
    It's much cleaner to define the GError toString() function right on the
    GLib.Error boxed prototype, and this also fixes the regression since all
    GError types get it, even if they are missing introspection information.

 gi/boxed.cpp  | 10 +++++--
 gi/gerror.cpp | 86 ++++++++++++++++++-----------------------------------------
 gi/gerror.h   |  6 ++++-
 3 files changed, 39 insertions(+), 63 deletions(-)
---
diff --git a/gi/boxed.cpp b/gi/boxed.cpp
index 5fd9f639..1b053117 100644
--- a/gi/boxed.cpp
+++ b/gi/boxed.cpp
@@ -391,10 +391,11 @@ boxed_new(JSContext             *context,
         if (!boxed_invoke_constructor(context, obj, constructor_name, args))
             return false;
 
-        // Define the expected Error properties and a better toString()
+        // Define the expected Error properties
         if (priv->gtype == G_TYPE_ERROR) {
             JS::RootedObject gerror(context, &args.rval().toObject());
-            gjs_define_error_properties(context, gerror);
+            if (!gjs_define_error_properties(context, gerror))
+                return false;
         }
 
         return true;
@@ -1172,6 +1173,11 @@ bool gjs_define_boxed_class(JSContext* context, JS::HandleObject in_object,
                                    priv->info))
         return false;
 
+    if (priv->gtype == G_TYPE_ERROR &&
+        !JS_DefineFunction(context, prototype, "toString", gjs_gerror_to_string,
+                           0, GJS_MODULE_PROP_FLAGS))
+        return false;
+
     JS::RootedObject gtype_obj(context,
         gjs_gtype_create_gtype_wrapper(context, priv->gtype));
     if (!gtype_obj)
diff --git a/gi/gerror.cpp b/gi/gerror.cpp
index 9f591c4f..848fc0af 100644
--- a/gi/gerror.cpp
+++ b/gi/gerror.cpp
@@ -46,8 +46,6 @@ typedef struct {
 
 extern struct JSClass gjs_error_class;
 
-static void define_error_properties(JSContext *, JS::HandleObject);
-
 GJS_DEFINE_PRIV_FROM_JS(Error, gjs_error_class)
 
 GJS_NATIVE_CONSTRUCTOR_DECLARE(error)
@@ -111,7 +109,8 @@ GJS_NATIVE_CONSTRUCTOR_DECLARE(error)
     priv->gerror = g_error_new_literal(priv->domain, code, message.get());
 
     /* We assume this error will be thrown in the same line as the constructor */
-    define_error_properties(context, object);
+    if (!gjs_define_error_properties(context, object))
+        return false;
 
     GJS_NATIVE_CONSTRUCTOR_FINISH(boxed);
 
@@ -197,18 +196,13 @@ error_get_code(JSContext *context,
     return true;
 }
 
-GJS_JSAPI_RETURN_CONVENTION
-static bool
-error_to_string(JSContext *context,
-                unsigned   argc,
-                JS::Value *vp)
-{
+bool gjs_gerror_to_string(JSContext* context, unsigned argc, JS::Value* vp) {
     GJS_GET_THIS(context, argc, vp, rec, self);
 
     GjsAutoChar descr;
 
     // An error created via `new GLib.Error` will have a Boxed* private pointer,
-    // not an Error*, so we can't call regular error_to_string() on it.
+    // not an Error*, so we can't call regular gjs_gerror_to_string() on it.
     if (gjs_typecheck_boxed(context, self, nullptr, G_TYPE_ERROR, false)) {
         auto* gerror =
             static_cast<GError*>(gjs_c_struct_from_boxed(context, self));
@@ -300,7 +294,7 @@ JSPropertySpec gjs_error_proto_props[] = {
 };
 
 JSFunctionSpec gjs_error_proto_funcs[] = {
-    JS_FN("toString", error_to_string, 0, GJS_MODULE_PROP_FLAGS),
+    JS_FN("toString", gjs_gerror_to_string, 0, GJS_MODULE_PROP_FLAGS),
     JS_FS_END};
 
 static JSFunctionSpec gjs_error_constructor_funcs[] = {
@@ -386,41 +380,33 @@ find_error_domain_info(GQuark domain)
 /* define properties that JS Error() expose, such as
    fileName, lineNumber and stack
 */
-static void
-define_error_properties(JSContext       *cx,
-                        JS::HandleObject obj)
-{
+GJS_JSAPI_RETURN_CONVENTION
+bool gjs_define_error_properties(JSContext* cx, JS::HandleObject obj) {
     JS::RootedObject frame(cx);
     JS::RootedString stack(cx);
     JS::RootedString source(cx);
     uint32_t line, column;
-    JS::AutoSaveExceptionState exc(cx);
 
     if (!JS::CaptureCurrentStack(cx, &frame) ||
-        !JS::BuildStackString(cx, frame, &stack)) {
-        exc.restore();
-        return;
+        !JS::BuildStackString(cx, frame, &stack))
+        return false;
+
+    auto ok = JS::SavedFrameResult::Ok;
+    if (JS::GetSavedFrameSource(cx, frame, &source) != ok ||
+        JS::GetSavedFrameLine(cx, frame, &line) != ok ||
+        JS::GetSavedFrameColumn(cx, frame, &column) != ok) {
+        gjs_throw(cx, "Error getting saved frame information");
+        return false;
     }
 
-    JS::SavedFrameResult result;
-    result = JS::GetSavedFrameSource(cx, frame, &source);
-    g_assert(result == JS::SavedFrameResult::Ok);
-
-    result = JS::GetSavedFrameLine(cx, frame, &line);
-    g_assert(result == JS::SavedFrameResult::Ok);
-
-    result = JS::GetSavedFrameColumn(cx, frame, &column);
-    g_assert(result == JS::SavedFrameResult::Ok);
-
-    if (!gjs_object_define_property(cx, obj, GJS_STRING_STACK, stack,
-                                    JSPROP_ENUMERATE) ||
-        !gjs_object_define_property(cx, obj, GJS_STRING_FILENAME, source,
-                                    JSPROP_ENUMERATE) ||
-        !gjs_object_define_property(cx, obj, GJS_STRING_LINE_NUMBER, line,
-                                    JSPROP_ENUMERATE) ||
-        !gjs_object_define_property(cx, obj, GJS_STRING_COLUMN_NUMBER, column,
-                                    JSPROP_ENUMERATE))
-        exc.restore();
+    return gjs_object_define_property(cx, obj, GJS_STRING_STACK, stack,
+                                      JSPROP_ENUMERATE) &&
+           gjs_object_define_property(cx, obj, GJS_STRING_FILENAME, source,
+                                      JSPROP_ENUMERATE) &&
+           gjs_object_define_property(cx, obj, GJS_STRING_LINE_NUMBER, line,
+                                      JSPROP_ENUMERATE) &&
+           gjs_object_define_property(cx, obj, GJS_STRING_COLUMN_NUMBER, column,
+                                      JSPROP_ENUMERATE);
 }
 
 GJS_USE
@@ -513,8 +499,8 @@ gjs_error_from_gerror(JSContext             *context,
     g_base_info_ref( (GIBaseInfo*) priv->info);
     priv->gerror = g_error_copy(gerror);
 
-    if (add_stack)
-        define_error_properties(context, obj);
+    if (add_stack && !gjs_define_error_properties(context, obj))
+        return nullptr;
 
     return obj;
 }
@@ -624,23 +610,3 @@ bool gjs_throw_gerror(JSContext* cx, GError* error) {
 
     return false;
 }
-
-/*
- * gjs_define_error_properties:
- *
- * Define the expected properties of a JS Error object on a newly-created
- * boxed object. This is required when creating a GLib.Error via the default
- * constructor, for example: `new GLib.Error(GLib.quark_from_string('my-error'),
- * 0, 'message')`.
- *
- * This function doesn't throw an exception if it fails.
- */
-void gjs_define_error_properties(JSContext* cx, JS::HandleObject obj) {
-    JS::AutoSaveExceptionState saved_exc(cx);
-
-    define_error_properties(cx, obj);
-
-    if (!JS_DefineFunction(cx, obj, "toString", error_to_string, 0,
-                           GJS_MODULE_PROP_FLAGS))
-        saved_exc.restore();
-}
diff --git a/gi/gerror.h b/gi/gerror.h
index 2ac3e944..db0e9072 100644
--- a/gi/gerror.h
+++ b/gi/gerror.h
@@ -52,7 +52,11 @@ GJS_JSAPI_RETURN_CONVENTION
 GError *gjs_gerror_make_from_error(JSContext       *cx,
                                    JS::HandleObject obj);
 
-void gjs_define_error_properties(JSContext* cx, JS::HandleObject obj);
+GJS_JSAPI_RETURN_CONVENTION
+bool gjs_gerror_to_string(JSContext* cx, unsigned argc, JS::Value* vp);
+
+GJS_JSAPI_RETURN_CONVENTION
+bool gjs_define_error_properties(JSContext* cx, JS::HandleObject obj);
 
 bool gjs_throw_gerror(JSContext* cx, GError* error);