GNOME Bugzilla – Bug 773789
rhythmbox crashed with SIGSEGV in grl_util_fetch_done ( grl-lua-library.c )
Last modified: 2018-09-24 09:50:35 UTC
I don't have the steps to reproduce this issue. Hence, I am pasting a complete backtrace to see if that might help. (gdb) backtrace full
+ Trace 236800
root@unstable:~# dpkg -l | grep -i grilo | awk '{ print $2, $3 }' gir1.2-grilo-0.3:amd64 0.3.2-2 grilo-plugins-0.3:amd64 0.3.3-1 grilo-plugins-0.3-dbgsym:amd64 0.3.3-1 libgrilo-0.3-0:amd64 0.3.2-2 libgrilo-0.3-0-dbgsym:amd64 0.3.2-2 libgrilo-0.3-bin 0.3.2-2 libgrilo-0.3-bin-dbgsym 0.3.2-2 libgrilo-0.3-dev:amd64 0.3.2-2 libgrilo-0.3-doc 0.3.2-2 root@unstable:~# uname -a Linux unstable 4.7.0-1-amd64 #1 SMP Debian 4.7.8-1 (2016-10-19) x86_64 GNU/Linux
Got another crash. I've saved the core file + app for information if needed.
+ Trace 236899
I think I understood the issue. When the source fetches multiple urls at once but eventually get cancelled: 1) first time on grl_util_fetch_done(), release each fo->results[i] and then release fo->results 2) second time on grl_util_fetch_done(), try to free fo->results[i] and crash as result was freed before Cooking a patch.
Created attachment 340996 [details] [review] lua-factory: Improve error handling in fetch callback When we did not fetch all requested urls, we want to go to free_fetch_op in order to clean FetchOperation resources; After successfully providing the fetched data to the Lua Source or in case of G_IO_ERROR_CANCELLED we want to terminate the FetchOperation with all shared resources. This also fix a leak of GCancellable that was not being properly unref'ed
Created attachment 340997 [details] [review] lua-factory: Fix double free on grl_util_fetch_done In case we are canceling a fetch operation with multiple urls, we should avoid freeing fo->results and its elements more then once.
Easy to reproduce with grilo-test-ui-0.3, the trick is being fast to trigger the cancelation: 1-) launch grilo-test-ui-0.3 2-) click once to select Radio France 3-) put the mouse in the Back button 4-) Press Enter to enter in Radio France 5-) Click in the back button as fast as you can. It will trigger the cancelation of multiple urls fetch, causing the crash.
Created attachment 341006 [details] [review] lua-factory: Fix possible crash when fetch ops are cancelled When an operation was cancelled, we would go to free our operation data straight away, including struct members shared with other operations. As those could be on-going, and we know that the operations data members that are private to our operation are valid, go check whether we are the last operation, and only free the shared resources in that case.
Created attachment 341007 [details] [review] lua-factory: Fix memory leak The cancellable referenced in each fetch operation's data was not unreferenced except the one in the last fetch in the sequence.
I prefer my patches. The only thing I'm unsure is whether we should touch the lua _State when we're cancelled, as we don't hold a reference to it, and the lua-factory might already have been destroyed already. Or maybe it's still alive because it's through its userdata cleanup that we're getting cancelled. Ideas, Victor?
(In reply to Bastien Nocera from comment #9) > I prefer my patches. :) I'm fine with either but I would have appreciated a review on my patch if you saw any issue there so I can make it better next time. > The only thing I'm unsure is whether we should touch the lua_State when > we're cancelled, as we don't hold a reference to it, and the lua-factory > might already have been destroyed already. > > Or maybe it's still alive because it's through its userdata cleanup that > we're getting cancelled. No, I think you are right and it could happen that grl_util_fetch_done() being called when LuaFactory plugin has been destroyed. > > Ideas, Victor? No sure. We might need to start using GWeakNotify to verify this kind of situation and perhaps a helper functions in grl-lua-library-operations to check if operation is still valid. Although I would say that GCancellable should used in Grilo itself so plugins don't need to workaround it. Cheers PS: Ack both your patches
Don't see the crash anymore with attachment 341006 [details] [review] and attachment 341007 [details] [review]. But, seeing a lot of assertion errors: (rhythmbox:19687): Grilo-CRITICAL **: grl_source_get_id: assertion 'GRL_IS_SOURCE (source)' failed (rhythmbox:19687): Grilo-DEBUG: [lua-library-operations] grl-lua-library-operations.c:899: grl_lua_operations_set_source_state | (null) (op-id: 0) state: finalized (rhythmbox:19687): Grilo-CRITICAL **: grl_source_get_id: assertion 'GRL_IS_SOURCE (source)' failed (rhythmbox:19687): Grilo-DEBUG: [lua-library-operations] grl-lua-library-operations.c:588: watchdog_operation_gc | (null) (op-id: 0) current state is: finalized (num-async-op: 32610) (rhythmbox:19687): Grilo-CRITICAL **: grl_source_get_id: assertion 'GRL_IS_SOURCE (source)' failed (rhythmbox:19687): Grilo-WARNING **: [lua-library-operations] grl-lua-library-operations.c:614: Source '(null)' is broken, as the finishing callback was called while 32610 operations are still ongoing
Created attachment 342246 [details] Grilo debug logs with assertion failures Assertion failures don't happen every time though. Seems to be dependent on the timing of the cancellations.
Got a randomly reproducible crash with [ 341006, 341007 ] (gdb) bt
+ Trace 237005
Assertion failures in different places: (rhythmbox:12003): libsoup-CRITICAL **: async_send_request_return_result: assertion 'item->task != NULL' failed (rhythmbox:12003): Grilo-DEBUG: [lua-library] grl-lua-library.c:486: fetch operation was cancelled (rhythmbox:12003): libsoup-CRITICAL **: async_send_request_return_result: assertion 'item->task != NULL' failed (rhythmbox:12003): Grilo-DEBUG: [lua-library] grl-lua-library.c:486: fetch operation was cancelled (rhythmbox:12003): libsoup-CRITICAL **: async_send_request_return_result: assertion 'item->task != NULL' failed
Assertion failures in different places: (rhythmbox:12382): Grilo-CRITICAL **: grl_source_get_id: assertion 'GRL_IS_SOURCE (source)' failed (rhythmbox:12382): Grilo-DEBUG: [lua-library-operations] grl-lua-library-operations.c:852: grl_lua_operations_pcall | (null) (op-id: 0) (rhythmbox:12382): Grilo-CRITICAL **: grl_source_get_id: assertion 'GRL_IS_SOURCE (source)' failed (rhythmbox:12382): Grilo-DEBUG: [lua-library-operations] grl-lua-library-operations.c:899: grl_lua_operations_set_source_state | (null) (op-id: 0) state: running (rhythmbox:12382): Grilo-CRITICAL **: grl_source_get_id: assertion 'GRL_IS_SOURCE (source)' failed (rhythmbox:12382): Grilo-DEBUG: [lua-library-operations] grl-lua-library-operations.c:277: priv_state_operations_create_source_state | (null) (op-id: 0)
Based on comment #11 and comment #12, these patches improves the issue. Attachment 341006 [details] pushed as a051aa0 - lua-factory: Fix possible crash when fetch ops are cancelled Attachment 341007 [details] pushed as 98ba622 - lua-factory: Fix memory leak Keeping the bug open to address the criticals afterwards
-- 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/grilo/issues/105.