From 3c7e05501e4028b3390f45c15ade0a073c4548fb Mon Sep 17 00:00:00 2001 From: Michael Zanetti Date: Wed, 10 Aug 2022 23:29:53 +0200 Subject: [PATCH] Fix a potential info->aborted() call after the plugin called info->finish() It can happen that a plugin calls finish() in a slot which normally would be dispatched before the timeout but due to high system load the slot is invoked only after the timeout. This in turn would cause Qt to also queue up this timeout slot and by the time the system processes slots, the plugin comes in first and we'd fire an aborted() signal in the plugin after it called finish(), potentially badly breaking the plugin as a plugin developer would not expect this to happen. So we'll have to verify here that the plugin did not finish() by now before aborting. --- libnymea/integrations/browseractioninfo.cpp | 15 +++++++++++++-- libnymea/integrations/browseresult.cpp | 15 +++++++++++++-- libnymea/integrations/browseritemactioninfo.cpp | 15 +++++++++++++-- libnymea/integrations/browseritemresult.cpp | 15 +++++++++++++-- libnymea/integrations/thingactioninfo.cpp | 15 +++++++++++++-- libnymea/integrations/thingdiscoveryinfo.cpp | 15 +++++++++++++-- libnymea/integrations/thingpairinginfo.cpp | 15 +++++++++++++-- libnymea/integrations/thingsetupinfo.cpp | 15 +++++++++++++-- 8 files changed, 104 insertions(+), 16 deletions(-) diff --git a/libnymea/integrations/browseractioninfo.cpp b/libnymea/integrations/browseractioninfo.cpp index 6461a678..16a413ff 100644 --- a/libnymea/integrations/browseractioninfo.cpp +++ b/libnymea/integrations/browseractioninfo.cpp @@ -43,11 +43,22 @@ BrowserActionInfo::BrowserActionInfo(Thing *thing, ThingManager *thingManager, c { connect(this, &BrowserActionInfo::finished, this, &BrowserActionInfo::deleteLater, Qt::QueuedConnection); + // TODO 2.0: Create a base class and move this finished() of all Info classes logic handling there + // That will badly break the ABI, so copy/pasting this for now. if (timeout > 0) { QTimer::singleShot(timeout, this, [this] { - emit aborted(); + // It can happen that a plugin calls finish() in a slot which normally would be dispatched before the timeout + // but due to high system load the slot is invoked only after the timeout. This in turn would cause Qt to also queue up + // this timeout slot and by the time the system processes slots, the plugin comes in first and we'd fire an aborted() + // signal in the plugin after it called finish(), potentially badly breaking the plugin as a plugin developer would not + // expect this to happen. So we'll have to verify here that the plugin did not finish() by now before aborting. if (!m_finished) { - finish(Thing::ThingErrorTimeout); + emit aborted(); + // Also it could happen the the plugin calls finish() *in* the aborted() signal, in which case + // we don't want to call finish() here now to not print a warning about duplicate finished calls + if (!m_finished) { + finish(Thing::ThingErrorTimeout); + } } }); } diff --git a/libnymea/integrations/browseresult.cpp b/libnymea/integrations/browseresult.cpp index dbcce180..5148422e 100644 --- a/libnymea/integrations/browseresult.cpp +++ b/libnymea/integrations/browseresult.cpp @@ -44,11 +44,22 @@ BrowseResult::BrowseResult(Thing *thing, ThingManager *thingManager, const QStri { connect(this, &BrowseResult::finished, this, &BrowseResult::deleteLater, Qt::QueuedConnection); + // TODO 2.0: Create a base class and move this finished() of all Info classes logic handling there + // That will badly break the ABI, so copy/pasting this for now. if (timeout > 0) { QTimer::singleShot(timeout, this, [this] { - emit aborted(); + // It can happen that a plugin calls finish() in a slot which normally would be dispatched before the timeout + // but due to high system load the slot is invoked only after the timeout. This in turn would cause Qt to also queue up + // this timeout slot and by the time the system processes slots, the plugin comes in first and we'd fire an aborted() + // signal in the plugin after it called finish(), potentially badly breaking the plugin as a plugin developer would not + // expect this to happen. So we'll have to verify here that the plugin did not finish() by now before aborting. if (!m_finished) { - finish(Thing::ThingErrorTimeout); + emit aborted(); + // Also it could happen the the plugin calls finish() *in* the aborted() signal, in which case + // we don't want to call finish() here now to not print a warning about duplicate finished calls + if (!m_finished) { + finish(Thing::ThingErrorTimeout); + } } }); } diff --git a/libnymea/integrations/browseritemactioninfo.cpp b/libnymea/integrations/browseritemactioninfo.cpp index ead4910e..2d264bff 100644 --- a/libnymea/integrations/browseritemactioninfo.cpp +++ b/libnymea/integrations/browseritemactioninfo.cpp @@ -43,11 +43,22 @@ BrowserItemActionInfo::BrowserItemActionInfo(Thing *thing, ThingManager *thingMa { connect(this, &BrowserItemActionInfo::finished, this, &BrowserItemActionInfo::deleteLater, Qt::QueuedConnection); + // TODO 2.0: Create a base class and move this finished() of all Info classes logic handling there + // That will badly break the ABI, so copy/pasting this for now. if (timeout > 0) { QTimer::singleShot(timeout, this, [this] { - emit aborted(); + // It can happen that a plugin calls finish() in a slot which normally would be dispatched before the timeout + // but due to high system load the slot is invoked only after the timeout. This in turn would cause Qt to also queue up + // this timeout slot and by the time the system processes slots, the plugin comes in first and we'd fire an aborted() + // signal in the plugin after it called finish(), potentially badly breaking the plugin as a plugin developer would not + // expect this to happen. So we'll have to verify here that the plugin did not finish() by now before aborting. if (!m_finished) { - finish(Thing::ThingErrorTimeout); + emit aborted(); + // Also it could happen the the plugin calls finish() *in* the aborted() signal, in which case + // we don't want to call finish() here now to not print a warning about duplicate finished calls + if (!m_finished) { + finish(Thing::ThingErrorTimeout); + } } }); } diff --git a/libnymea/integrations/browseritemresult.cpp b/libnymea/integrations/browseritemresult.cpp index 642f21c7..5a79cc52 100644 --- a/libnymea/integrations/browseritemresult.cpp +++ b/libnymea/integrations/browseritemresult.cpp @@ -44,11 +44,22 @@ BrowserItemResult::BrowserItemResult(Thing *thing, ThingManager *thingManager, c { connect(this, &BrowserItemResult::finished, this, &BrowserItemResult::deleteLater, Qt::QueuedConnection); + // TODO 2.0: Create a base class and move this finished() of all Info classes logic handling there + // That will badly break the ABI, so copy/pasting this for now. if (timeout > 0) { QTimer::singleShot(timeout, this, [this] { - emit aborted(); + // It can happen that a plugin calls finish() in a slot which normally would be dispatched before the timeout + // but due to high system load the slot is invoked only after the timeout. This in turn would cause Qt to also queue up + // this timeout slot and by the time the system processes slots, the plugin comes in first and we'd fire an aborted() + // signal in the plugin after it called finish(), potentially badly breaking the plugin as a plugin developer would not + // expect this to happen. So we'll have to verify here that the plugin did not finish() by now before aborting. if (!m_finished) { - finish(Thing::ThingErrorTimeout); + emit aborted(); + // Also it could happen the the plugin calls finish() *in* the aborted() signal, in which case + // we don't want to call finish() here now to not print a warning about duplicate finished calls + if (!m_finished) { + finish(Thing::ThingErrorTimeout); + } } }); } diff --git a/libnymea/integrations/thingactioninfo.cpp b/libnymea/integrations/thingactioninfo.cpp index 665c822e..d1c0df42 100644 --- a/libnymea/integrations/thingactioninfo.cpp +++ b/libnymea/integrations/thingactioninfo.cpp @@ -44,11 +44,22 @@ ThingActionInfo::ThingActionInfo(Thing *thing, const Action &action, ThingManage { connect(this, &ThingActionInfo::finished, this, &ThingActionInfo::deleteLater, Qt::QueuedConnection); + // TODO 2.0: Create a base class and move this finished() of all Info classes logic handling there + // That will badly break the ABI, so copy/pasting this for now. if (timeout > 0) { QTimer::singleShot(timeout, this, [this] { - emit aborted(); + // It can happen that a plugin calls finish() in a slot which normally would be dispatched before the timeout + // but due to high system load the slot is invoked only after the timeout. This in turn would cause Qt to also queue up + // this timeout slot and by the time the system processes slots, the plugin comes in first and we'd fire an aborted() + // signal in the plugin after it called finish(), potentially badly breaking the plugin as a plugin developer would not + // expect this to happen. So we'll have to verify here that the plugin did not finish() by now before aborting. if (!m_finished) { - finish(Thing::ThingErrorTimeout); + emit aborted(); + // Also it could happen the the plugin calls finish() *in* the aborted() signal, in which case + // we don't want to call finish() here now to not print a warning about duplicate finished calls + if (!m_finished) { + finish(Thing::ThingErrorTimeout); + } } }); } diff --git a/libnymea/integrations/thingdiscoveryinfo.cpp b/libnymea/integrations/thingdiscoveryinfo.cpp index 14aa8e0d..be02c177 100644 --- a/libnymea/integrations/thingdiscoveryinfo.cpp +++ b/libnymea/integrations/thingdiscoveryinfo.cpp @@ -43,11 +43,22 @@ ThingDiscoveryInfo::ThingDiscoveryInfo(const ThingClassId &thingClassId, const P { connect(this, &ThingDiscoveryInfo::finished, this, &ThingDiscoveryInfo::deleteLater, Qt::QueuedConnection); + // TODO 2.0: Create a base class and move this finished() of all Info classes logic handling there + // That will badly break the ABI, so copy/pasting this for now. if (timeout > 0) { QTimer::singleShot(timeout, this, [this] { - emit aborted(); + // It can happen that a plugin calls finish() in a slot which normally would be dispatched before the timeout + // but due to high system load the slot is invoked only after the timeout. This in turn would cause Qt to also queue up + // this timeout slot and by the time the system processes slots, the plugin comes in first and we'd fire an aborted() + // signal in the plugin after it called finish(), potentially badly breaking the plugin as a plugin developer would not + // expect this to happen. So we'll have to verify here that the plugin did not finish() by now before aborting. if (!m_finished) { - finish(Thing::ThingErrorTimeout); + emit aborted(); + // Also it could happen the the plugin calls finish() *in* the aborted() signal, in which case + // we don't want to call finish() here now to not print a warning about duplicate finished calls + if (!m_finished) { + finish(Thing::ThingErrorTimeout); + } } }); } diff --git a/libnymea/integrations/thingpairinginfo.cpp b/libnymea/integrations/thingpairinginfo.cpp index eaa8251a..6e3c4b88 100644 --- a/libnymea/integrations/thingpairinginfo.cpp +++ b/libnymea/integrations/thingpairinginfo.cpp @@ -46,11 +46,22 @@ ThingPairingInfo::ThingPairingInfo(const PairingTransactionId &pairingTransactio { connect(this, &ThingPairingInfo::finished, this, &ThingPairingInfo::deleteLater, Qt::QueuedConnection); + // TODO 2.0: Create a base class and move this finished() of all Info classes logic handling there + // That will badly break the ABI, so copy/pasting this for now. if (timeout > 0) { QTimer::singleShot(timeout, this, [this] { - emit aborted(); + // It can happen that a plugin calls finish() in a slot which normally would be dispatched before the timeout + // but due to high system load the slot is invoked only after the timeout. This in turn would cause Qt to also queue up + // this timeout slot and by the time the system processes slots, the plugin comes in first and we'd fire an aborted() + // signal in the plugin after it called finish(), potentially badly breaking the plugin as a plugin developer would not + // expect this to happen. So we'll have to verify here that the plugin did not finish() by now before aborting. if (!m_finished) { - finish(Thing::ThingErrorTimeout); + emit aborted(); + // Also it could happen the the plugin calls finish() *in* the aborted() signal, in which case + // we don't want to call finish() here now to not print a warning about duplicate finished calls + if (!m_finished) { + finish(Thing::ThingErrorTimeout); + } } }); } diff --git a/libnymea/integrations/thingsetupinfo.cpp b/libnymea/integrations/thingsetupinfo.cpp index 3599fa7e..ec2ef439 100644 --- a/libnymea/integrations/thingsetupinfo.cpp +++ b/libnymea/integrations/thingsetupinfo.cpp @@ -44,11 +44,22 @@ ThingSetupInfo::ThingSetupInfo(Thing *thing, ThingManager *thingManager, quint32 { connect(this, &ThingSetupInfo::finished, this, &ThingSetupInfo::deleteLater, Qt::QueuedConnection); + // TODO 2.0: Create a base class and move this finished() of all Info classes logic handling there + // That will badly break the ABI, so copy/pasting this for now. if (timeout > 0) { QTimer::singleShot(timeout, this, [this] { - emit aborted(); + // It can happen that a plugin calls finish() in a slot which normally would be dispatched before the timeout + // but due to high system load the slot is invoked only after the timeout. This in turn would cause Qt to also queue up + // this timeout slot and by the time the system processes slots, the plugin comes in first and we'd fire an aborted() + // signal in the plugin after it called finish(), potentially badly breaking the plugin as a plugin developer would not + // expect this to happen. So we'll have to verify here that the plugin did not finish() by now before aborting. if (!m_finished) { - finish(Thing::ThingErrorTimeout); + emit aborted(); + // Also it could happen the the plugin calls finish() *in* the aborted() signal, in which case + // we don't want to call finish() here now to not print a warning about duplicate finished calls + if (!m_finished) { + finish(Thing::ThingErrorTimeout); + } } }); }