This breaks the logic for the KDBusService crash fix. If this (using unique_ptr)
needs to be done it needs to carefully release() and reset() at every step, and
then it's just easier to manually delete it where appropriate IMHO.

Hi! Sorry, could you please elaborate why does it break the logic? Behavior of the code before and after the change should be the same because unique_ptr calls delete app upon going out of scope.

I've examined the code, and the only thing I can imagine that could be different is if app destructor needs to be called in specific order with regard to some other object. But in this case offhand more appropriate fix would be to create a struct-wrapper, which would wrap these objects, and later then destroy them in the exact order needed.

Make global unique_ptr* to app then in deleteQApplication use app->reset().

I don't have a problem w/ reverting this. @kkharlamov if you want to try again, please make sure you add the other people here for their comments on it.

I don't have a problem w/ reverting this. @kkharlamov if you want to try again, please make sure you add the other people here for their comments on it.

It looks to me as if everyone knows something I don't. @anthonyfieroni @hindenburg could please somebody answer this comment?

In D26764#596862, @kkharlamov wrote:

Hi! Sorry, could you please elaborate why does it break the logic? Behavior of the code before and after the change should be the same because unique_ptr calls delete app upon going out of scope.

https://phabricator.kde.org/source/konsole/browse/master/src/main.cpp$172-175
KDBusService call exit then deleteQApplication is called qApp is https://phabricator.kde.org/source/konsole/browse/master/src/main.cpp$112 so it's deleted twice.

atexit([&app]() {
    app.reset();
});

Could be better approach, then remove deleteQApplication and needToDeleteQApplication.

In D26764#600319, @anthonyfieroni wrote:

atexit([&app]() {
    app.reset();
});

Could be better approach, then remove deleteQApplication and needToDeleteQApplication.

Thanks! Note that unfortunately in C++ lambda-closures can not be converted to a raw function pointer, and atexit is an old C-like function that can't accept lambdas.

However, looking at docs for atexit, it seems the best way to work around that situation is to make app inside the main function a static variable. Per docs, on exit call destructors for static objects are called. I gotta experiment with it though, and to look through the code to make sure everything is okay. Hopefully, I'll craft up an MR in under an hour, and will put a link to it here in comments.

Makes me wonder though: I found that whole KDBus workaround was introduced at commit fe334292b. In particular, it introduced the comment about allocating qApp on the heap for KDBus workaround.

Until now this comment sounded to me a bit magical, as if having it on the heap works around some KDBus bug. But now I figured, the only reason it was put on the heap is to be able to destruct qApp on exit.

But if I make qApp a static variable, the exit() call gonna call destructor either way (I've tested this claim). This means I can simplify the code by downgrading the variable to be stored on the stack.

Ultimately, all I need is to revert the commit fe334292b that first introduced this workaround, and in the same commit replace QApplication app(argc, argv); with static QApplication app(argc, argv);

Okay, so, this seems not to be so simple as it appears to be. While making the variable static does help with destructing it on exit() call, however it causes some other crash when konsole exits. I can only guess.

Since fixing a crash is an urgent thing, and I don't have time right now to delve into why declaring QApplication as static causes another crash, I'm okay with reverting the commit as temporary workaround. @sandsmark I think though, to make sure nobody gonna commit such code again, it would help to add a comment just above auto app = new QApplication(argc, argv);, explaining why it's there. E.g. // allocate it on the heap so we can destroy it on exit() for the KDBus workaround.

And then, the following comment is confusing, it was the sole reason nobody noticed anyting wrong when RAII refactor happened. All it actually says is simply "we've used new to allocate the variable, so let's not forget to call delete". It's better to get rid of it entirely:

// Since we've allocated the QApplication on the heap for the KDBusService workaround,
// we need to delete it manually before returning from main().

Although, if somebody has motivation/time for this, I'd sure prefer an approach with reverting the whole workaround thing and just decalring the app as static. That really should be enough, no of this boilerplate code is needed.

Actually remove all atexit usage, it does not needed at all
https://en.cppreference.com/w/cpp/utility/program/exit

The destructors of objects with thread local storage duration that are associated with the current thread, the destructors of objects with static storage duration, and the functions registered with std::atexit are executed concurrently, while maintaining the following guarantees:

In D26764#600607, @anthonyfieroni wrote:

Actually remove all atexit usage, it does not needed at all
https://en.cppreference.com/w/cpp/utility/program/exit

The destructors of objects with thread local storage duration that are associated with the current thread, the destructors of objects with static storage duration, and the functions registered with std::atexit are executed concurrently, while maintaining the following guarantees:

Right, but as I mentioned doing that causes another crash. Stacktrace:

#0  0x0000000000000018 in ?? ()
#1  0x00007ffff78cd1d6 in ?? () from /usr/lib/libKF5GlobalAccel.so.5
#2  0x00007ffff5cb8792 in qt_call_post_routines() () from /usr/lib/libQt5Core.so.5
#3  0x00007ffff67f5b37 in QApplication::~QApplication() () from /usr/lib/libQt5Widgets.so.5
#4  0x00007ffff553d6a7 in __run_exit_handlers () from /usr/lib/libc.so.6
#5  0x00007ffff553d85e in exit () from /usr/lib/libc.so.6
#6  0x00007ffff552615a in __libc_start_main () from /usr/lib/libc.so.6
#7  0x000055555555506e in _start ()

Btw, if somebody wants to make it work, I pushed the code here. If you make it work, you can take ownership over the changes too, I don't mind.

Is there a consensus here? Should we revert to previous code and then if someone wants to put in another merge request.

In D26764#602941, @hindenburg wrote:

Is there a consensus here? Should we revert to previous code and then if someone wants to put in another merge request.

Well, I personally agreed to this a few comments above.