From 40084ad481771f85f0dd183d3e07c9d3ceefa4a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 27 Nov 2017 17:22:33 -0500 Subject: [PATCH 1/5] object: check in gjs_typecheck_object if the object has been finalized --- gi/object.cpp | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/gi/object.cpp b/gi/object.cpp index 94c8ee29a..1214b8ac6 100644 --- a/gi/object.cpp +++ b/gi/object.cpp @@ -69,6 +69,7 @@ struct ObjectInstance { GTypeClass *klass; unsigned js_object_finalized : 1; + unsigned g_object_finalized : 1; }; static std::stack object_init_list; @@ -920,7 +921,10 @@ static void wrapped_gobj_dispose_notify(gpointer data, GObject *where_the_object_was) { - wrapped_gobject_list.erase(static_cast(data)); + ObjectInstance *priv = static_cast(data); + + priv->g_object_finalized = true; + wrapped_gobject_list.erase(priv); #if DEBUG_DISPOSE gjs_debug(GJS_DEBUG_GOBJECT, "Wrapped GObject %p disposed", where_the_object_was); #endif @@ -2115,6 +2119,17 @@ gjs_typecheck_object(JSContext *context, return false; } + if (priv->g_object_finalized) { + if (throw_error) { + gjs_throw(context, + "Object is %s.%s.prototype, has been already deallocated - cannot convert to GObject*", + priv->info ? g_base_info_get_namespace( (GIBaseInfo*) priv->info) : "", + priv->info ? g_base_info_get_name( (GIBaseInfo*) priv->info) : g_type_name(priv->gtype)); + } + + return false; + } + g_assert(priv->gtype == G_OBJECT_TYPE(priv->gobj)); if (expected_type != G_TYPE_NONE) -- GitLab From f94a9d07c70004438b5edcf7e5c7ead195ddebfb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 27 Nov 2017 17:42:59 -0500 Subject: [PATCH 2/5] object: don't resolve or set,get properties on finalized elements --- gi/object.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/gi/object.cpp b/gi/object.cpp index 1214b8ac6..24ca7153a 100644 --- a/gi/object.cpp +++ b/gi/object.cpp @@ -391,6 +391,9 @@ object_instance_get_prop(JSContext *context, if (priv->gobj == NULL) /* prototype, not an instance. */ return true; + if (priv->g_object_finalized) + return false; + if (!get_prop_from_g_param(context, obj, priv, name, value_p)) return false; @@ -501,6 +504,9 @@ object_instance_set_prop(JSContext *context, if (priv->gobj == NULL) /* prototype, not an instance. */ return result.succeed(); + if (priv->g_object_finalized) + return false; + ret = set_g_param_from_prop(context, priv, name, g_param_was_set, value_p, result); if (g_param_was_set || !ret) return ret; @@ -738,6 +744,11 @@ object_instance_resolve(JSContext *context, return true; } + if (priv->g_object_finalized) { + *resolved = false; + return false; + } + /* If we have no GIRepository information (we're a JS GObject subclass), * we need to look at exposing interfaces. Look up our interfaces through * GType data, and then hope that *those* are introspectable. */ @@ -1422,7 +1433,7 @@ object_instance_trace(JSTracer *tracer, ObjectInstance *priv; priv = (ObjectInstance *) JS_GetPrivate(obj); - if (priv == NULL) + if (priv == NULL || priv->g_object_finalized) return; for (GClosure *closure : priv->closures) -- GitLab From b4cae4b37aa1fc9fe737549121fdf19cab80aa1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 29 Nov 2017 15:42:25 -0500 Subject: [PATCH 3/5] object: add better logging when invalid access happens Throw errors in any case. --- gi/object.cpp | 50 ++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 40 insertions(+), 10 deletions(-) diff --git a/gi/object.cpp b/gi/object.cpp index 24ca7153a..38229830d 100644 --- a/gi/object.cpp +++ b/gi/object.cpp @@ -391,8 +391,14 @@ object_instance_get_prop(JSContext *context, if (priv->gobj == NULL) /* prototype, not an instance. */ return true; - if (priv->g_object_finalized) + if (priv->g_object_finalized) { + gjs_throw(context, "Object %s.%s (%p), has been already finalized. " + "Impossible to get any property from it.", + priv->info ? g_base_info_get_namespace( (GIBaseInfo*) priv->info) : "", + priv->info ? g_base_info_get_name( (GIBaseInfo*) priv->info) : g_type_name(priv->gtype), + priv->gobj); return false; + } if (!get_prop_from_g_param(context, obj, priv, name, value_p)) return false; @@ -504,8 +510,15 @@ object_instance_set_prop(JSContext *context, if (priv->gobj == NULL) /* prototype, not an instance. */ return result.succeed(); - if (priv->g_object_finalized) + if (priv->g_object_finalized) { + gjs_throw(context, "Object %s.%s (%p), has been already finalized. " + "Impossible to set any property to it.", + priv->info ? g_base_info_get_namespace( (GIBaseInfo*) priv->info) : "", + priv->info ? g_base_info_get_name( (GIBaseInfo*) priv->info) : g_type_name(priv->gtype), + priv->gobj); + return false; + } ret = set_g_param_from_prop(context, priv, name, g_param_was_set, value_p, result); if (g_param_was_set || !ret) @@ -746,6 +759,13 @@ object_instance_resolve(JSContext *context, if (priv->g_object_finalized) { *resolved = false; + + gjs_throw(context, "Object %s.%s (%p), has been already finalized. " + "Impossible to resolve it.", + priv->info ? g_base_info_get_namespace( (GIBaseInfo*) priv->info) : "", + priv->info ? g_base_info_get_name( (GIBaseInfo*) priv->info) : g_type_name(priv->gtype), + priv->gobj); + return false; } @@ -932,7 +952,7 @@ static void wrapped_gobj_dispose_notify(gpointer data, GObject *where_the_object_was) { - ObjectInstance *priv = static_cast(data); + auto *priv = static_cast(data); priv->g_object_finalized = true; wrapped_gobject_list.erase(priv); @@ -1433,9 +1453,18 @@ object_instance_trace(JSTracer *tracer, ObjectInstance *priv; priv = (ObjectInstance *) JS_GetPrivate(obj); - if (priv == NULL || priv->g_object_finalized) + if (priv == NULL) return; + if (priv->g_object_finalized) { + g_debug("Object %s.%s (%p), has been already finalized. " + "Impossible to trace it.", + priv->info ? g_base_info_get_namespace( (GIBaseInfo*) priv->info) : "", + priv->info ? g_base_info_get_name( (GIBaseInfo*) priv->info) : g_type_name(priv->gtype), + priv->gobj); + return; + } + for (GClosure *closure : priv->closures) gjs_closure_trace(closure, tracer); } @@ -2131,12 +2160,13 @@ gjs_typecheck_object(JSContext *context, } if (priv->g_object_finalized) { - if (throw_error) { - gjs_throw(context, - "Object is %s.%s.prototype, has been already deallocated - cannot convert to GObject*", - priv->info ? g_base_info_get_namespace( (GIBaseInfo*) priv->info) : "", - priv->info ? g_base_info_get_name( (GIBaseInfo*) priv->info) : g_type_name(priv->gtype)); - } + gjs_throw(context, + "Object %s.%s (%p), has been already deallocated - impossible to access to it. " + "This might be caused by the fact that the object has been destroyed from C " + "code using something such as destroy(), dispose(), or remove() vfuncs", + priv->info ? g_base_info_get_namespace( (GIBaseInfo*) priv->info) : "", + priv->info ? g_base_info_get_name( (GIBaseInfo*) priv->info) : g_type_name(priv->gtype), + priv->gobj); return false; } -- GitLab From 81225d067160a6850eb513ef44b393aeb24ac3ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 29 Nov 2017 15:45:54 -0500 Subject: [PATCH 4/5] object: reset the keep alive flag on wrapper when object is disposed --- gi/object.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/gi/object.cpp b/gi/object.cpp index 38229830d..69bba126b 100644 --- a/gi/object.cpp +++ b/gi/object.cpp @@ -955,6 +955,7 @@ wrapped_gobj_dispose_notify(gpointer data, auto *priv = static_cast(data); priv->g_object_finalized = true; + priv->keep_alive.reset(); wrapped_gobject_list.erase(priv); #if DEBUG_DISPOSE gjs_debug(GJS_DEBUG_GOBJECT, "Wrapped GObject %p disposed", where_the_object_was); -- GitLab From 6be9eb1325845c31fc88894e3276768a4c9f1ed7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Thu, 30 Nov 2017 17:16:37 -0500 Subject: [PATCH 5/5] installed-tests/js: add testGObjectDestructionAccess to verify access to destryed objects Check if proper errors are emitted when accessing to properties or methods of destroyed objects. --- Makefile-test.am | 1 + .../js/testGObjectDestructionAccess.js | 41 +++++++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 installed-tests/js/testGObjectDestructionAccess.js diff --git a/Makefile-test.am b/Makefile-test.am index 5ea78ad53..5c9f428ce 100644 --- a/Makefile-test.am +++ b/Makefile-test.am @@ -199,6 +199,7 @@ common_jstests_files = \ installed-tests/js/testGIMarshalling.js \ installed-tests/js/testGLib.js \ installed-tests/js/testGObjectClass.js \ + installed-tests/js/testGObjectDestructionAccess.js \ installed-tests/js/testGObjectInterface.js \ installed-tests/js/testGTypeClass.js \ installed-tests/js/testGio.js \ diff --git a/installed-tests/js/testGObjectDestructionAccess.js b/installed-tests/js/testGObjectDestructionAccess.js new file mode 100644 index 000000000..786abe425 --- /dev/null +++ b/installed-tests/js/testGObjectDestructionAccess.js @@ -0,0 +1,41 @@ +// -*- mode: js; indent-tabs-mode: nil -*- +imports.gi.versions.Gtk = '3.0'; + +const Gtk = imports.gi.Gtk; + +describe('Access to destroyed GObject', () => { + let destroyedWindow; + + beforeAll(() => { + Gtk.init(null); + }); + + beforeEach(() => { + destroyedWindow = new Gtk.Window({type: Gtk.WindowType.TOPLEVEL}); + destroyedWindow.destroy(); + }); + + it('Get property', () => { + expect(() => { + let title = destroyedWindow.title; + }).toThrowError(/Object Gtk.Window \(0x[a-f0-9]+\), has been already finalized. Impossible to get any property from it./) + }); + + it('Set property', () => { + expect(() => { + destroyedWindow.title = 'I am dead'; + }).toThrowError(/Object Gtk.Window \(0x[a-f0-9]+\), has been already finalized. Impossible to set any property to it./) + }); + + it('Access to getter method', () => { + expect(() => { + let title = destroyedWindow.get_title(); + }).toThrowError(/Object Gtk.Window \(0x[a-f0-9]+\), has been already deallocated.*/) + }); + + it('Access to setter method', () => { + expect(() => { + destroyedWindow.set_title('I am dead'); + }).toThrowError(/Object Gtk.Window \(0x[a-f0-9]+\), has been already deallocated.*/) + }); +}); -- GitLab