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

4 files changed, 123 insertions, 1 deletions

diff --git a/gi/function.cpp b/gi/function.cpp
index 8d2fe77d..86c32b1f 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 828a2599..4a2e6caa 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 68a2cc83..5069b866 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 90233b01..0474940b 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__ */