GNOME Bugzilla – Bug 734716
bus: signal watched added in a new thread-default context can't be removed
Last modified: 2014-08-13 15:46:49 UTC
The watch is attached to the current thread-default main context but gst_bus_remove_signal_watch() invokes g_source_remove() which operates on the default main context. So in that configuration the source won't be removed and GLib emits a critical warning. I think the solution would be to get the thread-default context in _remove_signal_watch(), look for the source in that context and destroy it. I'll post a patch :)
(In reply to comment #0) > I think the solution would be to get the thread-default context in > _remove_signal_watch(), look for the source in that context and destroy it. > I'll post a patch :) No, you'll have to remember the main context that was the default when the watch was added :) To get around this I just called g_source_destroy() in the past.
(In reply to comment #1) > (In reply to comment #0) > > > I think the solution would be to get the thread-default context in > > _remove_signal_watch(), look for the source in that context and destroy it. > > I'll post a patch :) > > No, you'll have to remember the main context that was the default when the > watch was added :) Oh right, in my code I remove the signal watch before popping the thread-default context but we can't assume everyone would do that :) > To get around this I just called g_source_destroy() in the > past. Alright, do you know why that code was changed?
Created attachment 283280 [details] [review] Patch
Review of attachment 283280 [details] [review]: ::: gst/gstbus.c @@ +1317,3 @@ gst_bus_remove_signal_watch (GstBus * bus) { + GSource *source = 0; Use NULL instead of 0 @@ +1333,3 @@ + GST_DEBUG_OBJECT (bus, "removing signal watch %u", + bus->priv->signal_watch_id); %p @@ +1337,2 @@ + bus->priv->signal_watch_id = 0; + source = bus->priv->watch_id; And change the priv->watch_id guint to a GSource * and make sure to actually store the GSource there instead of the return value of attach(). And also unref the source after you're done with it, not just destroy it.
Created attachment 283291 [details] [review] bus: destroy signal watch from the context it was mapped to Don't rely on g_source_remove() because it operates on the main context. If a signal watch was added to a new thread-default context g_source_remove() would have no effect. So simply use g_source_destroy() to avoid this problem. Additionally the source_id was removed from GstBusPrivate because it was redundant with the signal watch GSource also stored in that structure.
Comment on attachment 283291 [details] [review] bus: destroy signal watch from the context it was mapped to valgrind is not happy in the unit test: Running suite(s): GstBus ==16859== Invalid read of size 8 ==16859== at 0x5A4E835: g_source_unref (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0) ==16859== by 0x403635: test_watch_with_poll (gstbus.c:397) ==16859== by 0x4E40D52: srunner_run_all (check_run.c:372) ==16859== by 0x4E3AF60: gst_check_run_suite (gstcheck.c:817) ==16859== by 0x402E78: main (gstbus.c:729) ==16859== Address 0x6cd19e0 is 32 bytes inside a block of size 104 free'd ==16859== at 0x4C29730: free (vg_replace_malloc.c:468) ==16859== by 0x5A4D0A7: g_source_unref_internal (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0) ==16859== by 0x5A4D458: g_source_destroy_internal (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0) ==16859== by 0x52E85B4: gst_bus_remove_signal_watch (gstbus.c:1341) ==16859== by 0x403635: test_watch_with_poll (gstbus.c:397) ==16859== by 0x4E40D52: srunner_run_all (check_run.c:372) ==16859== by 0x4E3AF60: gst_check_run_suite (gstcheck.c:817) ==16859== by 0x402E78: main (gstbus.c:729) ==16859== ==16859== Invalid read of size 4 ==16859== at 0x5A4D0E8: g_source_unref_internal (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0) ==16859== by 0x403635: test_watch_with_poll (gstbus.c:397) ==16859== by 0x4E40D52: srunner_run_all (check_run.c:372) ==16859== by 0x4E3AF60: gst_check_run_suite (gstcheck.c:817) ==16859== by 0x402E78: main (gstbus.c:729) ==16859== Address 0x6cd19d8 is 24 bytes inside a block of size 104 free'd ==16859== at 0x4C29730: free (vg_replace_malloc.c:468) ==16859== by 0x5A4D0A7: g_source_unref_internal (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0) ==16859== by 0x5A4D458: g_source_destroy_internal (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4000.0) ==16859== by 0x52E85B4: gst_bus_remove_signal_watch (gstbus.c:1341) ==16859== by 0x403635: test_watch_with_poll (gstbus.c:397) ==16859== by 0x4E40D52: srunner_run_all (check_run.c:372) ==16859== by 0x4E3AF60: gst_check_run_suite (gstcheck.c:817) ==16859== by 0x402E78: main (gstbus.c:729) ==16859==
Ah it's due to the _unref after _destroy. Will update the patch!
Created attachment 283296 [details] [review] bus: destroy signal watch from the context it was mapped to Don't rely on g_source_remove() because it operates on the main context. If a signal watch was added to a new thread-default context g_source_remove() would have no effect. So simply use g_source_destroy() to avoid this problem. Additionally the source_id was removed from GstBusPrivate because it was redundant with the signal watch GSource also stored in that structure.
commit b1f23598099da82835df60bc2fb09501233f9118 Author: Philippe Normand <philn@igalia.com> Date: Wed Aug 13 14:12:00 2014 +0200 bus: destroy signal watch from the context it was mapped to Don't rely on g_source_remove() because it operates on the main context. If a signal watch was added to a new thread-default context g_source_remove() would have no effect. So simply use g_source_destroy() to avoid this problem. Additionally the source_id was removed from GstBusPrivate because it was redundant with the signal watch GSource also stored in that structure. https://bugzilla.gnome.org/show_bug.cgi?id=734716