[gjs] js: Call JS_Init() and JS_ShutDown()

[Philip Chimento <pchimento src gnome org>]

commit 6b25ce3b97b66494b1bba8275a6fe0b0830a94af
Author: Philip Chimento <philip endlessm com>
Date:   Fri Nov 4 19:02:55 2016 -0700

    js: Call JS_Init() and JS_ShutDown()
    
    Starting with mozjs31, JS_Init() is required. Calling JS_ShutDown() on
    exit is not required, but may become so in the future.
    
    This does so in the constructor and destructor of a static object.
    Normally this is discouraged because the order in which the constructors
    and destructors are called is not guaranteed, but I don't think that is a
    problem here since it's unlikely that anyone will try to use GJS API from
    a static constructor.
    
    However, API clients still must unref any GjsContext before the program
    shuts down. Usually this is not a problem, unless a JS script calls
    System.exit(), in which case the code would exit immediately without
    unreffing the GjsContext before JS_ShutDown was called. Instead, we make
    System.exit() throw an uncatchable exception, which is propagated all the
    way up to JS::Evaluate() and turned into a GError with code
    GJS_ERROR_SYSTEM_EXIT, thrown from gjs_context_eval().
    
    For this, we need to expose GjsError and its error codes in the public
    API. (They should have been exposed already, because gjs_context_eval()
    could already throw GJS_ERROR_FAILED.)
    
    Finally, the tests added as part of this patch made the testSystemExit
    script obsolete.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=751252

 Makefile-insttest.am                      |    5 ++---
 Makefile-test.am                          |    1 -
 Makefile.am                               |    5 +++--
 NEWS                                      |   17 +++++++++++++++++
 gjs/console.cpp                           |    4 ++--
 gjs/context-private.h                     |    3 ---
 gjs/context.cpp                           |   26 +++++++++++++++++++++-----
 gjs/gjs.h                                 |    1 +
 gjs/runtime.cpp                           |   19 +++++++++++++++++++
 installed-tests/scripts/testSystemExit.js |    3 ---
 modules/console.cpp                       |   14 +++++++++++++-
 test/gjs-tests.cpp                        |   27 +++++++++++++++++++++++++++
 test/testCommandLine.sh                   |    7 +++++++
 util/error.h                              |    3 ++-
 14 files changed, 114 insertions(+), 21 deletions(-)
---
diff --git a/Makefile-insttest.am b/Makefile-insttest.am
index cd53df7..0eec7b0 100644
--- a/Makefile-insttest.am
+++ b/Makefile-insttest.am
@@ -4,7 +4,7 @@ EXTRA_DIST += \
        installed-tests/js/jsunit.gresources.xml        \
        $(NULL)
 
-MAINTAINERCLEANFILES += jsunit.test testSystemExit.test
+MAINTAINERCLEANFILES += jsunit.test
 
 gjsinsttestdir = $(pkglibexecdir)/installed-tests
 installedtestmetadir = $(datadir)/installed-tests/gjs
@@ -22,9 +22,8 @@ if BUILDOPT_INSTALL_TESTS
 
 gjsinsttest_PROGRAMS += jsunit
 gjsinsttest_DATA += $(TEST_INTROSPECTION_TYPELIBS)
-installedtestmeta_DATA += jsunit.test testSystemExit.test
+installedtestmeta_DATA += jsunit.test
 jstests_DATA += $(common_jstests_files)
-jsscripttests_DATA += installed-tests/scripts/testSystemExit.js
 pkglib_LTLIBRARIES += libregress.la libwarnlib.la libgimarshallingtests.la
 
 if ENABLE_CAIRO
diff --git a/Makefile-test.am b/Makefile-test.am
index f0e8bc9..4f73e85 100644
--- a/Makefile-test.am
+++ b/Makefile-test.am
@@ -269,7 +269,6 @@ EXTRA_DIST +=                                               \
        installed-tests/js/testCairo.js                 \
        installed-tests/js/testGtk.js                   \
        installed-tests/js/testGDBus.js                 \
-       installed-tests/scripts/testSystemExit.js       \
        $(NULL)
 
 ### TEST EXECUTION #####################################################
diff --git a/Makefile.am b/Makefile.am
index 0ec0302..6ae6c7e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -37,7 +37,9 @@ gjs_public_includedir = $(includedir)/gjs-1.0
 ########################################################################
 nobase_gjs_public_include_HEADERS =    \
        gjs/context.h           \
-       gjs/gjs.h
+       gjs/gjs.h                       \
+       util/error.h                    \
+       $(NULL)
 
 ########################################################################
 pkgconfig_DATA = gjs-1.0.pc
@@ -96,7 +98,6 @@ libgjs_la_SOURCES =           \
        modules/modules.cpp     \
        modules/modules.h       \
        util/error.cpp          \
-       util/error.h                    \
        util/hash-x32.cpp               \
        util/hash-x32.h                 \
        util/glib.cpp           \
diff --git a/NEWS b/NEWS
index d7cf1f3..5497531 100644
--- a/NEWS
+++ b/NEWS
@@ -103,6 +103,23 @@ NEXT
   arguments into account for the time being, while still passing them on to the
   script. A warning will be logged if you are using the deprecated behaviour.
 
+- News for GJS embedders such as gnome-shell:
+
+  * New API: gjs_error_quark() is now exposed, and the error domain GJS_ERROR
+    and codes GJS_ERROR_FAILED and GJS_ERROR_SYSTEM_EXIT.
+
+  * Backwards-incompatible change: Calling System.exit() from JS code will now
+    not abort the program immediately, but instead will return immediately from
+    gjs_context_eval() so that you can unref your GjsContext before internal
+    resources are released on exit.
+
+    If gjs_context_eval() or gjs_context_eval_file() returns an error with code
+    GJS_ERROR_SYSTEM_EXIT, it means that the JS code called System.exit(). The
+    exit code will be found in the 'exit_status_p' out parameter to
+    gjs_context_eval() or gjs_context_eval_file(). If you receive this error,
+    you should do any cleanup needed and exit your program with the given exit
+    code.
+
 Version 1.46.0
 --------------
 
diff --git a/gjs/console.cpp b/gjs/console.cpp
index 1ebfe2e..d922665 100644
--- a/gjs/console.cpp
+++ b/gjs/console.cpp
@@ -286,8 +286,8 @@ main(int argc, char **argv)
     /* evaluate the script */
     if (!gjs_context_eval(js_context, script, len,
                           filename, &code, &error)) {
-        code = 1;
-        g_printerr("%s\n", error->message);
+        if (!g_error_matches(error, GJS_ERROR, GJS_ERROR_SYSTEM_EXIT))
+            g_printerr("%s\n", error->message);
         g_clear_error(&error);
         goto out;
     }
diff --git a/gjs/context-private.h b/gjs/context-private.h
index 045909f..5b24b77 100644
--- a/gjs/context-private.h
+++ b/gjs/context-private.h
@@ -37,9 +37,6 @@ void         _gjs_context_schedule_gc_if_needed       (GjsContext *js_context);
 void _gjs_context_exit(GjsContext *js_context,
                        uint8_t     exit_code);
 
-bool _gjs_context_should_exit(GjsContext *js_context,
-                              uint8_t    *exit_code_p);
-
 G_END_DECLS
 
 #endif  /* __GJS_CONTEXT_PRIVATE_H__ */
diff --git a/gjs/context.cpp b/gjs/context.cpp
index 5c8c267..c3edd89 100644
--- a/gjs/context.cpp
+++ b/gjs/context.cpp
@@ -568,15 +568,22 @@ _gjs_context_exit(GjsContext *js_context,
     js_context->exit_code = exit_code;
 }
 
-bool
-_gjs_context_should_exit(GjsContext *js_context,
-                         uint8_t    *exit_code_p)
+static bool
+context_should_exit(GjsContext *js_context,
+                    uint8_t    *exit_code_p)
 {
     if (exit_code_p != NULL)
         *exit_code_p = js_context->exit_code;
     return js_context->should_exit;
 }
 
+static void
+context_reset_exit(GjsContext *js_context)
+{
+    js_context->should_exit = false;
+    js_context->exit_code = 0;
+}
+
 /**
  * gjs_context_maybe_gc:
  * @context: a #GjsContext
@@ -671,14 +678,22 @@ gjs_context_eval(GjsContext   *js_context,
     if (!gjs_eval_with_scope(js_context->context, JS::NullPtr(), script,
                              script_len, filename, &retval)) {
         uint8_t code;
-        if (_gjs_context_should_exit(js_context, &code))
-            exit(code);
+        if (context_should_exit(js_context, &code)) {
+            /* exit_status_p is public API so can't be changed, but should be
+             * uint8_t, not int */
+            *exit_status_p = code;
+            g_set_error(error, GJS_ERROR, GJS_ERROR_SYSTEM_EXIT,
+                        "Exit with code %d", code);
+            goto out;  /* Don't log anything */
+        }
 
         gjs_log_exception(js_context->context);
         g_set_error(error,
                     GJS_ERROR,
                     GJS_ERROR_FAILED,
                     "JS_EvaluateScript() failed");
+        /* No exit code from script, but we don't want to exit(0) */
+        *exit_status_p = 1;
         goto out;
     }
 
@@ -698,6 +713,7 @@ gjs_context_eval(GjsContext   *js_context,
 
  out:
     g_object_unref(G_OBJECT(js_context));
+    context_reset_exit(js_context);
     return ret;
 }
 
diff --git a/gjs/gjs.h b/gjs/gjs.h
index b2a32f2..5cb1810 100644
--- a/gjs/gjs.h
+++ b/gjs/gjs.h
@@ -25,5 +25,6 @@
 #define __GJS_GJS_H__
 
 #include <gjs/context.h>
+#include <util/error.h>
 
 #endif /* __GJS_GJS_H__ */
diff --git a/gjs/runtime.cpp b/gjs/runtime.cpp
index 6fd9467..5619e89 100644
--- a/gjs/runtime.cpp
+++ b/gjs/runtime.cpp
@@ -222,6 +222,24 @@ gjs_finalize_callback(JSFreeOp         *fop,
     data->in_gc_sweep = false;
 }
 
+class GjsInit {
+public:
+    GjsInit() {
+        if (!JS_Init())
+            g_error("Could not initialize Javascript");
+    }
+
+    ~GjsInit() {
+        JS_ShutDown();
+    }
+
+    operator bool() {
+        return true;
+    }
+};
+
+static GjsInit gjs_is_inited;
+
 JSRuntime *
 gjs_runtime_for_current_thread(void)
 {
@@ -229,6 +247,7 @@ gjs_runtime_for_current_thread(void)
     RuntimeData *data;
 
     if (!runtime) {
+        g_assert(gjs_is_inited);
         runtime = JS_NewRuntime(32*1024*1024 /* max bytes */, JS_USE_HELPER_THREADS);
         if (runtime == NULL)
             g_error("Failed to create javascript runtime");
diff --git a/modules/console.cpp b/modules/console.cpp
index 817c722..abcb175 100644
--- a/modules/console.cpp
+++ b/modules/console.cpp
@@ -53,6 +53,7 @@
 #include <glib/gprintf.h>
 
 #include "console.h"
+#include "gjs/context.h"
 #include "gjs/jsapi-private.h"
 #include "gjs/jsapi-wrapper.h"
 
@@ -195,7 +196,18 @@ gjs_console_interact(JSContext *context,
         JS::CompileOptions options(context);
         options.setUTF8(true)
                .setFileAndLine("typein", startline);
-        JS::Evaluate(context, object, options, buffer->str, buffer->len, &result);
+        if (!JS::Evaluate(context, object, options, buffer->str, buffer->len,
+                          &result)) {
+            /* If this was an uncatchable exception, throw another uncatchable
+             * exception on up to the surrounding JS::Evaluate() in main(). This
+             * happens when you run gjs-console and type imports.system.exit(0);
+             * at the prompt. If we don't throw another uncatchable exception
+             * here, then it's swallowed and main() won't exit. */
+            if (!JS_IsExceptionPending(context)) {
+                argv.rval().set(result);
+                return false;
+            }
+        }
 
         gjs_schedule_gc_if_needed(context);
 
diff --git a/test/gjs-tests.cpp b/test/gjs-tests.cpp
index 38a2216..5111a76 100644
--- a/test/gjs-tests.cpp
+++ b/test/gjs-tests.cpp
@@ -30,6 +30,7 @@
 #include "gjs/jsapi-util.h"
 #include "gjs/jsapi-wrapper.h"
 #include "gjs-test-utils.h"
+#include "util/error.h"
 
 static void
 gjstest_test_func_gjs_context_construct_destroy(void)
@@ -59,6 +60,31 @@ gjstest_test_func_gjs_context_construct_eval(void)
     g_object_unref (context);
 }
 
+static void
+gjstest_test_func_gjs_context_exit(void)
+{
+    GjsContext *context = gjs_context_new();
+    GError *error = NULL;
+    int status;
+
+    bool ok = gjs_context_eval(context, "imports.system.exit(0);", -1,
+                               "<input>", &status, &error);
+    g_assert_false(ok);
+    g_assert_error(error, GJS_ERROR, GJS_ERROR_SYSTEM_EXIT);
+    g_assert_cmpuint(status, ==, 0);
+
+    g_clear_error(&error);
+
+    ok = gjs_context_eval(context, "imports.system.exit(42);", -1, "<input>",
+                          &status, &error);
+    g_assert_false(ok);
+    g_assert_error(error, GJS_ERROR, GJS_ERROR_SYSTEM_EXIT);
+    g_assert_cmpuint(status, ==, 42);
+
+    g_clear_error(&error);
+    g_object_unref(context);
+}
+
 #define JS_CLASS "\
 const Lang    = imports.lang; \
 const GObject = imports.gi.GObject; \
@@ -254,6 +280,7 @@ main(int    argc,
 
     g_test_add_func("/gjs/context/construct/destroy", gjstest_test_func_gjs_context_construct_destroy);
     g_test_add_func("/gjs/context/construct/eval", gjstest_test_func_gjs_context_construct_eval);
+    g_test_add_func("/gjs/context/exit", gjstest_test_func_gjs_context_exit);
     g_test_add_func("/gjs/gobject/js_defined_type", gjstest_test_func_gjs_gobject_js_defined_type);
     g_test_add_func("/gjs/jsutil/strip_shebang/no_shebang", 
gjstest_test_strip_shebang_no_advance_for_no_shebang);
     g_test_add_func("/gjs/jsutil/strip_shebang/have_shebang", 
gjstest_test_strip_shebang_advance_for_shebang);
diff --git a/test/testCommandLine.sh b/test/testCommandLine.sh
index 7717ac0..45d03c5 100755
--- a/test/testCommandLine.sh
+++ b/test/testCommandLine.sh
@@ -35,6 +35,13 @@ report_xfail () {
     fi
 }
 
+# Test that System.exit() works in gjs-console
+"$gjs" -c 'imports.system.exit(0)' || \
+    fail "System.exit(0) should exit successfully"
+if "gjs" -c 'imports.system.exit(42)' -ne 42; then
+    fail "System.exit(42) should exit with the correct exit code"
+fi
+
 # gjs --help prints GJS help
 "$gjs" --help >/dev/null
 report "--help should succeed"
diff --git a/util/error.h b/util/error.h
index 85b2cb6..01fa96b 100644
--- a/util/error.h
+++ b/util/error.h
@@ -32,7 +32,8 @@ GQuark gjs_error_quark(void);
 #define GJS_ERROR gjs_error_quark()
 
 typedef enum {
-    GJS_ERROR_FAILED
+    GJS_ERROR_FAILED,
+    GJS_ERROR_SYSTEM_EXIT,
 } GjsError;
 
 G_END_DECLS