kio slaves + kcrash + drkonqi

[Harald Sitter]

On Sun, Sep 16, 2018 at 10:48 AM David Faure <faure at kde.org> wrote:
> How about just killing the KDE_DEBUG check *and* the code that prints out a
> backtrace?

I'm always in favor of less code, so I'll prepare a diff for this.

> > - 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).

I had an assert fail in GetFileSystemFreeSpace which caused a drkonqi
to repeatedly start, but with what I've learned since then I think
that was caused by dolphin repeatedly trying to get the info. This
isn't something restart handling would or could prevent. With that in
mind this type of issue likely needs solving in drkonqi, i.e. I think
the problem is with the presentation of the repeated crashing more
than the crashing itself.

> > 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?

Well, it was more useful than no backtrace at all, but as you say the
actual symbol names are useless as far as the salve is concerned.

> > 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...

I've had a look and indeed I can not find any explicit call to make
sure libgcc is loaded.

> > (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).

Yes I agree.

So, the new plan is to remove the handler code from SlaveBase and
instead initialize KCrash. backtrace() code goes away entirely.
If someone wants the backtrace() feature it should be moved to KCrash
and before setting the crash handler libgcc needs to be explicitly
lazy-loaded with a call to any libgcc function. I would suggest people
check out coredumpd or another core handler though.

HS