bleakgrey / tootle

GTK-based Mastodon client for Linux
GNU General Public License v3.0
401 stars 61 forks source link

race condition crash clicking toot then "back" before it loads #16

Closed martensitingale closed 6 years ago

martensitingale commented 6 years ago

I get a variety of different crashes when clicking a toot and then pressing "back" before the toot has loaded. It seems the crashes occur when the toot does finish downloading.

Thread 1 "com.github.blea" received signal SIGSEGV, Segmentation fault.
0x00005555555750c1 in tootle_status_widget_rebind (self=0x555556bba400) at tootle/src/Widgets/StatusWidget.vala:196
196         title_user.label = "<b>%s</b>".printf (formal.account.display_name);
(gdb)  bt
#0  0x00005555555750c1 in tootle_status_widget_rebind (self=0x555556bba400) at tootle/src/Widgets/StatusWidget.vala:196
#1  0x0000555555574f5a in tootle_status_widget_construct (object_type=93825002901856, status=0x555556dc0770) at tootle/src/Widgets/StatusWidget.vala:185
#2  0x0000555555574f86 in tootle_status_widget_new (status=0x555556dc0770) at tootle/src/Widgets/StatusWidget.vala:112
#3  0x0000555555584a2a in tootle_status_view_prepend (self=0x555556dc0a80, status=0x555556dc0770, is_root=1) at tootle/src/Views/StatusView.vala:17
#4  0x000055555558510e in __lambda59_ (self=0x555556dc0a80, sess=0x5555558aa100, mess=0x555556cade60) at tootle/src/Views/StatusView.vala:49
#5  0x000055555558530d in ___lambda59__soup_session_callback (session=0x5555558aa100, msg=0x555556cade60, self=0x555556dc0a80) at tootle/src/Views/StatusView.vala:36
#6  0x00005555555625a1 in __lambda9_ (_data1_=0x5555566f3c40, sess=0x5555558aa100, mess=0x555556cade60) at tootle/src/NetManager.vala:112
#7  0x000055555556265e in ___lambda9__soup_session_callback (session=0x5555558aa100, msg=0x555556cade60, self=0x5555566f3c40) at tootle/src/NetManager.vala:94
#8  0x00007ffff5f7eeaf in  () at /usr/lib/libsoup-2.4.so.1
#9  0x00007ffff5f7f8ea in  () at /usr/lib/libsoup-2.4.so.1
#10 0x00007ffff5f7f987 in  () at /usr/lib/libsoup-2.4.so.1
#11 0x00007ffff66f11d6 in g_main_context_dispatch () at /usr/lib/libglib-2.0.so.0
#12 0x00007ffff66f15b1 in  () at /usr/lib/libglib-2.0.so.0
#13 0x00007ffff66f163e in g_main_context_iteration () at /usr/lib/libglib-2.0.so.0
#14 0x00007ffff6cb297e in g_application_run () at /usr/lib/libgio-2.0.so.0
#15 0x000055555555d13f in tootle_application_main (args=0x7fffffffde98, args_length1=1) at tootle/src/Application.vala
martensitingale commented 6 years ago

I believe this is caused by the callback object getting freed too early; could someone verify that the following patch fixes the problem?

diff --git a/src/NetManager.vala b/src/NetManager.vala
index 23ed95b..fc049cd 100644 
--- a/src/NetManager.vala
+++ b/src/NetManager.vala
@@ -83,7 +84,7 @@ public class Tootle.NetManager : GLib.Object {                  
         return yield session.websocket_connect_async (msg, null, null, null);    
     }    

-    public Soup.Message queue (Soup.Message msg, Soup.SessionCallback? cb = null) {          
+    public Soup.Message queue (Soup.Message msg, owned Soup.SessionCallback? cb = null) {          
         requests_processing++;
         started ();
bleakgrey commented 6 years ago

I'll give it a test in a moment

bleakgrey commented 6 years ago

I can't believe it works

martensitingale commented 6 years ago

For what it's worth, my debugging workflow here was actually prompted by working on the image cache (which was showing similar crashes). In GDB, the callback data object (which was lowered to a C variable called "data2" would show garbage values for its refcounts and other fields when I inspected it at the point of the crash. I noticed that in the generated C code in the build dir, there was a patttern of:

ValaGeneratedCallbackData43* _tmp0_ = g_slice_new0(sizeof(ValaGeneratedCallbackData43));
_tmp0_->self=foo;
_tmp0_->bar=baz;
etc.
my_async_function_start(a, b, _tmp0_);
unref(_tmp0_);

which results in tmp0 being freed when the async function completes. Changing the async functions to accept owned callbacks, on the other hand, causes Vala code generation to insert an additional reference-count increment before passing to the async function, so the callback data lives until the async function completes.

bleakgrey commented 6 years ago

Either way this one "owned" just owned my forever uncommitted system for checking whether a pending request is still relevant and needs to be handled

Well played, sir