Review Request: Introduce two new KCrash methods: void setLaunchDrKonqi(bool) and bool launchDrKonqi().

[George Kiagiadakis]

> On 2009-10-25 20:45:48, Oswald Buddenhagen wrote:
> > /trunk/KDE/kdelibs/kdeui/util/kcrash.h, line 118
> > <http://reviewboard.kde.org/r/1970/diff/1/?file=13307#file13307line118>
> >
> >     this should mention that it will auto-install the default crash handler like the setEmergencySaveFunction() doc does.
> >     
> >     additionally, you could add a doc to defaultCrashHandler() that it is the provider of the other features, so the setters will have no effect whatsovever if it is not called directly or indirectly.

FYI: I have another patch lying around to improve the documentation of the other methods:
https://gkiagia.homelinux.net/git/?p=kdelibs.git;a=commitdiff;h=d8e51f65ee1616baf3816fdfff2418a43b840246


- George


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1970/#review2805
-----------------------------------------------------------


On 2009-10-26 09:37:06, George Kiagiadakis wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1970/
> -----------------------------------------------------------
> 
> (Updated 2009-10-26 09:37:06)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> This patch introduces two new methods in the KCrash namespace: void setLaunchDrKonqi(bool) and bool launchDrKonqi(). The reason for introducing these functions is simple. At the moment, the KCrash::setCrashHandler function enables launching drkonqi on a crash if its argument is the default crash handler, but the setEmergencySaveFunction() and setFlags(AutoRestart) methods disable it completely if it was not previously enabled, so for example, the following code both launches drkonqi and calls the emergency save function on a crash:
> 
>     KCrash::setCrashHandler(KCrash::defaultCrashHandler);
>     KCrash::setEmergencySaveFunction(myfunc);
> 
> ...but the following code only calls the emergency save function on a crash:
> 
>     KCrash::setEmergencySaveFunction(myfunc);
>     KCrash::setCrashHandler(KCrash::defaultCrashHandler);
> 
> This happens because setEmergencySaveFunction will set the internal s_launchDrKonqi variable to false if it finds that no crash handler has been already installed and after that, there is no way to make this variable true again. By separating the function of enabling/disabling drkonqi from the function of setting the crash handler (which is a completely different thing...), we have more control over the behavior of KCrash and we eliminate weird behavior like in the above example.
> 
> Note that with this patch, the behavior of the KCrash API changes a little bit. Now, if someone wants to enable launching drkonqi for his application's crashes without using KApplication, he has to use KCrash::setLaunchDrKonqi(true) and not just KCrash::setCrashHandler(KCrash::defaultCrashHandler). Setting the crash handler to the default one no more has any visible effect if none of its functionality is explicitly enabled. Fortunately, by looking at lxr.kde.org, it seems that in our svn there is only one application that uses setCrashHandler() with the semantic of enabling drkonqi and that is the kdm greeter, which I can easily patch when this patch is in kdelibs.
> 
> The real reason that motivated me to write this patch is drkonqi itself. As it grows more complex with each kde release, it is possible that it crashes just like any other kde application. Currently, catching crashes of drkonqi with drkonqi is disabled, but I want to enable it in a safe way. My algorithm involves the following steps:
> 1) set the KDE_DEBUG environment variable to true so that KApplication does not set the crash handler.
> 2) initialize KApplication and the drkonqi backend that will give us information about the crashed application. This involves setting an emergency save function that will kill the crashed app if drkonqi crashes, as drkonqi keeps the crashed app in a stopped state (SIGSTOP) while it is running. If drkonqi crashes, there is nobody that will continue/kill the crashed app, so this emergency save function is necessary.
> 3) now that the backend is functional and can give us information about the crashed app, if the app is *not* drkonqi itself, set the crash handler so that drkonqi can catch its own crash. This step ensures that drkonqi will not start spawning itself recursively if for instance it crashes at startup for some random reason, but it will still enable us to catch some rare crash in some specific function.
> 
> With the current KCrash API, it is impossible to implement this algorithm, unless steps 2 and 3 are reversed, but that breaks the whole backend-based design, which I'd rather not do. Of course, I didn't write this patch only for supporting this functionality in drkonqi. I think it is a nice improvement to this API, since the current one behaves weird, as demonstrated above.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/kdeui/kernel/kapplication.cpp 1040176 
>   /trunk/KDE/kdelibs/kdeui/util/kcrash.h 1040176 
>   /trunk/KDE/kdelibs/kdeui/util/kcrash.cpp 1040176 
> 
> Diff: http://reviewboard.kde.org/r/1970/diff
> 
> 
> Testing
> -------
> 
> Application crashes are still handled fine with drkonqi without changing the applications and drkonqi can now catch its own crashes just fine.
> 
> 
> Thanks,
> 
> George
> 
>