Make Project::open() method use async KIO methods.
AbandonedPublic
Actions

Authored by arrowd on Feb 5 2019, 12:43 PM.

Details

Reviewers

Group Reviewers

Summary

In TestCore::shutdown() there is a qWait(1) line that causes signals from queued connections to arrive. At this point, however, the reciever object is deleted already.
Fix testReload testcase by waiting for these signals inside the test function.

Full ASan report:

PASS   : TestCMakeManager::testReload()
=================================================================
==8081==ERROR: AddressSanitizer: heap-use-after-free on address 0x6040006073e0 at pc 0x00080085b9f8 bp 0x7fffffffa8f0 sp 0x7fffffffa8e8
READ of size 8 at 0x6040006073e0 thread T0
    #0 0x80085b9f7 in KDevelop::StatusBar::showProgress(KDevelop::IStatus*, int, int, int) /home/arr/projects/kdevelop/kdevplatform/shell/statusbar.cpp:243:56
    #1 0x800b1683c in KDevelop::StatusBar::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) /home/arr/projects/kdevelop/build/kdevplatform/shell/KDevPlatformShell_autogen/EWIEGA46WW/moc_statusbar.cpp:115:21
    #2 0x804e51650 in QObject::event(QEvent*) (/usr/local/lib/qt5/libQt5Core.so.5+0x451650)
    #3 0x8040b755e in QWidget::event(QEvent*) (/usr/local/lib/qt5/libQt5Widgets.so.5+0x2b755e)
    #4 0x804213241 in QStatusBar::event(QEvent*) (/usr/local/lib/qt5/libQt5Widgets.so.5+0x413241)
    #5 0x80407dee0 in QApplicationPrivate::notify_helper(QObject*, QEvent*) (/usr/local/lib/qt5/libQt5Widgets.so.5+0x27dee0)
    #6 0x80407f27c in QApplication::notify(QObject*, QEvent*) (/usr/local/lib/qt5/libQt5Widgets.so.5+0x27f27c)
    #7 0x804e26ef0 in QCoreApplication::notifyInternal2(QObject*, QEvent*) (/usr/local/lib/qt5/libQt5Core.so.5+0x426ef0)
    #8 0x804e27db8 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) (/usr/local/lib/qt5/libQt5Core.so.5+0x427db8)
    #9 0x804e7c347  (/usr/local/lib/qt5/libQt5Core.so.5+0x47c347)
    #10 0x8069f0b76 in g_main_context_dispatch (/usr/local/lib/libglib-2.0.so.0+0xc9b76)
    #11 0x8069f0f02  (/usr/local/lib/libglib-2.0.so.0+0xc9f02)
    #12 0x8069f0fb3 in g_main_context_iteration (/usr/local/lib/libglib-2.0.so.0+0xc9fb3)
    #13 0x804e7bda5 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (/usr/local/lib/qt5/libQt5Core.so.5+0x47bda5)
    #14 0x804e22a3d in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) (/usr/local/lib/qt5/libQt5Core.so.5+0x422a3d)
    #15 0x803c9fbfd in KJob::exec() (/usr/local/lib/libKF5CoreAddons.so.5+0x5cbfd)
    #16 0x8007dbdc2 in KDevelop::ProjectPrivate::initProjectFiles() /home/arr/projects/kdevelop/kdevplatform/shell/project.cpp:229:24
    #17 0x8007d222f in KDevelop::Project::open(KDevelop::Path const&) /home/arr/projects/kdevelop/kdevplatform/shell/project.cpp:494:13
    #18 0x800798ae2 in KDevelop::ProjectControllerPrivate::importProject(QUrl const&) /home/arr/projects/kdevelop/kdevplatform/shell/projectcontroller.cpp:353:24
    #19 0x800786613 in KDevelop::ProjectController::openProject(QUrl const&) /home/arr/projects/kdevelop/kdevplatform/shell/projectcontroller.cpp:884:12
    #20 0x2d9a0e in loadProject(QString const&, QString const&) /home/arr/projects/kdevelop/plugins/cmake/tests/testhelpers.h:138:51
    #21 0x2edfbf in TestCMakeManager::testExecutableOutputPath() /home/arr/projects/kdevelop/plugins/cmake/tests/test_cmakemanager.cpp:381:25
    #22 0x30d3aa in TestCMakeManager::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) /home/arr/projects/kdevelop/build/plugins/cmake/tests/test_cmakemanager_autogen/EWIEGA46WW/moc_test_cmakemanager.cpp:153:22
    #23 0x804e320ce in QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const (/usr/local/lib/qt5/libQt5Core.so.5+0x4320ce)
    #24 0x8004611f8  (/usr/local/lib/qt5/libQt5Test.so.5+0x1e1f8)
    #25 0x800461bee  (/usr/local/lib/qt5/libQt5Test.so.5+0x1ebee)
    #26 0x800462988  (/usr/local/lib/qt5/libQt5Test.so.5+0x1f988)
    #27 0x80046317b in QTest::qRun() (/usr/local/lib/qt5/libQt5Test.so.5+0x2017b)
    #28 0x800462ef3 in QTest::qExec(QObject*, int, char**) (/usr/local/lib/qt5/libQt5Test.so.5+0x1fef3)
    #29 0x2da091 in main /home/arr/projects/kdevelop/plugins/cmake/tests/test_cmakemanager.cpp:39:1

0x6040006073e0 is located 16 bytes inside of 40-byte region [0x6040006073d0,0x6040006073f8)
freed by thread T0 here:
    #0 0x2d31f2 in operator delete(void*) /usr/src/contrib/compiler-rt/lib/asan/asan_new_delete.cc:149:3
    #1 0x8007cff31 in KDevelop::ProjectProgress::~ProjectProgress() /home/arr/projects/kdevelop/kdevplatform/shell/project.cpp:105:1
    #2 0x8007d0d42 in KDevelop::Project::~Project() /home/arr/projects/kdevelop/kdevplatform/shell/project.cpp:427:5
    #3 0x8007d0d88 in KDevelop::Project::~Project() /home/arr/projects/kdevelop/kdevplatform/shell/project.cpp:426:1
    #4 0x804e51461 in QObject::event(QEvent*) (/usr/local/lib/qt5/libQt5Core.so.5+0x451461)
    #5 0x80407dee0 in QApplicationPrivate::notify_helper(QObject*, QEvent*) (/usr/local/lib/qt5/libQt5Widgets.so.5+0x27dee0)
    #6 0x80407f27c in QApplication::notify(QObject*, QEvent*) (/usr/local/lib/qt5/libQt5Widgets.so.5+0x27f27c)
    #7 0x804e26ef0 in QCoreApplication::notifyInternal2(QObject*, QEvent*) (/usr/local/lib/qt5/libQt5Core.so.5+0x426ef0)
    #8 0x804e27db8 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) (/usr/local/lib/qt5/libQt5Core.so.5+0x427db8)
    #9 0x800461331  (/usr/local/lib/qt5/libQt5Test.so.5+0x1e331)
    #10 0x800461bee  (/usr/local/lib/qt5/libQt5Test.so.5+0x1ebee)
    #11 0x800462988  (/usr/local/lib/qt5/libQt5Test.so.5+0x1f988)
    #12 0x80046317b in QTest::qRun() (/usr/local/lib/qt5/libQt5Test.so.5+0x2017b)
    #13 0x800462ef3 in QTest::qExec(QObject*, int, char**) (/usr/local/lib/qt5/libQt5Test.so.5+0x1fef3)
    #14 0x2da091 in main /home/arr/projects/kdevelop/plugins/cmake/tests/test_cmakemanager.cpp:39:1

previously allocated by thread T0 here:
    #0 0x2d2612 in operator new(unsigned long) /usr/src/contrib/compiler-rt/lib/asan/asan_new_delete.cc:92:3
    #1 0x8007d0a57 in KDevelop::Project::Project(QObject*) /home/arr/projects/kdevelop/kdevplatform/shell/project.cpp:421:19
    #2 0x800798908 in KDevelop::ProjectControllerPrivate::importProject(QUrl const&) /home/arr/projects/kdevelop/kdevplatform/shell/projectcontroller.cpp:350:29
    #3 0x800786613 in KDevelop::ProjectController::openProject(QUrl const&) /home/arr/projects/kdevelop/kdevplatform/shell/projectcontroller.cpp:884:12
    #4 0x2d9a0e in loadProject(QString const&, QString const&) /home/arr/projects/kdevelop/plugins/cmake/tests/testhelpers.h:138:51
    #5 0x2ec539 in TestCMakeManager::testReload() /home/arr/projects/kdevelop/plugins/cmake/tests/test_cmakemanager.cpp:338:25
    #6 0x30d39c in TestCMakeManager::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) /home/arr/projects/kdevelop/build/plugins/cmake/tests/test_cmakemanager_autogen/EWIEGA46WW/moc_test_cmakemanager.cpp:152:22
    #7 0x804e320ce in QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const (/usr/local/lib/qt5/libQt5Core.so.5+0x4320ce)
    #8 0x8004611f8  (/usr/local/lib/qt5/libQt5Test.so.5+0x1e1f8)
    #9 0x800461bee  (/usr/local/lib/qt5/libQt5Test.so.5+0x1ebee)
    #10 0x800462988  (/usr/local/lib/qt5/libQt5Test.so.5+0x1f988)
    #11 0x80046317b in QTest::qRun() (/usr/local/lib/qt5/libQt5Test.so.5+0x2017b)
    #12 0x800462ef3 in QTest::qExec(QObject*, int, char**) (/usr/local/lib/qt5/libQt5Test.so.5+0x1fef3)
    #13 0x2da091 in main /home/arr/projects/kdevelop/plugins/cmake/tests/test_cmakemanager.cpp:39:1

Diff Detail

Repository

R32 KDevelop

Branch

asan_fix

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 7909
Build 7927: arc lint + arc unit

arrowd created this revision.Feb 5 2019, 12:43 PM

Restricted Application added a project: KDevelop. · View Herald TranscriptFeb 5 2019, 12:43 PM

Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript

arrowd requested review of this revision.Feb 5 2019, 12:43 PM

Harbormaster completed remote builds in B7909: Diff 50948.Feb 5 2019, 12:43 PM

arrowd added a reviewer: KDevelop.Feb 8 2019, 1:28 PM

I'm afraid to say that this is just a workaround, not a proper fix... I ran into this issue recently too - the real problem - imo - is that we synchronously execute the stat jobs in KDevelop::ProjectPrivate::initProjectFiles... if you look at the ASAN report you'll see that TestCore::shutdown isn't referenced there at all!

it's not really an option to add random waits to the tests in the hope to workaround the issue everywhere. Rather, we should try to remove such nested eventloops somehow, even if that means refactoring the codebase...

This revision now requires changes to proceed.Feb 10 2019, 7:04 PM

I have little Qt experience, why nested event loops are bad? What are options to avoid them?

I'm happy to invest some time into it, but need a bit of guidance.

the proper way would be to have Project::open return void, and instead report failure via a new 'failedToOpen' signal. And then internally handle the async code without a nested event loop. We'll probably need this in more places, and I wonder what the current best-practice is for that. Eventually async/await can be used, but we want to have something that works with the compilers we support today. Hm. I'd like to prevent us from trying to handle async code in an adhoc fashion. Rather, we should try to adopt a proper framework like e.g. KAsync? QtPromise? Something similar?

The problem with nested event loops are that they introduce execution flows that are very hard to reason about and can easily cause issues like the one you see here. Quite often, you can run into issues where an object spawns a nested eventloop, which then somehow handles a deleteLater event for the object itself, thereby destroying the object. When the nested eventloop exits, this was destroyed and anything can happen. I believe something like that happens here too.

KAsync looks a bit unmaintained. On the other hand, QtPromise seems to be an active project. Should I start looking into the latter?

yes, QtPromise or AsyncFuture (https://github.com/benlau/asyncfuture) could be used - I wouldn't be opposed to introducing it as a thirdparty dependency (or git submodule)

@brauch, @kfunk, @apol what do you think? we could then leverage that new utility in quite a few places where we currently deal with async code in an ad-hoc fashion

As long as it integrates well with the rest of the KDevelop build I'm fine. Would probably just copy the QtPromise source tree into maybe kdevelop.git:3rdparty/ and create a CMakeLists.txt which creates a STATIC or OBJECT library for it? QtPromise looks easy enough to build.

After finishing initial implementation I found out that QtPromise requires exceptions enabled. KDevelop seem to have exceptions disabled right now.

Oh, really? Hmm! I wouldn't be opposed to enable compilation with exceptions myself, what do the others say? We don't need to use them excessively, but for error handling in async promise chains, that would be quite useful I think?

In D18758#449319, @mwolff wrote:

Oh, really? Hmm! I wouldn't be opposed to enable compilation with exceptions myself, what do the others say? We don't need to use them excessively, but for error handling in async promise chains, that would be quite useful I think?

Yep, this is what I'm using to make a QPromise "fail".

In D18758#449319, @mwolff wrote:

Oh, really? Hmm! I wouldn't be opposed to enable compilation with exceptions myself, what do the others say? We don't need to use them excessively, but for error handling in async promise chains, that would be quite useful I think?

That would mean adding a bunch of noexcept all over the place or risk quite some performance penalty. I'd prefer keeping it localised.

In D18758#450081, @apol wrote:

In D18758#449319, @mwolff wrote:

Oh, really? Hmm! I wouldn't be opposed to enable compilation with exceptions myself, what do the others say? We don't need to use them excessively, but for error handling in async promise chains, that would be quite useful I think?

That would mean adding a bunch of noexcept all over the place or risk quite some performance penalty. I'd prefer keeping it localised.

We only get bigger binaries - and the impact is left to be seen. That said, I also would prefer to keep it localized. We could probably get away with only enabling exceptions wherever QtPromise is being used, so make it an interface property of that target in cmake?

Changes:

Import QtPromise 0.5.0 into kdevplatform/3dparty/ directory.
Remove synchronious KIO calls from Project::open() and use QPromises there instead.

Current problems are:

The code looks messy. I'm probably missing how to handle these QPromises properly, without so much boilerplate code.
The change didn't actually help to fix the initial problem. Moreover, the test now crashes even without ASan on master, which probably means that KDevelop is unusable on FreeBSD ATM.

ASan report looks pretty much the same, only source lines changed:

PASS   : TestCMakeManager::initTestCase()
PASS   : TestCMakeManager::testReload()
=================================================================
==89230==ERROR: AddressSanitizer: heap-use-after-free on address 0x604000443b60 at pc 0x000800ae7dd0 bp 0x7fffffffc130 sp 0x7fffffffc128
READ of size 8 at 0x604000443b60 thread T0
    #0 0x800ae7dcf in KDevelop::StatusBar::showProgress(KDevelop::IStatus*, int, int, int) /home/arr/projects/kdevelop/kdevplatform/shell/statusbar.cpp:243:56
    #1 0x800824e3f in KDevelop::StatusBar::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) /home/arr/projects/kdevelop/build/kdevplatform/shell/KDevPlatformShell_autogen/EWIEGA46WW/moc_statusbar.cpp:115:21
    #2 0x8056666a9 in QMetaCallEvent::placeMetaCall(QObject*) /wrkdirs/usr/ports/devel/qt5-core/work/qtbase-everywhere-src-5.12.2/src/corelib/kernel/qobject.cpp:520:9
    #3 0x805668248 in QObject::event(QEvent*) /wrkdirs/usr/ports/devel/qt5-core/work/qtbase-everywhere-src-5.12.2/src/corelib/kernel/qobject.cpp:1260:18
    #4 0x804378f50 in QWidget::event(QEvent*) /wrkdirs/usr/ports/x11-toolkits/qt5-widgets/work/qtbase-everywhere-src-5.12.2/src/widgets/kernel/qwidget.cpp:9383:25
    #5 0x8045b0a71 in QStatusBar::event(QEvent*) /wrkdirs/usr/ports/x11-toolkits/qt5-widgets/work/qtbase-everywhere-src-5.12.2/src/widgets/widgets/qstatusbar.cpp:753:21
    #6 0x8043184de in QApplicationPrivate::notify_helper(QObject*, QEvent*) /wrkdirs/usr/ports/x11-toolkits/qt5-widgets/work/qtbase-everywhere-src-5.12.2/src/widgets/kernel/qapplication.cpp:3736:37
    #7 0x80431d5d6 in QApplication::notify(QObject*, QEvent*) /wrkdirs/usr/ports/x11-toolkits/qt5-widgets/work/qtbase-everywhere-src-5.12.2/src/widgets/kernel/qapplication.cpp:3687:18
    #8 0x80561640d in QCoreApplication::notifyInternal2(QObject*, QEvent*) /wrkdirs/usr/ports/devel/qt5-core/work/qtbase-everywhere-src-5.12.2/src/corelib/kernel/qcoreapplication.cpp:1060:18
    #9 0x805617047 in QCoreApplication::sendEvent(QObject*, QEvent*) /wrkdirs/usr/ports/devel/qt5-core/work/qtbase-everywhere-src-5.12.2/src/corelib/kernel/qcoreapplication.cpp:1450:12
    #10 0x805617ccd in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) /wrkdirs/usr/ports/devel/qt5-core/work/qtbase-everywhere-src-5.12.2/src/corelib/kernel/qcoreapplication.cpp:1799:9
    #11 0x805616f4e in QCoreApplication::sendPostedEvents(QObject*, int) /wrkdirs/usr/ports/devel/qt5-core/work/qtbase-everywhere-src-5.12.2/src/corelib/kernel/qcoreapplication.cpp:1653:5
    #12 0x8056c630d in postEventSourceDispatch(_GSource*, int (*)(void*), void*) /wrkdirs/usr/ports/devel/qt5-core/work/qtbase-everywhere-src-5.12.2/src/corelib/kernel/qeventdispatcher_glib.cpp:276:5
    #13 0x80714d3b6 in g_main_context_dispatch (/usr/local/lib/libglib-2.0.so.0+0xce3b6)
    #14 0x80714d742  (/usr/local/lib/libglib-2.0.so.0+0xce742)
    #15 0x80714d7f3 in g_main_context_iteration (/usr/local/lib/libglib-2.0.so.0+0xce7f3)
    #16 0x8056c5484 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) /wrkdirs/usr/ports/devel/qt5-core/work/qtbase-everywhere-src-5.12.2/src/corelib/kernel/qeventdispatcher_glib.cpp:422:19
    #17 0x805616ba0 in QCoreApplication::processEvents(QFlags<QEventLoop::ProcessEventsFlag>, int) /wrkdirs/usr/ports/devel/qt5-core/work/qtbase-everywhere-src-5.12.2/src/corelib/kernel/qcoreapplication.cpp:1308:42
    #18 0x8056bb7ff in QTest::qWait(int) /wrkdirs/usr/ports/devel/qt5-core/work/qtbase-everywhere-src-5.12.2/src/corelib/kernel/qtestsupport_core.cpp:104:9
    #19 0x8004018dc in KDevelop::TestCore::shutdown() /home/arr/projects/kdevelop/kdevplatform/tests/testcore.cpp:94:9
    #20 0x3099a0 in TestCMakeManager::cleanupTestCase() /home/arr/projects/kdevelop/plugins/cmake/tests/test_cmakemanager.cpp:55:5
    #21 0x302e5c in TestCMakeManager::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) /home/arr/projects/kdevelop/build/plugins/cmake/tests/test_cmakemanager_autogen/EWIEGA46WW/moc_test_cmakemanager.cpp:136:21
    #22 0x805625ed8 in QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const /wrkdirs/usr/ports/devel/qt5-core/work/qtbase-everywhere-src-5.12.2/src/corelib/kernel/qmetaobject.cpp:2295:13
    #23 0x800495a9b  (/usr/local/lib/qt5/libQt5Test.so.5+0x1ea9b)
    #24 0x8004962db in QTest::qRun() (/usr/local/lib/qt5/libQt5Test.so.5+0x1f2db)
    #25 0x800495ed3 in QTest::qExec(QObject*, int, char**) (/usr/local/lib/qt5/libQt5Test.so.5+0x1eed3)
    #26 0x308abd in main /home/arr/projects/kdevelop/plugins/cmake/tests/test_cmakemanager.cpp:39:1
    #27 0x25a10e in _start /usr/src/lib/csu/amd64/crt1.c:76:7

0x604000443b60 is located 16 bytes inside of 40-byte region [0x604000443b50,0x604000443b78)
freed by thread T0 here:
    #0 0x301822 in operator delete(void*) /usr/src/contrib/compiler-rt/lib/asan/asan_new_delete.cc:167:3
    #1 0x8009e5b41 in KDevelop::ProjectProgress::~ProjectProgress() /home/arr/projects/kdevelop/kdevplatform/shell/project.cpp:108:1
    #2 0x8009e6c22 in KDevelop::Project::~Project() /home/arr/projects/kdevelop/kdevplatform/shell/project.cpp:466:5
    #3 0x8009e6c68 in KDevelop::Project::~Project() /home/arr/projects/kdevelop/kdevplatform/shell/project.cpp:465:1
    #4 0x80566861a in qDeleteInEventHandler(QObject*) /wrkdirs/usr/ports/devel/qt5-core/work/qtbase-everywhere-src-5.12.2/src/corelib/kernel/qobject.cpp:4647:5
    #5 0x8056681d1 in QObject::event(QEvent*) /wrkdirs/usr/ports/devel/qt5-core/work/qtbase-everywhere-src-5.12.2/src/corelib/kernel/qobject.cpp:1251:9
    #6 0x8043184de in QApplicationPrivate::notify_helper(QObject*, QEvent*) /wrkdirs/usr/ports/x11-toolkits/qt5-widgets/work/qtbase-everywhere-src-5.12.2/src/widgets/kernel/qapplication.cpp:3736:37
    #7 0x80431a59b in QApplication::notify(QObject*, QEvent*) /wrkdirs/usr/ports/x11-toolkits/qt5-widgets/work/qtbase-everywhere-src-5.12.2/src/widgets/kernel/qapplication.cpp:3093:18
    #8 0x80561640d in QCoreApplication::notifyInternal2(QObject*, QEvent*) /wrkdirs/usr/ports/devel/qt5-core/work/qtbase-everywhere-src-5.12.2/src/corelib/kernel/qcoreapplication.cpp:1060:18
    #9 0x805617047 in QCoreApplication::sendEvent(QObject*, QEvent*) /wrkdirs/usr/ports/devel/qt5-core/work/qtbase-everywhere-src-5.12.2/src/corelib/kernel/qcoreapplication.cpp:1450:12
    #10 0x805617ccd in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) /wrkdirs/usr/ports/devel/qt5-core/work/qtbase-everywhere-src-5.12.2/src/corelib/kernel/qcoreapplication.cpp:1799:9
    #11 0x805616f4e in QCoreApplication::sendPostedEvents(QObject*, int) /wrkdirs/usr/ports/devel/qt5-core/work/qtbase-everywhere-src-5.12.2/src/corelib/kernel/qcoreapplication.cpp:1653:5
    #12 0x800494375  (/usr/local/lib/qt5/libQt5Test.so.5+0x1d375)
    #13 0x800494be5  (/usr/local/lib/qt5/libQt5Test.so.5+0x1dbe5)
    #14 0x800495976  (/usr/local/lib/qt5/libQt5Test.so.5+0x1e976)
    #15 0x8004962db in QTest::qRun() (/usr/local/lib/qt5/libQt5Test.so.5+0x1f2db)
    #16 0x800495ed3 in QTest::qExec(QObject*, int, char**) (/usr/local/lib/qt5/libQt5Test.so.5+0x1eed3)
    #17 0x308abd in main /home/arr/projects/kdevelop/plugins/cmake/tests/test_cmakemanager.cpp:39:1
    #18 0x25a10e in _start /usr/src/lib/csu/amd64/crt1.c:76:7
    #19 0x8003a0fff  (<unknown module>)

previously allocated by thread T0 here:
    #0 0x300be2 in operator new(unsigned long) /usr/src/contrib/compiler-rt/lib/asan/asan_new_delete.cc:106:3
    #1 0x8009e6889 in KDevelop::Project::Project(QObject*) /home/arr/projects/kdevelop/kdevplatform/shell/project.cpp:460:19
    #2 0x800998238 in KDevelop::ProjectControllerPrivate::importProject(QUrl const&) /home/arr/projects/kdevelop/kdevplatform/shell/projectcontroller.cpp:345:29
    #3 0x8009813d2 in KDevelop::ProjectController::openProject(QUrl const&) /home/arr/projects/kdevelop/kdevplatform/shell/projectcontroller.cpp:879:12
    #4 0x308439 in loadProject(QString const&, QString const&) /home/arr/projects/kdevelop/plugins/cmake/tests/testhelpers.h:138:51
    #5 0x31b1d2 in TestCMakeManager::testReload() /home/arr/projects/kdevelop/plugins/cmake/tests/test_cmakemanager.cpp:338:25
    #6 0x302f3c in TestCMakeManager::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) /home/arr/projects/kdevelop/build/plugins/cmake/tests/test_cmakemanager_autogen/EWIEGA46WW/moc_test_cmakemanager.cpp:152:22
    #7 0x805625ed8 in QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const /wrkdirs/usr/ports/devel/qt5-core/work/qtbase-everywhere-src-5.12.2/src/corelib/kernel/qmetaobject.cpp:2295:13
    #8 0x800494242  (/usr/local/lib/qt5/libQt5Test.so.5+0x1d242)
    #9 0x800494be5  (/usr/local/lib/qt5/libQt5Test.so.5+0x1dbe5)
    #10 0x800495976  (/usr/local/lib/qt5/libQt5Test.so.5+0x1e976)
    #11 0x8004962db in QTest::qRun() (/usr/local/lib/qt5/libQt5Test.so.5+0x1f2db)
    #12 0x800495ed3 in QTest::qExec(QObject*, int, char**) (/usr/local/lib/qt5/libQt5Test.so.5+0x1eed3)
    #13 0x308abd in main /home/arr/projects/kdevelop/plugins/cmake/tests/test_cmakemanager.cpp:39:1
    #14 0x25a10e in _start /usr/src/lib/csu/amd64/crt1.c:76:7
    #15 0x8003a0fff  (<unknown module>)

SUMMARY: AddressSanitizer: heap-use-after-free /home/arr/projects/kdevelop/kdevplatform/shell/statusbar.cpp:243:56 in KDevelop::StatusBar::showProgress(KDevelop::IStatus*, int, int, int)

Harbormaster completed remote builds in B12012: Diff 58398.May 21 2019, 10:12 AM

Can't we really fix this without bringing a dependency?
If we really want the dependency, can we just link against it instead of dragging it into our source code?

I'm more interested in fixing the intial issue. With KIO jobs in openProject turned async, the crash now happens right in qWait(1) of KDevelop::TestCore::shutdown(). And what's strange is that adding qWait(1) at the end of testReload() test still fixes issue.

Maybe the bug is in TestCore itself?

Quite possibly, it doesn't happen outside of tests after all, right?

In D18758#469623, @apol wrote:

Quite possibly, it doesn't happen outside of tests after all, right?

Yes, clicking "Reload" in a context menu for a CMake project doesn't trigger this bug. Not sure if the test does the same, though.

ok, sorry for the rabbit hole I sent you down. I still think that long-term we will need something like QPromise, but phabricator doesn't even let me view the interesting changes in shell/... so I cannot comment on the boilerplate

I'll have another look at this issue now and see if I can find an alternative route...

so, I've now committed an alternative fix (or so I hope...) see:

commit bd048e67f056b5be25ed57fb2be947444f68c24e
Author: Milian Wolff <mail@milianw.de>
Date:   Mon Jun 17 22:26:32 2019 +0200

    Guard against crashes when IStatus object gets destroyed at bad times

I'm very sorry to have wasted your time with QPromise as a fix for this issue. But I'll have a look at your raw patch if I can convince phab somehow to show it to me. It may be valuable in the future. I still think that KDevelop should make better use of async code and not rely on nested event loops that much... A lot of work though :(

having looked at the raw diff quickly, I like what I'm seeing. What boilerplate are you referring to?

I actually wouldn't mind getting this patch into master and building on top of that. But if the others dislike that, then I'm OK with that too obviously. I sadly don't have the time for kdev anymore that would be required to drive this forward...

R32:bd048e67f056b5be25ed57fb2be947444f68c24e

In D18758#481339, @mwolff wrote:
so, I've now committed an alternative fix (or so I hope...) see:
commit bd048e67f056b5be25ed57fb2be947444f68c24e
Author: Milian Wolff <mail@milianw.de>
Date:   Mon Jun 17 22:26:32 2019 +0200

    Guard against crashes when IStatus object gets destroyed at bad times

I confirm this fixes the issue for me. Yay, thanks!

having looked at the raw diff quickly, I like what I'm seeing. What boilerplate are you referring to?

At project.cpp:282, mostly. This construct looks awful to me:

            });
            return retPromise;
        }
        return QtPromise::resolve();
    });

    return retPromise;
}
return QtPromise::resolve();

And it gets only worser with more indirection. In first version of this patch I made a mistake here, forgot a return statement, and this made QtPromise silently not to resolve inner future. Coming from Haskell, I was thinking there should be better way to handle nested futures.

In D18758#481378, @arrowd wrote:
R32:bd048e67f056b5be25ed57fb2be947444f68c24e
In D18758#481339, @mwolff wrote:
so, I've now committed an alternative fix (or so I hope...) see:
commit bd048e67f056b5be25ed57fb2be947444f68c24e
Author: Milian Wolff <mail@milianw.de>
Date:   Mon Jun 17 22:26:32 2019 +0200

    Guard against crashes when IStatus object gets destroyed at bad times
I confirm this fixes the issue for me. Yay, thanks!

Awesome, thanks a lot for checking!

having looked at the raw diff quickly, I like what I'm seeing. What boilerplate are you referring to?

At project.cpp:282, mostly. This construct looks awful to me:
            });
            return retPromise;
        }
        return QtPromise::resolve();
    });
 
    return retPromise;
}
return QtPromise::resolve();
And it gets only worser with more indirection. In first version of this patch I made a mistake here, forgot a return statement, and this made QtPromise silently not to resolve inner future. Coming from Haskell, I was thinking there should be better way to handle nested futures.

Two things I want to mention here:

First, if you compare this code to any alternative that isn't based on promises, you should hopefully agree that it's still way more readable than any alternative. At least that's my current gut feeling, but I haven't yet worked extensively with QtPromise.

Secondly, some of the boiler plate could potentially be simplified if we add a helper function that takes a KJob, wraps it in a promise and starts the job, but also connects to the error signal to fail the promise? I don't think it will help the "branching" in the promise chain, but at least it should simplify the code a bit to make it clear what everything is doing. Something like the following pseudo code:

startJob(stat project file)
   .fail(show error dialog)
   .then(return startJob(stat developer file))
   .fail(return startJob(stat developer dir))
   .fail(return startJob(create developer dir))
   .then(return startJob(copy project file))
   .fail(show error dialog)
   .then(return startJob(copy developer file))

I'm not sure chaining works like this, so potentially some of this needs to be branched for the error handling, yet overall it should be quite OK and not too much different from the non-async code I believe? Food for thought mostly, we first need to figure out whether we want this at all or not...

arrowd retitled this revision from Fix ASan error in test_cmakemanager by catching all signals before objects they are delivered to are deleted. to Make Project::open() method use async KIO methods..Sep 26 2019, 10:27 AM

So, do we want to get this in?

Short recap:

Project::open() executes KIO jobs synchronously at the moment.
To make the code asynchronous we need some 3rd-party library implementing futures, like QtPromise.
The original bug that made me work on this is fixed already.

Should we abandon this?

In D18758#678706, @arrowd wrote:

So, do we want to get this in?

Short recap:

Project::open() executes KIO jobs synchronously at the moment.

To make the code asynchronous we need some 3rd-party library implementing futures, like QtPromise.

The original bug that made me work on this is fixed already.

Should we abandon this?

Let's abandon it, I'd be wary of bringing in such a big dependency into our codebase just like that.

In D18758#678707, @apol wrote:

Let's abandon it, I'd be wary of bringing in such a big dependency into our codebase just like that.

Yeah, I was thinking the same.

Revision Contents
Changeset List

			Path	Packages
M			plugins/cmake/tests/test_cmakemanager.cpp (3 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	50948	fbb955c		Feb 5 2019, 12:43 PM	★	★
Diff 2	58398	f3e26e2	Changes:	May 21 2019, 10:11 AM	★	★

Commit	Tree	Parents	Author	Summary	Date
e54793b8d05d	9e32b0dbfbd8	fbb955c772ec	Gleb Popov	Fix ASan error in test_cmakemanager by catching all signals before objects they… (Show More…)	Feb 5 2019, 12:37 PM

Diff 50948

View Options

plugins/cmake/tests/test_cmakemanager.cpp