kio slaves + kcrash + drkonqi

[David Faure]

Hi Harald,

On mardi 11 septembre 2018 14:25:15 CEST Harald Sitter wrote:
> The debugging faq [1] suggest that this should behave like KCrash. by
> default all crashes get drkonqi'd unless you set KDE_DEBUG at which
> point drkonqi is disabled. However, having quickly jumped through the
> kdelibs history I am not sure that was true even before the kf5 shake
> up...

Well, it used to be that *just* linking to KCrash would enable it.
The need to call initialize() is new in KF5.
So when KDE_DEBUG wasn't set, I'm pretty sure you would get drkonqi,
and when it was set, the point was to disable drkonqi anyway.

> More importantly, why would one bypass kcrash at all?

I personally disable drkonqi globally and look for core dumps instead,
so I can use gdb to e.g. print local variables.
 
> - SlaveBase calls KCrash::initialize()

Yes. That's what's missing.

> - SlaveBase drops the KDE_DEBUG check

If you want, I suppose KCrash itself will check for it anyway.

> - SlaveBase uses KCrash::setCrashHandler to set its own crash handler
> to print the backtrace on stderr

I'm not sure about this part.
1) that code itself might crash, especially if it has to allocate memory [more 
on that below]
2) such backtraces are often useless because they only contain exported 
symbols. Due to hidden visibility being enabled by default, everything else is 
"???" (the glibc backtrace functionality isn't as clever as gdb...).
When this code was written, we didn't have hidden visibility, so it was more 
useful.

How about just killing the KDE_DEBUG check *and* the code that prints out a 
backtrace?

> - Possibly KCRASH_AUTO_RESTARTED needs setting by something in KIO.
> From what I see, if I make a slave crash on every invocation, KIO
> would try to repeat the action it wanted to do by restarting the salve
> ad infinitum when instead it should abort at some point so as to not
> spam the user with drkonqis.

I don't see why KIO would try to repeat the action.
The app creates a job, it fails because the kioslave crashed, end of story.
There is no autorestart here.

Although, I can imagine that if you copy one directory with 1000 files and the 
kioslave only crashes in the file copy code, you'd get 1000 drkonqis.
But this seems a bit corner case and is all unrelated to this KF5 porting 
issue (the missing call to KCrash::initialize).

> The end result is that we still have the stderr backtrace, which I
> think is very useful. 

Did you test it? Was it really useful?

> In fact, by dropping the KDE_DEBUG condition
> it's always enabled even when the user disables drkonqi using
> KDE_DEBUG=1.

Either this is unsafe (because of memory allocations) and we shouldn't do it,
or if we think it's safe and useful, then as you say it should be done by 
kcrash. Conclusion, no, this code shouldn't be kept at all in SlaveBase...

More details on whether this is safe: the man page says

"backtrace() and backtrace_symbols_fd() don't call malloc() explicitly, but 
they are part of libgcc, which gets loaded dynamically when first used.  
Dynamic loading usually triggers a call to malloc(3).  If you need certain 
calls to these two functions to not allocate memory (in signal handlers, for 
example), you need to make sure libgcc is loaded beforehand."

I'm pretty sure we don't do that...

> (In fact, I'd even argue that maybe kcrash itself should do the
> backtrace() print. It seems useful in all crash scenarios I'd imagine,
> in particular consider drkonqi can be disabled or not installed. But
> then I don't know if/what performance penalty it'd entail with regards
> to auto restarts.)

My opinion is that a debugger is much better suited for getting useful 
backtraces (including parameter values, and the ability to inspect local 
variables).

-- 
David Faure, faure at kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5