[gjs: 4/5] gerror: Handle any value when converting thrown value to GError




commit 2df5407c156480102f24f921a64ac2e5d8f498fb
Author: Philip Chimento <philip chimento gmail com>
Date:   Sat Apr 17 21:45:03 2021 -0700

    gerror: Handle any value when converting thrown value to GError
    
    In JS you can throw not only Error objects, but any value. If throwing a
    value from a signal handler or vfunc with a GError** parameter, then that
    value should be converted to a GError whether it is an Error object or
    not.
    
    Rename gjs_gerror_make_from_error() to gjs_gerror_make_from_thrown_value()
    and refactor it to get the pending exception (thrown value) from the
    context, and convert that into a GError.
    
    If the object does not have a 'name' and 'message' property then it's
    converted into a GJS_JS_ERROR_ERROR with an appropriate debug message; if
    it does, then it's converted into the appropriate type of GJS_JS_ERROR.

 gi/function.cpp                         | 36 ++++++++++----------------
 gi/gerror.cpp                           | 45 +++++++++++++++++++++++++--------
 gi/gerror.h                             |  3 +--
 installed-tests/js/testGIMarshalling.js | 42 ++++++++++++++++++++++++++++++
 4 files changed, 92 insertions(+), 34 deletions(-)
---
diff --git a/gi/function.cpp b/gi/function.cpp
index 5810adea..b0a27a7c 100644
--- a/gi/function.cpp
+++ b/gi/function.cpp
@@ -378,10 +378,6 @@ void GjsCallbackTrampoline::callback_closure(GIArgument** args, void* result) {
     JS::RootedValue rval(context);
 
     g_callable_info_load_return_type(m_info, &ret_type);
-    GIArgument* error_argument = nullptr;
-
-    if (g_callable_info_can_throw_gerror(m_info))
-        error_argument = args[n_args + c_args_offset];
 
     if (!callback_closure_inner(context, this_object, &rval, args, &ret_type,
                                 n_args, c_args_offset, result)) {
@@ -409,25 +405,21 @@ void GjsCallbackTrampoline::callback_closure(GIArgument** args, void* result) {
             set_return_ffi_arg_from_giargument(&ret_type, result, &argument);
         }
 
-        // If an exception has been thrown, log it. Unless the callback has a
-        // GError** argument, then try to make a GError from it if it's an
-        // object (and otherwise fall back to logging)
-        if (error_argument) {
-            JS::RootedValue v_exception(context);
-            JS_GetPendingException(context, &v_exception);
-            if (v_exception.isObject()) {
-                JS_ClearPendingException(context);  // don't log
-                JS::RootedObject exc_object(context, &v_exception.toObject());
-                GError* local_error =
-                    gjs_gerror_make_from_error(context, exc_object);
-
-                // the GError ** pointer is the last argument, and is not
-                // included in the n_args
-                auto* gerror = gjs_arg_get<GError**>(error_argument);
-                g_propagate_error(gerror, local_error);
-            }
+        // If the callback has a GError** argument, then make a GError from the
+        // value that was thrown. Otherwise, log it as "uncaught" (critical
+        // instead of warning)
+
+        if (!g_callable_info_can_throw_gerror(m_info)) {
+            gjs_log_exception_uncaught(context);
+            return;
         }
-        gjs_log_exception_uncaught(context);
+
+        // The GError** pointer is the last argument, and is not included in
+        // the n_args
+        GIArgument* error_argument = args[n_args + c_args_offset];
+        auto* gerror = gjs_arg_get<GError**>(error_argument);
+        GError* local_error = gjs_gerror_make_from_thrown_value(context);
+        g_propagate_error(gerror, local_error);
     }
 }
 
diff --git a/gi/gerror.cpp b/gi/gerror.cpp
index ddbae7db..08745a59 100644
--- a/gi/gerror.cpp
+++ b/gi/gerror.cpp
@@ -6,6 +6,8 @@
 
 #include <stdint.h>
 
+#include <string>
+
 #include <girepository.h>
 #include <glib-object.h>
 
@@ -443,14 +445,20 @@ static GError* gerror_from_error_impl(JSContext* cx, JS::HandleObject obj) {
     if (!JS_GetPropertyById(cx, obj, atoms.name(), &v_name))
         return nullptr;
 
-    JS::UniqueChars name = gjs_string_to_utf8(cx, v_name);
-    if (!name)
-        return nullptr;
-
     JS::RootedValue v_message(cx);
     if (!JS_GetPropertyById(cx, obj, atoms.message(), &v_message))
         return nullptr;
 
+    if (!v_name.isString() || !v_message.isString()) {
+        return g_error_new_literal(
+            GJS_JS_ERROR, GJS_JS_ERROR_ERROR,
+            "Object thrown with unexpected name or message property");
+    }
+
+    JS::UniqueChars name = gjs_string_to_utf8(cx, v_name);
+    if (!name)
+        return nullptr;
+
     JS::UniqueChars message = gjs_string_to_utf8(cx, v_message);
     if (!message)
         return nullptr;
@@ -467,15 +475,32 @@ static GError* gerror_from_error_impl(JSContext* cx, JS::HandleObject obj) {
 }
 
 /*
- * gjs_gerror_make_from_error:
+ * gjs_gerror_make_from_thrown_value:
  *
- * Attempts to convert a JavaScript exception into a #GError. This function is
- * infallible and will always return a #GError with some message, even if the
- * exception object couldn't be converted.
+ * Attempts to convert a JavaScript thrown value (pending on @cx) into a
+ * #GError. This function is infallible and will always return a #GError with
+ * some message, even if the exception value couldn't be converted.
+ *
+ * Clears the pending exception on @cx.
  *
  * Returns: (transfer full): a new #GError
  */
-GError* gjs_gerror_make_from_error(JSContext* cx, JS::HandleObject obj) {
+GError* gjs_gerror_make_from_thrown_value(JSContext* cx) {
+    g_assert(JS_IsExceptionPending(cx) &&
+             "Should be called when an exception is pending");
+
+    JS::RootedValue exc(cx);
+    JS_GetPendingException(cx, &exc);
+    JS_ClearPendingException(cx);  // don't log
+
+    if (!exc.isObject()) {
+        return g_error_new(GJS_JS_ERROR, GJS_JS_ERROR_ERROR,
+                           "Non-exception %s value %s thrown",
+                           JS::InformalValueTypeName(exc),
+                           gjs_debug_value(exc).c_str());
+    }
+
+    JS::RootedObject obj(cx, &exc.toObject());
     GError* retval = gerror_from_error_impl(cx, obj);
     if (retval)
         return retval;
@@ -484,7 +509,7 @@ GError* gjs_gerror_make_from_error(JSContext* cx, JS::HandleObject obj) {
     // the exception into one
     gjs_log_exception(cx);  // log the inner exception
     return g_error_new_literal(GJS_JS_ERROR, GJS_JS_ERROR_INTERNAL_ERROR,
-                               "Failed to convert JS exception into GError");
+                               "Failed to convert JS thrown value into GError");
 }
 
 /*
diff --git a/gi/gerror.h b/gi/gerror.h
index f64112c0..8841a994 100644
--- a/gi/gerror.h
+++ b/gi/gerror.h
@@ -158,8 +158,7 @@ class ErrorInstance : public GIWrapperInstance<ErrorBase, ErrorPrototype,
 };
 
 GJS_JSAPI_RETURN_CONVENTION
-GError *gjs_gerror_make_from_error(JSContext       *cx,
-                                   JS::HandleObject obj);
+GError* gjs_gerror_make_from_thrown_value(JSContext* cx);
 
 GJS_JSAPI_RETURN_CONVENTION
 bool gjs_define_error_properties(JSContext* cx, JS::HandleObject obj);
diff --git a/installed-tests/js/testGIMarshalling.js b/installed-tests/js/testGIMarshalling.js
index 0be8fbd2..c8dec15c 100644
--- a/installed-tests/js/testGIMarshalling.js
+++ b/installed-tests/js/testGIMarshalling.js
@@ -1193,6 +1193,31 @@ let VFuncTester = GObject.registerClass(class VFuncTester extends GIMarshallingT
                 code: GLib.SpawnError.TOO_BIG,
                 message: 'This test is Too Big to Fail',
             });
+        case 4:
+            throw null;  // eslint-disable-line no-throw-literal
+        case 5:
+            throw undefined;  // eslint-disable-line no-throw-literal
+        case 6:
+            throw 42;  // eslint-disable-line no-throw-literal
+        case 7:
+            throw true;  // eslint-disable-line no-throw-literal
+        case 8:
+            throw 'a string';  // eslint-disable-line no-throw-literal
+        case 9:
+            throw 42n;  // eslint-disable-line no-throw-literal
+        case 10:
+            throw Symbol('a symbol');
+        case 11:
+            throw {plain: 'object'};  // eslint-disable-line no-throw-literal
+        case 12:
+            // eslint-disable-next-line no-throw-literal
+            throw {name: 'TypeError', message: 'an error message'};
+        case 13:
+            // eslint-disable-next-line no-throw-literal
+            throw {name: 1, message: 'an error message'};
+        case 14:
+            // eslint-disable-next-line no-throw-literal
+            throw {name: 'TypeError', message: false};
         }
     }
 
@@ -1373,6 +1398,23 @@ describe('Virtual function', function () {
         }
     });
 
+    it('marshals an error out parameter with a primitive value', function () {
+        expect(() => tester.vfunc_meth_with_error(4)).toThrowError(/null/);
+        expect(() => tester.vfunc_meth_with_error(5)).toThrowError(/undefined/);
+        expect(() => tester.vfunc_meth_with_error(6)).toThrowError(/42/);
+        expect(() => tester.vfunc_meth_with_error(7)).toThrowError(/true/);
+        expect(() => tester.vfunc_meth_with_error(8)).toThrowError(/"a string"/);
+        expect(() => tester.vfunc_meth_with_error(9)).toThrow();  // TODO(ptomato): toThrowError(/42n/)
+        expect(() => tester.vfunc_meth_with_error(10)).toThrowError(/Symbol\("a symbol"\)/);
+    });
+
+    it('marshals an error out parameter with a plain object', function () {
+        expect(() => tester.vfunc_meth_with_error(11)).toThrowError(/Object/);
+        expect(() => tester.vfunc_meth_with_error(12)).toThrowError(TypeError, /an error message/);
+        expect(() => tester.vfunc_meth_with_error(13)).toThrowError(/Object/);
+        expect(() => tester.vfunc_meth_with_error(14)).toThrowError(Error, /Object/);
+    });
+
     it('marshals an enum return value', function () {
         expect(tester.vfunc_return_enum()).toEqual(GIMarshallingTests.Enum.VALUE2);
     });


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]