From 9a7bebaaa2a12ba5dff7305c1e3ee88c973ad729 Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Thu, 28 Apr 2016 18:04:04 -0700 Subject: [PATCH 1/2] Fix dangling reference into container --- libraries/networking/src/AssetClient.cpp | 138 +++++++++++++++++------ libraries/networking/src/AssetClient.h | 3 + 2 files changed, 107 insertions(+), 34 deletions(-) diff --git a/libraries/networking/src/AssetClient.cpp b/libraries/networking/src/AssetClient.cpp index 34ba5fc785..080b0c9b90 100644 --- a/libraries/networking/src/AssetClient.cpp +++ b/libraries/networking/src/AssetClient.cpp @@ -325,47 +325,117 @@ void AssetClient::handleAssetGetReply(QSharedPointer message, S // Check if we have any pending requests for this node auto messageMapIt = _pendingRequests.find(senderNode); - if (messageMapIt != _pendingRequests.end()) { + if (messageMapIt == _pendingRequests.end()) { + return; + } - // Found the node, get the MessageID -> Callback map - auto& messageCallbackMap = messageMapIt->second; - - // Check if we have this pending request - auto requestIt = messageCallbackMap.find(messageID); - if (requestIt != messageCallbackMap.end()) { - auto& callbacks = requestIt->second; - - // Store message in case we need to disconnect from it later. - callbacks.message = message; - - if (message->isComplete()) { - callbacks.completeCallback(true, error, message->readAll()); - messageCallbackMap.erase(requestIt); - } else { - connect(message.data(), &ReceivedMessage::progress, this, [this, length, message, &callbacks]() { - callbacks.progressCallback(message->getSize(), length); - }); - connect(message.data(), &ReceivedMessage::completed, this, [this, messageID, message, &messageCallbackMap, &callbacks]() { - if (message->failed()) { - callbacks.completeCallback(false, AssetServerError::NoError, QByteArray()); - } else { - callbacks.completeCallback(true, AssetServerError::NoError, message->readAll()); - } - - // We should never get to this point without the associated senderNode and messageID - // in our list of pending requests. If the senderNode had disconnected or the message - // had been canceled, we should have been disconnected from the ReceivedMessage - // signals and thus never had this lambda called. - messageCallbackMap.erase(messageID); - }); - } - } + // Found the node, get the MessageID -> Callback map + auto& messageCallbackMap = messageMapIt->second; + // Check if we have this pending request + auto requestIt = messageCallbackMap.find(messageID); + if (requestIt == messageCallbackMap.end()) { // Although the messageCallbackMap may now be empty, we won't delete the node until we have disconnected from // it to avoid constantly creating/deleting the map on subsequent requests. + return; + } + + auto& callbacks = requestIt->second; + + // Store message in case we need to disconnect from it later. + callbacks.message = message; + + if (message->isComplete()) { + callbacks.completeCallback(true, error, message->readAll()); + messageCallbackMap.erase(requestIt); + } else { + auto weakNode = senderNode.toWeakRef(); + + connect(message.data(), &ReceivedMessage::progress, this, [this, weakNode, messageID, length]() { + handleProgressCallback(weakNode, messageID, length); + }); + connect(message.data(), &ReceivedMessage::completed, this, [this, weakNode, messageID]() { + handleCompleteCallback(weakNode, messageID); + }); } } +void AssetClient::handleProgressCallback(const QWeakPointer& node, MessageID messageID, DataOffset length) { + auto senderNode = node.toStrongRef(); + + if (!senderNode) { + return; + } + + // Check if we have any pending requests for this node + auto messageMapIt = _pendingRequests.find(senderNode); + if (messageMapIt == _pendingRequests.end()) { + return; + } + + // Found the node, get the MessageID -> Callback map + auto& messageCallbackMap = messageMapIt->second; + + // Check if we have this pending request + auto requestIt = messageCallbackMap.find(messageID); + if (requestIt == messageCallbackMap.end()) { + return; + } + + auto& callbacks = requestIt->second; + auto& message = callbacks.message; + + if (!message) { + return; + } + + callbacks.progressCallback(message->getSize(), length); +} + +void AssetClient::handleCompleteCallback(const QWeakPointer& node, MessageID messageID) { + auto senderNode = node.toStrongRef(); + + if (!senderNode) { + return; + } + + // Check if we have any pending requests for this node + auto messageMapIt = _pendingRequests.find(senderNode); + if (messageMapIt == _pendingRequests.end()) { + return; + } + + // Found the node, get the MessageID -> Callback map + auto& messageCallbackMap = messageMapIt->second; + + // Check if we have this pending request + auto requestIt = messageCallbackMap.find(messageID); + if (requestIt == messageCallbackMap.end()) { + return; + } + + auto& callbacks = requestIt->second; + auto& message = callbacks.message; + + if (!message) { + return; + } + + + if (message->failed()) { + callbacks.completeCallback(false, AssetServerError::NoError, QByteArray()); + } else { + callbacks.completeCallback(true, AssetServerError::NoError, message->readAll()); + } + + // We should never get to this point without the associated senderNode and messageID + // in our list of pending requests. If the senderNode had disconnected or the message + // had been canceled, we should have been disconnected from the ReceivedMessage + // signals and thus never had this lambda called. + messageCallbackMap.erase(messageID); +} + + MessageID AssetClient::getAssetMapping(const AssetPath& path, MappingOperationCallback callback) { Q_ASSERT(QThread::currentThread() == thread()); diff --git a/libraries/networking/src/AssetClient.h b/libraries/networking/src/AssetClient.h index c0e95c0d1d..c5c9b5fa43 100644 --- a/libraries/networking/src/AssetClient.h +++ b/libraries/networking/src/AssetClient.h @@ -74,6 +74,9 @@ private slots: void handleAssetGetReply(QSharedPointer message, SharedNodePointer senderNode); void handleAssetUploadReply(QSharedPointer message, SharedNodePointer senderNode); + void handleProgressCallback(const QWeakPointer& node, MessageID messageID, DataOffset length); + void handleCompleteCallback(const QWeakPointer& node, MessageID messageID); + void handleNodeKilled(SharedNodePointer node); private: From 6d9080162cb11b7beacb9d8844f3e0ec3add4370 Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Fri, 29 Apr 2016 17:39:53 -0700 Subject: [PATCH 2/2] Make slots private method --- libraries/networking/src/AssetClient.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libraries/networking/src/AssetClient.h b/libraries/networking/src/AssetClient.h index c5c9b5fa43..f951be762d 100644 --- a/libraries/networking/src/AssetClient.h +++ b/libraries/networking/src/AssetClient.h @@ -74,9 +74,6 @@ private slots: void handleAssetGetReply(QSharedPointer message, SharedNodePointer senderNode); void handleAssetUploadReply(QSharedPointer message, SharedNodePointer senderNode); - void handleProgressCallback(const QWeakPointer& node, MessageID messageID, DataOffset length); - void handleCompleteCallback(const QWeakPointer& node, MessageID messageID); - void handleNodeKilled(SharedNodePointer node); private: @@ -96,6 +93,9 @@ private: bool cancelGetAssetRequest(MessageID id); bool cancelUploadAssetRequest(MessageID id); + void handleProgressCallback(const QWeakPointer& node, MessageID messageID, DataOffset length); + void handleCompleteCallback(const QWeakPointer& node, MessageID messageID); + struct GetAssetRequestData { QSharedPointer message; ReceivedAssetCallback completeCallback;