[gjs/wip/ptomato/develop: 5/11] object: Associate callbacks with the object on which they're installed
- From: Philip Chimento <pchimento src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [gjs/wip/ptomato/develop: 5/11] object: Associate callbacks with the object on which they're installed
- Date: Mon, 18 Sep 2017 08:13:33 +0000 (UTC)
commit fce338f6dae4173d67b27884f595f41cf9f7aa87
Author: Giovanni Campagna <gcampagna src gnome org>
Date: Sun Sep 17 14:08:24 2017 -0700
object: Associate callbacks with the object on which they're installed
Callbacks installed with GObject methods are now traced from that
object instead of being rooted, so cycles no longer cause leaks.
This implementing by making GjsCallbackTrampoline use GjsClosure
internally, and generalizing the existing code that dealt with
signal connections.
Also, to keep tests ok, we now have class finalization functions
for custom types (to unregister the closures).
https://bugzilla.gnome.org/show_bug.cgi?id=679688
gi/closure.cpp | 18 ++++------
gi/closure.h | 3 +-
gi/function.cpp | 59 +++++++++++++++++-------------
gi/function.h | 15 ++++----
gi/object.cpp | 109 ++++++++++++++++++++++++++++++++++++++++---------------
gi/object.h | 10 ++++-
gi/repo.cpp | 6 ++--
gi/value.cpp | 2 +-
8 files changed, 142 insertions(+), 80 deletions(-)
---
diff --git a/gi/closure.cpp b/gi/closure.cpp
index b92e22c..9aad756 100644
--- a/gi/closure.cpp
+++ b/gi/closure.cpp
@@ -185,8 +185,9 @@ closure_finalize(gpointer data,
self->~Closure();
}
-void
+bool
gjs_closure_invoke(GClosure *closure,
+ JS::HandleObject this_obj,
const JS::HandleValueArray& args,
JS::MutableHandleValue retval)
{
@@ -198,11 +199,11 @@ gjs_closure_invoke(GClosure *closure,
if (c->obj == nullptr) {
/* We were destroyed; become a no-op */
c->context = NULL;
- return;
+ return false;
}
context = c->context;
- JS_BeginRequest(context);
+ JSAutoRequest ar(context);
JSAutoCompartment ac(context, c->obj);
if (JS_IsExceptionPending(context)) {
@@ -212,10 +213,7 @@ gjs_closure_invoke(GClosure *closure,
}
JS::RootedValue v_closure(context, JS::ObjectValue(*c->obj));
- if (!gjs_call_function_value(context,
- /* "this" object; null is some kind of default presumably */
- nullptr,
- v_closure, args, retval)) {
+ if (!gjs_call_function_value(context, this_obj, v_closure, args, retval)) {
/* Exception thrown... */
gjs_debug_closure("Closure invocation failed (exception should "
"have been thrown) closure %p callable %p",
@@ -223,7 +221,7 @@ gjs_closure_invoke(GClosure *closure,
if (!gjs_log_exception(context))
gjs_debug_closure("Closure invocation failed but no exception was set?"
"closure %p", closure);
- goto out;
+ return false;
}
if (gjs_log_exception(context)) {
@@ -232,9 +230,7 @@ gjs_closure_invoke(GClosure *closure,
}
JS_MaybeGC(context);
-
- out:
- JS_EndRequest(context);
+ return true;
}
bool
diff --git a/gi/closure.h b/gi/closure.h
index 3d8c04e..cbb9e51 100644
--- a/gi/closure.h
+++ b/gi/closure.h
@@ -36,7 +36,8 @@ GClosure* gjs_closure_new (JSContext *context,
const char *description,
bool root_function);
-void gjs_closure_invoke(GClosure *closure,
+bool gjs_closure_invoke(GClosure *closure,
+ JS::HandleObject this_obj,
const JS::HandleValueArray& args,
JS::MutableHandleValue retval);
diff --git a/gi/function.cpp b/gi/function.cpp
index 9a8e72e..7a955e2 100644
--- a/gi/function.cpp
+++ b/gi/function.cpp
@@ -84,10 +84,10 @@ gjs_callback_trampoline_unref(GjsCallbackTrampoline *trampoline)
trampoline->ref_count--;
if (trampoline->ref_count == 0) {
+ g_closure_unref(trampoline->js_function);
g_callable_info_free_closure(trampoline->info, trampoline->closure);
g_base_info_unref( (GIBaseInfo*) trampoline->info);
g_free (trampoline->param_types);
- trampoline->~GjsCallbackTrampoline();
g_slice_free(GjsCallbackTrampoline, trampoline);
}
}
@@ -173,7 +173,6 @@ gjs_callback_closure(ffi_cif *cif,
void *data)
{
JSContext *context;
- JSObject *func_obj;
GjsCallbackTrampoline *trampoline;
int i, n_args, n_jsargs, n_outargs;
GITypeInfo ret_type;
@@ -184,7 +183,7 @@ gjs_callback_closure(ffi_cif *cif,
g_assert(trampoline);
gjs_callback_trampoline_ref(trampoline);
- context = trampoline->context;
+ context = gjs_closure_get_context(trampoline->js_function);
if (G_UNLIKELY(_gjs_context_is_sweeping(context))) {
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+ "
@@ -203,8 +202,8 @@ gjs_callback_closure(ffi_cif *cif,
}
JS_BeginRequest(context);
- func_obj = &trampoline->js_function.get().toObject();
- JSAutoCompartment ac(context, func_obj);
+ JSAutoCompartment ac(context,
+ gjs_closure_get_callable(trampoline->js_function));
n_args = g_callable_info_get_n_args(trampoline->info);
@@ -217,7 +216,6 @@ gjs_callback_closure(ffi_cif *cif,
g_error("Unable to reserve space for vector");
JS::RootedValue rval(context);
- JS::RootedValue rooted_function(context, trampoline->js_function);
JS::RootedObject this_object(context);
for (i = 0, n_jsargs = 0; i < n_args; i++) {
@@ -290,13 +288,8 @@ gjs_callback_closure(ffi_cif *cif,
}
}
- if (!JS_CallFunctionValue(context,
- this_object,
- rooted_function,
- jsargs,
- &rval)) {
+ if (!gjs_closure_invoke(trampoline->js_function, this_object, jsargs, &rval))
goto out;
- }
g_callable_info_load_return_type(trampoline->info, &ret_type);
ret_type_is_void = g_type_info_get_tag (&ret_type) == GI_TYPE_TAG_VOID;
@@ -449,11 +442,12 @@ gjs_destroy_notify_callback(gpointer data)
}
GjsCallbackTrampoline*
-gjs_callback_trampoline_new(JSContext *context,
- JS::HandleValue function,
- GICallableInfo *callable_info,
- GIScopeType scope,
- bool is_vfunc)
+gjs_callback_trampoline_new(JSContext *context,
+ JS::HandleValue function,
+ GICallableInfo *callable_info,
+ GIScopeType scope,
+ JS::HandleObject scope_object,
+ bool is_vfunc)
{
GjsCallbackTrampoline *trampoline;
int n_args, i;
@@ -467,13 +461,21 @@ gjs_callback_trampoline_new(JSContext *context,
trampoline = g_slice_new(GjsCallbackTrampoline);
new (trampoline) GjsCallbackTrampoline();
trampoline->ref_count = 1;
- trampoline->context = context;
trampoline->info = callable_info;
g_base_info_ref((GIBaseInfo*)trampoline->info);
- if (is_vfunc)
- trampoline->js_function = function;
- else
- trampoline->js_function.root(context, function);
+
+ /* The rule is:
+ * - async and call callbacks are rooted
+ * - callbacks in GObjects methods are traced from the object
+ * (and same for vfuncs, which are associated with a GObject prototype)
+ */
+ bool should_root = scope != GI_SCOPE_TYPE_NOTIFIED || !scope_object;
+ trampoline->js_function = gjs_closure_new(context, &function.toObject(),
+ g_base_info_get_name(callable_info),
+ should_root);
+ if (!should_root && scope_object)
+ gjs_object_associate_closure(context, scope_object,
+ trampoline->js_function);
/* Analyze param types and directions, similarly to init_cached_function_data */
n_args = g_callable_info_get_n_args(trampoline->info);
@@ -569,13 +571,16 @@ static bool
gjs_fill_method_instance(JSContext *context,
JS::HandleObject obj,
Function *function,
- GIArgument *out_arg)
+ GIArgument *out_arg,
+ bool& is_gobject)
{
GIBaseInfo *container = g_base_info_get_container((GIBaseInfo *) function->info);
GIInfoType type = g_base_info_get_type(container);
GType gtype = g_registered_type_info_get_g_type ((GIRegisteredTypeInfo *)container);
GITransfer transfer = g_callable_info_get_instance_ownership_transfer (function->info);
+ is_gobject = false;
+
if (type == GI_INFO_TYPE_STRUCT || type == GI_INFO_TYPE_BOXED) {
/* GError must be special cased */
if (g_type_is_a(gtype, G_TYPE_ERROR)) {
@@ -638,6 +643,7 @@ gjs_fill_method_instance(JSContext *context,
if (!gjs_typecheck_object(context, obj, gtype, true))
return false;
out_arg->v_pointer = gjs_g_object_from_object(context, obj);
+ is_gobject = true;
if (transfer == GI_TRANSFER_EVERYTHING)
g_object_ref (out_arg->v_pointer);
} else if (g_type_is_a(gtype, G_TYPE_PARAM)) {
@@ -651,6 +657,7 @@ gjs_fill_method_instance(JSContext *context,
if (!gjs_typecheck_object(context, obj, gtype, true))
return false;
out_arg->v_pointer = gjs_g_object_from_object(context, obj);
+ is_gobject = true;
if (transfer == GI_TRANSFER_EVERYTHING)
g_object_ref (out_arg->v_pointer);
} else {
@@ -740,6 +747,7 @@ gjs_invoke_c_function(JSContext *context,
bool failed, postinvoke_release_failed;
bool is_method;
+ bool is_object_method = false;
GITypeInfo return_info;
GITypeTag return_tag;
JS::AutoValueVector return_values(context);
@@ -800,8 +808,8 @@ gjs_invoke_c_function(JSContext *context,
js_arg_pos = 0; /* index into argv */
if (is_method) {
- if (!gjs_fill_method_instance(context, obj,
- function, &in_arg_cvalues[0]))
+ if (!gjs_fill_method_instance(context, obj, function,
+ &in_arg_cvalues[0], is_object_method))
return false;
ffi_arg_pointers[0] = &in_arg_cvalues[0];
++c_arg_pos;
@@ -900,6 +908,7 @@ gjs_invoke_c_function(JSContext *context,
current_arg,
callable_info,
scope,
+ is_object_method ? obj : nullptr,
false);
closure = trampoline->closure;
g_base_info_unref(callable_info);
diff --git a/gi/function.h b/gi/function.h
index 90445ff..9d3283a 100644
--- a/gi/function.h
+++ b/gi/function.h
@@ -28,7 +28,6 @@
#include <glib.h>
#include "gjs/jsapi-util.h"
-#include "gjs/jsapi-util-root.h"
#include <girepository.h>
#include <girffi.h>
@@ -44,10 +43,9 @@ typedef enum {
struct GjsCallbackTrampoline {
gint ref_count;
- JSContext *context;
GICallableInfo *info;
- GjsMaybeOwned<JS::Value> js_function;
+ GClosure *js_function;
ffi_cif cif;
ffi_closure *closure;
@@ -56,11 +54,12 @@ struct GjsCallbackTrampoline {
GjsParamType *param_types;
};
-GjsCallbackTrampoline* gjs_callback_trampoline_new(JSContext *context,
- JS::HandleValue function,
- GICallableInfo *callable_info,
- GIScopeType scope,
- bool is_vfunc);
+GjsCallbackTrampoline* gjs_callback_trampoline_new(JSContext *context,
+ JS::HandleValue function,
+ GICallableInfo *callable_info,
+ GIScopeType scope,
+ JS::HandleObject scope_object,
+ bool is_vfunc);
void gjs_callback_trampoline_unref(GjsCallbackTrampoline *trampoline);
void gjs_callback_trampoline_ref(GjsCallbackTrampoline *trampoline);
diff --git a/gi/object.cpp b/gi/object.cpp
index 1257434..ea78b50 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -61,8 +61,9 @@ struct ObjectInstance {
GjsMaybeOwned<JSObject *> keep_alive;
GType gtype;
- /* a list of all signal connections, used when tracing */
- std::set<GClosure *> signals;
+ /* a list of all GClosures installed on this object (from
+ * signals, trampolines and explicit GClosures), used when tracing */
+ std::set<GClosure *> closures;
/* the GObjectClass wrapped by this JS Object (only used for
prototypes) */
@@ -1253,17 +1254,17 @@ associate_js_gobject (JSContext *context,
}
static void
-invalidate_all_signals(ObjectInstance *priv)
+invalidate_all_closures(ObjectInstance *priv)
{
/* Can't loop directly through the items, since invalidating an item's
* closure might have the effect of removing the item from the set in the
* invalidate notifier */
- while (!priv->signals.empty()) {
+ while (!priv->closures.empty()) {
/* This will also free cd, through the closure invalidation mechanism */
- GClosure *closure = *priv->signals.begin();
+ GClosure *closure = *priv->closures.begin();
g_closure_invalidate(closure);
/* Erase element if not already erased */
- priv->signals.erase(closure);
+ priv->closures.erase(closure);
}
}
@@ -1292,7 +1293,7 @@ disassociate_js_gobject(GObject *gobj)
gobj, G_OBJECT_TYPE_NAME(gobj));
}
- invalidate_all_signals(priv);
+ invalidate_all_closures(priv);
release_native_object(priv);
/* Mark that a JS object once existed, but it doesn't any more */
@@ -1434,19 +1435,19 @@ object_instance_trace(JSTracer *tracer,
if (priv == NULL)
return;
- for (GClosure *closure : priv->signals)
+ for (GClosure *closure : priv->closures)
gjs_closure_trace(closure, tracer);
for (auto vfunc : priv->vfuncs)
- vfunc->js_function.trace(tracer, "ObjectInstance::vfunc");
+ gjs_closure_trace(vfunc->js_function, tracer);
}
static void
-signal_connection_invalidated(void *data,
- GClosure *closure)
+closure_invalidated(void *data,
+ GClosure *closure)
{
auto priv = static_cast<ObjectInstance *>(data);
- priv->signals.erase(closure);
+ priv->closures.erase(closure);
}
static void
@@ -1470,7 +1471,7 @@ object_instance_finalize(JSFreeOp *fop,
/* This applies only to instances, not prototypes, but it's possible that
* an instance's GObject is already freed at this point. */
- invalidate_all_signals(priv);
+ invalidate_all_closures(priv);
/* Object is instance, not prototype, AND GObject is not already freed */
if (priv->gobj) {
@@ -1497,8 +1498,6 @@ object_instance_finalize(JSFreeOp *fop,
/* We have to leak the trampolines, since the GType's vtable still refers
* to them */
- for (auto iter : priv->vfuncs)
- iter->js_function.reset();
priv->vfuncs.clear();
if (priv->keep_alive.rooted()) {
@@ -1559,7 +1558,9 @@ gjs_lookup_object_constructor_from_info(JSContext *context,
/* In case we're looking for a private type, and we don't find it,
we need to define it first.
*/
- gjs_define_object_class(context, in_object, NULL, gtype, &constructor);
+ JS::RootedObject ignored(context);
+ gjs_define_object_class(context, in_object, NULL, gtype, &constructor,
+ &ignored);
} else {
if (G_UNLIKELY (!value.isObject()))
return NULL;
@@ -1609,6 +1610,16 @@ gjs_lookup_object_prototype(JSContext *context,
return proto;
}
+static void
+do_associate_closure(ObjectInstance *priv,
+ GClosure *closure)
+{
+ /* This is a weak reference, and will be cleared when the closure is
+ * invalidated */
+ priv->closures.insert(closure);
+ g_closure_add_invalidate_notifier(closure, priv, closure_invalidated);
+}
+
static bool
real_connect_func(JSContext *context,
unsigned argc,
@@ -1658,11 +1669,7 @@ real_connect_func(JSContext *context,
closure = gjs_closure_new_for_signal(context, &argv[1].toObject(), "signal callback", signal_id);
if (closure == NULL)
return false;
-
- /* This is a weak reference, and will be cleared when the closure is invalidated */
- priv->signals.insert(closure);
- g_closure_add_invalidate_notifier(closure, priv,
- signal_connection_invalidated);
+ do_associate_closure(priv, closure);
id = g_signal_connect_closure_by_id(priv->gobj,
signal_id,
@@ -1929,10 +1936,11 @@ gjs_define_object_class(JSContext *context,
JS::HandleObject in_object,
GIObjectInfo *info,
GType gtype,
- JS::MutableHandleObject constructor)
+ JS::MutableHandleObject constructor,
+ JS::MutableHandleObject prototype)
{
const char *constructor_name;
- JS::RootedObject prototype(context), parent_proto(context);
+ JS::RootedObject parent_proto(context);
ObjectInstance *priv;
const char *ns;
@@ -2001,7 +2009,7 @@ gjs_define_object_class(JSContext *context,
NULL,
/* funcs of constructor, MyConstructor.myfunc() */
NULL,
- &prototype,
+ prototype,
constructor)) {
g_error("Can't init class %s", constructor_name);
}
@@ -2324,7 +2332,8 @@ gjs_hook_up_vfunc(JSContext *cx,
JS::RootedValue v_function(cx, JS::ObjectValue(*function));
trampoline = gjs_callback_trampoline_new(cx, v_function, callback_info,
- GI_SCOPE_TYPE_NOTIFIED, true);
+ GI_SCOPE_TYPE_NOTIFIED,
+ object, true);
*((ffi_closure **)method_ptr) = trampoline->closure;
priv->vfuncs.push_back(trampoline);
@@ -2805,6 +2814,30 @@ gjs_register_interface(JSContext *cx,
return true;
}
+static void
+gjs_object_base_init(void *klass)
+{
+ auto priv = static_cast<ObjectInstance *>(g_type_get_qdata(G_OBJECT_CLASS_TYPE(klass),
+ gjs_object_priv_quark()));
+
+ if (priv) {
+ for (GClosure *closure : priv->closures)
+ g_closure_ref(closure);
+ }
+}
+
+static void
+gjs_object_base_finalize(void *klass)
+{
+ auto priv = static_cast<ObjectInstance *>(g_type_get_qdata(G_OBJECT_CLASS_TYPE(klass),
+ gjs_object_priv_quark()));
+
+ if (priv) {
+ for (GClosure *closure : priv->closures)
+ g_closure_unref(closure);
+ }
+}
+
static bool
gjs_register_type(JSContext *cx,
unsigned argc,
@@ -2818,8 +2851,8 @@ gjs_register_type(JSContext *cx,
GTypeInfo type_info = {
0, /* class_size */
- (GBaseInitFunc) NULL,
- (GBaseFinalizeFunc) NULL,
+ gjs_object_base_init,
+ gjs_object_base_finalize,
(GClassInitFunc) gjs_object_class_init,
(GClassFinalizeFunc) NULL,
@@ -2892,8 +2925,13 @@ gjs_register_type(JSContext *cx,
gjs_add_interface(instance_type, iface_types[i]);
/* create a custom JSClass */
- JS::RootedObject module(cx, gjs_lookup_private_namespace(cx)), constructor(cx);
- gjs_define_object_class(cx, module, NULL, instance_type, &constructor);
+ JS::RootedObject module(cx, gjs_lookup_private_namespace(cx));
+ JS::RootedObject constructor(cx), prototype(cx);
+ gjs_define_object_class(cx, module, nullptr, instance_type, &constructor,
+ &prototype);
+
+ ObjectInstance *priv = priv_from_js(cx, prototype);
+ g_type_set_qdata(instance_type, gjs_object_priv_quark(), priv);
argv.rval().setObject(*constructor);
@@ -3022,3 +3060,16 @@ gjs_lookup_object_constructor(JSContext *context,
value_p.setObject(*constructor);
return true;
}
+
+bool
+gjs_object_associate_closure(JSContext *cx,
+ JS::HandleObject object,
+ GClosure *closure)
+{
+ ObjectInstance *priv = priv_from_js(cx, object);
+ if (!priv)
+ return false;
+
+ do_associate_closure(priv, closure);
+ return true;
+}
diff --git a/gi/object.h b/gi/object.h
index 299c848..8cb1adc 100644
--- a/gi/object.h
+++ b/gi/object.h
@@ -25,7 +25,8 @@
#define __GJS_OBJECT_H__
#include <stdbool.h>
-#include <glib.h>
+
+#include <glib-object.h>
#include <girepository.h>
#include "gjs/jsapi-util.h"
@@ -35,7 +36,8 @@ void gjs_define_object_class(JSContext *context,
JS::HandleObject in_object,
GIObjectInfo *info,
GType gtype,
- JS::MutableHandleObject constructor);
+ JS::MutableHandleObject constructor,
+ JS::MutableHandleObject prototype);
bool gjs_lookup_object_constructor(JSContext *context,
GType gtype,
@@ -67,6 +69,10 @@ void gjs_object_define_static_methods(JSContext *context,
bool gjs_define_private_gi_stuff(JSContext *cx,
JS::MutableHandleObject module);
+bool gjs_object_associate_closure(JSContext *cx,
+ JS::HandleObject obj,
+ GClosure *closure);
+
G_END_DECLS
#endif /* __GJS_OBJECT_H__ */
diff --git a/gi/repo.cpp b/gi/repo.cpp
index 4cc1e6c..1b037da 100644
--- a/gi/repo.cpp
+++ b/gi/repo.cpp
@@ -422,9 +422,9 @@ gjs_define_info(JSContext *context,
if (g_type_is_a (gtype, G_TYPE_PARAM)) {
gjs_define_param_class(context, in_object);
} else if (g_type_is_a (gtype, G_TYPE_OBJECT)) {
- JS::RootedObject ignored(context);
- gjs_define_object_class(context, in_object,
- (GIObjectInfo *) info, gtype, &ignored);
+ JS::RootedObject ignored1(context), ignored2(context);
+ gjs_define_object_class(context, in_object, info, gtype,
+ &ignored1, &ignored2);
} else if (G_TYPE_IS_INSTANTIATABLE(gtype)) {
JS::RootedObject ignored1(context), ignored2(context);
if (!gjs_define_fundamental_class(context, in_object,
diff --git a/gi/value.cpp b/gi/value.cpp
index 9a4ba93..f6d7f69 100644
--- a/gi/value.cpp
+++ b/gi/value.cpp
@@ -269,7 +269,7 @@ closure_marshal(GClosure *closure,
g_base_info_unref((GIBaseInfo *)type_info_for[i]);
JS::RootedValue rval(context);
- gjs_closure_invoke(closure, argv, &rval);
+ gjs_closure_invoke(closure, nullptr, argv, &rval);
if (return_value != NULL) {
if (rval.isUndefined()) {
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
