From b5254f1ea50f88058d95e6dc1157c09530d2e8f3 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Tue, 26 Jun 2018 16:25:05 -0700 Subject: [PATCH] cleanup race on deleteLater after callback processed --- domain-server/src/DomainGatekeeper.cpp | 9 +- domain-server/src/DomainServer.cpp | 9 +- .../src/DomainServerSettingsManager.cpp | 6 +- interface/src/DiscoverabilityManager.cpp | 4 +- interface/src/commerce/Ledger.cpp | 2 +- interface/src/ui/LoginDialog.cpp | 9 +- interface/src/ui/Snapshot.cpp | 2 +- interface/src/ui/SnapshotUploader.cpp | 2 +- libraries/networking/src/AccountManager.cpp | 108 +++++++----------- libraries/networking/src/AccountManager.h | 12 +- libraries/networking/src/AddressManager.cpp | 5 +- .../networking/src/UserActivityLogger.cpp | 2 +- libraries/ui/src/Tooltip.cpp | 2 +- 13 files changed, 66 insertions(+), 106 deletions(-) diff --git a/domain-server/src/DomainGatekeeper.cpp b/domain-server/src/DomainGatekeeper.cpp index 2d78149c7a..2a6b78744e 100644 --- a/domain-server/src/DomainGatekeeper.cpp +++ b/domain-server/src/DomainGatekeeper.cpp @@ -660,9 +660,8 @@ void DomainGatekeeper::requestUserPublicKey(const QString& username, bool isOpti // even if we have a public key for them right now, request a new one in case it has just changed JSONCallbackParameters callbackParams; - callbackParams.jsonCallbackReceiver = this; + callbackParams.callbackReceiver = this; callbackParams.jsonCallbackMethod = "publicKeyJSONCallback"; - callbackParams.errorCallbackReceiver = this; callbackParams.errorCallbackMethod = "publicKeyJSONErrorCallback"; @@ -893,9 +892,8 @@ void DomainGatekeeper::getGroupMemberships(const QString& username) { JSONCallbackParameters callbackParams; - callbackParams.jsonCallbackReceiver = this; + callbackParams.callbackReceiver = this; callbackParams.jsonCallbackMethod = "getIsGroupMemberJSONCallback"; - callbackParams.errorCallbackReceiver = this; callbackParams.errorCallbackMethod = "getIsGroupMemberErrorCallback"; const QString GET_IS_GROUP_MEMBER_PATH = "api/v1/groups/members/%2"; @@ -960,9 +958,8 @@ void DomainGatekeeper::getIsGroupMemberErrorCallback(QNetworkReply* requestReply void DomainGatekeeper::getDomainOwnerFriendsList() { JSONCallbackParameters callbackParams; - callbackParams.jsonCallbackReceiver = this; + callbackParams.callbackReceiver = this; callbackParams.jsonCallbackMethod = "getDomainOwnerFriendsListJSONCallback"; - callbackParams.errorCallbackReceiver = this; callbackParams.errorCallbackMethod = "getDomainOwnerFriendsListErrorCallback"; const QString GET_FRIENDS_LIST_PATH = "api/v1/user/friends"; diff --git a/domain-server/src/DomainServer.cpp b/domain-server/src/DomainServer.cpp index 5c602b6629..eccd1c1e20 100644 --- a/domain-server/src/DomainServer.cpp +++ b/domain-server/src/DomainServer.cpp @@ -514,7 +514,7 @@ void DomainServer::getTemporaryName(bool force) { // request a temporary name from the metaverse auto accountManager = DependencyManager::get(); - JSONCallbackParameters callbackParameters { this, "handleTempDomainSuccess", this, "handleTempDomainError" }; + JSONCallbackParameters callbackParameters { this, "handleTempDomainSuccess", "handleTempDomainError" }; accountManager->sendRequest("/api/v1/domains/temporary", AccountManagerAuth::None, QNetworkAccessManager::PostOperation, callbackParameters); } @@ -1345,7 +1345,7 @@ void DomainServer::sendPendingTransactionsToServer() { JSONCallbackParameters transactionCallbackParams; - transactionCallbackParams.jsonCallbackReceiver = this; + transactionCallbackParams.callbackReceiver = this; transactionCallbackParams.jsonCallbackMethod = "transactionJSONCallback"; while (i != _pendingAssignmentCredits.end()) { @@ -1449,7 +1449,7 @@ void DomainServer::sendHeartbeatToMetaverse(const QString& networkAddress) { DependencyManager::get()->sendRequest(DOMAIN_UPDATE.arg(uuidStringWithoutCurlyBraces(getID())), AccountManagerAuth::Optional, QNetworkAccessManager::PutOperation, - JSONCallbackParameters(nullptr, QString(), this, "handleMetaverseHeartbeatError"), + JSONCallbackParameters(this, QString(), "handleMetaverseHeartbeatError"), domainUpdateJSON.toUtf8()); } @@ -1531,9 +1531,8 @@ void DomainServer::sendICEServerAddressToMetaverseAPI() { // make sure we hear about failure so we can retry JSONCallbackParameters callbackParameters; - callbackParameters.errorCallbackReceiver = this; + callbackParameters.callbackReceiver = this; callbackParameters.errorCallbackMethod = "handleFailedICEServerAddressUpdate"; - callbackParameters.jsonCallbackReceiver = this; callbackParameters.jsonCallbackMethod = "handleSuccessfulICEServerAddressUpdate"; static bool printedIceServerMessage = false; diff --git a/domain-server/src/DomainServerSettingsManager.cpp b/domain-server/src/DomainServerSettingsManager.cpp index fda9119148..2bcaa8899e 100644 --- a/domain-server/src/DomainServerSettingsManager.cpp +++ b/domain-server/src/DomainServerSettingsManager.cpp @@ -1815,9 +1815,8 @@ void DomainServerSettingsManager::apiRefreshGroupInformation() { void DomainServerSettingsManager::apiGetGroupID(const QString& groupName) { JSONCallbackParameters callbackParams; - callbackParams.jsonCallbackReceiver = this; + callbackParams.callbackReceiver = this; callbackParams.jsonCallbackMethod = "apiGetGroupIDJSONCallback"; - callbackParams.errorCallbackReceiver = this; callbackParams.errorCallbackMethod = "apiGetGroupIDErrorCallback"; const QString GET_GROUP_ID_PATH = "api/v1/groups/names/%1"; @@ -1882,9 +1881,8 @@ void DomainServerSettingsManager::apiGetGroupIDErrorCallback(QNetworkReply* requ void DomainServerSettingsManager::apiGetGroupRanks(const QUuid& groupID) { JSONCallbackParameters callbackParams; - callbackParams.jsonCallbackReceiver = this; + callbackParams.callbackReceiver = this; callbackParams.jsonCallbackMethod = "apiGetGroupRanksJSONCallback"; - callbackParams.errorCallbackReceiver = this; callbackParams.errorCallbackMethod = "apiGetGroupRanksErrorCallback"; const QString GET_GROUP_RANKS_PATH = "api/v1/groups/%1/ranks"; diff --git a/interface/src/DiscoverabilityManager.cpp b/interface/src/DiscoverabilityManager.cpp index 191908ab04..e3a99475ef 100644 --- a/interface/src/DiscoverabilityManager.cpp +++ b/interface/src/DiscoverabilityManager.cpp @@ -97,7 +97,7 @@ void DiscoverabilityManager::updateLocation() { locationObject.insert(AVAILABILITY_KEY_IN_LOCATION, findableByString(static_cast(_mode.get()))); JSONCallbackParameters callbackParameters; - callbackParameters.jsonCallbackReceiver = this; + callbackParameters.callbackReceiver = this; callbackParameters.jsonCallbackMethod = "handleHeartbeatResponse"; // figure out if we'll send a fresh location or just a simple heartbeat @@ -121,7 +121,7 @@ void DiscoverabilityManager::updateLocation() { // we still send a heartbeat to the metaverse server for stats collection JSONCallbackParameters callbackParameters; - callbackParameters.jsonCallbackReceiver = this; + callbackParameters.callbackReceiver = this; callbackParameters.jsonCallbackMethod = "handleHeartbeatResponse"; accountManager->sendRequest(API_USER_HEARTBEAT_PATH, AccountManagerAuth::Optional, diff --git a/interface/src/commerce/Ledger.cpp b/interface/src/commerce/Ledger.cpp index 1f36148049..702251f867 100644 --- a/interface/src/commerce/Ledger.cpp +++ b/interface/src/commerce/Ledger.cpp @@ -68,7 +68,7 @@ Handler(updateItem) void Ledger::send(const QString& endpoint, const QString& success, const QString& fail, QNetworkAccessManager::Operation method, AccountManagerAuth::Type authType, QJsonObject request) { auto accountManager = DependencyManager::get(); const QString URL = "/api/v1/commerce/"; - JSONCallbackParameters callbackParams(this, success, this, fail); + JSONCallbackParameters callbackParams(this, success, fail); qCInfo(commerce) << "Sending" << endpoint << QJsonDocument(request).toJson(QJsonDocument::Compact); accountManager->sendRequest(URL + endpoint, authType, diff --git a/interface/src/ui/LoginDialog.cpp b/interface/src/ui/LoginDialog.cpp index 11fb8efba7..4d8592a9d3 100644 --- a/interface/src/ui/LoginDialog.cpp +++ b/interface/src/ui/LoginDialog.cpp @@ -113,9 +113,8 @@ void LoginDialog::linkSteam() { } JSONCallbackParameters callbackParams; - callbackParams.jsonCallbackReceiver = this; + callbackParams.callbackReceiver = this; callbackParams.jsonCallbackMethod = "linkCompleted"; - callbackParams.errorCallbackReceiver = this; callbackParams.errorCallbackMethod = "linkFailed"; const QString LINK_STEAM_PATH = "api/v1/user/steam/link"; @@ -141,9 +140,8 @@ void LoginDialog::createAccountFromStream(QString username) { } JSONCallbackParameters callbackParams; - callbackParams.jsonCallbackReceiver = this; + callbackParams.callbackReceiver = this; callbackParams.jsonCallbackMethod = "createCompleted"; - callbackParams.errorCallbackReceiver = this; callbackParams.errorCallbackMethod = "createFailed"; const QString CREATE_ACCOUNT_FROM_STEAM_PATH = "api/v1/user/steam/create"; @@ -204,9 +202,8 @@ void LoginDialog::createFailed(QNetworkReply* reply) { void LoginDialog::signup(const QString& email, const QString& username, const QString& password) { JSONCallbackParameters callbackParams; - callbackParams.jsonCallbackReceiver = this; + callbackParams.callbackReceiver = this; callbackParams.jsonCallbackMethod = "signupCompleted"; - callbackParams.errorCallbackReceiver = this; callbackParams.errorCallbackMethod = "signupFailed"; QJsonObject payload; diff --git a/interface/src/ui/Snapshot.cpp b/interface/src/ui/Snapshot.cpp index efcc85b23e..60c039ce1f 100644 --- a/interface/src/ui/Snapshot.cpp +++ b/interface/src/ui/Snapshot.cpp @@ -493,7 +493,7 @@ void Snapshot::uploadSnapshot(const QString& filename, const QUrl& href) { multiPart->append(imagePart); auto accountManager = DependencyManager::get(); - JSONCallbackParameters callbackParams(uploader, "uploadSuccess", uploader, "uploadFailure"); + JSONCallbackParameters callbackParams(uploader, "uploadSuccess", "uploadFailure"); accountManager->sendRequest(SNAPSHOT_UPLOAD_URL, AccountManagerAuth::Required, QNetworkAccessManager::PostOperation, callbackParams, nullptr, multiPart); diff --git a/interface/src/ui/SnapshotUploader.cpp b/interface/src/ui/SnapshotUploader.cpp index 96af59f923..694d0fa8f8 100644 --- a/interface/src/ui/SnapshotUploader.cpp +++ b/interface/src/ui/SnapshotUploader.cpp @@ -60,7 +60,7 @@ void SnapshotUploader::uploadSuccess(QNetworkReply* reply) { rootObject.insert("user_story", userStoryObject); auto accountManager = DependencyManager::get(); - JSONCallbackParameters callbackParams(this, "createStorySuccess", this, "createStoryFailure"); + JSONCallbackParameters callbackParams(this, "createStorySuccess", "createStoryFailure"); accountManager->sendRequest(STORY_UPLOAD_URL, AccountManagerAuth::Required, diff --git a/libraries/networking/src/AccountManager.cpp b/libraries/networking/src/AccountManager.cpp index 8cb48317b6..5b3196a2bf 100644 --- a/libraries/networking/src/AccountManager.cpp +++ b/libraries/networking/src/AccountManager.cpp @@ -47,15 +47,12 @@ Q_DECLARE_METATYPE(JSONCallbackParameters) const QString ACCOUNTS_GROUP = "accounts"; -JSONCallbackParameters::JSONCallbackParameters(QObject* jsonCallbackReceiver, const QString& jsonCallbackMethod, - QObject* errorCallbackReceiver, const QString& errorCallbackMethod, - QObject* updateReceiver, const QString& updateSlot) : - jsonCallbackReceiver(jsonCallbackReceiver), +JSONCallbackParameters::JSONCallbackParameters(QObject* callbackReceiver, + const QString& jsonCallbackMethod, + const QString& errorCallbackMethod) : + callbackReceiver(callbackReceiver), jsonCallbackMethod(jsonCallbackMethod), - errorCallbackReceiver(errorCallbackReceiver), - errorCallbackMethod(errorCallbackMethod), - updateReceiver(updateReceiver), - updateSlot(updateSlot) + errorCallbackMethod(errorCallbackMethod) { } @@ -323,54 +320,50 @@ void AccountManager::sendRequest(const QString& path, } } - - if (!callbackParams.isEmpty()) { - if (callbackParams.updateReceiver && !callbackParams.updateSlot.isEmpty()) { - callbackParams.updateReceiver->connect(networkReply, SIGNAL(uploadProgress(qint64, qint64)), - callbackParams.updateSlot.toStdString().c_str()); + connect(networkReply, &QNetworkReply::finished, this, [this, networkReply] { + // double check if the finished network reply had a session ID in the header and make + // sure that our session ID matches that value if so + if (networkReply->hasRawHeader(METAVERSE_SESSION_ID_HEADER)) { + _sessionID = networkReply->rawHeader(METAVERSE_SESSION_ID_HEADER); } - } + }); - // There's a cleaner way to fire the JSON/error callbacks below and ensure that deleteLater is called for the - // request reply - unfortunately it requires Qt 5.10 which the Android build does not support as of 06/26/18 - if (callbackParams.jsonCallbackReceiver) { - connect(networkReply, &QNetworkReply::finished, callbackParams.jsonCallbackReceiver, [callbackParams, networkReply] { - if (networkReply->error() == QNetworkReply::NoError) { - if (callbackParams.jsonCallbackReceiver && !callbackParams.jsonCallbackMethod.isEmpty()) { - bool invoked = QMetaObject::invokeMethod(callbackParams.jsonCallbackReceiver, - qPrintable(callbackParams.jsonCallbackMethod), - Q_ARG(QNetworkReply*, networkReply)); + if (callbackParams.isEmpty()) { + connect(networkReply, &QNetworkReply::finished, networkReply, &QNetworkReply::deleteLater); + } else { + // There's a cleaner way to fire the JSON/error callbacks below and ensure that deleteLater is called for the + // request reply - unfortunately it requires Qt 5.10 which the Android build does not support as of 06/26/18 - if (!invoked) { - QString error = "Could not invoke " + callbackParams.jsonCallbackMethod + " with QNetworkReply* " - + "on errorCallbackReceiver."; - qCWarning(networking) << error; - Q_ASSERT_X(invoked, "AccountManager::passErrorToCallback", qPrintable(error)); - } - } else { - if (VERBOSE_HTTP_REQUEST_DEBUGGING) { - qCDebug(networking) << "Received JSON response from metaverse API that has no matching callback."; - qCDebug(networking) << QJsonDocument::fromJson(networkReply->readAll()); - } + connect(networkReply, &QNetworkReply::finished, callbackParams.callbackReceiver, + [callbackParams, networkReply] { + if (networkReply->error() == QNetworkReply::NoError) { + if (!callbackParams.jsonCallbackMethod.isEmpty()) { + bool invoked = QMetaObject::invokeMethod(callbackParams.callbackReceiver, + qPrintable(callbackParams.jsonCallbackMethod), + Q_ARG(QNetworkReply*, networkReply)); + + if (!invoked) { + QString error = "Could not invoke " + callbackParams.jsonCallbackMethod + " with QNetworkReply* " + + "on callbackReceiver."; + qCWarning(networking) << error; + Q_ASSERT_X(invoked, "AccountManager::passErrorToCallback", qPrintable(error)); + } + } else { + if (VERBOSE_HTTP_REQUEST_DEBUGGING) { + qCDebug(networking) << "Received JSON response from metaverse API that has no matching callback."; + qCDebug(networking) << QJsonDocument::fromJson(networkReply->readAll()); } - - networkReply->deleteLater(); } - }); - } - - if (callbackParams.errorCallbackReceiver) { - connect(networkReply, &QNetworkReply::finished, callbackParams.errorCallbackReceiver, [callbackParams, networkReply]{ - if (networkReply->error() != QNetworkReply::NoError) { - if (callbackParams.errorCallbackReceiver && !callbackParams.errorCallbackMethod.isEmpty()) { - bool invoked = QMetaObject::invokeMethod(callbackParams.errorCallbackReceiver, + } else { + if (!callbackParams.errorCallbackMethod.isEmpty()) { + bool invoked = QMetaObject::invokeMethod(callbackParams.callbackReceiver, qPrintable(callbackParams.errorCallbackMethod), Q_ARG(QNetworkReply*, networkReply)); if (!invoked) { QString error = "Could not invoke " + callbackParams.errorCallbackMethod + " with QNetworkReply* " - + "on errorCallbackReceiver."; + + "on callbackReceiver."; qCWarning(networking) << error; Q_ASSERT_X(invoked, "AccountManager::passErrorToCallback", qPrintable(error)); } @@ -382,29 +375,11 @@ void AccountManager::sendRequest(const QString& path, qCDebug(networking) << networkReply->readAll(); } } - - networkReply->deleteLater(); } + + networkReply->deleteLater(); }); } - - // double check if the finished network reply had a session ID in the header and make - // sure that our session ID matches that value if so - connect(networkReply, &QNetworkReply::finished, this, [this, callbackParams, networkReply]{ - if (networkReply->error() == QNetworkReply::NoError) { - if (networkReply->hasRawHeader(METAVERSE_SESSION_ID_HEADER)) { - _sessionID = networkReply->rawHeader(METAVERSE_SESSION_ID_HEADER); - } - - if (!callbackParams.jsonCallbackReceiver) { - networkReply->deleteLater(); - } - } else { - if (!callbackParams.errorCallbackReceiver) { - networkReply->deleteLater(); - } - } - }); } } @@ -830,9 +805,8 @@ void AccountManager::processGeneratedKeypair(QByteArray publicKey, QByteArray pr // setup callback parameters so we know once the keypair upload has succeeded or failed JSONCallbackParameters callbackParameters; - callbackParameters.jsonCallbackReceiver = this; + callbackParameters.callbackReceiver = this; callbackParameters.jsonCallbackMethod = "publicKeyUploadSucceeded"; - callbackParameters.errorCallbackReceiver = this; callbackParameters.errorCallbackMethod = "publicKeyUploadFailed"; sendRequest(uploadPath, AccountManagerAuth::Optional, QNetworkAccessManager::PutOperation, diff --git a/libraries/networking/src/AccountManager.h b/libraries/networking/src/AccountManager.h index 59c5e12c14..a79b69fe2b 100644 --- a/libraries/networking/src/AccountManager.h +++ b/libraries/networking/src/AccountManager.h @@ -28,18 +28,14 @@ class JSONCallbackParameters { public: - JSONCallbackParameters(QObject* jsonCallbackReceiver = nullptr, const QString& jsonCallbackMethod = QString(), - QObject* errorCallbackReceiver = nullptr, const QString& errorCallbackMethod = QString(), - QObject* updateReceiver = nullptr, const QString& updateSlot = QString()); + JSONCallbackParameters(QObject* callbackReceiver = nullptr, const QString& jsonCallbackMethod = QString(), + const QString& errorCallbackMethod = QString()); - bool isEmpty() const { return !jsonCallbackReceiver && !errorCallbackReceiver; } + bool isEmpty() const { return !callbackReceiver; } - QObject* jsonCallbackReceiver; + QObject* callbackReceiver; QString jsonCallbackMethod; - QObject* errorCallbackReceiver; QString errorCallbackMethod; - QObject* updateReceiver; - QString updateSlot; }; namespace AccountManagerAuth { diff --git a/libraries/networking/src/AddressManager.cpp b/libraries/networking/src/AddressManager.cpp index ea0ab44107..3fe75c5495 100644 --- a/libraries/networking/src/AddressManager.cpp +++ b/libraries/networking/src/AddressManager.cpp @@ -215,9 +215,8 @@ const JSONCallbackParameters& AddressManager::apiCallbackParameters() { static JSONCallbackParameters callbackParams; if (!hasSetupParameters) { - callbackParams.jsonCallbackReceiver = this; + callbackParams.callbackReceiver = this; callbackParams.jsonCallbackMethod = "handleAPIResponse"; - callbackParams.errorCallbackReceiver = this; callbackParams.errorCallbackMethod = "handleAPIError"; } @@ -917,7 +916,7 @@ void AddressManager::lookupShareableNameForDomainID(const QUuid& domainID) { // no error callback handling // in the case of an error we simply assume there is no default place name - callbackParams.jsonCallbackReceiver = this; + callbackParams.callbackReceiver = this; callbackParams.jsonCallbackMethod = "handleShareableNameAPIResponse"; DependencyManager::get()->sendRequest(GET_DOMAIN_ID.arg(uuidStringWithoutCurlyBraces(domainID)), diff --git a/libraries/networking/src/UserActivityLogger.cpp b/libraries/networking/src/UserActivityLogger.cpp index 31357591ae..a5ee417939 100644 --- a/libraries/networking/src/UserActivityLogger.cpp +++ b/libraries/networking/src/UserActivityLogger.cpp @@ -65,7 +65,7 @@ void UserActivityLogger::logAction(QString action, QJsonObject details, JSONCall // if no callbacks specified, call our owns if (params.isEmpty()) { - params.errorCallbackReceiver = this; + params.callbackReceiver = this; params.errorCallbackMethod = "requestError"; } diff --git a/libraries/ui/src/Tooltip.cpp b/libraries/ui/src/Tooltip.cpp index 22bc15d72f..bd2c4e6d8f 100644 --- a/libraries/ui/src/Tooltip.cpp +++ b/libraries/ui/src/Tooltip.cpp @@ -83,7 +83,7 @@ void Tooltip::requestHyperlinkImage() { auto accountManager = DependencyManager::get(); JSONCallbackParameters callbackParams; - callbackParams.jsonCallbackReceiver = this; + callbackParams.callbackReceiver = this; callbackParams.jsonCallbackMethod = "handleAPIResponse"; accountManager->sendRequest(GET_PLACE.arg(_title),