Bug 172823 - kded4 crashes with SIGSEGV during resume from suspend
Summary: kded4 crashes with SIGSEGV during resume from suspend
Status: CLOSED FIXED
Alias: None
Product: solid
Classification: Frameworks and Libraries
Component: powermanagement-daemon (show other bugs)
Version: unspecified
Platform: Compiled Sources Linux
: NOR crash
Target Milestone: ---
Assignee: Dario Freddi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-10-14 22:02 UTC by Hugh Daschbach
Modified: 2010-10-02 12:49 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
Convert m_battery from (Solid::Battery *) to QPointer<Solid::Battery> (703 bytes, text/plain)
2008-10-16 07:51 UTC, Hugh Daschbach
Details
Another crash, this time from svn revision 877702 (7.52 KB, text/plain)
2008-11-02 18:55 UTC, Hugh Daschbach
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Hugh Daschbach 2008-10-14 22:02:55 UTC
Version:           Repository revision 869805 (using Devel)
Compiler:          gcc 4.2.3 Ubuntu 4.2.3-2ubuntu7
OS:                Linux
Installed from:    Compiled sources

kded4 intermittently crashes on laptop resume from sleep mode (suspend to memory).  This seems to happen most often when the power cable is plugged in during suspend, but not during resume; or when the power cable is unplugged during suspend, but plugged in during resume.

Application: KDE Daemon (kded4), signal SIGSEGV
0x00007fa1d5ca1b81 in nanosleep () from /lib/libc.so.6
[Current thread is 0 (LWP 7397)]

Thread 2 (Thread 0x4211c950 (LWP 19044)):
#0  0x00007fa1d5cd4db2 in select () from /lib/libc.so.6
#1  0x00007fa1d8eb4325 in QProcessManager::run (this=0x6498d0) at io/qprocess_unix.cpp:301
#2  0x00007fa1d8df1d62 in QThreadPrivate::start (arg=0x6498d0) at thread/qthread_unix.cpp:185
#3  0x00007fa1d8b703f7 in start_thread () from /lib/libpthread.so.0
#4  0x00007fa1d5cdbb3d in clone () from /lib/libc.so.6
#5  0x0000000000000000 in ?? ()

Thread 1 (Thread 0x7fa1da802780 (LWP 7397)):
[KCrash Handler]
#5  0x00007fa1d51f6b42 in QPointer<QObject>::operator QObject* (this=0x8) at /home/kde/qt-copy/include/QtCore/qpointer.h:74
#6  0x00007fa1d51f665f in Solid::DeviceInterfacePrivate::backendObject (this=0x0) at /home/kde/kde/src/KDE/kdelibs/solid/solid/deviceinterface.cpp:75
#7  0x00007fa1d51fb8ae in Solid::Battery::chargePercent (this=0x7cf490) at /home/kde/kde/src/KDE/kdelibs/solid/solid/battery.cpp:59
#8  0x00007fa1cfa1e280 in PowerDevilDaemon::reloadProfile (this=0x812a70, state=2) at /home/kde/kde/src/KDE/kdebase/workspace/powerdevil/daemon/PowerDevilDaemon.cpp:1026
#9  0x00007fa1cfa1ffeb in PowerDevilDaemon::acAdapterStateChanged (this=0x812a70, state=2, forced=false) at /home/kde/kde/src/KDE/kdebase/workspace/powerdevil/daemon/PowerDevilDaemon.cpp:294
#10 0x00007fa1cfa21155 in PowerDevilDaemon::qt_metacall (this=0x812a70, _c=QMetaObject::InvokeMetaMethod, _id=29, _a=0x7fffe2843970)
    at /home/kde/kde/build/KDE/kdebase/workspace/powerdevil/daemon/PowerDevilDaemon.moc:222
#11 0x00007fa1d8ee77b6 in QMetaObject::activate (sender=0x6878d0, from_signal_index=<value optimized out>, to_signal_index=5, argv=0x51) at kernel/qobject.cpp:3022
#12 0x00007fa1cf7e48a0 in Solid::Control::PowerManager::Notifier::acAdapterStateChanged (this=0x6878d0, _t1=2) at /home/kde/kde/build/KDE/kdebase/workspace/libs/solid/control/powermanager.moc:96
#13 0x00007fa1cf7e49a1 in Solid::Control::PowerManager::Notifier::qt_metacall (this=0x6878d0, _c=QMetaObject::InvokeMetaMethod, _id=1, _a=0x7fffe2843af0)
    at /home/kde/kde/build/KDE/kdebase/workspace/libs/solid/control/powermanager.moc:75
#14 0x00007fa1cf7e4a23 in Solid::Control::PowerManagerPrivate::qt_metacall (this=0x6878d0, _c=QMetaObject::InvokeMetaMethod, _id=5, _a=0x7fffe2843af0)
    at /home/kde/kde/build/KDE/kdebase/workspace/libs/solid/control/powermanager_p.moc:61
#15 0x00007fa1d8ee77b6 in QMetaObject::activate (sender=0x68a160, from_signal_index=<value optimized out>, to_signal_index=5, argv=0x51) at kernel/qobject.cpp:3022
#16 0x00007fa1cf3c314e in Solid::Control::Ifaces::PowerManager::acAdapterStateChanged (this=0x68a160, _t1=2) at /home/kde/kde/build/KDE/kdebase/workspace/libs/solid/control/ifaces/powermanager.moc:96
#17 0x00007fa1cf1acf6c in HalPower::slotPlugStateChanged (this=0x68a160, newState=false) at /home/kde/kde/src/KDE/kdebase/workspace/solid/hal/halpower.cpp:525
#18 0x00007fa1cf1ad4f9 in HalPower::qt_metacall (this=0x68a160, _c=QMetaObject::InvokeMetaMethod, _id=1, _a=0x7fffe2843c60) at /home/kde/kde/build/KDE/kdebase/workspace/solid/hal/halpower.moc:76
#19 0x00007fa1d8ee77b6 in QMetaObject::activate (sender=0x675850, from_signal_index=<value optimized out>, to_signal_index=4, argv=0x51) at kernel/qobject.cpp:3022
#20 0x00007fa1d51fb263 in Solid::AcAdapter::plugStateChanged (this=0x675850, _t1=false, _t2=@0x7fffe2843e20) at /home/kde/kde/build/KDE/kdelibs/solid/solid/acadapter.moc:104
#21 0x00007fa1d51fb38f in Solid::AcAdapter::qt_metacall (this=0x675850, _c=QMetaObject::InvokeMetaMethod, _id=0, _a=0x7fffe2843de0) at /home/kde/kde/build/KDE/kdelibs/solid/solid/acadapter.moc:70
#22 0x00007fa1d8ee77b6 in QMetaObject::activate (sender=0xb76d40, from_signal_index=<value optimized out>, to_signal_index=4, argv=0x51) at kernel/qobject.cpp:3022
#23 0x00007fa1d521828d in Solid::Backends::Hal::AcAdapter::plugStateChanged (this=0xb76d40, _t1=false, _t2=@0x7fffe2843e20)
    at /home/kde/kde/build/KDE/kdelibs/solid/solid/backends/hal/halacadapter.moc:86
#24 0x00007fa1d52183d4 in Solid::Backends::Hal::AcAdapter::slotPropertyChanged (this=0xb76d40, changes=@0x7fffe2844000) at /home/kde/kde/src/KDE/kdelibs/solid/solid/backends/hal/halacadapter.cpp:45
#25 0x00007fa1d5218484 in Solid::Backends::Hal::AcAdapter::qt_metacall (this=0xb76d40, _c=QMetaObject::InvokeMetaMethod, _id=1, _a=0x7fffe2843f80)
    at /home/kde/kde/build/KDE/kdelibs/solid/solid/backends/hal/halacadapter.moc:75
#26 0x00007fa1d8ee77b6 in QMetaObject::activate (sender=0x861320, from_signal_index=<value optimized out>, to_signal_index=4, argv=0x51) at kernel/qobject.cpp:3022
#27 0x00007fa1d52207b3 in Solid::Backends::Hal::HalDevice::propertyChanged (this=0x861320, _t1=@0x7fffe2844000) at /home/kde/kde/build/KDE/kdelibs/solid/solid/backends/hal/haldevice.moc:91
#28 0x00007fa1d522094a in Solid::Backends::Hal::HalDevice::slotPropertyModified (this=0x861320, changes=@0x753960) at /home/kde/kde/src/KDE/kdelibs/solid/solid/backends/hal/haldevice.cpp:440
#29 0x00007fa1d5220a2f in Solid::Backends::Hal::HalDevice::qt_metacall (this=0x861320, _c=QMetaObject::InvokeMetaMethod, _id=2, _a=0x7fffe28442b0)
    at /home/kde/kde/build/KDE/kdelibs/solid/solid/backends/hal/haldevice.moc:79
#30 0x00007fa1d91ed38d in QDBusConnectionPrivate::deliverCall (this=0x68a590, object=0x861320, msg=@0x7db1a8, metaTypes=@0x7db1b0, slotIdx=6) at qdbusintegrator.cpp:849
#31 0x00007fa1d91f525f in QDBusCallDeliveryEvent::placeMetaCall (this=0x8, object=0x2) at qdbusintegrator_p.h:130
#32 0x00007fa1d8ee2397 in QObject::event (this=0x861320, e=0x7db160) at kernel/qobject.cpp:1146
#33 0x00007fa1d68c943f in QApplicationPrivate::notify_helper (this=0x62c3d0, receiver=0x861320, e=0x7db160) at kernel/qapplication.cpp:3803
#34 0x00007fa1d68cb5f5 in QApplication::notify (this=0x7fffe2844ca0, receiver=0x861320, e=0x7db160) at kernel/qapplication.cpp:3768
#35 0x00007fa1d9aec616 in KApplication::notify (this=0x7fffe2844ca0, receiver=0x861320, event=0x7db160) at /home/kde/kde/src/KDE/kdelibs/kdeui/kernel/kapplication.cpp:307
#36 0x00007fa1d8ed35d9 in QCoreApplication::notifyInternal (this=0x7fffe2844ca0, receiver=0x861320, event=0x7db160) at kernel/qcoreapplication.cpp:587
#37 0x00007fa1d8ed48fb in QCoreApplicationPrivate::sendPostedEvents (receiver=0x0, event_type=0, data=0x603360) at ../../include/QtCore/../../src/corelib/kernel/qcoreapplication.h:209
#38 0x00007fa1d8efc543 in postEventSourceDispatch (s=<value optimized out>) at ../../include/QtCore/../../src/corelib/kernel/qcoreapplication.h:214
#39 0x00007fa1d38dd3d4 in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
#40 0x00007fa1d38e06e5 in ?? () from /usr/lib/libglib-2.0.so.0
#41 0x00007fa1d38e0bcb in g_main_context_iteration () from /usr/lib/libglib-2.0.so.0
#42 0x00007fa1d8efc83f in QEventDispatcherGlib::processEvents (this=0x61d580, flags=<value optimized out>) at kernel/qeventdispatcher_glib.cpp:319
#43 0x00007fa1d695210f in QGuiEventDispatcherGlib::processEvents (this=0x8, flags=<value optimized out>) at kernel/qguieventdispatcher_glib.cpp:198
#44 0x00007fa1d8ed2a55 in QEventLoop::processEvents (this=<value optimized out>, flags=@0x7fffe2844bc0) at kernel/qeventloop.cpp:143
#45 0x00007fa1d8ed2bab in QEventLoop::exec (this=0x7fffe2844c00, flags=@0x7fffe2844c10) at kernel/qeventloop.cpp:194
#46 0x00007fa1d8ed4bf9 in QCoreApplication::exec () at kernel/qcoreapplication.cpp:845
#47 0x00007fa1da41c7f8 in kdemain (argc=1, argv=0x7fffe28450e8) at /home/kde/kde/src/KDE/kdelibs/kded/kded.cpp:900
#48 0x0000000000400813 in main (argc=1, argv=0x7fffe28450e8) at /home/kde/kde/build/KDE/kdelibs/kded/kded4_dummy.cpp:3
Comment 1 David Faure 2008-10-14 23:07:49 UTC
*** This bug has been confirmed by popular vote. ***
Comment 2 Dario Freddi 2008-10-15 20:22:15 UTC
Can't reproduce here, however, I think it's not a PowerDevil bug, since the actual crash happens in Solid. I'll talk with ervin about this
Comment 3 Hugh Daschbach 2008-10-16 07:51:26 UTC
Created attachment 27931 [details]
Convert m_battery from (Solid::Battery *) to QPointer<Solid::Battery>

Converting m_battery to a QPointer seems to fix the crash.  But I don't know if this has any side effects, like losing track of a battery altogether.

Initial testing encouraging.  Need a few more days of suspend/resume to see if this does eliminate the crash.
Comment 4 Dario Freddi 2008-10-16 21:23:23 UTC
This should be irrilevant, since if it was a null pointer issue, the crash would have happened sooner. Moreover, m_battery is created just once and never touched.
I also noticed just now that the revision you filed the bug against is rather old, please test this in a more recent revision.
Comment 5 Hugh Daschbach 2008-10-17 06:04:35 UTC
I don't think the issue is a NULL m_battery pointer, but a stale m_battery pointer.

The NULL pointer is the private data associated with a deleted Solid::Battery object.  If I read this correctly, Solid::Battery maintains a pointer to a Solid::BatteryPrivate object.  This gets zeroed when the Solid::Battery object gets torn down.

But there doesn't seem to be any communication mechanism for Solid::Battery to tell PowerDevilDaemon that the Solid object is being destroyed.  That leaves PowerDevilDaemon with a stale m_battery pointer.

That's why I plugged in a QPointer.  That allows the QObject destructor to zero out m_battery when the Solid::Battery object gets destroyed.

As for the age of the revision, my tree is about a week old.  I'll update, back out the QPointer change, and give this another try.
Comment 6 Dario Freddi 2008-10-26 12:31:40 UTC
Any news about this?
Comment 7 Hugh Daschbach 2008-10-26 16:26:10 UTC
I haven't see this crash since I've update to svn revision 874911.  It was an intermittent issue.  So I'm not sure it's fixed.  But I cannot say that it isn't.

How about if we close this ticket?  If I see the crash again, I'll ask to have it reopened.
Comment 8 Dario Freddi 2008-10-26 19:59:29 UTC
Seems to me a good solution. Thanks for hitting me back.
Comment 9 Hugh Daschbach 2008-11-02 18:55:22 UTC
Created attachment 28284 [details]
Another crash, this time from svn revision 877702

This crash occurred upon unplugging the AC adapter, rather than during resume.  This continues to be intermittent -- this is the first such crash that I've seen in a few weeks.
Comment 10 Dario Freddi 2008-11-02 19:02:42 UTC
This time is definitely a Solid bug, hope to solve it in a pair of minutes
Comment 11 Dario Freddi 2008-11-02 19:13:45 UTC
SVN commit 879212 by dafre:

BUG: 172823
 * Check if the pointer is actually not 0
 * Initialize the QPointer in the constructor


 M  +6 -1      deviceinterface.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=879212
Comment 12 Dario Freddi 2008-11-02 19:27:00 UTC
Should definitely be fixed now, closing the bug again, reopen if you experience some other crashes. Thanks!
Comment 13 Dario Freddi 2008-11-02 21:27:48 UTC
Reopening the bug. Turns out it was not so straightforward as it seemed.
Comment 14 Hugh Daschbach 2008-11-07 18:00:40 UTC
I've seen this crash again.  I'm not attaching the traceback because the NULL pointer in the traceback seems to be distracting from the cause of the problem.

I have, however, added debug messages to the Solid::Battery and PowerDevilDaemon constructors and destructors.  The output seems to suggest that the problem is that PowerDevilDaemon::m_battery is stale at the time of the crash.

The PowerDevilDaemon constructor searches for the primary battery as an instance of Solid::DeviceInterface::Battery.  It saves a pointer to the instance, assuming that the result of the search is immutable.

But the Solid::Battery object is transient.  From what I can see, part of the O/S support suspend/resume issues an "rmmod battery" when the system is being put to sleep, and an "modprobe battery" during resume.

When the O/S deregisters the battery, the associated Solid::Battery instance is destroyed.  So PowerDevilDaemon::m_battery pointer becomes invalid.

Without a better idea of the design principles behind the app/library architecture, I don't have a suggestion for how to fix this.  But seeing the Solid::Battery object destroyed explains why my QPointer change stopped the crashes.  And it confirms that QPointer is not sufficient to fix the problem, since that would simply erase the m_battery pointer with on provision to regenerate the pointer on resume.

I hope this helps decide how best to fix this issue.
Comment 15 Dario Freddi 2008-11-07 18:05:06 UTC
Thanks Hugh, this was something I was thinking about, but your explaination definitely proves that m_battery becoming invalid is the issue.

I already know how to fix that, but what comes to my mind is that other application use a persistant m_battery pointer (battery applet, for example).

I guess I'll go for a fix-it-all round, thanks again for the informations
Comment 16 Dario Freddi 2008-11-07 18:23:27 UTC
Should be fixed now. What was driving me mad was that kind of behavior while m_battery was still, somehow "alive".

In my last commit I added a cache reloader to m_battery, that gets triggered everytime m_battery->isValid() returns false. If the crash won't happen again, I'll dig into similar code and fix that too.
Comment 17 Hugh Daschbach 2008-11-08 08:29:23 UTC
I've picked up the change.  I'll give it a test.  But it will be a couple days before I can report progress.

My concern is this doesn't look foolproof, in that it doesn't really prevent the stale pointer.  It simply attempts to detect a stale pointer via DeviceInterface::isValid().  Once the memory for a destroyed Solid::Battery object is released it may be reused for almost anything.  This potentially leaves what used to be the Solid::Battery backendObject pointer set to something that isn't zero and isn't even a valid pointer at all.

I think that if PowerDevilDaemon is going to cache a pointer to a transient object, it needs to be proactive in clearing the pointer when the transient instance is destroyed.  Qt makes this simple enough by either declaring m_battery as a QPointer<Solid::Battery> instead of a direct Solid::Battery *, or by connecting to the m_battery object's SIGNAL(destroyed(QObject *)) and manually clearing the pointer upon receiving the signal that the Solid::Battery object is being torn down (sent by QObject::~QObject().)

That has the advantage of preventing the pointer from ever going stale.  And it allows you to replace the:

 if (!m_battery->isValid()) {
     recacheBatteryPointer();
 }

with

 if (!m_battery) {
     recacheBatteryPointer();
 }

However, in reloadProfile(), I think you need to do something like:

 if (!m_battery) {
     recacheBatteryPointer();
 }
 if (!m_battery) {
     setCurrentProfile(PowerDevilSettings::aCProfile());
 } else {
    ...

Because there's no guarantee that recacheBatteryPointer() will find the object it is looking for.  So guard against m_battery == NULL upon return from recacheBatteryPointer().

My two cents, anyway.  I'll be offline for the next day and a half or so.  I'll give what you've got in the repository a test when I get back.

Comment 18 Dario Freddi 2008-11-08 12:36:26 UTC
The first part is actually incorrect, since if it was as you said, you wouldn't have experienced a crash but just a constant profile on AC, since the pointer is checked against being NULL. It is the underlying interface reference that gets destroyed, so the pointer is valid in C++ sense, but is invalid in Solid sense, so isValid() should be the correct test case.

In the second part, you are right, recacheBatteryPointer might return 0, so better add a check there.
Comment 19 Oleg Smirsky 2008-11-08 16:04:16 UTC
I'm not sure, whether it is a related problem, but my KDED4 crashes on start, if power devil (1.4) is installed. Version 1.2 didn't have that problem.
Comment 20 Dario Freddi 2008-11-08 16:04:54 UTC
SVN commit 881608 by dafre:

BUG: 172823
I was almost sure I converted m_battery to a QPointer, but I didn't, and I totally forgot about that. So also your first
statement makes sense.
I have simplified the code a bit making it more versatile: now recacheBatteryPointer() returns true or false if the pointer is
valid or not, and triggers the recaching dynamically. This should be definitely be it, let's hope this bug has finally gone :)


 M  +27 -19    PowerDevilDaemon.cpp  
 M  +3 -2      PowerDevilDaemon.h  


WebSVN link: http://websvn.kde.org/?view=rev&revision=881608
Comment 21 Dario Freddi 2008-11-08 16:09:04 UTC
Oleg, the problem you're referring to is similar to bug #173930

Marking as fixed, this definitely should be it.
Comment 22 Dario Freddi 2008-11-08 16:10:18 UTC
Oleg, sorry, I meant bug #174516
Comment 23 Hugh Daschbach 2008-11-10 14:59:37 UTC
881608 resolves the last of my concerns.  No failures with initial testing of this fix.  

Thanks for working through this.