[gjs: 2/3] jsapi-util: Append cause when calling gjs_throw with exception pending

[Philip Chimento <pchimento src gnome org>]

commit 49af97baa54eae84f4a9469ef469ff92b402ce3d
Author: Philip Chimento <philip chimento gmail com>
Date:   Sat Feb 12 13:03:16 2022 -0800

    jsapi-util: Append cause when calling gjs_throw with exception pending
    
    Previously, when calling gjs_throw() with an exception already pending,
    the new exception would be dropped, after logging it in the debug output
    in case it contained extra information.
    
    Now that we have Error.cause, we can append the new exception to the cause
    chain of the currently pending exception. This potentially gives more
    relevant information immediately to the programmer without having to dig
    in the debug log.
    
    In the case where the end of the cause chain is not an object, or there is
    a reference cycle in the cause chain, we fall back to the same debug
    logging that we did before.

 gjs/jsapi-util-error.cpp | 89 +++++++++++++++++++++++++++++++++++++++---------
 test/gjs-tests.cpp       | 31 +++++++++++++++++
 2 files changed, 103 insertions(+), 17 deletions(-)
---
diff --git a/gjs/jsapi-util-error.cpp b/gjs/jsapi-util-error.cpp
index 781c4dce3..cefaba363 100644
--- a/gjs/jsapi-util-error.cpp
+++ b/gjs/jsapi-util-error.cpp
@@ -11,19 +11,69 @@
 #include <js/CharacterEncoding.h>
 #include <js/ErrorReport.h>
 #include <js/Exception.h>
+#include <js/GCHashTable.h>  // for GCHashSet
+#include <js/HashTable.h>    // for DefaultHasher
 #include <js/RootingAPI.h>
 #include <js/TypeDecls.h>
 #include <js/Utility.h>  // for UniqueChars
 #include <js/ValueArray.h>
 #include <jsapi.h>       // for BuildStackString, Construct, JS_GetClassObject
 #include <jspubtd.h>     // for JSProtoKey, JSProto_Error, JSProto...
+#include <mozilla/HashTable.h>  // for HashSet<>::AddPtr
 
 #include "gjs/atoms.h"
 #include "gjs/context-private.h"
 #include "gjs/jsapi-util.h"
+#include "gjs/macros.h"
 #include "util/log.h"
 #include "util/misc.h"
 
+namespace js {
+class SystemAllocPolicy;
+}
+
+using CauseSet = JS::GCHashSet<JSObject*, js::DefaultHasher<JSObject*>,
+                               js::SystemAllocPolicy>;
+
+GJS_JSAPI_RETURN_CONVENTION
+static bool get_last_cause_impl(JSContext* cx, JS::HandleValue v_exc,
+                                JS::MutableHandleObject last_cause,
+                                JS::MutableHandle<CauseSet> seen_causes) {
+    if (!v_exc.isObject()) {
+        last_cause.set(nullptr);
+        return true;
+    }
+    JS::RootedObject exc(cx, &v_exc.toObject());
+    CauseSet::AddPtr entry = seen_causes.lookupForAdd(exc);
+    if (entry) {
+        last_cause.set(nullptr);
+        return true;
+    }
+    if (!seen_causes.add(entry, exc)) {
+        JS_ReportOutOfMemory(cx);
+        return false;
+    }
+
+    JS::RootedValue v_cause(cx);
+    const GjsAtoms& atoms = GjsContextPrivate::atoms(cx);
+    if (!JS_GetPropertyById(cx, exc, atoms.cause(), &v_cause))
+        return false;
+
+    if (v_cause.isUndefined()) {
+        last_cause.set(exc);
+        return true;
+    }
+
+    return get_last_cause_impl(cx, v_cause, last_cause, seen_causes);
+}
+
+GJS_JSAPI_RETURN_CONVENTION
+static bool get_last_cause(JSContext* cx, JS::HandleValue v_exc,
+                           JS::MutableHandleObject last_cause) {
+    JS::Rooted<CauseSet> seen_causes(cx);
+    return get_last_cause_impl(cx, v_exc, last_cause, &seen_causes);
+}
+
 /*
  * See:
  * https://bugzilla.mozilla.org/show_bug.cgi?id=166436
@@ -42,22 +92,6 @@
 
     s = g_strdup_vprintf(format, args);
 
-    if (JS_IsExceptionPending(context)) {
-        /* Often it's unclear whether a given jsapi.h function
-         * will throw an exception, so we will throw ourselves
-         * "just in case"; in those cases, we don't want to
-         * overwrite an exception that already exists.
-         * (Do log in case our second exception adds more info,
-         * but don't log as topic ERROR because if the exception is
-         * caught we don't want an ERROR in the logs.)
-         */
-        gjs_debug(GJS_DEBUG_CONTEXT,
-                  "Ignoring second exception: '%s'",
-                  s);
-        g_free(s);
-        return;
-    }
-
     JS::RootedObject constructor(context);
     JS::RootedValue v_constructor(context), exc_val(context);
     JS::RootedObject new_exc(context);
@@ -90,7 +124,28 @@
     }
 
     exc_val.setObject(*new_exc);
-    JS_SetPendingException(context, exc_val);
+
+    if (JS_IsExceptionPending(context)) {
+        // Often it's unclear whether a given jsapi.h function will throw an
+        // exception, so we will throw ourselves "just in case"; in those cases,
+        // we append the new exception as the cause of the original exception.
+        // The second exception may add more info.
+        const GjsAtoms& atoms = GjsContextPrivate::atoms(context);
+        JS::RootedValue pending(context);
+        JS_GetPendingException(context, &pending);
+        JS::RootedObject last_cause(context);
+        if (!get_last_cause(context, pending, &last_cause))
+            goto out;
+        if (last_cause) {
+            if (!JS_SetPropertyById(context, last_cause, atoms.cause(),
+                                    exc_val))
+                goto out;
+        } else {
+            gjs_debug(GJS_DEBUG_CONTEXT, "Ignoring second exception: '%s'", s);
+        }
+    } else {
+        JS_SetPendingException(context, exc_val);
+    }
 
     result = true;
 
diff --git a/test/gjs-tests.cpp b/test/gjs-tests.cpp
index 329c081c3..122e36390 100644
--- a/test/gjs-tests.cpp
+++ b/test/gjs-tests.cpp
@@ -603,6 +603,36 @@ static void gjstest_test_func_gjs_jsapi_util_error_throw(GjsUnitTestFixture* fx,
     g_assert_true(&exc.toObject() == &previous.toObject());
 }
 
+static void test_jsapi_util_error_throw_cause(GjsUnitTestFixture* fx,
+                                              const void*) {
+    g_test_expect_message("Gjs", G_LOG_LEVEL_WARNING,
+                          "JS ERROR: Error: Exception 1\n"
+                          "Caused by: Error: Exception 2\n");
+
+    gjs_throw(fx->cx, "Exception 1");
+    gjs_throw(fx->cx, "Exception 2");
+    gjs_log_exception(fx->cx);
+
+    g_test_expect_message("Gjs", G_LOG_LEVEL_WARNING,
+                          "JS ERROR: Error: Exception 1\n"
+                          "Caused by: Error: Exception 2\n"
+                          "Caused by: Error: Exception 3\n");
+
+    gjs_throw(fx->cx, "Exception 1");
+    gjs_throw(fx->cx, "Exception 2");
+    gjs_throw(fx->cx, "Exception 3");
+    gjs_log_exception(fx->cx);
+
+    g_test_expect_message("Gjs", G_LOG_LEVEL_WARNING, "JS ERROR: 42");
+
+    JS::RootedValue non_object(fx->cx, JS::Int32Value(42));
+    JS_SetPendingException(fx->cx, non_object);
+    gjs_throw(fx->cx, "This exception will be dropped");
+    gjs_log_exception(fx->cx);
+
+    g_test_assert_expected_messages();
+}
+
 static void test_jsapi_util_string_utf8_nchars_to_js(GjsUnitTestFixture* fx,
                                                      const void*) {
     JS::RootedValue v_out(fx->cx);
@@ -1125,6 +1155,7 @@ main(int    argc,
 
     ADD_JSAPI_UTIL_TEST("error/throw",
                         gjstest_test_func_gjs_jsapi_util_error_throw);
+    ADD_JSAPI_UTIL_TEST("error/throw-cause", test_jsapi_util_error_throw_cause);
     ADD_JSAPI_UTIL_TEST("string/js/string/utf8",
                         gjstest_test_func_gjs_jsapi_util_string_js_string_utf8);
     ADD_JSAPI_UTIL_TEST("string/utf8-nchars-to-js",