From 6b2c9cbede6ade42081ca222aff56968b3a1b724 Mon Sep 17 00:00:00 2001 From: amzn-phist <52085794+amzn-phist@users.noreply.github.com> Date: Fri, 6 Aug 2021 16:23:01 -0500 Subject: [PATCH] Fix a crash when reloading AudioControlEditor controls (#2729) * Fix a crash when reloading ACE controls data The crash was due to destruction of xml_node that was held in a unique_ptr. Rapidxml has a very rudimentary memory allocation design, so in most cases dynamic allocations aren't even made. The memory_pool does all the cleanup in its destructor, so having a unique_ptr run its default_delete was causing the crash. Signed-off-by: amzn-phist <52085794+amzn-phist@users.noreply.github.com> * Fix numerical conversion warnings Wwise source files needed a few fixes for the numerical conversion warning changes that went in recently. Signed-off-by: amzn-phist <52085794+amzn-phist@users.noreply.github.com> --- .../Source/Engine/FileIOHandler_wwise.cpp | 8 +-- .../Code/Source/Editor/AudioControl.cpp | 2 +- .../Code/Source/Editor/AudioControl.h | 50 ++++++++++--------- .../Source/Editor/AudioControlsLoader.cpp | 8 +-- .../Source/Editor/AudioControlsWriter.cpp | 6 +-- 5 files changed, 38 insertions(+), 36 deletions(-) diff --git a/Gems/AudioEngineWwise/Code/Source/Engine/FileIOHandler_wwise.cpp b/Gems/AudioEngineWwise/Code/Source/Engine/FileIOHandler_wwise.cpp index 9571dd86a9..30084565d6 100644 --- a/Gems/AudioEngineWwise/Code/Source/Engine/FileIOHandler_wwise.cpp +++ b/Gems/AudioEngineWwise/Code/Source/Engine/FileIOHandler_wwise.cpp @@ -69,7 +69,7 @@ namespace Audio AkDeviceSettings deviceSettings; AK::StreamMgr::GetDefaultDeviceSettings(deviceSettings); - deviceSettings.uIOMemorySize = poolSize; + deviceSettings.uIOMemorySize = aznumeric_cast(poolSize); deviceSettings.uSchedulerTypeFlags = AK_SCHEDULER_BLOCKING; Platform::SetThreadProperties(deviceSettings.threadProperties); @@ -198,7 +198,7 @@ namespace Audio deviceDesc.bCanWrite = true; deviceDesc.deviceID = m_deviceID; AK_CHAR_TO_UTF16(deviceDesc.szDeviceName, "IO::IArchive", AZ_ARRAY_SIZE(deviceDesc.szDeviceName)); - deviceDesc.uStringSize = AKPLATFORM::AkUtf16StrLen(deviceDesc.szDeviceName); + deviceDesc.uStringSize = aznumeric_cast(AKPLATFORM::AkUtf16StrLen(deviceDesc.szDeviceName)); } AkUInt32 CBlockingDevice_wwise::GetDeviceData() @@ -219,7 +219,7 @@ namespace Audio AkDeviceSettings deviceSettings; AK::StreamMgr::GetDefaultDeviceSettings(deviceSettings); - deviceSettings.uIOMemorySize = poolSize; + deviceSettings.uIOMemorySize = aznumeric_cast(poolSize); deviceSettings.uSchedulerTypeFlags = AK_SCHEDULER_DEFERRED_LINED_UP; Platform::SetThreadProperties(deviceSettings.threadProperties); @@ -336,7 +336,7 @@ namespace Audio deviceDesc.bCanWrite = false; deviceDesc.deviceID = m_deviceID; AK_CHAR_TO_UTF16(deviceDesc.szDeviceName, "IO::IStreamer", AZ_ARRAY_SIZE(deviceDesc.szDeviceName)); - deviceDesc.uStringSize = AKPLATFORM::AkUtf16StrLen(deviceDesc.szDeviceName); + deviceDesc.uStringSize = aznumeric_cast(AKPLATFORM::AkUtf16StrLen(deviceDesc.szDeviceName)); } AkUInt32 CStreamingDevice_wwise::GetDeviceData() diff --git a/Gems/AudioSystem/Code/Source/Editor/AudioControl.cpp b/Gems/AudioSystem/Code/Source/Editor/AudioControl.cpp index ce268022a8..c0a8fb8dde 100644 --- a/Gems/AudioSystem/Code/Source/Editor/AudioControl.cpp +++ b/Gems/AudioSystem/Code/Source/Editor/AudioControl.cpp @@ -345,7 +345,7 @@ namespace AudioControls { for (auto& connectionNode : m_connectionNodes) { - if (TConnectionPtr connection = audioSystemImpl->CreateConnectionFromXMLNode(connectionNode.m_xmlNode.get(), m_type)) + if (TConnectionPtr connection = audioSystemImpl->CreateConnectionFromXMLNode(connectionNode.m_xmlNode, m_type)) { AddConnection(connection); connectionNode.m_isValid = true; diff --git a/Gems/AudioSystem/Code/Source/Editor/AudioControl.h b/Gems/AudioSystem/Code/Source/Editor/AudioControl.h index 37e67c815c..024e8eb6df 100644 --- a/Gems/AudioSystem/Code/Source/Editor/AudioControl.h +++ b/Gems/AudioSystem/Code/Source/Editor/AudioControl.h @@ -25,46 +25,48 @@ namespace AudioControls { SRawConnectionData(AZ::rapidxml::xml_node* node, bool isValid) { - m_xmlNode = AZStd::move(DeepCopyNode(node)); + m_xmlNode = DeepCopyNode(node); m_isValid = isValid; } - AZStd::unique_ptr> m_xmlNode{}; + AZ::rapidxml::xml_node* m_xmlNode = nullptr; // indicates if the connection is valid for the currently loaded middleware bool m_isValid{ false }; + private: // Rapid XML provides a 'clone_node' utility that will copy an entire node tree, // but it only copies pointers of any strings in the node names and values. - // This causes problems with storing raw xml nodes as this class does because strings - // will be pointing into the memory pool of an xml document that has gone out of scope. + // This causes problems with storage of xml trees, as this class does, because strings + // will be pointing into an xml document's file buffer that has gone out of scope. // This function is a rewritten version of 'clone_node' that does the deep copy of strings // into the new destination tree. - [[nodiscard]] static AZStd::unique_ptr> DeepCopyNode(AZ::rapidxml::xml_node* srcNode) + [[nodiscard]] AZ::rapidxml::xml_node* DeepCopyNode(AZ::rapidxml::xml_node* srcNode) { - AZStd::unique_ptr> destNode; - if (srcNode) + if (!srcNode) { - XmlAllocator& xmlAlloc(AudioControls::s_xmlAllocator); - destNode.reset(xmlAlloc.allocate_node(srcNode->type())); + return nullptr; + } - destNode->name(xmlAlloc.allocate_string(srcNode->name(), srcNode->name_size()), srcNode->name_size()); - destNode->value(xmlAlloc.allocate_string(srcNode->value(), srcNode->value_size()), srcNode->value_size()); + XmlAllocator& xmlAlloc(AudioControls::s_xmlAllocator); + AZ::rapidxml::xml_node* destNode = xmlAlloc.allocate_node(srcNode->type()); - for (AZ::rapidxml::xml_node* child = srcNode->first_node(); child != nullptr; child = child->next_sibling()) - { - destNode->append_node(DeepCopyNode(child).release()); - } + destNode->name(xmlAlloc.allocate_string(srcNode->name(), srcNode->name_size()), srcNode->name_size()); + destNode->value(xmlAlloc.allocate_string(srcNode->value(), srcNode->value_size()), srcNode->value_size()); - for (AZ::rapidxml::xml_attribute* attr = srcNode->first_attribute(); attr != nullptr; attr = attr->next_attribute()) - { - destNode->append_attribute(xmlAlloc.allocate_attribute( - xmlAlloc.allocate_string(attr->name(), attr->name_size()), - xmlAlloc.allocate_string(attr->value(), attr->value_size()), - attr->name_size(), - attr->value_size() - )); - } + for (AZ::rapidxml::xml_node* child = srcNode->first_node(); child != nullptr; child = child->next_sibling()) + { + destNode->append_node(DeepCopyNode(child)); + } + + for (AZ::rapidxml::xml_attribute* attr = srcNode->first_attribute(); attr != nullptr; attr = attr->next_attribute()) + { + destNode->append_attribute(xmlAlloc.allocate_attribute( + xmlAlloc.allocate_string(attr->name(), attr->name_size()), + xmlAlloc.allocate_string(attr->value(), attr->value_size()), + attr->name_size(), + attr->value_size() + )); } return destNode; diff --git a/Gems/AudioSystem/Code/Source/Editor/AudioControlsLoader.cpp b/Gems/AudioSystem/Code/Source/Editor/AudioControlsLoader.cpp index 220cd32b5c..30889e33f0 100644 --- a/Gems/AudioSystem/Code/Source/Editor/AudioControlsLoader.cpp +++ b/Gems/AudioSystem/Code/Source/Editor/AudioControlsLoader.cpp @@ -475,7 +475,7 @@ namespace AudioControls control->AddConnection(connection); } - control->m_connectionNodes.push_back(SRawConnectionData(childNode, connection != nullptr)); + control->m_connectionNodes.emplace_back(childNode, connection != nullptr); childNode = childNode->next_sibling(); } @@ -517,7 +517,7 @@ namespace AudioControls { control->AddConnection(connection); } - control->m_connectionNodes.push_back(SRawConnectionData(connectionNode, connection != nullptr)); + control->m_connectionNodes.emplace_back(connectionNode, connection != nullptr); connectionNode = connectionNode->next_sibling(); } configGroupNode = configGroupNode->next_sibling(); @@ -534,7 +534,7 @@ namespace AudioControls { control->AddConnection(connection); } - control->m_connectionNodes.push_back(SRawConnectionData(connectionNode, connection != nullptr)); + control->m_connectionNodes.emplace_back(connectionNode, connection != nullptr); connectionNode = connectionNode->next_sibling(); } } @@ -576,7 +576,7 @@ namespace AudioControls requestNode->append_node(valueNode); - childControl->m_connectionNodes.push_back(SRawConnectionData(requestNode, false)); + childControl->m_connectionNodes.emplace_back(requestNode, false); return childControl; } diff --git a/Gems/AudioSystem/Code/Source/Editor/AudioControlsWriter.cpp b/Gems/AudioSystem/Code/Source/Editor/AudioControlsWriter.cpp index 91f9aa5138..eb022b706f 100644 --- a/Gems/AudioSystem/Code/Source/Editor/AudioControlsWriter.cpp +++ b/Gems/AudioSystem/Code/Source/Editor/AudioControlsWriter.cpp @@ -356,8 +356,8 @@ namespace AudioControls { if (!connectionNode.m_isValid) { - auto nodeCopy = SRawConnectionData::DeepCopyNode(connectionNode.m_xmlNode.get()); - node->append_node(nodeCopy.release()); + XmlAllocator& xmlAlloc(AudioControls::s_xmlAllocator); + node->append_node(xmlAlloc.clone_node(connectionNode.m_xmlNode)); } } @@ -371,7 +371,7 @@ namespace AudioControls childNode != nullptr) { node->append_node(childNode); - control->m_connectionNodes.push_back(SRawConnectionData(childNode, true)); + control->m_connectionNodes.emplace_back(childNode, true); } } }