GNOME Bugzilla – Bug 788113
Remove custom Javascript bindings
Last modified: 2018-10-07 03:49:42 UTC
Vala 0.38 includes bindings based on the custom version in Geary's tree at the moment. Once we can depend on vala 0.38 as a minimum, we should remove the custom bindings.
May be able to revert patch for Bug 783882 at the same time.
Fix pushed to master as commit 43341cd3. ricoz, I think this means you can re-enable Geary in the vala CI. Thanks for the patch.
I actually reverted merging the branch because it's causing a crash in Geary.JS.to_string_released() when running client tests. In that method, the call to JS.String.get_utf8_cstring() (JSStringGetUTF8CString in the WK API) is failing, the ref'ed array that is being passed in is getting mangled, then the call to dup' it on return is segfaulting: Thread 1 "geary-test-clie" received signal SIGSEGV, Segmentation fault. __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:93 93 ../sysdeps/x86_64/multiarch/strlen-avx2.S: No such file or directory. (gdb) bt
+ Trace 238621
ricoz, I eyeballed the binding for JSStringGetUTF8CString and it seems somewhat okay, but the vala code: public inline string to_string_released(global::JS.String js) { int len = js.get_maximum_utf8_cstring_size(); string str = string.nfill(len, 0); js.get_utf8_cstring(str, len); js.release(); return str; } Produces generated code that looks like this: inline gchar* geary_js_to_string_released (struct OpaqueJSString* js) { gchar* result = NULL; gsize len = 0UL; guint8* str = NULL; guint8* _tmp0_; gint str_length1; gint _str_size_; guint8* _tmp1_; gint _tmp1__length1; gchar* _tmp2_; #line 104 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" g_return_val_if_fail (js != NULL, NULL); #line 105 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" len = JSStringGetMaximumUTF8CStringSize (js); #line 106 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" _tmp0_ = g_new0 (guint8, len); #line 106 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" str = _tmp0_; #line 106 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" str_length1 = len; #line 106 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" _str_size_ = str_length1; #line 107 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" JSStringGetUTF8CString (js, &str, (gsize) (&str_length1)); #line 108 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" _tmp1_ = str; #line 108 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" _tmp1__length1 = str_length1; #line 108 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" _tmp2_ = g_strdup ((const gchar*) _tmp1_); #line 108 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" result = _tmp2_; #line 108 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" str = (g_free (str), NULL); #line 108 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" return result; #line 521 "util-js.c" } The last arg in the call to JSStringGetUTF8CString() above seems bogus?
Err, whoops, actually from your branch the vala function looks like: 104 public inline string to_string_released(owned global::JS.String js) { 105 size_t len = js.get_maximum_utf8_cstring_size(); 106 uint8[] str = new uint8[len]; 107 js.get_utf8_cstring(ref str); 108 return (string) str; 109 } And the generated source looks like: inline gchar* geary_js_to_string_released (struct OpaqueJSString* js) { gchar* result = NULL; gsize len = 0UL; guint8* str = NULL; guint8* _tmp0_; gint str_length1; gint _str_size_; guint8* _tmp1_; gint _tmp1__length1; gchar* _tmp2_; #line 104 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" g_return_val_if_fail (js != NULL, NULL); #line 105 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" len = JSStringGetMaximumUTF8CStringSize (js); #line 106 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" _tmp0_ = g_new0 (guint8, len); #line 106 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" str = _tmp0_; #line 106 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" str_length1 = len; #line 106 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" _str_size_ = str_length1; #line 107 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" JSStringGetUTF8CString (js, &str, (gsize) (&str_length1)); #line 108 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" _tmp1_ = str; #line 108 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" _tmp1__length1 = str_length1; #line 108 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" _tmp2_ = g_strdup ((const gchar*) _tmp1_); #line 108 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" result = _tmp2_; #line 108 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" str = (g_free (str), NULL); #line 108 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" _JSStringRelease0 (js); #line 108 "/home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala" return result; #line 528 "util-js.c" } Note the third arg is taking the JSStringGetUTF8CString tags the address of the var, not the value itself, and the cast (gsize) should actually be (size_t).
"Note the third arg of JSStringGetUTF8CString is taking the address" Scheesh.
I see, the buffer parameter of String.get_utf8_cstring() should *not* be ref. so the binding is wrong here :( Regarding the casting problem this seems to be a different bug in vala itself.
Yeah, after copying in javascriptcoregtk-4.0.vapi to the source tree and removing the ref, the client unit tests all pass fine. I just pushed the branch wip/vala-js-bindings with this workaround, but there's not much to see there aside from removing the ref and the array_length_type attribute though. Any chance of getting that fix into the vala tree? I'll probably run with workaround for a while, until that becomes available.
Created attachment 372231 [details] [review] Don't ref string arg for JavaScriptCore-4.0 JS.String.get_utf8_cstring ricotz: This patch is against vala 0.40 for the metadata file, not the VAPI. I assume the latter gets generated from the former?
(In reply to Michael Gratton from comment #8) > Created attachment 372231 [details] [review] [review] > Don't ref string arg for JavaScriptCore-4.0 JS.String.get_utf8_cstring > > ricotz: This patch is against vala 0.40 for the metadata file, not the VAPI. > I assume the latter gets generated from the former? I have made the change locally and will it push it soon. Dropping the array_length_type attribute is not wanted.
(In reply to Michael Gratton from comment #7) > I just pushed the branch wip/vala-js-bindings with this workaround, but > there's not much to see there aside from removing the ref and the > array_length_type attribute though. Please don't push this and carry a copy which defeats the purpose of this bug. > Any chance of getting that fix into the vala tree? I'll probably run with > workaround for a while, until that becomes available. Will do asap.
Okay, just pushed an update to that branch that uses a private extern for JSStringGetUTF8CString as a workaround. Are you happier with that? I haven't tested it against WebKitGTK 2.21 yet though since the flatpak build against org.gnome.Platform/x86_64/master from gnome-nightly is failing for other reasons (there hasn't been a change to the way internal VAPI are generated lately, has there?).
This has landed. Cheers!