[gjs: 1/10] js: Move all functions requiring GjsContext out of jsapi-util

From: Cosimo Cecchi <cosimoc src gnome org>
To: commits-list gnome org
Cc:
Subject: [gjs: 1/10] js: Move all functions requiring GjsContext out of jsapi-util
Date: Sun, 2 Dec 2018 02:09:03 +0000 (UTC)
commit 685b32f01220c22f2713a6973c7eef3b7f81e90f
Author: Philip Chimento <philip chimento gmail com>
Date:   Wed Aug 29 23:08:32 2018 -0400

    js: Move all functions requiring GjsContext out of jsapi-util
    
    This moves gjs_eval_with_scope() and gjs_call_function_value() to be
    methods of GjsContextPrivate. Since they call into JS, they schedule an
    idle garbage collection afterwards, and so they depend on GjsContext to
    do the scheduling.
    
    Also merges gjs_schedule_gc_if_needed() into
    GjsContextPrivate::schedule_gc_if_needed().

 gi/boxed.cpp          |   9 ++--
 gi/closure.cpp        |   3 +-
 gi/foreign.cpp        |   6 ++-
 gi/function.cpp       |   2 +-
 gi/object.cpp         |   6 +--
 gi/repo.cpp           |   3 +-
 gjs/context-private.h |  10 ++++-
 gjs/context.cpp       | 112 +++++++++++++++++++++++++++++++++++++++++++++++++-
 gjs/importer.cpp      |   5 ++-
 gjs/jsapi-util.cpp    |  91 ----------------------------------------
 gjs/jsapi-util.h      |  15 -------
 gjs/module.cpp        |   4 +-
 modules/console.cpp   |   3 +-
 13 files changed, 143 insertions(+), 126 deletions(-)
---
diff --git a/gi/boxed.cpp b/gi/boxed.cpp
index e430efb3..eaae200b 100644
--- a/gi/boxed.cpp
+++ b/gi/boxed.cpp
@@ -317,11 +317,11 @@ boxed_invoke_constructor(JSContext             *context,
                          JS::HandleId           constructor_name,
                          JS::CallArgs&          args)
 {
+    GjsContextPrivate* gjs = GjsContextPrivate::from_cx(context);
     JS::RootedObject js_constructor(context);
 
-    const GjsAtoms& atoms = GjsContextPrivate::atoms(context);
-    if (!gjs_object_require_property(context, obj, NULL, atoms.constructor(),
-                                     &js_constructor))
+    if (!gjs_object_require_property(
+            context, obj, nullptr, gjs->atoms().constructor(), &js_constructor))
         return false;
 
     JS::RootedValue js_constructor_func(context);
@@ -329,8 +329,7 @@ boxed_invoke_constructor(JSContext             *context,
                                      constructor_name, &js_constructor_func))
         return false;
 
-    return gjs_call_function_value(context, nullptr, js_constructor_func,
-                                   args, args.rval());
+    return gjs->call_function(nullptr, js_constructor_func, args, args.rval());
 }
 
 GJS_JSAPI_RETURN_CONVENTION
diff --git a/gi/closure.cpp b/gi/closure.cpp
index 5bf082d3..afc3d3e3 100644
--- a/gi/closure.cpp
+++ b/gi/closure.cpp
@@ -240,7 +240,8 @@ gjs_closure_invoke(GClosure                   *closure,
                           " - closure %p", closure);
     }
 
-    gjs_schedule_gc_if_needed(context);
+    GjsContextPrivate* gjs = GjsContextPrivate::from_cx(context);
+    gjs->schedule_gc_if_needed();
     return true;
 }
 
diff --git a/gi/foreign.cpp b/gi/foreign.cpp
index ac4ef624..bb65da49 100644
--- a/gi/foreign.cpp
+++ b/gi/foreign.cpp
@@ -28,6 +28,7 @@
 
 #include "arg.h"
 #include "foreign.h"
+#include "gjs/context-private.h"
 #include "gjs/jsapi-wrapper.h"
 
 static struct {
@@ -75,8 +76,9 @@ gjs_foreign_load_foreign_module(JSContext *context,
         //        and only execute this statement if isn't
         script = g_strdup_printf("imports.%s;", gi_namespace);
         JS::RootedValue retval(context);
-        if (!gjs_eval_with_scope(context, nullptr, script, strlen(script),
-                                 "<internal>", &retval)) {
+        GjsContextPrivate* gjs = GjsContextPrivate::from_cx(context);
+        if (!gjs->eval_with_scope(nullptr, script, strlen(script), "<internal>",
+                                  &retval)) {
             g_critical("ERROR importing foreign module %s\n", gi_namespace);
             g_free(script);
             return false;
diff --git a/gi/function.cpp b/gi/function.cpp
index eb8ace4a..8973b677 100644
--- a/gi/function.cpp
+++ b/gi/function.cpp
@@ -472,7 +472,7 @@ out:
     }
 
     gjs_callback_trampoline_unref(trampoline);
-    gjs_schedule_gc_if_needed(context);
+    gjs->schedule_gc_if_needed();
 
     JS_EndRequest(context);
 }
diff --git a/gi/object.cpp b/gi/object.cpp
index e6687caa..63ea5e7b 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -1678,13 +1678,13 @@ GJS_NATIVE_CONSTRUCTOR_DECLARE(object_instance)
      * might be traced and we will end up dereferencing a null pointer */
     JS_SetPrivate(object, ObjectInstance::new_for_js_object(context, object));
 
-    const GjsAtoms& atoms = GjsContextPrivate::atoms(context);
+    GjsContextPrivate* gjs = GjsContextPrivate::from_cx(context);
     if (!gjs_object_require_property(context, object, "GObject instance",
-                                     atoms.init(), &initer))
+                                     gjs->atoms().init(), &initer))
         return false;
 
     argv.rval().setUndefined();
-    ret = gjs_call_function_value(context, object, initer, argv, argv.rval());
+    ret = gjs->call_function(object, initer, argv, argv.rval());
 
     if (argv.rval().isUndefined())
         argv.rval().setObject(*object);
diff --git a/gi/repo.cpp b/gi/repo.cpp
index e5284000..e4e0ca3f 100644
--- a/gi/repo.cpp
+++ b/gi/repo.cpp
@@ -149,7 +149,8 @@ static bool resolve_namespace_object(JSContext* context,
               "Defined namespace '%s' %p in GIRepository %p", ns_name.get(),
               gi_namespace.get(), repo_obj.get());
 
-    gjs_schedule_gc_if_needed(context);
+    GjsContextPrivate* gjs = GjsContextPrivate::from_cx(context);
+    gjs->schedule_gc_if_needed();
     return true;
 }
 
diff --git a/gjs/context-private.h b/gjs/context-private.h
index a87c6811..3b499c90 100644
--- a/gjs/context-private.h
+++ b/gjs/context-private.h
@@ -128,9 +128,17 @@ class GjsContextPrivate {
     GJS_JSAPI_RETURN_CONVENTION
     bool eval(const char* script, ssize_t script_len, const char* filename,
               int* exit_status_p, GError** error);
+    GJS_JSAPI_RETURN_CONVENTION
+    bool eval_with_scope(JS::HandleObject scope_object, const char* script,
+                         ssize_t script_len, const char* filename,
+                         JS::MutableHandleValue retval);
+    GJS_JSAPI_RETURN_CONVENTION
+    bool call_function(JS::HandleObject this_obj, JS::HandleValue func_val,
+                       const JS::HandleValueArray& args,
+                       JS::MutableHandleValue rval);
 
     void schedule_gc(void) { schedule_gc_internal(true); }
-    void schedule_gc_if_needed(void) { schedule_gc_internal(false); }
+    void schedule_gc_if_needed(void);
 
     void exit(uint8_t exit_code);
     GJS_USE bool should_exit(uint8_t* exit_code_p) const;
diff --git a/gjs/context.cpp b/gjs/context.cpp
index 3f70d04f..6fc48c78 100644
--- a/gjs/context.cpp
+++ b/gjs/context.cpp
@@ -28,6 +28,8 @@
 #include <unistd.h>
 
 #include <array>
+#include <codecvt>
+#include <locale>
 #include <unordered_map>
 
 #include <gio/gio.h>
@@ -574,6 +576,20 @@ void GjsContextPrivate::schedule_gc_internal(bool force_gc) {
                                               nullptr);
 }
 
+/*
+ * GjsContextPrivate::schedule_gc_if_needed:
+ *
+ * Does a minor GC immediately if the JS engine decides one is needed, but also
+ * schedules a full GC in the next idle time.
+ */
+void GjsContextPrivate::schedule_gc_if_needed(void) {
+    // We call JS_MaybeGC immediately, but defer a check for a full GC cycle
+    // to an idle handler.
+    JS_MaybeGC(m_cx);
+
+    schedule_gc_internal(false);
+}
+
 void GjsContextPrivate::exit(uint8_t exit_code) {
     g_assert(!m_should_exit);
     m_should_exit = true;
@@ -812,8 +828,7 @@ bool GjsContextPrivate::eval(const char* script, ssize_t script_len,
         gjs_profiler_start(m_profiler);
 
     JS::RootedValue retval(m_cx);
-    bool ok = gjs_eval_with_scope(m_cx, nullptr, script, script_len, filename,
-                                  &retval);
+    bool ok = eval_with_scope(nullptr, script, script_len, filename, &retval);
 
     /* The promise job queue should be drained even on error, to finish
      * outstanding async tasks before the context is torn down. Drain after
@@ -892,6 +907,99 @@ gjs_context_eval_file(GjsContext    *js_context,
                             exit_status_p, error);
 }
 
+/*
+ * GjsContextPrivate::eval_with_scope:
+ * @scope_object: an object to use as the global scope, or nullptr
+ * @script: JavaScript program encoded in UTF-8
+ * @script_len: length of @script
+ * @filename: filename to use as the origin of @script
+ * @retval: location for the return value of @script
+ *
+ * Executes @script with a local scope so that nothing from the script leaks out
+ * into the global scope.
+ * If @scope_object is given, then everything that @script placed in the global
+ * namespace is defined on @scope_object.
+ * Otherwise, the global definitions are just discarded.
+ */
+bool GjsContextPrivate::eval_with_scope(JS::HandleObject scope_object,
+                                        const char* script, ssize_t script_len,
+                                        const char* filename,
+                                        JS::MutableHandleValue retval) {
+    int start_line_number = 1;
+    JSAutoRequest ar(m_cx);
+    size_t real_len = script_len;
+
+    if (script_len < 0)
+        real_len = strlen(script);
+
+    script = gjs_strip_unix_shebang(script, &real_len, &start_line_number);
+
+    /* log and clear exception if it's set (should not be, normally...) */
+    if (JS_IsExceptionPending(m_cx)) {
+        g_warning("eval_in_scope called with a pending exception");
+        return false;
+    }
+
+    JS::RootedObject eval_obj(m_cx, scope_object);
+    if (!eval_obj)
+        eval_obj = JS_NewPlainObject(m_cx);
+
+    JS::CompileOptions options(m_cx);
+    options.setFileAndLine(filename, start_line_number).setSourceIsLazy(true);
+
+    std::wstring_convert<std::codecvt_utf8_utf16<char16_t>, char16_t> convert;
+    std::u16string utf16_string = convert.from_bytes(script);
+    JS::SourceBufferHolder buf(utf16_string.c_str(), utf16_string.size(),
+                               JS::SourceBufferHolder::NoOwnership);
+
+    JS::AutoObjectVector scope_chain(m_cx);
+    if (!scope_chain.append(eval_obj)) {
+        JS_ReportOutOfMemory(m_cx);
+        return false;
+    }
+
+    if (!JS::Evaluate(m_cx, scope_chain, options, buf, retval))
+        return false;
+
+    schedule_gc_if_needed();
+
+    if (JS_IsExceptionPending(m_cx)) {
+        g_warning(
+            "EvaluateScript returned true but exception was pending; "
+            "did somebody call gjs_throw() without returning false?");
+        return false;
+    }
+
+    gjs_debug(GJS_DEBUG_CONTEXT, "Script evaluation succeeded");
+
+    return true;
+}
+
+/*
+ * GjsContextPrivate::call_function:
+ * @this_obj: Object to use as the 'this' for the function call
+ * @func_val: Callable to call, as a JS value
+ * @args: Arguments to pass to the callable
+ * @rval: Location for the return value
+ *
+ * Use this instead of JS_CallFunctionValue(), because it schedules a GC if
+ * one is needed. It's good practice to check if a GC should be run every time
+ * we return from JS back into C++.
+ */
+bool GjsContextPrivate::call_function(JS::HandleObject this_obj,
+                                      JS::HandleValue func_val,
+                                      const JS::HandleValueArray& args,
+                                      JS::MutableHandleValue rval) {
+    JSAutoRequest ar(m_cx);
+
+    if (!JS_CallFunctionValue(m_cx, this_obj, func_val, args, rval))
+        return false;
+
+    schedule_gc_if_needed();
+
+    return true;
+}
+
 bool
 gjs_context_define_string_array(GjsContext  *js_context,
                                 const char    *array_name,
diff --git a/gjs/importer.cpp b/gjs/importer.cpp
index 9b315821..7a9a60e9 100644
--- a/gjs/importer.cpp
+++ b/gjs/importer.cpp
@@ -308,6 +308,7 @@ import_module_init(JSContext       *context,
     gsize script_len = 0;
     GError *error = NULL;
 
+    GjsContextPrivate* gjs = GjsContextPrivate::from_cx(context);
     JS::RootedValue ignored(context);
 
     if (!(g_file_load_contents(file, NULL, &script, &script_len, NULL, &error))) {
@@ -325,8 +326,8 @@ import_module_init(JSContext       *context,
 
     full_path = g_file_get_parse_name (file);
 
-    if (!gjs_eval_with_scope(context, module_obj, script, script_len,
-                             full_path, &ignored))
+    if (!gjs->eval_with_scope(module_obj, script, script_len, full_path,
+                              &ignored))
         goto out;
 
     ret = true;
diff --git a/gjs/jsapi-util.cpp b/gjs/jsapi-util.cpp
index 1641db85..817352be 100644
--- a/gjs/jsapi-util.cpp
+++ b/gjs/jsapi-util.cpp
@@ -524,26 +524,6 @@ gjs_log_exception(JSContext  *context)
     return retval;
 }
 
-bool
-gjs_call_function_value(JSContext                  *context,
-                        JS::HandleObject            obj,
-                        JS::HandleValue             fval,
-                        const JS::HandleValueArray& args,
-                        JS::MutableHandleValue      rval)
-{
-    bool result;
-
-    JS_BeginRequest(context);
-
-    result = JS_CallFunctionValue(context, obj, fval, args, rval);
-
-    if (result)
-        gjs_schedule_gc_if_needed(context);
-
-    JS_EndRequest(context);
-    return result;
-}
-
 #ifdef __linux__
 static void
 _linux_get_self_process_size (gulong *vm_size,
@@ -633,17 +613,6 @@ gjs_maybe_gc (JSContext *context)
     gjs_gc_if_needed(context);
 }
 
-void
-gjs_schedule_gc_if_needed (JSContext *context)
-{
-    /* We call JS_MaybeGC immediately, but defer a check for a full
-     * GC cycle to an idle handler.
-     */
-    JS_MaybeGC(context);
-
-    GjsContextPrivate::from_cx(context)->schedule_gc_if_needed();
-}
-
 /**
  * gjs_strip_unix_shebang:
  *
@@ -697,63 +666,3 @@ gjs_strip_unix_shebang(const char  *script,
 
     return script;
 }
-
-bool
-gjs_eval_with_scope(JSContext             *context,
-                    JS::HandleObject       object,
-                    const char            *script,
-                    ssize_t                script_len,
-                    const char            *filename,
-                    JS::MutableHandleValue retval)
-{
-    int start_line_number = 1;
-    JSAutoRequest ar(context);
-    size_t real_len = script_len;
-
-    if (script_len < 0)
-        real_len = strlen(script);
-
-    script = gjs_strip_unix_shebang(script,
-                                    &real_len,
-                                    &start_line_number);
-
-    /* log and clear exception if it's set (should not be, normally...) */
-    if (JS_IsExceptionPending(context)) {
-        g_warning("gjs_eval_in_scope called with a pending exception");
-        return false;
-    }
-
-    JS::RootedObject eval_obj(context, object);
-    if (!eval_obj)
-        eval_obj = JS_NewPlainObject(context);
-
-    JS::CompileOptions options(context);
-    options.setFileAndLine(filename, start_line_number);
-
-    std::wstring_convert<std::codecvt_utf8_utf16<char16_t>, char16_t> convert;
-    std::u16string utf16_string = convert.from_bytes(script);
-    JS::SourceBufferHolder buf(utf16_string.c_str(), utf16_string.size(),
-                               JS::SourceBufferHolder::NoOwnership);
-
-    JS::AutoObjectVector scope_chain(context);
-    if (!scope_chain.append(eval_obj)) {
-        JS_ReportOutOfMemory(context);
-        return false;
-    }
-
-    if (!JS::Evaluate(context, scope_chain, options, buf, retval))
-        return false;
-
-    gjs_schedule_gc_if_needed(context);
-
-    if (JS_IsExceptionPending(context)) {
-        g_warning("EvaluateScript returned true but exception was pending; "
-                  "did somebody call gjs_throw() without returning false?");
-        return false;
-    }
-
-    gjs_debug(GJS_DEBUG_CONTEXT,
-              "Script evaluation succeeded");
-
-    return true;
-}
diff --git a/gjs/jsapi-util.h b/gjs/jsapi-util.h
index 55cfa666..40d93309 100644
--- a/gjs/jsapi-util.h
+++ b/gjs/jsapi-util.h
@@ -250,12 +250,6 @@ GJS_USE
 char *gjs_value_debug_string(JSContext      *context,
                              JS::HandleValue value);
 
-GJS_JSAPI_RETURN_CONVENTION
-bool gjs_call_function_value(JSContext                  *context,
-                             JS::HandleObject            obj,
-                             JS::HandleValue             fval,
-                             const JS::HandleValueArray& args,
-                             JS::MutableHandleValue      rval);
 
 void gjs_warning_reporter(JSContext     *cx,
                           JSErrorReport *report);
@@ -315,17 +309,8 @@ bool        gjs_unichar_from_string          (JSContext       *context,
 /* Functions intended for more "internal" use */
 
 void gjs_maybe_gc (JSContext *context);
-void gjs_schedule_gc_if_needed(JSContext *cx);
 void gjs_gc_if_needed(JSContext *cx);
 
-GJS_JSAPI_RETURN_CONVENTION
-bool gjs_eval_with_scope(JSContext             *context,
-                         JS::HandleObject       object,
-                         const char            *script,
-                         ssize_t                script_len,
-                         const char            *filename,
-                         JS::MutableHandleValue retval);
-
 GJS_USE
 const char * gjs_strip_unix_shebang(const char *script,
                                     size_t     *script_len,
diff --git a/gjs/module.cpp b/gjs/module.cpp
index e2ab5709..ff180f37 100644
--- a/gjs/module.cpp
+++ b/gjs/module.cpp
@@ -26,6 +26,7 @@
 
 #include <gio/gio.h>
 
+#include "gjs/context-private.h"
 #include "jsapi-util.h"
 #include "jsapi-wrapper.h"
 #include "module.h"
@@ -110,7 +111,8 @@ class GjsModule {
         if (!JS::Evaluate(cx, scope_chain, options, buf, &ignored_retval))
             return false;
 
-        gjs_schedule_gc_if_needed(cx);
+        GjsContextPrivate* gjs = GjsContextPrivate::from_cx(cx);
+        gjs->schedule_gc_if_needed();
 
         gjs_debug(GJS_DEBUG_IMPORTER, "Importing module %s succeeded", m_name);
 
diff --git a/modules/console.cpp b/modules/console.cpp
index 20d2f048..05384a04 100644
--- a/modules/console.cpp
+++ b/modules/console.cpp
@@ -286,7 +286,8 @@ gjs_console_eval_and_print(JSContext  *cx,
             return false;
     }
 
-    gjs_schedule_gc_if_needed(cx);
+    GjsContextPrivate* gjs = GjsContextPrivate::from_cx(cx);
+    gjs->schedule_gc_if_needed();
 
     if (result.isUndefined())
         return true;
[Date Prev][Date Next] [Thread Prev][Thread Next] [Thread Index] [Date Index] [Author Index]