From fa4106292e69a06d49d5e8af80b49e6d0db8f255 Mon Sep 17 00:00:00 2001 From: Sergei Ilinykh Date: Mon, 8 Jul 2024 23:23:26 +0300 Subject: [PATCH] ServerInfoManager: use self-deleting qobject isntead of callback to handle slot context automatically this solves a crash on s5b proxy discovery and early jingle session close --- src/xmpp/xmpp-im/httpfileupload.cpp | 121 ++++++++++---------- src/xmpp/xmpp-im/jingle-s5b.cpp | 68 +++++------ src/xmpp/xmpp-im/xmpp_serverinfomanager.cpp | 47 +++++--- src/xmpp/xmpp-im/xmpp_serverinfomanager.h | 74 ++++++------ 4 files changed, 163 insertions(+), 147 deletions(-) diff --git a/src/xmpp/xmpp-im/httpfileupload.cpp b/src/xmpp/xmpp-im/httpfileupload.cpp index efbdc696..4b7eed01 100644 --- a/src/xmpp/xmpp-im/httpfileupload.cpp +++ b/src/xmpp/xmpp-im/httpfileupload.cpp @@ -21,13 +21,13 @@ #include "xmpp_client.h" #include "xmpp_serverinfomanager.h" -#include "xmpp_tasks.h" #include "xmpp_xmlcommon.h" #include #include #include #include +#include using namespace XMPP; @@ -99,70 +99,69 @@ void HttpFileUpload::start() if (featureOptions.isEmpty()) { featureOptions << (QSet() << xmlns_v0_2_5) << (QSet() << xmlns_v0_3_1); } - d->client->serverInfoManager()->queryServiceInfo( + auto query = d->client->serverInfoManager()->queryServiceInfo( QLatin1String("store"), QLatin1String("file"), featureOptions, - QRegularExpression("^(upload|http|stor|file|dis|drive).*"), ServerInfoManager::SQ_CheckAllOnNoMatch, - [this](const QList &items) { - d->httpHosts.clear(); - for (const auto &item : items) { - const QStringList &l = item.features().list(); - XEP0363::version ver = XEP0363::vUnknown; - QString xmlns; - quint64 sizeLimit = 0; - if (l.contains(xmlns_v0_3_1)) { - ver = XEP0363::v0_3_1; - xmlns = xmlns_v0_3_1; - } else if (l.contains(xmlns_v0_2_5)) { - ver = XEP0363::v0_2_5; - xmlns = xmlns_v0_2_5; + QRegularExpression("^(upload|http|stor|file|dis|drive).*"), ServiceInfoQuery::CheckAllOnNoMatch); + connect(query, &ServiceInfoQuery::finished, this, [this](const QList &items) { + d->httpHosts.clear(); + for (const auto &item : items) { + const QStringList &l = item.features().list(); + XEP0363::version ver = XEP0363::vUnknown; + QString xmlns; + quint64 sizeLimit = 0; + if (l.contains(xmlns_v0_3_1)) { + ver = XEP0363::v0_3_1; + xmlns = xmlns_v0_3_1; + } else if (l.contains(xmlns_v0_2_5)) { + ver = XEP0363::v0_2_5; + xmlns = xmlns_v0_2_5; + } + if (ver != XEP0363::vUnknown) { + QVector> hosts; + const XData::Field field = item.registeredExtension(xmlns).getField(QLatin1String("max-file-size")); + if (field.isValid() && field.type() == XData::Field::Field_TextSingle) + sizeLimit = field.value().at(0).toULongLong(); + HttpHost host; + host.ver = ver; + host.jid = item.jid(); + host.sizeLimit = sizeLimit; + QVariant metaProps(d->client->serverInfoManager()->serviceMeta(host.jid, "httpprops")); + if (metaProps.isValid()) { + host.props = HostProps(metaProps.value()); + } else { + host.props = SecureGet | SecurePut; + if (ver == XEP0363::v0_3_1) + host.props |= NewestVer; } - if (ver != XEP0363::vUnknown) { - QVector> hosts; - const XData::Field field = item.registeredExtension(xmlns).getField(QLatin1String("max-file-size")); - if (field.isValid() && field.type() == XData::Field::Field_TextSingle) - sizeLimit = field.value().at(0).toULongLong(); - HttpHost host; - host.ver = ver; - host.jid = item.jid(); - host.sizeLimit = sizeLimit; - QVariant metaProps(d->client->serverInfoManager()->serviceMeta(host.jid, "httpprops")); - if (metaProps.isValid()) { - host.props = HostProps(metaProps.value()); - } else { - host.props = SecureGet | SecurePut; - if (ver == XEP0363::v0_3_1) - host.props |= NewestVer; - } - int value = 0; - if (host.props & SecureGet) - value += 5; - if (host.props & SecurePut) - value += 5; - if (host.props & NewestVer) - value += 3; - if (host.props & Failure) - value -= 15; - if (!sizeLimit || d->fileSize < sizeLimit) - hosts.append({ host, value }); - - // no sorting in preference order. most preferred go first - std::sort(hosts.begin(), hosts.end(), - [](const auto &a, const auto &b) { return a.second > b.second; }); - for (auto &hp : hosts) { - d->httpHosts.append(hp.first); - } + int value = 0; + if (host.props & SecureGet) + value += 5; + if (host.props & SecurePut) + value += 5; + if (host.props & NewestVer) + value += 3; + if (host.props & Failure) + value -= 15; + if (!sizeLimit || d->fileSize < sizeLimit) + hosts.append({ host, value }); + + // no sorting in preference order. most preferred go first + std::sort(hosts.begin(), hosts.end(), [](const auto &a, const auto &b) { return a.second > b.second; }); + for (auto &hp : hosts) { + d->httpHosts.append(hp.first); } } - // d->currentHost = d->httpHosts.begin(); - d->client->httpFileUploadManager()->setDiscoHosts(d->httpHosts); - if (d->httpHosts.isEmpty()) { // if empty as the last resort check all services - d->result.statusCode = HttpFileUpload::ErrorCode::NoUploadService; - d->result.statusString = "No suitable http upload services were found"; - done(State::Error); - } else { - tryNextServer(); - } - }); + } + // d->currentHost = d->httpHosts.begin(); + d->client->httpFileUploadManager()->setDiscoHosts(d->httpHosts); + if (d->httpHosts.isEmpty()) { // if empty as the last resort check all services + d->result.statusCode = HttpFileUpload::ErrorCode::NoUploadService; + d->result.statusString = "No suitable http upload services were found"; + done(State::Error); + } else { + tryNextServer(); + } + }); } void HttpFileUpload::tryNextServer() diff --git a/src/xmpp/xmpp-im/jingle-s5b.cpp b/src/xmpp/xmpp-im/jingle-s5b.cpp index af82ca8b..35cb6eb5 100644 --- a/src/xmpp/xmpp-im/jingle-s5b.cpp +++ b/src/xmpp/xmpp-im/jingle-s5b.cpp @@ -698,42 +698,42 @@ namespace XMPP { namespace Jingle { namespace S5B { proxyDiscoveryInProgress = true; QList> featureOptions = { { "http://jabber.org/protocol/bytestreams" } }; - q->_pad->session()->manager()->client()->serverInfoManager()->queryServiceInfo( + auto query = q->_pad->session()->manager()->client()->serverInfoManager()->queryServiceInfo( QStringLiteral("proxy"), QStringLiteral("bytestreams"), featureOptions, - QRegularExpression("proxy.*|socks.*|stream.*|s5b.*"), ServerInfoManager::SQ_CheckAllOnNoMatch, - [this](const QList &items) { - if (!proxyDiscoveryInProgress) { // check if new results are ever/still expected - // seems like we have successful connection via higher priority channel. so nobody cares - // about proxy - return; - } - auto m = static_cast(q->_pad->manager()); - Jid userProxy = m->userProxy(); - - bool userProxyFound = !userProxy.isValid(); - for (const auto &i : items) { - quint16 localPref = 0; - if (!userProxyFound && i.jid() == userProxy) { - localPref = 1; - userProxyFound = true; - continue; - } - Candidate c(q, i.jid(), generateCid(), localPref); - localCandidates.emplace(c.cid(), c); - qDebug("new local candidate: %s", qPrintable(c.toString())); - queryS5BProxy(i.jid(), c.cid()); - } - if (!userProxyFound) { - Candidate c(q, userProxy, generateCid(), 1); - localCandidates.emplace(c.cid(), c); - qDebug("new local candidate: %s", qPrintable(c.toString())); - queryS5BProxy(userProxy, c.cid()); - } else if (items.count() == 0) { - // seems like we don't have any proxy - proxyDiscoveryInProgress = false; - checkAndFinishNegotiation(); + QRegularExpression("proxy.*|socks.*|stream.*|s5b.*"), ServiceInfoQuery::CheckAllOnNoMatch); + q->connect(query, &ServiceInfoQuery::finished, q, [this](const QList &items) { + if (!proxyDiscoveryInProgress) { // check if new results are ever/still expected + // seems like we have successful connection via higher priority channel. so nobody cares + // about proxy + return; + } + auto m = static_cast(q->_pad->manager()); + Jid userProxy = m->userProxy(); + + bool userProxyFound = !userProxy.isValid(); + for (const auto &i : items) { + quint16 localPref = 0; + if (!userProxyFound && i.jid() == userProxy) { + localPref = 1; + userProxyFound = true; + continue; } - }); + Candidate c(q, i.jid(), generateCid(), localPref); + localCandidates.emplace(c.cid(), c); + qDebug("new local candidate: %s", qPrintable(c.toString())); + queryS5BProxy(i.jid(), c.cid()); + } + if (!userProxyFound) { + Candidate c(q, userProxy, generateCid(), 1); + localCandidates.emplace(c.cid(), c); + qDebug("new local candidate: %s", qPrintable(c.toString())); + queryS5BProxy(userProxy, c.cid()); + } else if (items.count() == 0) { + // seems like we don't have any proxy + proxyDiscoveryInProgress = false; + checkAndFinishNegotiation(); + } + }); } void tryConnectToRemoteCandidate() diff --git a/src/xmpp/xmpp-im/xmpp_serverinfomanager.cpp b/src/xmpp/xmpp-im/xmpp_serverinfomanager.cpp index 0072c80c..d592f39c 100644 --- a/src/xmpp/xmpp-im/xmpp_serverinfomanager.cpp +++ b/src/xmpp/xmpp-im/xmpp_serverinfomanager.cpp @@ -94,6 +94,14 @@ void ServerInfoManager::queryServicesList() jtitems->go(true); } +void ServerInfoManager::finish(ServiceInfoQuery *q, const QList &items) +{ + emit q->finished(items); + if (q->parent() == this) { + q->deleteLater(); + } +} + void ServerInfoManager::checkPendingServiceQueries() { // if services list is not ready yet we have to exit. if it's failed we have to finish all pending queries @@ -102,17 +110,18 @@ void ServerInfoManager::checkPendingServiceQueries() const auto sqs = _serviceQueries; _serviceQueries.clear(); for (const auto &q : sqs) { - q.callback(QList()); + finish(q); } } return; } // services list is ready here and we can start checking it and sending disco#info to not cached entries - auto sqIt = _serviceQueries.begin(); - while (sqIt != _serviceQueries.end()) { + auto queryIt = _serviceQueries.begin(); + while (queryIt != _serviceQueries.end()) { // populate services to query for this service request + auto sqIt = *queryIt; if (!sqIt->servicesToQueryDefined) { sqIt->spareServicesToQuery.clear(); // grep all suitble service jids. moving forward preferred ones @@ -122,7 +131,7 @@ void ServerInfoManager::checkPendingServiceQueries() if (sqIt->nameHint.isValid()) { if (!sqIt->nameHint.isValid() || sqIt->nameHint.match(si.key()).hasMatch()) { sqIt->servicesToQuery.push_back(si.key()); - } else if (sqIt->options & SQ_CheckAllOnNoMatch) { + } else if (sqIt->options & ServiceInfoQuery::CheckAllOnNoMatch) { sqIt->spareServicesToQuery.push_back(si.key()); } } else { @@ -134,8 +143,8 @@ void ServerInfoManager::checkPendingServiceQueries() sqIt->spareServicesToQuery.clear(); } if (sqIt->servicesToQuery.empty()) { - sqIt->callback(QList()); - _serviceQueries.erase(sqIt++); + finish(sqIt); + _serviceQueries.erase(queryIt++); continue; } sqIt->servicesToQueryDefined = true; @@ -169,7 +178,7 @@ void ServerInfoManager::checkPendingServiceQueries() sqIt->features.constBegin(), sqIt->features.constEnd(), false, [&si](bool a, const QSet &b) { return a || si->item.features().test(b); }))) { sqIt->result.append(si->item); - if (sqIt->options & SQ_FinishOnFirstMatch) { + if (sqIt->options & ServiceInfoQuery::FinishOnFirstMatch) { break; } } @@ -210,20 +219,19 @@ void ServerInfoManager::checkPendingServiceQueries() } // if has at least one sufficient result - auto forceFinish = (!sqIt->result.isEmpty() && (sqIt->options & SQ_FinishOnFirstMatch)); // stop on first found + auto forceFinish = (!sqIt->result.isEmpty() + && (sqIt->options & ServiceInfoQuery::FinishOnFirstMatch)); // stop on first found // if nothing in progress then we have full result set or nothing found even in spare list if (forceFinish || !hasInProgress) { // self explanatory - auto callback = std::move(sqIt->callback); - auto result = sqIt->result; - _serviceQueries.erase(sqIt++); - callback(result); + _serviceQueries.erase(queryIt++); + finish(sqIt, sqIt->result); } else { - ++sqIt; + ++queryIt; } } } -void ServerInfoManager::appendQuery(const ServiceQuery &q) +void ServerInfoManager::appendQuery(ServiceInfoQuery *q) { _serviceQueries.push_back(q); if (_servicesListState == ST_InProgress) { @@ -236,11 +244,14 @@ void ServerInfoManager::appendQuery(const ServiceQuery &q) } } -void ServerInfoManager::queryServiceInfo(const QString &category, const QString &type, - const QList> &features, const QRegularExpression &nameHint, - SQOptions options, std::function &items)> callback) +ServiceInfoQuery *ServerInfoManager::queryServiceInfo(const QString &category, const QString &type, + const QList> &features, + const QRegularExpression &nameHint, + ServiceInfoQuery::Options options) { - appendQuery(ServiceQuery(type, category, features, nameHint, options, std::move(callback))); + auto query = new ServiceInfoQuery(type, category, features, nameHint, options, this); + appendQuery(query); + return query; } void ServerInfoManager::setServiceMeta(const Jid &service, const QString &key, const QVariant &value) diff --git a/src/xmpp/xmpp-im/xmpp_serverinfomanager.h b/src/xmpp/xmpp-im/xmpp_serverinfomanager.h index 0bb0343b..0b0ead20 100644 --- a/src/xmpp/xmpp-im/xmpp_serverinfomanager.h +++ b/src/xmpp/xmpp-im/xmpp_serverinfomanager.h @@ -28,7 +28,6 @@ #include #include -#include #include namespace XMPP { @@ -36,37 +35,43 @@ class Client; class Features; class Jid; -class ServerInfoManager : public QObject { +class ServiceInfoQuery : public QObject { Q_OBJECT public: - enum SQOption { - SQ_CheckAllOnNoMatch = 1, // check all if matched by name services do not match or no matched by name - SQ_FinishOnFirstMatch = 2, // first callback is final - SQ_CallbackOnAnyMatches = 4 // TODO don't wait while all services will be discovered. empty result list = final + enum Option { + CheckAllOnNoMatch = 1, // check all if matched by name services do not match or no matched by name + FinishOnFirstMatch = 2, // first callback is final + CallbackOnAnyMatches = 4 // TODO don't wait while all services will be discovered. empty result list = final }; - Q_DECLARE_FLAGS(SQOptions, SQOption) + Q_DECLARE_FLAGS(Options, Option) + + QList result; private: - struct ServiceQuery { - const QString type; - const QString category; - const QList> features; - const QRegularExpression nameHint; - const SQOptions options; - const std::function &item)> callback; - std::list servicesToQuery; - std::list spareServicesToQuery; // usually a fallback when the above is not matched - bool servicesToQueryDefined = false; - QList result; - - ServiceQuery(const QString &type, const QString &category, const QList> &features, - const QRegularExpression &nameHint, const SQOptions &options, - const std::function &item)> &&callback) : - type(type), category(category), features(features), nameHint(nameHint), options(options), callback(callback) - { - } - }; + friend class ServerInfoManager; + const QString type; + const QString category; + const QList> features; + const QRegularExpression nameHint; + const Options options; + std::list servicesToQuery; + std::list spareServicesToQuery; // usually a fallback when the above is not matched + bool servicesToQueryDefined = false; + + ServiceInfoQuery(const QString &type, const QString &category, const QList> &features, + const QRegularExpression &nameHint, const Options &options, QObject *parent) : + QObject(parent), type(type), category(category), features(features), nameHint(nameHint), options(options) + { + } + +signals: + void finished(const QList &item); +}; +class ServerInfoManager : public QObject { + Q_OBJECT +public: +private: enum ServicesState { ST_NotQueried, ST_InProgress, ST_Ready, ST_Failed }; struct ServiceInfo { @@ -103,11 +108,11 @@ class ServerInfoManager : public QObject { nameHint = (http\..*|) // search for service name like http.jabber.ru Result: disco info for upload.jabber.ru will be returned. */ - void queryServiceInfo(const QString &category, const QString &type, const QList> &features, - const QRegularExpression &nameHint, SQOptions options, - std::function &items)> callback); - void setServiceMeta(const Jid &service, const QString &key, const QVariant &value); - QVariant serviceMeta(const Jid &service, const QString &key); + ServiceInfoQuery *queryServiceInfo(const QString &category, const QString &type, + const QList> &features, const QRegularExpression &nameHint, + ServiceInfoQuery::Options options); + void setServiceMeta(const Jid &service, const QString &key, const QVariant &value); + QVariant serviceMeta(const Jid &service, const QString &key); signals: void featuresChanged(); @@ -123,7 +128,8 @@ private slots: private: void queryServicesList(); void checkPendingServiceQueries(); - void appendQuery(const ServiceQuery &q); + void appendQuery(ServiceInfoQuery *q); + void finish(ServiceInfoQuery *q, const QList &items = {}); private: XMPP::Client *_client = nullptr; @@ -133,8 +139,8 @@ private slots: QString _multicastService; QMap _extraServerInfo; // XEP-0128, XEP-0157 - std::list _serviceQueries; // a storage of pending requests as result of `queryService` call - ServicesState _servicesListState = ST_NotQueried; + std::list _serviceQueries; // a storage of pending requests as result of `queryService` call + ServicesState _servicesListState = ST_NotQueried; QMap _servicesInfo; // all the diso#info requests for services of this server jid=>(state,info)