After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 788113 - Remove custom Javascript bindings
Remove custom Javascript bindings
Status: RESOLVED FIXED
Product: geary
Classification: Other
Component: build
master
Other Linux
: Normal minor
: 0.13.0
Assigned To: Geary Maintainers
Geary Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-09-25 02:59 UTC by Michael Gratton
Modified: 2018-10-07 03:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Don't ref string arg for JavaScriptCore-4.0 JS.String.get_utf8_cstring (1.04 KB, patch)
2018-05-20 09:41 UTC, Michael Gratton
none Details | Review

Description Michael Gratton 2017-09-25 02:59:17 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.
Comment 1 Michael Gratton 2017-09-25 03:00:46 UTC
May be able to revert patch for Bug 783882 at the same time.
Comment 2 Michael Gratton 2018-05-19 00:17:00 UTC
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.
Comment 3 Michael Gratton 2018-05-19 04:07:25 UTC
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
  • #0 __strlen_avx2
    at ../sysdeps/x86_64/multiarch/strlen-avx2.S line 93
  • #1 g_strdup
    at ../../../../glib/gstrfuncs.c line 362
  • #2 geary_js_to_string_released
    at /home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala line 108
  • #3 geary_js_to_string
    at /home/mjg/Projects/GNOME/geary/src/engine/util/util-js.vala line 75
  • #4 web_kit_util_to_string
    at /home/mjg/Projects/GNOME/geary/src/client/util/util-webkit.vala line 49
  • #5 composer_web_view_on_cursor_context_changed
    at /home/mjg/Projects/GNOME/geary/src/client/composer/composer-web-view.vala line 538
  • #6 _composer_web_view_on_cursor_context_changed_client_web_view_java_script_message_handler
    at /home/mjg/Projects/GNOME/geary/src/client/composer/composer-web-view.vala line 135
  • #7 __lambda14_
    at /home/mjg/Projects/GNOME/geary/src/client/components/client-web-view.vala line 437
  • #8 ___lambda14__webkit_user_content_manager_script_message_received
    at /home/mjg/Projects/GNOME/geary/src/client/components/client-web-view.vala line 436
  • #12 <emit signal script-message-received:cursorContextChanged on instance 0x5555561b8730 [WebKitUserContentManager]>
    at ../../../../gobject/gsignal.c line 3447
  • #13 0x00007ffff350df29 in
  • #14 0x00007ffff330e84b in
  • #15 0x00007ffff3462ba6 in
  • #16 0x00007ffff31bcbae in
  • #17 0x00007ffff32a0e92 in
  • #18 0x00007ffff31b877b in
  • #19 0x00007ffff31b9065 in
  • #20 WTF::RunLoop::performWork()
  • #21 0x00007ffff2964f49 in
  • #22 g_main_dispatch
    at ../../../../glib/gmain.c line 3177
  • #23 g_main_context_dispatch
    at ../../../../glib/gmain.c line 3830
  • #24 g_main_context_iterate
    at ../../../../glib/gmain.c line 3903
  • #25 g_main_context_iteration
    at ../../../../glib/gmain.c line 3964
  • #26 gtk_main_iteration
    at ../../../../gtk/gtkmain.c line 1427
  • #27 composer_web_view_test_real_load_body_fixture
    at /home/mjg/Projects/GNOME/geary/test/client/composer/composer-web-view-test.vala line 181
  • #28 client_web_view_test_case_load_body_fixture
    at /home/mjg/Projects/GNOME/geary/test/client/components/client-web-view-test-case.vala line 38
  • #29 composer_web_view_test_get_html
    at /home/mjg/Projects/GNOME/geary/test/client/composer/composer-web-view-test.vala line 47
  • #30 _composer_web_view_test_get_html_test_case_test_method
    at /home/mjg/Projects/GNOME/geary/test/client/composer/composer-web-view-test.vala line 16
  • #31 test_case_adaptor_run
    at /home/mjg/Projects/GNOME/geary/test/test-case.vala line 294
  • #32 _test_case_adaptor_run_gtest_fixture_func
    at /home/mjg/Projects/GNOME/geary/test/test-case.vala line 189

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?
Comment 4 Michael Gratton 2018-05-19 04:18:48 UTC
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).
Comment 5 Michael Gratton 2018-05-19 04:19:51 UTC
"Note the third arg of JSStringGetUTF8CString is taking the address"

Scheesh.
Comment 6 Rico Tzschichholz 2018-05-20 07:02:35 UTC
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.
Comment 7 Michael Gratton 2018-05-20 09:14:10 UTC
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.
Comment 8 Michael Gratton 2018-05-20 09:41:41 UTC
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?
Comment 9 Rico Tzschichholz 2018-05-20 11:01:04 UTC
(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.
Comment 10 Rico Tzschichholz 2018-05-20 11:02:10 UTC
(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.
Comment 11 Michael Gratton 2018-05-22 12:50:19 UTC
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?).
Comment 12 Michael Gratton 2018-10-07 03:49:42 UTC
This has landed. Cheers!