GNOME Bugzilla – Bug 778383
repeatedly turning BT off and off crashes gnome
Last modified: 2017-02-14 18:07:12 UTC
A bit of background (not sure if important): in our setup RFKill is driving a GPIO to enable / disable the BT transceiver. This is a slow operation and it could take ~500ms for the RFKill subsystem to complete the operation. In the Bluetooth configuration panel if I click on the Bluetooth on/off toggle switch 5+ times in a row repeatedly, gnome (or BT configuration panel) crashes. This is what I have in the journal: gnome-settings-[886]: g_simple_async_result_take_error: assertion 'G_IS_SIMPLE_ASYNC_RESULT (simple)' failed gnome-settings-[886]: g_simple_async_result_complete_in_idle: assertion 'G_IS_SIMPLE_ASYNC_RESULT (simple)' failed gnome-settings-[886]: g_simple_async_result_take_error: assertion 'G_IS_SIMPLE_ASYNC_RESULT (simple)' failed gnome-settings-[886]: g_simple_async_result_complete_in_idle: assertion 'G_IS_SIMPLE_ASYNC_RESULT (simple)' failed gnome-settings-[886]: g_simple_async_result_take_error: assertion 'G_IS_SIMPLE_ASYNC_RESULT (simple)' failed gnome-settings-[886]: g_simple_async_result_complete_in_idle: assertion 'G_IS_SIMPLE_ASYNC_RESULT (simple)' failed kernel: BT_RADIO going: on kernel: BCM_BT: going ON gnome-settings-[886]: g_simple_async_result_take_error: assertion 'G_IS_SIMPLE_ASYNC_RESULT (simple)' failed gnome-settings-[886]: g_simple_async_result_complete_in_idle: assertion 'G_IS_SIMPLE_ASYNC_RESULT (simple)' failed gnome-settings-[886]: Failed to set RFKill: Stream has outstanding operation gnome-settings-daemon.desktop[886]: ** gnome-settings-daemon.desktop[886]: rfkill-plugin:ERROR:rfkill-glib.c:168:write_change_all_timeout_cb: assertion failed: (rfkill->priv->event) Trace:
+ Trace 237125
At least for `assertion 'G_IS_SIMPLE_ASYNC_RESULT (simple)' failed` what I think it's happening here is that cc_rfkill_glib_send_change_all_event() is rapidly called and several request for the asynchronous writing are queued on several GSimpleAsyncResult objects. When the first writing is done in write_change_all_done_cb() the latest rfkill->priv->simple object is freed. All the subsequent write_change_all_done_cb() callbacks find a NULL rfkill->priv->simple.
Created attachment 345322 [details] [review] rfkill: Fix indentation in D-Bus introspection XML This is a formatting fix only; no functional changes.
Created attachment 345323 [details] [review] build: Stop using deprecated macros in configure.ac Results of using autoupdate to move away from deprecated macros in configure.ac.
Created attachment 345324 [details] [review] build: Fix --enable-rfkill configure switch There was an additional comma in the AC_ARG_ENABLE call, which meant that no action-if-given was provided, meaning that enable_rfkill would be enabled by default if the rfkill switch was not passed — but if it was passed, enable_rfkill would not be set at all and the plugin would always be disabled.
Created attachment 345325 [details] [review] build: Drop dependency on gnome-common Move the autogen.sh script to be self-contained, so we drop our dependency on gnome-common. This does not (yet) port us to use any of the shiny new autoconf-archive macros, but since we invent our own (like EXTRA_COMPILE_WARNINGS) rather than using the gnome-common ones, the dependency is gone. See https://wiki.gnome.org/Projects/GnomeCommon/Migration.
Created attachment 345326 [details] [review] build: Fix shadowing of libexec_PROGRAMS in power plugin Makefile.am This was causing the gsd-power plugin to not be built if HAVE_GUDEV was defined (and gsd-backlight-helper was being built). Fixes the corresponding automake warning.
Created attachment 345327 [details] [review] build: Drop unused variables from smartcard Makefile.am Since commit 6d01a434ca2faa88acb2aed1224d23cea329f1f8, libsmartcard.la is no more, so all the variables which defined how to compile it are redundant. plugin_name is also unused. This fixes an automake warning about unused variables.
Created attachment 345328 [details] [review] rfkill: Port from GSimpleAsyncResult to GTask This introduces no functional changes: it’s a like-for-like port from GSimpleAsyncResult (deprecated) to GTask (not deprecated). This bumps the GLib dependency to 2.44 due to the use of g_autoptr() to simplify memory management. GTask itself was introduced in 2.36, which we already depended on.
Created attachment 345329 [details] [review] rfkill: Drop cc_rfkill_glib_send_event() due to being unused No need to complicate the code with it.
Created attachment 345330 [details] [review] rfkill: Update class definition to use G_DECLARE_DERIVABLE_TYPE Tidy up the code a bit. Introduces no functional changes.
Created attachment 345331 [details] [review] rfkill: Add some checks that a previous task is not overwritten These should not fail, as write_change_all_timeout_cb() clears those variables — so they’re more for documentation than anything else.
Created attachment 345332 [details] [review] rfkill: Remove stored GCancellable member There’s no need to store it separately from the GTask, as the GTask keeps a pointer to it. This introduces no functional changes.
Created attachment 345333 [details] [review] rfkill: Use local task variables where possible In order to keep the binding between a GTask object and a control flow, use the GTask passed as the user_data to the callbacks as much as possible, rather than referring back to the global GTask which might have changed in the mean time.
Created attachment 345334 [details] [review] rfkill: Move event data into the GTask’s task data This simplifies lifecycle management of the event struct. It always had the same lifecycle as the GTask already, so there are no functional changes; just code simplification.
Created attachment 345335 [details] [review] rfkill: Fix crash due to racy GTask management The change_all_timeout_id and task members of CcRfkillGlibPrivate are not necessarily both set and unset at the same time: it’s normal for (task != NULL) when (change_all_timeout_id == 0), when the code is doing its initial write before write_change_all_done_cb(); and when the code is doing its second write attempt before write_change_all_again_done_cb(). However, the code was checking (change_all_timeout_id != 0) to see if a task was pending when cc_rfkill_glib_send_change_all_event(). If a previous task was still in the middle of a write, this would result in its GTask being overwritten with the new GTask. When the first task’s write completed, it would then clear the second GTask. When the second task’s write completed, it would try to access the second GTask, and find it was NULL, causing a crash. This can happen on systems where the rfkill subsystem is slow (and hence blocks on writes for tens or hundreds of milliseconds); for example, if rfkill passes through a GPIO. Fix this by: • checking whether (task != NULL) to see if a task is in flight; and • only clearing priv->task if it matches the callback’s task.
Created attachment 345336 [details] [review] rfkill: Add a comment with basic testing instructions As much as I would love to, I don’t have time to write a unit testing harness which mocks up a slow rfkill FD. So this will have to do. Suggestion: run it in a tight loop, alternating between enabled and disabled. See what breaks.
Review of attachment 345322 [details] [review]: Sure.
Review of attachment 345323 [details] [review]: Looks fine.
Review of attachment 345324 [details] [review]: Sure.
Review of attachment 345325 [details] [review]: Sure.
Review of attachment 345326 [details] [review]: > This was causing the gsd-power plugin to not be built if HAVE_GUDEV was > defined (and gsd-backlight-helper was being built). Fixes the > corresponding automake warning. It should have, but it doesn't, as shown by the fact that the Fedora gnome-settings-daemon package contains both. Weird.
Review of attachment 345327 [details] [review]: ::: plugins/smartcard/Makefile.am @@ -1,1 @@ -plugin_name = smartcard It's still used. From configure.ac: PLUGIN_CFLAGS="-DG_LOG_DOMAIN=\"\\\"\$(plugin_name)-plugin\\\"\" -DPLUGIN_NAME=\"\\\"\$(plugin_name)\\\"\" "
Review of attachment 345328 [details] [review]: Looks fine, was this tested? ::: plugins/rfkill/rfkill-glib.c @@ +94,2 @@ else + g_task_return_int (task, ret); gssize, no?
Review of attachment 345329 [details] [review]: I'd have preferred keeping it, but seeing as it's unused. We can always revert if we want to use it.
Review of attachment 345330 [details] [review]: ::: plugins/rfkill/rfkill-glib.h @@ +32,3 @@ +#define CC_RFKILL_TYPE_GLIB cc_rfkill_glib_get_type () +G_DECLARE_DERIVABLE_TYPE (CcRfkillGlib, cc_rfkill_glib, CC_RFKILL, GLIB, GObject) Make it final please.
Review of attachment 345331 [details] [review]: Sure.
Review of attachment 345332 [details] [review]: This is nice.
Review of attachment 345333 [details] [review]: Useful as well.
Review of attachment 345334 [details] [review]: Yes!
Review of attachment 345335 [details] [review]: That looks fine.
Review of attachment 345336 [details] [review]: ::: plugins/rfkill/gsd-rfkill-manager.c @@ +28,3 @@ + * --method org.freedesktop.DBus.Properties.Set \ + * "org.gnome.SettingsDaemon.Rfkill" \ + * "[Bluetooth]AirplaneMode" \ Split this up into multiple examples please, one for Bluetooth, one for the global rfkill.
Review of attachment 345327 [details] [review]: ::: plugins/smartcard/Makefile.am @@ -1,1 @@ -plugin_name = smartcard Oops. I’ll add it back.
Review of attachment 345328 [details] [review]: ::: plugins/rfkill/rfkill-glib.c @@ +94,2 @@ else + g_task_return_int (task, ret); g_task_return_int() does use a gssize, confusingly. The types are correct here.
Review of attachment 345330 [details] [review]: ::: plugins/rfkill/rfkill-glib.h @@ +32,3 @@ +#define CC_RFKILL_TYPE_GLIB cc_rfkill_glib_get_type () +G_DECLARE_DERIVABLE_TYPE (CcRfkillGlib, cc_rfkill_glib, CC_RFKILL, GLIB, GObject) I can’t do that without dropping the CcRfkillGlibClass.changed signal vfunc. I could do that — it’s unused by any of the calling code (it just connects to the signal using g_signal_connect() as normal).
Review of attachment 345336 [details] [review]: ::: plugins/rfkill/gsd-rfkill-manager.c @@ +28,3 @@ + * --method org.freedesktop.DBus.Properties.Set \ + * "org.gnome.SettingsDaemon.Rfkill" \ + * "[Bluetooth]AirplaneMode" \ Righty.
Review of attachment 345330 [details] [review]: ::: plugins/rfkill/rfkill-glib.h @@ +32,3 @@ +#define CC_RFKILL_TYPE_GLIB cc_rfkill_glib_get_type () +G_DECLARE_DERIVABLE_TYPE (CcRfkillGlib, cc_rfkill_glib, CC_RFKILL, GLIB, GObject) It shows how old this code is... You can drop it.
Created attachment 345338 [details] [review] build: Drop unused variables from smartcard Makefile.am Since commit 6d01a434ca2faa88acb2aed1224d23cea329f1f8, libsmartcard.la is no more, so all the variables which defined how to compile it are redundant. This fixes an automake warning about unused variables.
Created attachment 345339 [details] [review] rfkill: Add a comment with basic testing instructions As much as I would love to, I don’t have time to write a unit testing harness which mocks up a slow rfkill FD. So this will have to do. Suggestion: run it in a tight loop, alternating between enabled and disabled. See what breaks.
Review of attachment 345338 [details] [review]: Yep.
Review of attachment 345330 [details] [review]: ::: plugins/rfkill/rfkill-glib.h @@ +32,3 @@ +#define CC_RFKILL_TYPE_GLIB cc_rfkill_glib_get_type () +G_DECLARE_DERIVABLE_TYPE (CcRfkillGlib, cc_rfkill_glib, CC_RFKILL, GLIB, GObject) Cool, will do so in a bit and will rebase all the patches.
Review of attachment 345339 [details] [review]: ::: plugins/rfkill/gsd-rfkill-manager.c @@ +29,3 @@ + * "org.gnome.SettingsDaemon.Rfkill" \ + * "AirplaneMode" \ + * "<true|false>" I don't really like this either, but I can't think of a better way to show it without have a gazillion different examples.
Created attachment 345343 [details] [review] rfkill: Update class definition to use G_DECLARE_FINAL_TYPE Tidy up the code a bit. This drops the CcRfkillGlibClass.changed signal handler vfunc, which was unused.
Created attachment 345344 [details] [review] rfkill: Add some checks that a previous task is not overwritten These should not fail, as write_change_all_timeout_cb() clears those variables — so they’re more for documentation than anything else.
Created attachment 345345 [details] [review] rfkill: Remove stored GCancellable member There’s no need to store it separately from the GTask, as the GTask keeps a pointer to it. This introduces no functional changes.
Created attachment 345346 [details] [review] rfkill: Use local task variables where possible In order to keep the binding between a GTask object and a control flow, use the GTask passed as the user_data to the callbacks as much as possible, rather than referring back to the global GTask which might have changed in the mean time.
Created attachment 345347 [details] [review] rfkill: Move event data into the GTask’s task data This simplifies lifecycle management of the event struct. It always had the same lifecycle as the GTask already, so there are no functional changes; just code simplification.
Created attachment 345348 [details] [review] rfkill: Fix crash due to racy GTask management The change_all_timeout_id and task members of CcRfkillGlibPrivate are not necessarily both set and unset at the same time: it’s normal for (task != NULL) when (change_all_timeout_id == 0), when the code is doing its initial write before write_change_all_done_cb(); and when the code is doing its second write attempt before write_change_all_again_done_cb(). However, the code was checking (change_all_timeout_id != 0) to see if a task was pending when cc_rfkill_glib_send_change_all_event(). If a previous task was still in the middle of a write, this would result in its GTask being overwritten with the new GTask. When the first task’s write completed, it would then clear the second GTask. When the second task’s write completed, it would try to access the second GTask, and find it was NULL, causing a crash. This can happen on systems where the rfkill subsystem is slow (and hence blocks on writes for tens or hundreds of milliseconds); for example, if rfkill passes through a GPIO. Fix this by: • checking whether (task != NULL) to see if a task is in flight; and • only clearing priv->task if it matches the callback’s task.
Created attachment 345349 [details] [review] rfkill: Add a comment with basic testing instructions As much as I would love to, I don’t have time to write a unit testing harness which mocks up a slow rfkill FD. So this will have to do. Suggestion: run it in a tight loop, alternating between enabled and disabled. See what breaks.
(In reply to Philip Withnall from comment #39) > Review of attachment 345330 [details] [review] [review]: > > ::: plugins/rfkill/rfkill-glib.h > @@ +32,3 @@ > > +#define CC_RFKILL_TYPE_GLIB cc_rfkill_glib_get_type () > +G_DECLARE_DERIVABLE_TYPE (CcRfkillGlib, cc_rfkill_glib, CC_RFKILL, GLIB, > GObject) > > Cool, will do so in a bit and will rebase all the patches. Updated that patch and rebased the other ones on top of it (trivial rebase).
Review of attachment 345343 [details] [review]: Great.
And acn on all the others, thanks!
Can you also please cherry-pick the rfkill specific changes to gnome-3-20 and gnome-3-22? If it's too much work, keep this opened, and I'll look into it.
(In reply to Bastien Nocera from comment #51) > Can you also please cherry-pick the rfkill specific changes to gnome-3-20 > and gnome-3-22? If it's too much work, keep this opened, and I'll look into > it. FYI, I haven’t pushed to master yet because I’m waiting on some feedback from a colleague who has access to some hardware with a particularly slow (GPIO-based) rfkill subsystem. I’d like to make sure this is working on that hardware before pushing, since I could only test on my laptop. I don’t think I’ll get time to do the cherry-picking. :-(
Attachment 345322 [details] pushed as 5a4e38f - rfkill: Fix indentation in D-Bus introspection XML Attachment 345323 [details] pushed as eea8d7e - build: Stop using deprecated macros in configure.ac Attachment 345324 [details] pushed as 73c718b - build: Fix --enable-rfkill configure switch Attachment 345325 [details] pushed as f67e21c - build: Drop dependency on gnome-common Attachment 345326 [details] pushed as e93bef7 - build: Fix shadowing of libexec_PROGRAMS in power plugin Makefile.am Attachment 345328 [details] pushed as d2e8840 - rfkill: Port from GSimpleAsyncResult to GTask Attachment 345329 [details] pushed as 449b22d - rfkill: Drop cc_rfkill_glib_send_event() due to being unused Attachment 345338 [details] pushed as a9a7f9f - build: Drop unused variables from smartcard Makefile.am Attachment 345343 [details] pushed as 75f2353 - rfkill: Update class definition to use G_DECLARE_FINAL_TYPE Attachment 345344 [details] pushed as 290790b - rfkill: Add some checks that a previous task is not overwritten Attachment 345345 [details] pushed as f456928 - rfkill: Remove stored GCancellable member Attachment 345346 [details] pushed as 769d350 - rfkill: Use local task variables where possible Attachment 345347 [details] pushed as dd33981 - rfkill: Move event data into the GTask’s task data Attachment 345348 [details] pushed as aa92455 - rfkill: Fix crash due to racy GTask management