I can't find the error message you mention in the source code; can you provide a test case?

g_warning in the patch isn't going to be right; g_warning always means there's a bug in the program. So if g_warning happens, this case is still broken.

The error message was:

 ERROR:gi/object.c:685:object_instance_constructor: assertion failed:
 (unthreadsafe_template_for_constructor.info == NULL ||
 strcmp(g_base_info_get_name( (GIBaseInfo*) priv->info), g_base_info_get_name(
 (GIBaseInfo*) unthreadsafe_template_for_constructor.info)) == 0)
 Shell killed with signal 6

To reproduce this, need run gnome-shell (with patch from https://bugzilla.gnome.org/show_bug.cgi?id=591912)
and use the zoom-window-on-scroll functionality in the overview

It isn't clear to me why this is happening. If you change your patch to have     g_warning("Have mismatch of prototype's type %s and required type %s.", , );

to print out the problem types, what does it say?

The patch looks like it fixes a symptom instead of the problem.

(In reply to comment #3)
> It isn't clear to me why this is happening. If you change your patch to have   
>  g_warning("Have mismatch of prototype's type %s and required type %s.", , );
> 
> to print out the problem types, what does it say?
> 
> The patch looks like it fixes a symptom instead of the problem.

Have mismatch of prototype's type Actor and required type Texture.

Log:
JS G FUNC: gtype of INTERFACE is ClutterActor
    JS G OBJ: Wrapping ClutterGLXTexturePixmap with JSObject
    JS G OBJ: No introspection data on 'ClutterGLXTexturePixmap' so trying parent type 'ClutterX11TexturePixmap'
    JS G OBJ: No introspection data on 'ClutterX11TexturePixmap' so trying parent type 'ClutterTexture'
   JS G REPO: Constructing instance of dynamic class _private_Clutter_ClutterGLXTexturePixmap 0x34eb670 from proto 0x3f70f40
    JS G OBJ: obj instance constructor, obj 0x4187e00 priv 0x3fac880 retval 0x4187e00
    JS G OBJ: obj instance __proto__ is 0x3f70f40
    JS G OBJ: obj instance constructing proto 0, obj class _private_Clutter_ClutterGLXTexturePixmap proto class _private_Clutter_ClutterGLXTexturePixmap

then assert.

what are the two info type names that don't match?

(In reply to comment #5)
> what are the two info type names that don't match?

Actor,Texture

Oops, I see you told us that before.

I would say the problem is that there should be a prototype of ClutterTexture and it should be used here... so the question is why there isn't. That would be the root cause bug, no?

Created attachment 149185 [details] [review]
Patch for this bug

gjs_define_object_class has the argument "info".
info can contain GIObjectInfo for self or parent.

parent_info = g_object_info_get_parent(info); -- Incorrect

Review of attachment 149185 [details] [review]:

It would be really helpful to provide a commit message with a patch like this that explains what the patch is doing; that is needed anyways in order to commit the patch and would help a lot in review. Without a lot of detailed analysis of the surrounding code or trying things out in practice it looks to me like a much simpler fix might be possible.

::: gi/object.c
@@ +1526,3 @@
 {
     JSObject *obj;
+    gint parents_not_found = -1;

gjs doesn't use gint

@@ +1546,3 @@
         gtype = G_TYPE_FROM_INSTANCE(gobj);
         while (info == NULL) {
+            parents_not_found++;

If you move this down a bit, you won't need the initalization to -1, but can use a more normal initialization to 0

@@ -1547,1 @@
-        proto = gjs_lookup_object_prototype(context, G_TYPE_FROM_INSTANCE(gobj), (GIObjectInfo*) info);

Isn't the base problem here that G_TYPE_FROM_INSTANCE(gobj) and 'info' don't match? Isn't the right fix to replace this whole patch with something like:

-proto = gjs_lookup_object_prototype(context, G_TYPE_FROM_INSTANCE(gobj), (GIObjectInfo*) info);
+proto = gjs_lookup_object_prototype(context, gtype, (GIObjectInfo*) info);

?

@@ +1576,3 @@
+            ns = gjs_lookup_namespace_object(context, (GIBaseInfo*) info);
+            if (ns)
+                gjs_define_object_class(context, ns, G_TYPE_FROM_INSTANCE(gobj), NULL, NULL, &proto);

This duplicates code from gjs_lookup_object_prototype(), and does it a little bit differently, that's not OK. If the "little bit differently" is needed, then passing a flag field to gjs_lookup_object_prototype() would be the best option; but see above.

Created attachment 149348 [details] [review]
Corrected patch

> Isn't the right fix to replace this whole patch with something like:
> -proto = gjs_lookup_object_prototype(context, G_TYPE_FROM_INSTANCE(gobj),
> (GIObjectInfo*) info);
> +proto = gjs_lookup_object_prototype(context, gtype, (GIObjectInfo*) info);
> ?

This will be a cause a new error "conflicting gtypes for prototype XXXX () (was YYYYY (31551824))".
But I found easy way, than before.

(In reply to comment #10)
> Created an attachment (id=149348) [details] [review]
> Corrected patch
> 
> > Isn't the right fix to replace this whole patch with something like:
> > -proto = gjs_lookup_object_prototype(context, G_TYPE_FROM_INSTANCE(gobj),
> > (GIObjectInfo*) info);
> > +proto = gjs_lookup_object_prototype(context, gtype, (GIObjectInfo*) info);
> > ?
> 
> This will be a cause a new error "conflicting gtypes for prototype XXXX () (was
> YYYYY (31551824))". But I found easy way, than before.

It initially seemed a little busted to me two have two different JS classes/prototypes for:

 introspectable=ClutterTexture, gtype=ClutterGLXTexturePixmap
 introspectable=ClutterTexture, gtype=ClutterTexture

But then it occurred to me that for interfaces, the introspectable parent type might be GObject in two cases with entirely different sets of exported interfaces, so we need to make the prototype correspond to the most derived GType, even when there is no corresponding GIObjectInfo.

Review of attachment 149348 [details] [review]:

At one level, this patch looks right to me, but at another level, it's taking an unclear piece of code that is doing the wrong thing and adding some undocumented extra checks to it, and that's going to make future maintenance really hard.

What actually needs to be done:

 - gjs_lookup_object_constructor() and gjs_lookup_object_class() need to be removed; they are unused, and make it harder to figure out what is going on in the code

 - The code to find 'info' in 'gjs_object_from_g_object' should be moved to gjs_define_object_class(), the code will be clearer with it in that location (gjs_lookup_object_prototype()  would lose the 'info' argument and just take a gtype, it would then probably call gjs_define_object_class() with NULL for in_object)

 - The comment in the code:

    /* FIXME: this traverses GIObjectInfo hierarchy too quickly. Should go up
     * the GType hierarchy to find the closest parent GIObjectInfo, but we
     * don't always have valid 'gtype' to do that. (This is only a problem when
     * g-i has only partial knowledge about the GType hierarchy, for example
     * with GIO where most concrete types are private and meant to be accessed
     * through interfaces only.)
     */

Needs to be improved, or we need to fix what is FIXME'd - it doesn't really make sense to say "we don't always have valid 'gtype' to do that" - then immediately below use a function that goes from an arbitrary GIObjectInfo to GType, as is with your patch.

I think what the FIXME is pointing to is that if we have a ClutterX11TexturePixmap and a ClutterGLXTexturePixmap, and create JS proxies for each of them then we'll end up with the JS heirarchy of:

                         Clutter.Texture
                        /               \
 Clutter.ClutterX11TexturePixmap        Clutter.ClutterGLXTexturePixmap

While we'd expect:

                         Clutter.Texture
                                |
                 Clutter.ClutterX11TexturePixmap        
                                |
                Clutter.ClutterGLXTexturePixmap

Though what it actually says "Should go up the GType hierarchy to find the closest parent GIObjectInfo" doesn't actually make any sense - if we have a GIObjectInfo we can reliably go to the parent GIObject info. So eitherthe FIXME needs to be adjusted to describe the real problem, or we should just walk up the GType heirarchy. I don't have a strong opinion on which better technically, but my instinct would be to switch to walking up the GType heirarchy - because that allows us to *eliminate* the comment, and that is always better.

(In reply to comment #12)

>  - The comment in the code:
> 
>     /* FIXME: this traverses GIObjectInfo hierarchy too quickly. Should go up
>      * the GType hierarchy to find the closest parent GIObjectInfo, but we
>      * don't always have valid 'gtype' to do that. (This is only a problem when
>      * g-i has only partial knowledge about the GType hierarchy, for example
>      * with GIO where most concrete types are private and meant to be accessed
>      * through interfaces only.)
>      */
> 
> Needs to be improved, or we need to fix what is FIXME'd - it doesn't really
> make sense to say "we don't always have valid 'gtype' to do that" - then
> immediately below use a function that goes from an arbitrary GIObjectInfo to
> GType, as is with your patch.

The problem was that the function is being called from (at least) two contexts, in one the concrete GType is known and passed in 'gtype' argument, but in the other we don't know the GType ('gtype' is G_TYPE_INVALID) so couldn't walk the GType hierarchy either.

I forgot where that other call site was. Maybe the code has changed since and the problem no longer exists.

(In reply to comment #13)
> (In reply to comment #12)
> 
> >  - The comment in the code:
> > 
> >     /* FIXME: this traverses GIObjectInfo hierarchy too quickly. Should go up
> >      * the GType hierarchy to find the closest parent GIObjectInfo, but we
> >      * don't always have valid 'gtype' to do that. (This is only a problem when
> >      * g-i has only partial knowledge about the GType hierarchy, for example
> >      * with GIO where most concrete types are private and meant to be accessed
> >      * through interfaces only.)
> >      */
> > 
> > Needs to be improved, or we need to fix what is FIXME'd - it doesn't really
> > make sense to say "we don't always have valid 'gtype' to do that" - then
> > immediately below use a function that goes from an arbitrary GIObjectInfo to
> > GType, as is with your patch.
> 
> The problem was that the function is being called from (at least) two contexts,
> in one the concrete GType is known and passed in 'gtype' argument, but in the
> other we don't know the GType ('gtype' is G_TYPE_INVALID) so couldn't walk the
> GType hierarchy either.
> 
> I forgot where that other call site was. Maybe the code has changed since and
> the problem no longer exists.

Yes, the code is called from two contexts:

 - Starting with a GObject we need to wrap (starts with GType)
 - Starting with a namespaced name (starts with a GIObjectInfo)

But in the second case, we should always be able to get to the GType using g_registered_type_info_get_g_type().

Created attachment 149471 [details] [review]
Patch for this bug

gjs_lookup_object_constructor() and gjs_lookup_object_class() removed.
In gjs_define_object_class, removed 'info' argument.
Now gjs_define_object_class create correct object's heirarchy.

Review of attachment 149471 [details] [review]:

Patch looks basically right to me, but if I'm understanding the flow correctly, there are some portions that can be cleaned up.

::: gi/object.c
@@ +1205,3 @@
+    GIBaseInfo *info = NULL;
+
+    for (; info == NULL; gtype = g_type_parent(gtype)) {

I think it's inherently confusing to have the info == NULL check here, but actually have the loop exit be done by the break below. It makes things more complicated than they need to be. This loop would be clearer if written:

 while (TRUE) {
   info = <loopup>
   if (info != NULL)
       break;
   <debug stuff>
   type = <parent>;
 }

@@ +1217,3 @@
+                  g_type_name(gtype), g_type_name(g_type_parent(gtype)));
+    }
+    g_assert(info);

Some people feel that any assertion that's correct is a good addition. I'm not of that opinion. I think assertions should generally be asserting something that is not immediatley obvious by inspection. Since the loop above clearly exits only when info is not NULL, I don't think an assertion is warrented.

@@ +1247,2 @@
+    if (!has_own_info && gtype == G_TYPE_OBJECT)
+        gjs_fatal("No introspection data on GObject - pretty much screwed");

This duplicates the check that is already in get_base_info() - is this meant to be

  if (has_own_info && gtype == G_TYPE_OBJECT) 

? If that is what you mean, it would be clearer as two branches of an if:

 if (info) {
 } else {
 }

Or consider always calling get_base_info() and returning the 'has_own_info' boolean from that as an extra out value.

@@ +1314,2 @@
      *
      * 'gtype' can be invalid when called from gjs_define_info()

This isn't true any more - we always pass in a gtype

@@ +1318,3 @@
+        constructor_name = g_type_name(gtype);
+    } else {
+        constructor_name = g_base_info_get_name( (GIBaseInfo*) info);

Extra space before '(GIBaseInfo)'

@@ +1351,3 @@
+    if (has_own_info) {
+        GIObjectInfo *parent_info;
+        parent_info = g_object_info_get_parent(info);

I don't quite understand why you have two branches here - it seems like we can always use g_type_parent(gtype), and using g_object_info_get_parent() isn't necessary.

@@ +1361,3 @@
+        }
+    } else {
+        if (g_type_parent(gtype) != G_TYPE_OBJECT) {

Why do you check for != G_TYPE_OBJECT? Don't we want GObject as the base class of our Javascript hierarchy? Presumably that's what happens in the other branch - that is, g_object_info_get_parent() returns the GIObjectInfo for GObject when relevant?

@@ +1368,3 @@
+                return NULL;
+            }
+            g_assert(g_base_info_equal(info, inf));

I don't see why you have this assertion - if the parent class isn't known to gobject-introspection, then this assertion will fail?

::: gi/object.h
@@ +33,3 @@
 G_BEGIN_DECLS
 
+GIObjectInfo* gjs_define_object_class       (JSContext     *context,

I don't like changing the return value from returning a true/false boolean to returning the GIOBjectInfo - it's not clear why the GIObjectInfo would be the most important or distinguished thing to return, rather than then constructor or prototype. So I would add the GIOObjectInfo as another out value like constructor_p or prototype_p and leave the return being a boolean.

::: gi/repo.c
@@ +434,3 @@
+        {
+            GType gtype;
+            GIBaseInfo *inf;            

On stylistic grounds, I would avoid having variables called 'inf' and 'info' being different things. I don't think you need inf here at all (if you take my suggestion of making the GIObjectInfo an out parameter, and also skip the assertion. This is an assertion that I *do* find useful since it's checking something that isn't obvious locally, but it's not worth an extra variable and unref.) But if you did want 'inf' here, I'd call it something descriptive like:

 GIBaseInfo *info_for_gtype;

Created attachment 151801 [details] [review]
Corrected patch

Review of attachment 151801 [details] [review]:

One small stylistic comment noted below.

For the commit message:

  gjs_define_object_class: generate correct object's heirarchy.

Would be better as:

  gjs_define_object_class(): generate correct object hierarchy

Then you need to describe the problem that was being fixed. If I understood the problem correctly and remember it now, it should be something like:

 When we are creating a new JS class for a GObject class and look up the JS class for the parent class, we  must make sure that we use a consistent GType and GITypeInfo. If the GITypeInfo and the GType are mismatched, then the parent class might be incorrectly registered. To fix this we remove the 'info' argument from gjs_define_object_class() and always just pass in a GType.

 The unused functions gjs_lookup_object_constructor() and gjs_lookup_object_class() are removed.

If you fix the problem below and use a commit message like that, this looks good to push. Thanks for working on this and coming up with a fix for this tricky section of code!

::: gi/object.c
@@ +1226,3 @@
+                        JSObject     **constructor_p,
+                        JSObject     **prototype_p,
+                        GIObjectInfo **class_info)

This should be class_info_p to match the other two out parameters. There's no consistent style here, but it needs to be consistent within the function.