Need suggestion on how to fix the common crash in plasma-desktop (kdelibs related)
Lamarque V. Souza
Hi,
There is a crash in WeatherEngine (kde-workspace) triggered by the fact that Plasma::DataEngineManager::self() (kdelib) is invalid when plasma-{desktop,device} are exiting. WeatherEngine::~WeatherEngine() calls WeatherEngine::unloadIons(), which tries to use the invalid Plasma::DataEngineManager::self(). The crash only happens if there is something using WeatherEngine when plasma-{desktop,device} exit, of course. There are several bug report about this crash:
https://bugs.kde.org/show_bug.cgi?id=223622
https://bugs.kde.org/show_bug.cgi?id=241509
https://bugs.kde.org/show_bug.cgi?id=261610
And counting. The reports were closed as upstrem but think we can prevent the crash by adding a method to Plasma::DataEngineManager to check if it is still valid. Using "if (Plasma::DataEngineManager::self()) {}" also triggers the crash because of the way Plasma::DataEngineManager::self() is implemented.
What do you think about adding a Plasma::DataEngineManager::valid() method?
OBS: That crash is in WeatherEngine since it entered kde-workspace in 2009.
--
Lamarque V. Souza
KDE's Network Management maintainer
Thomas Lübking
--
Cheers,
Thomas
Lamarque V. Souza
Em Tuesday 20 March 2012, Thomas Lübking escreveu:
> Tried to connect to QCoreApplication::aboutToQuit()?
It is already done but that does not cover the case where WeatherEngine is unloaded without plasma-{desktop,device} exiting. The Ions must be unloaded in that case or we will have a memory leak.
Lamarque V. Souza
Thomas Lübking
yourself "dontTry")?
David Faure
> WeatherEngine::~WeatherEngine() calls
> WeatherEngine::unloadIons(), which tries to use the invalid
> Plasma::DataEngineManager::self().
Since DataEngineManager uses K_GLOBAL_STATIC internally, just use the
isDestroyed() method to know if it has already been destroyed, before calling
self().
More precisely, either add a isDestroyed method to the public class, which
calls the one in the K_GLOBAL_STATIC, or let self() return 0 when
privateDataEngineManagerSelf.isDestroyed().
On a more philosophical note: this is exactly why "intelligent destructors"
are to be avoided at all costs. All this wouldn't happen if the WeatherEngine
destructor didn't call methods that do stuff.
--
David Faure, fa...@kde.org, http://www.davidfaure.fr
Sponsored by Nokia to work on KDE, incl. KDE Frameworks 5
Aaron J. Seigo
> There is a crash in WeatherEngine (kde-workspace) triggered by the fact
> that Plasma::DataEngineManager::self() (kdelib) is invalid when
> plasma-{desktop,device} are exiting. WeatherEngine::~WeatherEngine() calls
> WeatherEngine::unloadIons(), which tries to use the invalid
> Plasma::DataEngineManager::self(). The crash only happens if there is
this happens only when the application uncleanly exits. if you notice in the
bug reports you linked to there was a problem elsewhere (e.g. an xioerror, an
uncaught exception, etc.) and that caused an abort of the process with an
unclean exit which then triggers this problem. the cause of the crash was
never the WeatherEngine itself, but rather a crash in WeatherEngine was
triggered while the application was otherwise closing down due to an error
elsewhere that was itself bringing down the application.
DataEngines created by DataEngineManager *must* be released prior to
application exit. and normally this happens except in such cases where the
application is brought down by an abnormal situation.
so while you can make the changes David suggests, it will only change the
backtraces in those bug reports but not actually solve anything in the real
world. the aborts will still happen as a result of the underlying error.
p.s. this probably belongs on plasma-devel rather than kde-core-devel ...
--
Aaron J. Seigo
Lamarque V. Souza
Em Wednesday 21 March 2012, Thomas Lübking escreveu:
> hook on QObject::destroyed() for the dataenginemanager (and flag
> yourself "dontTry")?
<on>Didn't try it</on>
Unfortunately it also did not work :( WeatherEngine did not receive the signal, at least not before the crash. Thanks anyway.
Lamarque V. Souza
Em Wednesday 21 March 2012, David Faure escreveu:
> On Tuesday 20 March 2012 22:31:57 Lamarque V. Souza wrote:
> > WeatherEngine::~WeatherEngine() calls
> > WeatherEngine::unloadIons(), which tries to use the invalid
> > Plasma::DataEngineManager::self().
>
> Since DataEngineManager uses K_GLOBAL_STATIC internally, just use the
> isDestroyed() method to know if it has already been destroyed, before
> calling self().
I thought about doing that, but I would like to prevent an overhead that is needed only when plasma-{desktop,device} are exiting.
> More precisely, either add a isDestroyed method to the public class, which
> calls the one in the K_GLOBAL_STATIC, or let self() return 0 when
> privateDataEngineManagerSelf.isDestroyed().
The Plasma::DataEngineManager::valid() I suggested is exactly returning the negation of privateDataEngineManagerSelf.isDestroyed(). I just wanted to know if there were other options. Ok, let's add a Plasma::DataEngineManager::isDestroyed(). Can I backport that to 4.8? Wrong question, I will have to it to 4.8 so that somebody forward ports it to master, duh.
> On a more philosophical note: this is exactly why "intelligent destructors"
> are to be avoided at all costs. All this wouldn't happen if the
> WeatherEngine destructor didn't call methods that do stuff.
At least stuff with other object's pointer.
Lamarque V. Souza
Em Wednesday 21 March 2012, Aaron J. Seigo escreveu:
> On Tuesday, March 20, 2012 22:31:57 Lamarque V. Souza wrote:
> > There is a crash in WeatherEngine (kde-workspace) triggered by the fact
> >
> > that Plasma::DataEngineManager::self() (kdelib) is invalid when
> > plasma-{desktop,device} are exiting. WeatherEngine::~WeatherEngine()
> > calls WeatherEngine::unloadIons(), which tries to use the invalid
> > Plasma::DataEngineManager::self(). The crash only happens if there is
>
> this happens only when the application uncleanly exits. if you notice in
> the bug reports you linked to there was a problem elsewhere (e.g. an
> xioerror, an uncaught exception, etc.) and that caused an abort of the
> process with an unclean exit which then triggers this problem. the cause
> of the crash was never the WeatherEngine itself, but rather a crash in
> WeatherEngine was triggered while the application was otherwise closing
> down due to an error elsewhere that was itself bringing down the
> application.
a simple killall plasma-device suffices to make WeatherEngine hit the invalid Plasma::DataEngineManager::self(). I know that there is kquitapp, which I have just checked and it avoids the crash, but not everybody uses it.
> DataEngines created by DataEngineManager *must* be released prior to
> application exit. and normally this happens except in such cases where the
> application is brought down by an abnormal situation.
I do not consider a TERM signal an abnormal situation. Could we add a signal handler to plasma-{desktop,device} to re-route the TERM signal to the code kquitapp triggers in plasma-{desktop,device}?
> so while you can make the changes David suggests, it will only change the
> backtraces in those bug reports but not actually solve anything in the real
> world. the aborts will still happen as a result of the underlying error.
The crash does not happen if WeatherEngine's dtor is changed to do not use Plasma::DataEngineManager::self().
David Faure
> Em Wednesday 21 March 2012, David Faure escreveu:
> > On Tuesday 20 March 2012 22:31:57 Lamarque V. Souza wrote:
> > > WeatherEngine::~WeatherEngine() calls
> > > WeatherEngine::unloadIons(), which tries to use the invalid
> > > Plasma::DataEngineManager::self().
> >
> > Since DataEngineManager uses K_GLOBAL_STATIC internally, just use the
> > isDestroyed() method to know if it has already been destroyed, before
> > calling self().
>
> I thought about doing that, but I would like to prevent an overhead that
> is needed only when plasma-{desktop,device} are exiting.
Checking one bool can hardly be considered "overhead".
> > More precisely, either add a isDestroyed method to the public class, which
> > calls the one in the K_GLOBAL_STATIC, or let self() return 0 when
> > privateDataEngineManagerSelf.isDestroyed().
>
> The Plasma::DataEngineManager::valid() I suggested is exactly returning
> the negation of privateDataEngineManagerSelf.isDestroyed(). I just wanted to
> know if there were other options. Ok, let's add a
> Plasma::DataEngineManager::isDestroyed(). Can I backport that to 4.8? Wrong
> question, I will have to it to 4.8 so that somebody forward ports it to
> master, duh.
Yep.
> > On a more philosophical note: this is exactly why "intelligent
> > destructors"
> > are to be avoided at all costs. All this wouldn't happen if the
> > WeatherEngine destructor didn't call methods that do stuff.
>
> At least stuff with other object's pointer.
Yep.
Lamarque V. Souza
Just to make it clear. Signal TERM (kill -15, 15 is the default signal for the kill command) means "please, save your data and close yourself". It does not mean "abruptly killing the process", that is signal KILL. What I am asking for here is to support the Unix way of gently terminating a process and only that (support only signal TERM, not all other signals). The name "kill" for the command to send signals to process is misleading, not all signals mean something wrong happened. Signal SIGUSR1 for instance is commonly used to tell the process to re-read its configuration. Today we have dbus to send messages to processes, which I guess is what kquitapp uses, but not all programs support dbus, so I do not see why not support this Unix way of sending messages to a process.