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 674827 - gnome-settings-daemon crashed in engine_coldplug()
gnome-settings-daemon crashed in engine_coldplug()
Status: RESOLVED FIXED
Product: gnome-settings-daemon
Classification: Core
Component: power
3.4.x
Other Linux
: Normal critical
: ---
Assigned To: Richard Hughes
gnome-settings-daemon-maint
Depends on:
Blocks:
 
 
Reported: 2012-04-25 20:17 UTC by Michael Terry
Modified: 2013-01-22 21:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch (1016 bytes, patch)
2012-04-25 20:21 UTC, Michael Terry
committed Details | Review

Description Michael Terry 2012-04-25 20:17:53 UTC
Ubuntu got this crash report: https://bugs.launchpad.net/ubuntu/+source/gnome-settings-daemon/+bug/868928

  • #0 engine_coldplug
    at gsd-power-manager.c line 1100
  • #1 gsd_power_manager_start
    at gsd-power-manager.c line 3446
  • #2 impl_activate
    at gsd-power-plugin.c line 78
  • #3 _activate_plugin
    at gnome-settings-plugin-info.c line 408
  • #4 gnome_settings_plugin_info_activate
    at gnome-settings-plugin-info.c line 431

The relevant code is:

        array = up_client_get_devices (manager->priv->up_client);
        for (i=0;i<array->len;i++) {

It doesn't make sense to me.  I can't quite see how array can be NULL at that juncture, given that we call up_client_enumerate_devices_sync() beforehand.  But I do see that the code could be more defensive.  NULL is a valid return for up_client_get_devices(), so it should be safe-guarded.

Patch coming.
Comment 1 Michael Terry 2012-04-25 20:21:56 UTC
Created attachment 212832 [details] [review]
Proposed patch
Comment 2 Richard Hughes 2012-04-26 07:36:33 UTC
Hmm, up_client_get_devices should not return NULL unless there's a critical warning being printed. I think we need to find the root cause of why priv->up_client is invalid (or done_enumerate is FALSE) at this point.
Comment 3 Michael Terry 2012-04-26 13:36:40 UTC
Agreed, but I could not trace a path through the code that would end up with get_devices() failing.  :-/
Comment 4 Bastien Nocera 2012-06-18 10:53:35 UTC
Michael, when you've managed to get more info, or a more recent reproducer, let us know.
Comment 5 Tobias Mueller 2012-10-02 23:47:29 UTC
Closing this bug report as no further information has been provided. Please feel free to reopen this bug if you can provide the information asked for.
Thanks!
Comment 6 Michael Terry 2013-01-04 19:09:34 UTC
I'd like to petition for this patch again.

(I can't provide more recent reproducers as asked, since it wasn't a reliable crasher in the first place and I'm not about to remove the patch from Ubuntu and inflict crashes on our users.)

I sympathize that you want to find the root cause.  But that shouldn't prevent you from being defensive programmers and preventing a crash that users do see.

A) The API call up_client_get_devices() is allowed to return NULL and your code doesn't provide for that case.
B) Ubuntu was seeing quite a few of these crashes before we applied this patch.  We have gotten no regression reports since then.

Regardless of why up_client_get_devices() is returning NULL, I'd argue that the code should be prepared to handle it.  The perfect should not be the enemy of the good.
Comment 7 Bastien Nocera 2013-01-22 21:25:28 UTC
A simplified version pushed to master.