GNOME Bugzilla – Bug 760057
Crash with subclassed fundamental with no introspection
Last modified: 2017-01-14 05:48:32 UTC
This is to try to implement a ClutterContent object in JS. One of ClutterContent's method has the following prototype : void (* paint_content) (ClutterContent *content, ClutterActor *actor, ClutterPaintNode *node); ClutterPaintNode is a fundamental type and has several subclasses. All but 2 have introspection data available for them. But the remaining 2 are considered internal classes and so no introspection data (ClutterDummyNode & ClutterRootNode). But these are gtype that gjs might manipulate. Currently not having any introspection data triggers a crash. The attached patch provides a fix for this by looking at the parent classes to try to get introspection data for the object gjs wants to expose to JS.
Created attachment 318142 [details] [review] fundamental: Fix crash with subclassed fundamental with no introspection We could have a fundamental type A derived by a subclass B. Class A happens to be introspected, but B isn't. This is currently triggering a crash.
Ping?
Review of attachment 318142 [details] [review]: Looks reasonable except for the one comment; is this masking a case that currently isn't covered by the tests? You'll also need to ping someone to review the gobject-introspection counterpart bug, I'm not a reviewer there. ::: gi/fundamental.cpp @@ +640,3 @@ + + /* With no introspection data, we can't find any internal types. */ + g_irepository_find_by_gtype(g_irepository_get_default(), Before, info was passed along into gjs_lookup_fundamental_prototype() even if it was NULL. It looks like gjs_lookup_fundamental_prototype() had code to handle that case, though I'm not entirely sure what it means. So it seems that this patch would bypass that case now?
Hi Philip, Thanks a lot for reviewing this. I'm not sure whether anybody actually reviews introspection patches. So if you're interested in this, I guess you should review and I'll land.
Created attachment 343012 [details] [review] fundamental: Fix crash with subclassed fundamental with no introspection
Forgot to answer your question. I think you can currently crash gjs by implementing the ClutterContent interface and try to implement the paint_content() vfunc.
I meant with my question to refer to the "if (info == NULL) return NULL;" lines, but Bugzilla messed up the lines that it quoted in my review :-) I don't think you should have the NULL check there because gjs_lookup_fundamental_prototype() explicitly handles the case where info == NULL (it uses the "private namespace" which I'm not quite sure what it means) but now that path can never be triggered. So I think your patch might be breaking something but doesn't cause any tests to fail because whatever that case is, isn't covered in the test suite. I'm not positive though. Anyway, I tried it without the NULL check and your new test case still passes, so if it still fixes your original bug without the NULL check then it's probably fine. I know it was a long time ago, but do you remember what the NULL check was for? Thanks for the rebase and response!
Review of attachment 343012 [details] [review]: ::: gi/fundamental.cpp @@ +630,3 @@ + + /* With no introspection data, we can't find any internal types. */ + g_irepository_find_by_gtype(g_irepository_get_default(), 632-633 were the lines I meant, let's see if Bugzilla quotes it correctly this time... @@ +760,3 @@ G_TYPE_FROM_INSTANCE(gfundamental))); + if (!proto) + return object; May as well return NULL here since object can only be NULL at this point.
Review of attachment 343012 [details] [review]: ::: gi/fundamental.cpp @@ +630,3 @@ + + /* With no introspection data, we can't find any internal types. */ + g_irepository_find_by_gtype(g_irepository_get_default(), Right, I think we still need this check, it is possible we are given an object from a C library that doesn't have any introspection data nor parent type that we can find introspection data for. @@ +760,3 @@ G_TYPE_FROM_INSTANCE(gfundamental))); + if (!proto) + return object; Sure!
Created attachment 343128 [details] [review] fundamental: Fix crash with subclassed fundamental with no introspection
Comment on attachment 343012 [details] [review] fundamental: Fix crash with subclassed fundamental with no introspection >From 4b1f4213ce78a175b833bf6d655f8a4fda4836b3 Mon Sep 17 00:00:00 2001 >From: Lionel Landwerlin <llandwerlin@gmail.com> >Date: Sat, 2 Jan 2016 00:31:49 +0100 >Subject: [PATCH] fundamental: Fix crash with subclassed fundamental with no > introspection > >We could have a fundamental type A derived by a subclass B. Class A happens >to be introspected, but B isn't. This is currently triggering a crash. > >https://bugzilla.gnome.org/show_bug.cgi?id=760057 >--- > gi/fundamental.cpp | 18 ++++++++++++++++-- > installed-tests/js/testFundamental.js | 4 ++++ > 2 files changed, 20 insertions(+), 2 deletions(-) > >diff --git a/gi/fundamental.cpp b/gi/fundamental.cpp >index 56c7121..3a7e8e1 100644 >--- a/gi/fundamental.cpp >+++ b/gi/fundamental.cpp >@@ -619,8 +619,19 @@ gjs_lookup_fundamental_prototype_from_gtype(JSContext *context, > GIObjectInfo *info; > JSObject *proto; > >- info = (GIObjectInfo *) g_irepository_find_by_gtype(g_irepository_get_default(), >- gtype); >+ /* A given gtype might not have any definition in the introspection >+ * data. If that's the case, try to look for a definition of any of the >+ * parent type. */ >+ while ((info = (GIObjectInfo *) >+ g_irepository_find_by_gtype(g_irepository_get_default(), >+ gtype)) == NULL && >+ gtype != G_TYPE_INVALID) >+ gtype = g_type_parent(gtype); >+ >+ /* With no introspection data, we can't find any internal types. */ >+ if (info == NULL) >+ return NULL; >+ > proto = gjs_lookup_fundamental_prototype(context, info, gtype); > if (info) > g_base_info_unref((GIBaseInfo*)info); >@@ -747,6 +758,9 @@ gjs_object_from_g_fundamental(JSContext *context, > JS::RootedObject proto(context, > gjs_lookup_fundamental_prototype_from_gtype(context, > G_TYPE_FROM_INSTANCE(gfundamental))); >+ if (!proto) >+ return object; >+ > JS::RootedObject global(context, gjs_get_import_global(context)); > object = JS_NewObjectWithGivenProto(context, JS_GetClass(proto), proto, > global); >diff --git a/installed-tests/js/testFundamental.js b/installed-tests/js/testFundamental.js >index ff94a23..fc08de0 100644 >--- a/installed-tests/js/testFundamental.js >+++ b/installed-tests/js/testFundamental.js >@@ -4,4 +4,8 @@ describe('Fundamental type support', function () { > it('constructs a subtype of a fundamental type', function () { > expect(() => new Regress.TestFundamentalSubObject('plop')).not.toThrow(); > }); >+ >+ it('constructs a subtype of a hidden (no introspection data) fundamental type', function() { >+ expect(() => Regress.test_create_fundamental_hidden_class_instance()).not.toThrow(); >+ }); > }); >-- >2.11.0 >
Comment on attachment 343128 [details] [review] fundamental: Fix crash with subclassed fundamental with no introspection (In reply to Lionel Landwerlin from comment #10) > Review of attachment 343012 [details] [review] [review]: > > ::: gi/fundamental.cpp > @@ +630,3 @@ > + > + /* With no introspection data, we can't find any internal types. */ > + g_irepository_find_by_gtype(g_irepository_get_default(), > > Right, I think we still need this check, it is possible we are given an > object from a C library that doesn't have any introspection data nor parent > type that we can find introspection data for. I'm not convinced, because in the gjs_lookup_fundamental_prototype() call that follows the check, there is this code: if (info) { in_object = gjs_lookup_namespace_object(context, (GIBaseInfo*) info); constructor_name = g_base_info_get_name((GIBaseInfo*) info); } else { in_object = gjs_lookup_private_namespace(context); constructor_name = g_type_name(gtype); } That tells me that gjs_lookup_fundamental_prototype() is intended to be called with info == NULL if you are defining a prototype for a gtype with no introspection info, and in that case the prototype will get defined in the "private namespace" (though I'm not exactly sure what that is.) With the check, that code path can never be reached. Your new test still passes for me without the check, in any case. Is the original problem with ClutterContent still solved even if you remove the check?
Created attachment 343428 [details] [review] fundamental: Fix crash with subclassed fundamental with no introspection Looks like you're right. Plus the new test is passing so I guess it's all fine.
Thanks for the updated patch!
Attachment 343428 [details] pushed as d3c5225 - fundamental: Fix crash with subclassed fundamental with no introspection