[gjs: 10/16] gerror: Define toString() for boxed GErrors

[Cosimo Cecchi <cosimoc src gnome org>]

commit 8da83b72b1444ab7b574d39452151b07a37431a5
Author: Philip Chimento <philip chimento gmail com>
Date:   Mon Sep 17 23:21:26 2018 -0700

    gerror: Define toString() for boxed GErrors
    
    If you create a GError with the default GLib.Error constructor (which
    translates to g_error_new_literal() in C) you end up with a slightly
    strange error object, with a Boxed* private pointer instead of an Error*
    private pointer. This error object doesn't have the usual properties that
    are expected on a JS exception (such as 'stack', etc.) and doesn't have a
    nice toString() function.
    
    Previously there was a special case in gjs_log_exception() for printing
    these objects. We move this special case to the GError toString()
    function, so that the correct behaviour is available whenever you print
    the object, not just in gjs_log_exception().
    
    In order to catch this case, we have to special case G_TYPE_ERROR in the
    boxed_new() function.

 gi/boxed.cpp                         | 26 +++++++++++-----
 gi/gerror.cpp                        | 40 ++++++++++++++++++++++--
 gi/gerror.h                          |  2 ++
 gjs/jsapi-util.cpp                   | 59 ++++++++++++------------------------
 installed-tests/js/testExceptions.js |  8 ++---
 5 files changed, 82 insertions(+), 53 deletions(-)
---
diff --git a/gi/boxed.cpp b/gi/boxed.cpp
index fcc14896..66a79c22 100644
--- a/gi/boxed.cpp
+++ b/gi/boxed.cpp
@@ -25,16 +25,17 @@
 
 #include <string.h>
 
-#include "boxed.h"
 #include "arg.h"
-#include "object.h"
+#include "boxed.h"
+#include "function.h"
+#include "gi/gerror.h"
 #include "gjs/jsapi-class.h"
 #include "gjs/jsapi-wrapper.h"
 #include "gjs/mem.h"
-#include "repo.h"
-#include "proxyutils.h"
-#include "function.h"
 #include "gtype.h"
+#include "object.h"
+#include "proxyutils.h"
+#include "repo.h"
 
 #include <util/log.h>
 
@@ -365,9 +366,18 @@ boxed_new(JSContext             *context,
          * that is, Namespace.BoxedType, or object.constructor, given that
          * object was created with the right prototype. The ID is traced from
          * the object, so it's OK to create a handle from it. */
-        return boxed_invoke_constructor(context, obj,
-            JS::HandleId::fromMarkedLocation(priv->default_constructor_name.address()),
-            args);
+        JS::HandleId constructor_name = JS::HandleId::fromMarkedLocation(
+            priv->default_constructor_name.address());
+        if (!boxed_invoke_constructor(context, obj, constructor_name, args))
+            return false;
+
+        // Define the expected Error properties and a better toString()
+        if (priv->gtype == G_TYPE_ERROR) {
+            JS::RootedObject gerror(context, &args.rval().toObject());
+            gjs_define_error_properties(context, gerror);
+        }
+
+        return true;
     } else {
         gjs_throw(context, "Unable to construct struct type %s since it has no default constructor and 
cannot be allocated directly",
                   g_base_info_get_name((GIBaseInfo*) priv->info));
diff --git a/gi/gerror.cpp b/gi/gerror.cpp
index 703be0d3..b25cf598 100644
--- a/gi/gerror.cpp
+++ b/gi/gerror.cpp
@@ -199,15 +199,31 @@ error_to_string(JSContext *context,
                 unsigned   argc,
                 JS::Value *vp)
 {
-    GJS_GET_PRIV(context, argc, vp, rec, self, Error, priv);
+    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.
+    if (gjs_typecheck_boxed(context, self, nullptr, G_TYPE_ERROR, false)) {
+        auto* gerror =
+            static_cast<GError*>(gjs_c_struct_from_boxed(context, self));
+        descr =
+            g_strdup_printf("GLib.Error %s: %s",
+                            g_quark_to_string(gerror->domain), gerror->message);
 
+        return gjs_string_from_utf8(context, descr, rec.rval());
+    }
+
+    if (!do_base_typecheck(context, self, true))
+        return false;
+    Error* priv = priv_from_js(context, self);
     if (priv == NULL)
         return false;
 
     /* We follow the same pattern as standard JS errors, at the expense of
        hiding some useful information */
 
-    GjsAutoChar descr;
     if (priv->gerror == NULL) {
         descr = g_strdup_printf("%s.%s",
                                 g_base_info_get_namespace(priv->info),
@@ -610,3 +626,23 @@ 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 03e0c52d..1df4673b 100644
--- a/gi/gerror.h
+++ b/gi/gerror.h
@@ -47,6 +47,8 @@ bool      gjs_typecheck_gerror         (JSContext             *context,
 GError *gjs_gerror_make_from_error(JSContext       *cx,
                                    JS::HandleObject obj);
 
+void gjs_define_error_properties(JSContext* cx, JS::HandleObject obj);
+
 bool gjs_throw_gerror(JSContext* cx, GError* error);
 
 G_END_DECLS
diff --git a/gjs/jsapi-util.cpp b/gjs/jsapi-util.cpp
index ce66146c..8e5d0ca0 100644
--- a/gjs/jsapi-util.cpp
+++ b/gjs/jsapi-util.cpp
@@ -37,7 +37,6 @@
 #include "jsapi-class.h"
 #include "jsapi-util.h"
 #include "context-private.h"
-#include <gi/boxed.h>
 
 #include <string.h>
 #include <math.h>
@@ -485,47 +484,28 @@ gjs_value_debug_string(JSContext      *context,
     return debugstr;
 }
 
-static char *
-utf8_exception_from_non_gerror_value(JSContext      *cx,
-                                     JS::HandleValue exc)
-{
-    JS::RootedString exc_str(cx, JS::ToString(cx, exc));
-    if (!exc_str)
-        return nullptr;
-
-    GjsAutoJSChar utf8_exception = JS_EncodeStringToUTF8(cx, exc_str);
-    return utf8_exception.copy();
-}
-
 bool
 gjs_log_exception_full(JSContext       *context,
                        JS::HandleValue  exc,
                        JS::HandleString message)
 {
-    char *utf8_exception;
     bool is_syntax;
 
     JS_BeginRequest(context);
     JS::RootedObject exc_obj(context);
+    JS::RootedString exc_str(context, JS::ToString(context, exc));
+    GjsAutoJSChar utf8_exception;
+    if (exc_str)
+        utf8_exception = JS_EncodeStringToUTF8(context, exc_str);
+    if (!utf8_exception)
+        JS_ClearPendingException(context);
 
     is_syntax = false;
-
-    if (!exc.isObject()) {
-        utf8_exception = utf8_exception_from_non_gerror_value(context, exc);
-    } else {
+    if (exc.isObject()) {
         exc_obj = &exc.toObject();
-        if (gjs_typecheck_boxed(context, exc_obj, NULL, G_TYPE_ERROR, false)) {
-            GError *gerror = (GError *) gjs_c_struct_from_boxed(context, exc_obj);
-            utf8_exception = g_strdup_printf("GLib.Error %s: %s",
-                                             g_quark_to_string(gerror->domain),
-                                             gerror->message);
-        } else {
-            const JSClass *syntax_error =
-                js::Jsvalify(js::ProtoKeyToClass(JSProto_SyntaxError));
-            is_syntax = JS_InstanceOf(context, exc_obj, syntax_error, nullptr);
-
-            utf8_exception = utf8_exception_from_non_gerror_value(context, exc);
-        }
+        const JSClass* syntax_error =
+            js::Jsvalify(js::ProtoKeyToClass(JSProto_SyntaxError));
+        is_syntax = JS_InstanceOf(context, exc_obj, syntax_error, nullptr);
     }
 
     GjsAutoJSChar utf8_message;
@@ -557,10 +537,10 @@ gjs_log_exception_full(JSContext       *context,
         lineNumber = js_lineNumber.toInt32();
 
         if (message) {
-            g_critical("JS ERROR: %s: %s @ %s:%u", utf8_message.get(), utf8_exception,
-                       utf8_filename.get(), lineNumber);
+            g_critical("JS ERROR: %s: %s @ %s:%u", utf8_message.get(),
+                       utf8_exception.get(), utf8_filename.get(), lineNumber);
         } else {
-            g_critical("JS ERROR: %s @ %s:%u", utf8_exception,
+            g_critical("JS ERROR: %s @ %s:%u", utf8_exception.get(),
                        utf8_filename.get(), lineNumber);
         }
 
@@ -578,19 +558,20 @@ gjs_log_exception_full(JSContext       *context,
 
         if (message) {
             if (utf8_stack)
-                g_warning("JS ERROR: %s: %s\n%s", utf8_message.get(), utf8_exception, utf8_stack.get());
+                g_warning("JS ERROR: %s: %s\n%s", utf8_message.get(),
+                          utf8_exception.get(), utf8_stack.get());
             else
-                g_warning("JS ERROR: %s: %s", utf8_message.get(), utf8_exception);
+                g_warning("JS ERROR: %s: %s", utf8_message.get(),
+                          utf8_exception.get());
         } else {
             if (utf8_stack)
-                g_warning("JS ERROR: %s\n%s", utf8_exception, utf8_stack.get());
+                g_warning("JS ERROR: %s\n%s", utf8_exception.get(),
+                          utf8_stack.get());
             else
-                g_warning("JS ERROR: %s", utf8_exception);
+                g_warning("JS ERROR: %s", utf8_exception.get());
         }
     }
 
-    g_free(utf8_exception);
-
     JS_EndRequest(context);
 
     return true;
diff --git a/installed-tests/js/testExceptions.js b/installed-tests/js/testExceptions.js
index 5c5e05f3..4fa5a55a 100644
--- a/installed-tests/js/testExceptions.js
+++ b/installed-tests/js/testExceptions.js
@@ -116,13 +116,13 @@ describe('logError', function () {
 
     it('logs an error with no stack trace for an error created with the GLib.Error constructor', function () 
{
         GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_WARNING,
-            'JS ERROR: Gio.IOErrorEnum: a message');
+            'JS ERROR: Gio.IOErrorEnum: a message\n*');
         logError(new GLib.Error(Gio.IOErrorEnum, 0, 'a message'));
     });
 
     it('logs the quark for a JS-created GError type', function () {
         GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_WARNING,
-            'JS ERROR: GLib.Error my-error: a message');
+            'JS ERROR: GLib.Error my-error: a message\n*');
         logError(new GLib.Error(GLib.quark_from_string('my-error'), 0, 'a message'));
     });
 
@@ -157,13 +157,13 @@ describe('logError', function () {
 
     it('logs a GLib.Error with prefix', function () {
         GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_WARNING,
-            'JS ERROR: prefix: Gio.IOErrorEnum: a message');
+            'JS ERROR: prefix: Gio.IOErrorEnum: a message\n*');
         logError(new GLib.Error(Gio.IOErrorEnum, 0, 'a message'), 'prefix');
     });
 
     it('logs a JS-created GLib.Error with prefix', function () {
         GLib.test_expect_message('Gjs', GLib.LogLevelFlags.LEVEL_WARNING,
-            'JS ERROR: prefix: GLib.Error my-error: a message');
+            'JS ERROR: prefix: GLib.Error my-error: a message\n*');
         logError(new GLib.Error(GLib.quark_from_string('my-error'), 0, 'a message'), 'prefix');
     });
 });