[gjs] Check and prevent reentrancy to the JSAPI during finalization

[Giovanni Campagna <gcampagna src gnome org>]

commit dc31b7cfda356b2691dc90440ea54b461c1b90b4
Author: Giovanni Campagna <gcampagna src gnome org>
Date:   Sun Feb 23 22:16:58 2014 +0100

    Check and prevent reentrancy to the JSAPI during finalization
    
    Keep track of whether the runtime is currently doing GC sweeping,
    and prevent calling JS code at that time. This could happen with
    dangling signal connections or widgets/actors that are not properly
    destroyed, and causes an assert failure with debug libmozjs (and
    a crash otherwise)
    
    https://bugzilla.gnome.org/show_bug.cgi?id=725024

 gi/function.cpp |   17 ++++++++++++
 gi/value.cpp    |   27 ++++++++++++++++++-
 gjs/runtime.cpp |   78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 gjs/runtime.h   |    2 +
 4 files changed, 123 insertions(+), 1 deletions(-)
---
diff --git a/gi/function.cpp b/gi/function.cpp
index 8d2fe77..86c32b1 100644
--- a/gi/function.cpp
+++ b/gi/function.cpp
@@ -167,6 +167,7 @@ gjs_callback_closure(ffi_cif *cif,
                      void *data)
 {
     JSContext *context;
+    JSRuntime *runtime;
     JSObject *global;
     GjsCallbackTrampoline *trampoline;
     int i, n_args, n_jsargs, n_outargs;
@@ -181,6 +182,22 @@ gjs_callback_closure(ffi_cif *cif,
     gjs_callback_trampoline_ref(trampoline);
 
     context = trampoline->context;
+    runtime = JS_GetRuntime(context);
+    if (G_UNLIKELY (gjs_runtime_is_sweeping(runtime))) {
+        g_critical("Attempting to call back into JSAPI during the sweeping phase of GC. "
+                   "This is most likely caused by not destroying a Clutter actor or Gtk+ "
+                   "widget with ::destroy signals connected, but can also be caused by "
+                   "using the destroy() or dispose() vfuncs. Because it would crash the "
+                   "application, it has been blocked and the JS callback not invoked.");
+        /* A gjs_dumpstack() would be nice here, but we can't,
+           because that works by creating a new Error object and
+           reading the stack property, which is the worst possible
+           idea during a GC session.
+        */
+        gjs_callback_trampoline_unref(trampoline);
+        return;
+    }
+
     JS_BeginRequest(context);
     global = JS_GetGlobalObject(context);
     JSAutoCompartment ac(context, global);
diff --git a/gi/value.cpp b/gi/value.cpp
index 828a259..4a2e6ca 100644
--- a/gi/value.cpp
+++ b/gi/value.cpp
@@ -58,7 +58,7 @@ closure_marshal(GClosure        *closure,
 {
     JSContext *context;
     JSObject *global;
-
+    JSRuntime *runtime;
     int argc;
     jsval *argv;
     jsval rval;
@@ -75,6 +75,31 @@ closure_marshal(GClosure        *closure,
     }
 
     context = gjs_closure_get_context(closure);
+    runtime = JS_GetRuntime(context);
+    if (G_UNLIKELY (gjs_runtime_is_sweeping(runtime))) {
+        GSignalInvocationHint *hint = (GSignalInvocationHint*) invocation_hint;
+
+        g_critical("Attempting to call back into JSAPI during the sweeping phase of GC. "
+                   "This is most likely caused by not destroying a Clutter actor or Gtk+ "
+                   "widget with ::destroy signals connected, but can also be caused by "
+                   "using the destroy() or dispose() vfuncs. Because it would crash the "
+                   "application, it has been blocked and the JS callback not invoked.");
+        if (hint) {
+            gpointer instance;
+            g_signal_query(hint->signal_id, &signal_query);
+
+            instance = g_value_peek_pointer(&param_values[0]);
+            g_critical("The offending signal was %s on %s %p.", signal_query.signal_name,
+                       g_type_name(G_TYPE_FROM_INSTANCE(instance)), instance);
+        }
+        /* A gjs_dumpstack() would be nice here, but we can't,
+           because that works by creating a new Error object and
+           reading the stack property, which is the worst possible
+           idea during a GC session.
+        */
+        return;
+    }
+
     JS_BeginRequest(context);
     global = JS_GetGlobalObject(context);
     JSAutoCompartment ac(context, global);
diff --git a/gjs/runtime.cpp b/gjs/runtime.cpp
index 68a2cc8..5069b86 100644
--- a/gjs/runtime.cpp
+++ b/gjs/runtime.cpp
@@ -24,6 +24,19 @@
 #include <config.h>
 
 #include "compat.h"
+#include "runtime.h"
+
+struct RuntimeData {
+  JSBool in_gc_sweep;
+};
+
+JSBool
+gjs_runtime_is_sweeping (JSRuntime *runtime)
+{
+  RuntimeData *data = (RuntimeData*) JS_GetRuntimePrivate(runtime);
+
+  return data->in_gc_sweep;
+}
 
 /* Implementations of locale-specific operations; these are used
  * in the implementation of String.localeCompare(), Date.toLocaleDateString(),
@@ -137,6 +150,9 @@ static void
 destroy_runtime(gpointer data)
 {
     JSRuntime *runtime = (JSRuntime *) data;
+    RuntimeData *rtdata = (RuntimeData *) JS_GetRuntimePrivate(runtime);
+
+    g_free(rtdata);
     JS_DestroyRuntime(runtime);
 }
 
@@ -150,19 +166,81 @@ static JSLocaleCallbacks gjs_locale_callbacks =
     gjs_locale_to_unicode
 };
 
+void
+gjs_finalize_callback(JSFreeOp         *fop,
+                      JSFinalizeStatus  status,
+                      JSBool            isCompartment)
+{
+  JSRuntime *runtime;
+  RuntimeData *data;
+
+  runtime = fop->runtime();
+  data = (RuntimeData*) JS_GetRuntimePrivate(runtime);
+
+  /* Implementation note for mozjs 24:
+     sweeping happens in two phases, in the first phase all
+     GC things from the allocation arenas are queued for
+     sweeping, then the actual sweeping happens.
+     The first phase is marked by JSFINALIZE_GROUP_START,
+     the second one by JSFINALIZE_GROUP_END, and finally
+     we will see JSFINALIZE_COLLECTION_END at the end of
+     all GC.
+     (see jsgc.cpp, BeginSweepPhase/BeginSweepingZoneGroup
+     and SweepPhase, all called from IncrementalCollectSlice).
+     Incremental GC muds the waters, because BeginSweepPhase
+     is always run to entirety, but SweepPhase can be run
+     incrementally and mixed with JS code runs or even
+     native code, when MaybeGC/IncrementalGC return.
+
+     Luckily for us, objects are treated specially, and
+     are not really queued for deferred incremental
+     finalization (unless they are marked for background
+     sweeping). Instead, they are finalized immediately
+     during phase 1, so the following guarantees are
+     true (and we rely on them)
+     - phase 1 of GC will begin and end in the same JSAPI
+       call (ie, our callback will be called with GROUP_START
+       and the triggering JSAPI call will not return until
+       we see a GROUP_END)
+     - object finalization will begin and end in the same
+       JSAPI call
+     - therefore, if there is a finalizer frame somewhere
+       in the stack, gjs_runtime_is_sweeping() will return
+       TRUE.
+
+     Comments in mozjs24 imply that this behavior might
+     change in the future, but it hasn't changed in
+     mozilla-central as of 2014-02-23. In addition to
+     that, the mozilla-central version has a huge comment
+     in a different portion of the file, explaining
+     why finalization of objects can't be mixed with JS
+     code, so we can probably rely on this behavior.
+  */
+
+  if (status == JSFINALIZE_GROUP_START)
+    data->in_gc_sweep = JS_TRUE;
+  else if (status == JSFINALIZE_GROUP_END)
+    data->in_gc_sweep = JS_FALSE;
+}
+
 JSRuntime *
 gjs_runtime_for_current_thread(void)
 {
     JSRuntime *runtime = (JSRuntime *) g_private_get(&thread_runtime);
+    RuntimeData *data;
 
     if (!runtime) {
         runtime = JS_NewRuntime(32*1024*1024 /* max bytes */, JS_USE_HELPER_THREADS);
         if (runtime == NULL)
             g_error("Failed to create javascript runtime");
 
+        data = g_new0(RuntimeData, 1);
+        JS_SetRuntimePrivate(runtime, data);
+
         JS_SetNativeStackQuota(runtime, 1024*1024);
         JS_SetGCParameter(runtime, JSGC_MAX_BYTES, 0xffffffff);
         JS_SetLocaleCallbacks(runtime, &gjs_locale_callbacks);
+        JS_SetFinalizeCallback(runtime, gjs_finalize_callback);
 
         g_private_set(&thread_runtime, runtime);
     }
diff --git a/gjs/runtime.h b/gjs/runtime.h
index 90233b0..0474940 100644
--- a/gjs/runtime.h
+++ b/gjs/runtime.h
@@ -26,4 +26,6 @@
 
 JSRuntime * gjs_runtime_for_current_thread (void);
 
+JSBool      gjs_runtime_is_sweeping        (JSRuntime *runtime);
+
 #endif /* __GJS_RUNTIME_H__ */