[gjs: 1/5] closure: Clean up gjs_closure_invoke()

[Philip Chimento <pchimento src gnome org>]

commit 76a0fe83d5368951a5ff29bd2c97fefa18716eb2
Author: Philip Chimento <philip chimento gmail com>
Date:   Sun Mar 21 19:58:40 2021 -0700

    closure: Clean up gjs_closure_invoke()
    
    Remove the return_exception argument from gjs_closure_invoke(). With this
    argument, it was confusing whether an exception would be pending or not
    after the closure invocation, and it was confusing whether the JS return
    value represented an actual return value or an exception.
    
    Now, gjs_closure_invoke() always follows the JSAPI return convention.
    
    At the call site in function.cpp, with return_exception=true, previously
    the returned value was re-thrown as an exception, only to be cleared again
    and logged. Now it is just logged.
    
    At the call site in value.cpp, with return_exception=false, previously the
    exception would be logged in gjs_closure_invoke(), now we log it in the
    calling code in closure_marshal().

 gi/closure.cpp  | 24 +++---------------------
 gi/closure.h    | 11 ++++++-----
 gi/function.cpp | 33 ++++++++++++++++++---------------
 gi/value.cpp    | 23 +++++++++++++++++------
 4 files changed, 44 insertions(+), 47 deletions(-)
---
diff --git a/gi/closure.cpp b/gi/closure.cpp
index bd98390b..ec34b4b9 100644
--- a/gi/closure.cpp
+++ b/gi/closure.cpp
@@ -163,13 +163,9 @@ static void closure_finalize(void*, GClosure* closure) {
     self->~Closure();
 }
 
-bool
-gjs_closure_invoke(GClosure                   *closure,
-                   JS::HandleObject            this_obj,
-                   const JS::HandleValueArray& args,
-                   JS::MutableHandleValue      retval,
-                   bool                        return_exception)
-{
+bool gjs_closure_invoke(GClosure* closure, JS::HandleObject this_obj,
+                        const JS::HandleValueArray& args,
+                        JS::MutableHandleValue retval) {
     Closure *c;
     JSContext *context;
 
@@ -196,20 +192,6 @@ gjs_closure_invoke(GClosure                   *closure,
             "Closure invocation failed (exception should have been thrown) "
             "closure %p function %p",
             closure, c->func.debug_addr());
-        /* If an exception has been thrown, log it, unless the caller
-         * explicitly wants to handle it manually (for example to turn it
-         * into a GError), in which case it replaces the return value
-         * (which is not valid anyway) */
-        if (JS_IsExceptionPending(context)) {
-            if (return_exception)
-                JS_GetPendingException(context, retval);
-            else
-                gjs_log_exception_uncaught(context);
-        } else {
-            retval.setUndefined();
-            gjs_debug_closure("Closure invocation failed but no exception was set?"
-                              "closure %p", closure);
-        }
         return false;
     }
 
diff --git a/gi/closure.h b/gi/closure.h
index e6b2e091..87299f73 100644
--- a/gi/closure.h
+++ b/gi/closure.h
@@ -11,6 +11,8 @@
 
 #include <js/TypeDecls.h>
 
+#include "gjs/macros.h"
+
 class JSTracer;
 namespace JS {
 class HandleValueArray;
@@ -20,11 +22,10 @@ class HandleValueArray;
                                         const char* description,
                                         bool root_function);
 
-[[nodiscard]] bool gjs_closure_invoke(GClosure* closure,
-                                      JS::HandleObject this_obj,
-                                      const JS::HandleValueArray& args,
-                                      JS::MutableHandleValue retval,
-                                      bool return_exception);
+GJS_JSAPI_RETURN_CONVENTION
+bool gjs_closure_invoke(GClosure* closure, JS::HandleObject this_obj,
+                        const JS::HandleValueArray& args,
+                        JS::MutableHandleValue retval);
 
 [[nodiscard]] JSContext* gjs_closure_get_context(GClosure* closure);
 [[nodiscard]] bool gjs_closure_is_valid(GClosure* closure);
diff --git a/gi/function.cpp b/gi/function.cpp
index 687eee96..5810adea 100644
--- a/gi/function.cpp
+++ b/gi/function.cpp
@@ -409,20 +409,23 @@ void GjsCallbackTrampoline::callback_closure(GIArgument** args, void* result) {
             set_return_ffi_arg_from_giargument(&ret_type, result, &argument);
         }
 
-        // If the callback has a GError** argument and invoking the closure
-        // returned an error, try to make a GError from it
-        if (error_argument && rval.isObject()) {
-            JS::RootedObject exc_object(context, &rval.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);
-            JS_ClearPendingException(context);  // don't log
-        } else if (!rval.isUndefined()) {
-            JS_SetPendingException(context, rval);
+        // 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);
+            }
         }
         gjs_log_exception_uncaught(context);
     }
@@ -518,7 +521,7 @@ bool GjsCallbackTrampoline::callback_closure_inner(
         }
     }
 
-    if (!gjs_closure_invoke(m_js_function, this_object, jsargs, rval, true))
+    if (!gjs_closure_invoke(m_js_function, this_object, jsargs, rval))
         return false;
 
     if (n_outargs == 0 && ret_type_is_void) {
diff --git a/gi/value.cpp b/gi/value.cpp
index c17c6933..2b82d64d 100644
--- a/gi/value.cpp
+++ b/gi/value.cpp
@@ -7,6 +7,8 @@
 #include <stdint.h>
 #include <stdlib.h>  // for exit
 
+#include <string>
+
 #include <girepository.h>
 #include <glib-object.h>
 #include <glib.h>
@@ -259,12 +261,21 @@ closure_marshal(GClosure        *closure,
 
     JS::RootedValue rval(context);
 
-    if (!gjs_closure_invoke(closure, nullptr, argv, &rval, false)) {
-        // "Uncatchable" exception thrown, we have to exit. This
-        // matches the closure exit handling in function.cpp
-        uint8_t code;
-        if (gjs->should_exit(&code))
-            exit(code);
+    if (!gjs_closure_invoke(closure, nullptr, argv, &rval)) {
+        if (JS_IsExceptionPending(context)) {
+            gjs_log_exception_uncaught(context);
+        } else {
+            // "Uncatchable" exception thrown, we have to exit. This
+            // matches the closure exit handling in function.cpp
+            uint8_t code;
+            if (gjs->should_exit(&code))
+                exit(code);
+
+            // Some other uncatchable exception, e.g. out of memory
+            JSFunction* fn = gjs_closure_get_callable(closure);
+            g_error("Function %s terminated with uncatchable exception",
+                    gjs_debug_string(JS_GetFunctionDisplayId(fn)).c_str());
+        }
     }
 
     // null return_value means the closure wasn't expected to return a value.