From 141cbf887026835d83c3763f920daed3276b674e Mon Sep 17 00:00:00 2001 From: sudden6 Date: Mon, 20 Jan 2020 23:19:08 +0100 Subject: [PATCH] fix(coreav): fix assert because c-toxcore calls from unexpected thread c-toxcore calls the groupCallCallback from it's main thread instead of the ToxAV thread as expected, this was triggering an assertion. Aditionally the destructors of Core and CoreAV were fixed, because they now either crashed or deadlocked qTox when it was closed while a group call was still running. --- src/core/core.cpp | 8 +++++--- src/core/coreav.cpp | 16 +++++++++++++++- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/core/core.cpp b/src/core/core.cpp index 438bb5d12..34a10f053 100644 --- a/src/core/core.cpp +++ b/src/core/core.cpp @@ -484,12 +484,14 @@ Core::Core(QThread* coreThread) Core::~Core() { - // need to reset av first, because it uses tox - av.reset(); - + /* + * First stop the thread to stop the timer and avoid Core emitting callbacks + * into an already destructed CoreAV. + */ coreThread->exit(0); coreThread->wait(); + av.reset(); tox.reset(); } diff --git a/src/core/coreav.cpp b/src/core/coreav.cpp index 7508f71c3..41bc42a86 100644 --- a/src/core/coreav.cpp +++ b/src/core/coreav.cpp @@ -151,9 +151,16 @@ IAudioControl* CoreAV::getAudio() CoreAV::~CoreAV() { + /* Gracefully leave calls and group calls to avoid deadlocks in destructor */ for (const auto& call : calls) { cancelCall(call.first); } + for (const auto& call : groupCalls) { + leaveGroupCall(call.first); + } + + assert(calls.empty()); + assert(groupCalls.empty()); coreavThread->exit(0); coreavThread->wait(); @@ -452,12 +459,19 @@ void CoreAV::toggleMuteCallOutput(const Friend* f) void CoreAV::groupCallCallback(void* tox, uint32_t group, uint32_t peer, const int16_t* data, unsigned samples, uint8_t channels, uint32_t sample_rate, void* core) { + /* + * Currently group call audio decoding is handled in the Tox thread by c-toxcore, + * so we can be sure that this function is always called from the Core thread. + * To change this, an API change in c-toxcore is needed and this function probably must be + * changed. + * See https://github.com/TokTok/c-toxcore/issues/1364 for details. + */ + Q_UNUSED(tox); Core* c = static_cast(core); CoreAV* cav = c->getAv(); QReadLocker locker{&cav->callsLock}; - assert(QThread::currentThread() == cav->coreavThread.get()); const ToxPk peerPk = c->getGroupPeerPk(group, peer); const Settings& s = Settings::getInstance();