GNOME Bugzilla – Bug 773763
Various problems handling GHashTables as function arguments
Last modified: 2018-01-27 12:02:26 UTC
A GHashTable with value-type double (or any other type that cannot be stuffed into a pointer) is not supported properly in GJS. As I discovered while investigating, the test suite was not running marshalling tests for GHashTables at all, and so it seems that some other cases do not work either. The first patch is to add the existing GHashTable tests from gimarshallingtests.c, commenting out the ones that don't work for now.
Created attachment 338866 [details] [review] marshalling tests: Add tests for GHashTable This adds the GHashTable marshalling tests currently available in gimarshallingtests.c. In accordance with the current state of what works and what doesn't, broken tests are commented out.
Created attachment 338875 [details] [review] marshalling tests: Add tests for GHashTable This adds the GHashTable marshalling tests currently available in gimarshallingtests.c. In accordance with the current state of what works and what doesn't, broken tests are commented out.
Created attachment 338876 [details] [review] arg: Use JS_Enumerate to find hash table props As mentioned in a Mozilla bug [1], JS_NewPropertyIterator does not work correctly on indexed properties. Indeed, during the tests that are currently commented out, the 0 value would cause the iterator to return JSID_VOID and stop. The recommended replacement is JS_Enumerate, which correctly includes indexed properties. Unfortunately this does not fix any of the broken tests by itself, but it is a prerequisite. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1081660
Created attachment 338877 [details] [review] arg: Treat keys as strings when converting to hash Depending on whether a property is indexed or not, the jsid denoting it may be an integer or a string. In gjs_object_to_g_hash(), we convert integer jsids to strings so that all hash table keys are the same type. I'm not entirely sure this is the correct approach, since the real error happens in gjs_value_to_g_argument() where it complains of expecting a string value and getting a number. gjs_value_to_g_argument() isn't quite consistent here, since it will convert values to numbers if expecting a number, but won't convert values to strings if expecting a string. However, this is the simplest solution that is unlikely to change the way that other arguments are marshalled.
Created attachment 338878 [details] [review] arg: Set hash functions according to GI type Previously it was assumed that all GHashTable keys were strings when constructing a hash table from a JS object. This causes a segfault when you use, for example, GINT_TO_POINTER(-1) as a key, since g_str_hash will try to dereference the pointer. Sadly this does not fix any tests by itself, but it is a prerequisite.
Review of attachment 338875 [details] [review]: Looks good to me.
Review of attachment 338876 [details] [review]: OK
Review of attachment 338877 [details] [review]: ::: gi/arg.cpp @@ +378,3 @@ + JS::RootedString str(context, JS_ValueToString(context, key_js)); + key_js.setString(str); + } Yeah, I would rather understand if gjs_value_to_g_argument() should convert those values to strings automatically when the expected type is a string, instead of forcing it here. Or alternatively, have a good explanation of why it should not do that. That does not mean this is definitely not the right fix, but perhaps it would be worth exploring that route some more. Is there another situation where gjs_value_to_g_argument() is called with an integer value and a string type_info and that conversion is supposed to error out?
Review of attachment 338878 [details] [review]: I am not sure I fully understand this one. The only thing that ever goes in the hash table is the v_pointer of the argument, and in the previous patch you make them all strings. Is this an alternative patch to the previous one?
(In reply to Cosimo Cecchi from comment #8) > Review of attachment 338877 [details] [review] [review]: > > ::: gi/arg.cpp > @@ +378,3 @@ > + JS::RootedString str(context, JS_ValueToString(context, > key_js)); > + key_js.setString(str); > + } > > Yeah, I would rather understand if gjs_value_to_g_argument() should convert > those values to strings automatically when the expected type is a string, > instead of forcing it here. > Or alternatively, have a good explanation of why it should not do that. > > That does not mean this is definitely not the right fix, but perhaps it > would be worth exploring that route some more. Is there another situation > where gjs_value_to_g_argument() is called with an integer value and a string > type_info and that conversion is supposed to error out? Yes, that happens any time you call a GObject introspected function that has a string-typed parameter, and you supply a number in JS. For example, l = Gtk.Label.new('123'); // works l = Gtk.Label.new(123); // Error: Expected type utf8 for Argument 'str' but got type 'number' This seems to me a conscious decision of how GJS's GObject introspection should work, though my opinion is that it would be more "Javascript-y" to accept the number in that case. That said, I think the above patch may become obsolete as I am actually planning to stop calling gjs_value_to_g_argument() for the hashtable keys in my next patch, and write a new function value_to_ghashtable_key() instead. (In reply to Cosimo Cecchi from comment #9) > Review of attachment 338878 [details] [review] [review]: > > I am not sure I fully understand this one. The only thing that ever goes in > the hash table is the v_pointer of the argument, and in the previous patch > you make them all strings. Is this an alternative patch to the previous one? The previous patch dealt with the type of the key on the Javascript side, this patch deals with the type on the C side. If the GHashTable is expected to have string keys, then we should have g_str_hash and g_str_equal as before, but if the key-type is not string, then it most likely has int keys stuffed into pointers. In that case we must have g_direct_hash and g_direct_equal, since the g_str_* functions will try to dereference the pointers, which will crash.
Review of attachment 338878 [details] [review]: Me and Philip chatted about this and I understand now that this is correct.
(In reply to Philip Chimento from comment #10) > Yes, that happens any time you call a GObject introspected function that has > a string-typed parameter, and you supply a number in JS. For example, > > l = Gtk.Label.new('123'); // works > l = Gtk.Label.new(123); // Error: Expected type utf8 for Argument 'str' > but got type 'number' > > This seems to me a conscious decision of how GJS's GObject introspection > should work, though my opinion is that it would be more "Javascript-y" to > accept the number in that case. I see, that makes sense, and I think we should preserve the current behavior here. Looking forward to your next patch.
Comment on attachment 338878 [details] [review] arg: Set hash functions according to GI type Attachment 338878 [details] pushed as de5dd65 - arg: Set hash functions according to GI type
Created attachment 338921 [details] [review] arg: Don't convert GHashTable keys via GIArgument This caused problems with sign extension. An example would be a hash table with key-type int on the GObject introspection side, and inserting the key "-1" into it. The key that actually gets passed to g_hash_table_insert() is 0xffffffff, whereas when you try to look up the key on the C side, GINT_TO_POINTER(-1) is 0xffffffffffffffff, so the lookup fails. Moreover, JavaScript only has string properties on its objects (which can be represented as int32 or string JS::Values internally) so we can cut out a lot of code to deal with other value types, as well as issue a clear message that we don't support certain key-types for hashtables in GObject introspection. (We could move to using Map objects for the unsupported types in the future.)
Review of attachment 338921 [details] [review]: Nice, looks good to me.
Comment on attachment 338921 [details] [review] arg: Don't convert GHashTable keys via GIArgument Attachment 338921 [details] pushed as f57da29 - arg: Don't convert GHashTable keys via GIArgument
Created attachment 338994 [details] [review] arg: Allocate hash values that don't fit in pointers For GHashTables with value types that can't be stuffed into pointers, we use heap-allocated values instead. Since the types we are concerned with here are all primitive types, they are always copied, so the hash table can always own the values, and they can be freed unconditionally when the hash table GIArgument is released.
Created attachment 338995 [details] [review] tests: Test hashtables with allocated values In order to test GJS's support for GHashTables with float, double, gint64, and guint64 value types, here are some new GI marshalling tests.
Created attachment 338999 [details] [review] arg: Update rooting to match mozjs31 I forgot one extra root in the earlier commit.
Created attachment 339000 [details] [review] arg: Update rooting to match mozjs31 I forgot one extra root in the earlier commit.
Review of attachment 338994 [details] [review]: Looks good to me.
Review of attachment 338995 [details] [review]: Looks good to me.
Review of attachment 339000 [details] [review]: Looks good.
Comment on attachment 338995 [details] [review] tests: Test hashtables with allocated values Attachment 338995 [details] pushed as 88b42e5 - tests: Test hashtables with allocated values
Attachment 338994 [details] pushed as 28bbd24 - arg: Allocate hash values that don't fit in pointers Attachment 339000 [details] pushed as c82cf4d - arg: Update rooting to match mozjs31
I'll leave this bug open because there's still one test that's broken. It crashes with a double-free (sometimes.) I'm a bit at a loss there and I think it may be because of how GJS interprets in-out parameters which is currently not clear whether it's as intended.
Created attachment 339415 [details] [review] maint: Use NULL instead of g_direct_equal() According to the documentation, passing NULL as the hash table equal function has the same effect as g_direct_equal() only without the overhead of a function call.
Emmanuele had an extra comment on this patch set, hence the above patch
Review of attachment 339415 [details] [review]: Feel free to push with this change ::: gi/arg.cpp @@ +339,3 @@ if (key_type == GI_TYPE_TAG_UTF8 || key_type == GI_TYPE_TAG_FILENAME) return g_hash_table_new(g_str_hash, g_str_equal); + return g_hash_table_new(g_direct_hash, NULL); You can actually use NULL for both functions, since from the GHashTable docs: "If hash_func is NULL, g_direct_hash() is used."
Comment on attachment 339415 [details] [review] maint: Use NULL instead of g_direct_equal() Attachment 339415 [details] pushed as b1118b3 - maint: Use NULL instead of g_direct_equal()
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gjs/issues/100.