refactor: properly lock against Core thread

We we're calling toxav_* functions without synchronizing to any of the
Tox threads.

Additionally remove the call timeout, it creates timers from different
threads, which causes errors.

src/core/core.cpp

@@ -628,7 +628,7 @@ ToxCorePtr Core::makeToxCore(const QByteArray& savedata, const ICoreSettings* co
 
     // toxcore is successfully created, create toxav
     // TODO(sudden6): don't create CoreAv here, Core should be usable without CoreAV
-    core->av = CoreAV::makeCoreAV(core->tox.get());
+    core->av = CoreAV::makeCoreAV(core->tox.get(), core->coreLoopLock);
     if (!core->av) {
         qCritical() << "Toxav failed to start";
         if (err) {

src/core/coreav.cpp

@@ -68,11 +68,12 @@
  * deadlock.
  */
 
-CoreAV::CoreAV(std::unique_ptr<ToxAV, ToxAVDeleter> toxav)
+CoreAV::CoreAV(std::unique_ptr<ToxAV, ToxAVDeleter> toxav, QMutex& toxCoreLock)
     : audio{nullptr}
     , toxav{std::move(toxav)}
     , coreavThread{new QThread{this}}
     , iterateTimer{new QTimer{this}}
+    , coreLock{toxCoreLock}
 {
     assert(coreavThread);
     assert(iterateTimer);
@@ -104,7 +105,7 @@ void CoreAV::connectCallbacks(ToxAV& toxav)
  * @param core pointer to the Tox instance
  * @return CoreAV instance on success, {} on failure
  */
-CoreAV::CoreAVPtr CoreAV::makeCoreAV(Tox* core)
+CoreAV::CoreAVPtr CoreAV::makeCoreAV(Tox* core, QMutex &toxCoreLock)
 {
     TOXAV_ERR_NEW err;
     std::unique_ptr<ToxAV, ToxAVDeleter> toxav{toxav_new(core, &err)};
@@ -124,7 +125,7 @@ CoreAV::CoreAVPtr CoreAV::makeCoreAV(Tox* core)
 
     assert(toxav != nullptr);
 
-    return CoreAVPtr{new CoreAV{std::move(toxav)}};
+    return CoreAVPtr{new CoreAV{std::move(toxav), toxCoreLock}};
 }
 
 /**
@@ -235,9 +236,7 @@ bool CoreAV::isCallVideoEnabled(const Friend* f) const
 bool CoreAV::answerCall(uint32_t friendNum, bool video)
 {
     QMutexLocker locker{&callsLock};
-
+    QMutexLocker coreLocker{&coreLock};
-    // This callback should come from the Core thread
-    assert(QThread::currentThread() != coreavThread.get());
 
     qDebug() << QString("Answering call %1").arg(friendNum);
     auto it = calls.find(friendNum);
@@ -260,9 +259,7 @@ bool CoreAV::answerCall(uint32_t friendNum, bool video)
 bool CoreAV::startCall(uint32_t friendNum, bool video)
 {
     QMutexLocker locker{&callsLock};
-
+    QMutexLocker coreLocker{&coreLock};
-    // This callback should come from the Core thread
-    assert(QThread::currentThread() != coreavThread.get());
 
     qDebug() << QString("Starting call with %1").arg(friendNum);
     auto it = calls.find(friendNum);
@@ -280,17 +277,14 @@ bool CoreAV::startCall(uint32_t friendNum, bool video)
     assert(audio != nullptr);
     ToxFriendCallPtr call = ToxFriendCallPtr(new ToxFriendCall(friendNum, video, *this, *audio));
     assert(call != nullptr);
-    auto ret = calls.emplace(friendNum, std::move(call));
+    calls.emplace(friendNum, std::move(call));
-    ret.first->second->startTimeout(friendNum);
     return true;
 }
 
 bool CoreAV::cancelCall(uint32_t friendNum)
 {
     QMutexLocker locker{&callsLock};
-
+    QMutexLocker coreLocker{&coreLock};
-    // This callback should come from the Core thread
-    assert(QThread::currentThread() != coreavThread.get());
 
     qDebug() << QString("Cancelling call with %1").arg(friendNum);
     if (!toxav_call_control(toxav.get(), friendNum, TOXAV_CALL_CONTROL_CANCEL, nullptr)) {
@@ -380,6 +374,7 @@ void CoreAV::sendCallVideo(uint32_t callId, std::shared_ptr<VideoFrame> vframe)
 
     if (call.getNullVideoBitrate()) {
         qDebug() << "Restarting video stream to friend" << callId;
+        QMutexLocker coreLocker{&coreLock};
         toxav_video_set_bit_rate(toxav.get(), callId, VIDEO_DEFAULT_BITRATE, nullptr);
         call.setNullVideoBitrate(false);
     }
@@ -756,7 +751,6 @@ void CoreAV::stateCallback(ToxAV* toxav, uint32_t friendNum, uint32_t state, voi
     } else {
         // If our state was null, we started the call and were still ringing
         if (!call.getState() && state) {
-            call.stopTimeout();
             call.setActive(true);
             emit self->avStart(friendNum, call.getVideoEnabled());
         } else if ((call.getState() & TOXAV_FRIEND_CALL_STATE_SENDING_V)

src/core/coreav.h

@@ -46,7 +46,7 @@ class CoreAV : public QObject
 
 public:
     using CoreAVPtr = std::unique_ptr<CoreAV>;
-    static CoreAVPtr makeCoreAV(Tox* core);
+    static CoreAVPtr makeCoreAV(Tox* core, QMutex& coreLock);
 
     void setAudio(IAudioControl& newAudio);
     IAudioControl* getAudio();
@@ -112,7 +112,7 @@ private:
         }
     };
 
-    explicit CoreAV(std::unique_ptr<ToxAV, ToxAVDeleter> tox);
+    explicit CoreAV(std::unique_ptr<ToxAV, ToxAVDeleter> tox, QMutex &toxCoreLock);
     void connectCallbacks(ToxAV& toxav);
 
     void process();
@@ -149,6 +149,13 @@ private:
 
     // protect 'calls' and 'groupCalls' from being modified by ToxAV and Tox threads
     mutable QMutex callsLock{QMutex::Recursive};
+
+    /**
+     * @brief needed to synchronize with the Core thread, some toxav_* functions
+     *        must not execute at the same time as tox_iterate()
+     * @note This must be a recursive mutex as we're going to lock it in callbacks
+     */
+    QMutex& coreLock;
 };
 
 #endif // COREAV_H

src/core/toxcall.cpp

@@ -190,29 +190,6 @@ void ToxFriendCall::onAudioSinkInvalidated()
     sink = std::move(newSink);
 }
 
-void ToxFriendCall::startTimeout(uint32_t callId)
-{
-    if (!timeoutTimer) {
-        timeoutTimer = std::unique_ptr<QTimer>{new QTimer{}};
-        // We might move, so we need copies of members. CoreAV won't move while we're alive
-        CoreAV* avCopy = av;
-        QObject::connect(timeoutTimer.get(), &QTimer::timeout,
-                         [avCopy, callId]() { avCopy->timeoutCall(callId); });
-    }
-
-    if (!timeoutTimer->isActive())
-        timeoutTimer->start(CALL_TIMEOUT);
-}
-
-void ToxFriendCall::stopTimeout()
-{
-    if (!timeoutTimer)
-        return;
-
-    timeoutTimer->stop();
-    timeoutTimer.reset();
-}
-
 TOXAV_FRIEND_CALL_STATE ToxFriendCall::getState() const
 {
     return state;

src/core/toxcall.h

@@ -95,17 +95,11 @@ public:
     ToxFriendCall& operator=(ToxFriendCall&& other) = delete;
     ~ToxFriendCall();
 
-    void startTimeout(uint32_t callId);
-    void stopTimeout();
-
     TOXAV_FRIEND_CALL_STATE getState() const;
     void setState(const TOXAV_FRIEND_CALL_STATE& value);
 
     void playAudioBuffer(const int16_t* data, int samples, unsigned channels, int sampleRate) const;
 
-protected:
-    std::unique_ptr<QTimer> timeoutTimer;
-
 private:
     QMetaObject::Connection audioSinkInvalid;
     void onAudioSourceInvalidated();
@@ -113,7 +107,6 @@ private:
 
 private:
     TOXAV_FRIEND_CALL_STATE state{TOXAV_FRIEND_CALL_STATE_NONE};
-    static constexpr int CALL_TIMEOUT = 45000;
     std::unique_ptr<IAudioSink> sink = nullptr;
     uint32_t friendId;
 };