From 704443ac89eeb0774c489ddacbbf6289caa63a7f Mon Sep 17 00:00:00 2001 From: mbalfour Date: Tue, 11 May 2021 10:25:03 -0500 Subject: [PATCH] Fix up incorrect autoDelete use and handling. Several UI property handlers were incorrectly using the autoDelete feature by calling UnregisterPropertyType and deleting the pointer themselves, which caused double-delete crashes on application shutdown. PropertyManagerComponent now gracefully handles that condition but also explicitly asserts explaining how the code should be changed, and the "known offenders" have been fixed up to use autoDelete correctly. --- .../PropertyManagerComponent.cpp | 33 +++++++++++++++++-- .../Source/UI/GradientPreviewDataWidget.cpp | 23 +++++-------- .../Source/UI/GradientPreviewDataWidget.h | 3 -- .../Code/Editor/SystemComponent.cpp | 8 +---- .../Code/Editor/SystemComponent.h | 1 - .../ScriptEventsSystemEditorComponent.cpp | 8 +---- .../ScriptEventsSystemEditorComponent.h | 2 -- .../EditorVegetationSystemComponent.cpp | 5 +-- .../Editor/EditorVegetationSystemComponent.h | 3 -- 9 files changed, 41 insertions(+), 45 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/UI/PropertyEditor/PropertyManagerComponent.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/UI/PropertyEditor/PropertyManagerComponent.cpp index 21f3b7d1c6..bd61e6ceed 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/UI/PropertyEditor/PropertyManagerComponent.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/UI/PropertyEditor/PropertyManagerComponent.cpp @@ -63,23 +63,40 @@ namespace AzToolsFramework void PropertyManagerComponent::Deactivate() { + // Delete all remaining auto-delete or built-in handlers. for (auto it = m_builtInHandlers.begin(); it != m_builtInHandlers.end(); ++it) { - UnregisterPropertyType(*it); +#ifdef AZ_DEBUG_BUILD + // For debug builds, we'll take the extra time to delete each handler that we're deleting from m_Handlers. + // We loop through m_Handlers below to ensure that we don't have any other handlers still registered after + // we've deleted these. + AZStd::erase_if(m_Handlers, [it](const auto& item) { + auto const& [key, value] = item; + return (key == (*it)->GetHandlerName()) && (value == (*it)); + }); +#endif + delete *it; } m_builtInHandlers.clear(); - #ifdef _DEBUG +#ifdef AZ_DEBUG_BUILD + // Loop through all the remaining registered handlers (if any) and print out an error, as these all are probably memory + // leaks. UnregisterPropertyType should have been called on these already, and their pointers should have been deleted + // by the caller. auto it = m_Handlers.begin(); while (it != m_Handlers.end()) { AZ_Error("PropertyManager", false, "Property Handler 0x%08x is still registered during shutdown", it->first); ++it; } - #endif +#endif + + m_Handlers.clear(); + m_DefaultHandlers.clear(); + PropertyTypeRegistrationMessages::Bus::Handler::BusDisconnect(); } @@ -142,6 +159,16 @@ namespace AzToolsFramework } ++defaultIt; } + + if (pHandler->AutoDelete()) + { + m_builtInHandlers.erase(AZStd::remove(m_builtInHandlers.begin(), m_builtInHandlers.end(), pHandler), + m_builtInHandlers.end()); + AZ_Assert(false, + "Handlers with AutoDelete set should not call UnregisterPropertyType. To fix, do one of the following:\n" + " 1. Set AutoDelete to false in the handler, call UnregisterPropertyType, and the caller should delete the handler.\n" + " 2. Set AutoDelete to true in the handler and do NOT call UnregisterPropertyType or delete the handler."); + } } diff --git a/Gems/GradientSignal/Code/Source/UI/GradientPreviewDataWidget.cpp b/Gems/GradientSignal/Code/Source/UI/GradientPreviewDataWidget.cpp index 14f21c105d..d0f45be56d 100644 --- a/Gems/GradientSignal/Code/Source/UI/GradientPreviewDataWidget.cpp +++ b/Gems/GradientSignal/Code/Source/UI/GradientPreviewDataWidget.cpp @@ -25,8 +25,6 @@ namespace GradientSignal // GradientPreviewDataWidgetHandler // - GradientPreviewDataWidgetHandler* GradientPreviewDataWidgetHandler::s_instance = nullptr; - AZ::u32 GradientPreviewDataWidgetHandler::GetHandlerName() const { return AZ_CRC("GradientPreviewer", 0x1dbbba45); @@ -92,23 +90,18 @@ namespace GradientSignal { using namespace AzToolsFramework; - if (!s_instance) - { - s_instance = aznew GradientPreviewDataWidgetHandler(); - PropertyTypeRegistrationMessages::Bus::Broadcast(&PropertyTypeRegistrationMessages::Bus::Events::RegisterPropertyType, s_instance); - } + // Property handlers are set to auto-delete by default, which means that we're handing off ownership of the pointer to the + // PropertyManagerComponent, where it will get cleaned up on system shutdown. + auto propertyHandler = aznew GradientPreviewDataWidgetHandler(); + AZ_Assert(propertyHandler->AutoDelete(), + "GradientPreviewDataWidgetHandler is no longer set to auto-delete, it will leak memory."); + PropertyTypeRegistrationMessages::Bus::Broadcast( + &PropertyTypeRegistrationMessages::Bus::Events::RegisterPropertyType, propertyHandler); } void GradientPreviewDataWidgetHandler::Unregister() { - using namespace AzToolsFramework; - - if (s_instance) - { - PropertyTypeRegistrationMessages::Bus::Broadcast(&PropertyTypeRegistrationMessages::Bus::Events::UnregisterPropertyType, s_instance); - delete s_instance; - s_instance = nullptr; - } + // We don't need to call UnregisterPropertyType here because it's an autoDelete handler. } // diff --git a/Gems/GradientSignal/Code/Source/UI/GradientPreviewDataWidget.h b/Gems/GradientSignal/Code/Source/UI/GradientPreviewDataWidget.h index b5ee732ed7..cb5c0f8f20 100644 --- a/Gems/GradientSignal/Code/Source/UI/GradientPreviewDataWidget.h +++ b/Gems/GradientSignal/Code/Source/UI/GradientPreviewDataWidget.h @@ -76,8 +76,5 @@ namespace GradientSignal static void Register(); static void Unregister(); - - private: - static GradientPreviewDataWidgetHandler* s_instance; }; } diff --git a/Gems/ScriptCanvas/Code/Editor/SystemComponent.cpp b/Gems/ScriptCanvas/Code/Editor/SystemComponent.cpp index 7d9cb176a3..60e8e5f23a 100644 --- a/Gems/ScriptCanvas/Code/Editor/SystemComponent.cpp +++ b/Gems/ScriptCanvas/Code/Editor/SystemComponent.cpp @@ -134,7 +134,7 @@ namespace ScriptCanvasEditor PopulateEditorCreatableTypes(); - m_propertyHandlers.emplace_back(AzToolsFramework::RegisterGenericComboBoxHandler()); + AzToolsFramework::RegisterGenericComboBoxHandler(); SystemRequestBus::Handler::BusConnect(); ScriptCanvasExecutionBus::Handler::BusConnect(); @@ -177,12 +177,6 @@ namespace ScriptCanvasEditor SystemRequestBus::Handler::BusDisconnect(); AzFramework::AssetCatalogEventBus::Handler::BusDisconnect(); - for (auto&& propertyHandler : m_propertyHandlers) - { - AzToolsFramework::PropertyTypeRegistrationMessages::Bus::Broadcast(&AzToolsFramework::PropertyTypeRegistrationMessages::UnregisterPropertyType, propertyHandler.get()); - } - m_propertyHandlers.clear(); - m_jobContext.reset(); m_jobManager.reset(); m_assetTracker.Deactivate(); diff --git a/Gems/ScriptCanvas/Code/Editor/SystemComponent.h b/Gems/ScriptCanvas/Code/Editor/SystemComponent.h index 07ea44e482..f2ebad38cd 100644 --- a/Gems/ScriptCanvas/Code/Editor/SystemComponent.h +++ b/Gems/ScriptCanvas/Code/Editor/SystemComponent.h @@ -147,7 +147,6 @@ namespace ScriptCanvasEditor AZStd::unique_ptr m_jobManager; AZStd::unique_ptr m_jobContext; - AZStd::vector> m_propertyHandlers; AZStd::unordered_set m_creatableTypes; AssetTracker m_assetTracker; diff --git a/Gems/ScriptEvents/Code/Source/Editor/ScriptEventsSystemEditorComponent.cpp b/Gems/ScriptEvents/Code/Source/Editor/ScriptEventsSystemEditorComponent.cpp index 5a4bc33550..f895517f8d 100644 --- a/Gems/ScriptEvents/Code/Source/Editor/ScriptEventsSystemEditorComponent.cpp +++ b/Gems/ScriptEvents/Code/Source/Editor/ScriptEventsSystemEditorComponent.cpp @@ -236,17 +236,11 @@ namespace ScriptEventsEditor moduleConfiguration->RegisterAssetHandler(); } - m_propertyHandlers.emplace_back(AzToolsFramework::RegisterGenericComboBoxHandler()); + AzToolsFramework::RegisterGenericComboBoxHandler(); } void ScriptEventEditorSystemComponent::Deactivate() { - for (auto&& propertyHandler : m_propertyHandlers) - { - AzToolsFramework::PropertyTypeRegistrationMessages::Bus::Broadcast(&AzToolsFramework::PropertyTypeRegistrationMessages::UnregisterPropertyType, propertyHandler.get()); - } - m_propertyHandlers.clear(); - using namespace ScriptEvents; ScriptEventsSystemComponentImpl* moduleConfiguration = nullptr; ScriptEventModuleConfigurationRequestBus::BroadcastResult(moduleConfiguration, &ScriptEventModuleConfigurationRequests::GetSystemComponentImpl); diff --git a/Gems/ScriptEvents/Code/Source/Editor/ScriptEventsSystemEditorComponent.h b/Gems/ScriptEvents/Code/Source/Editor/ScriptEventsSystemEditorComponent.h index d80dcffcf9..a5d34a9a9e 100644 --- a/Gems/ScriptEvents/Code/Source/Editor/ScriptEventsSystemEditorComponent.h +++ b/Gems/ScriptEvents/Code/Source/Editor/ScriptEventsSystemEditorComponent.h @@ -90,8 +90,6 @@ namespace ScriptEventsEditor void Deactivate() override; //////////////////////////////////////////////////////////////////////// - AZStd::vector> m_propertyHandlers; - // Script Event Assets AZStd::unordered_map> m_scriptEvents; diff --git a/Gems/Vegetation/Code/Source/Editor/EditorVegetationSystemComponent.cpp b/Gems/Vegetation/Code/Source/Editor/EditorVegetationSystemComponent.cpp index 00849ed94b..17174fa132 100644 --- a/Gems/Vegetation/Code/Source/Editor/EditorVegetationSystemComponent.cpp +++ b/Gems/Vegetation/Code/Source/Editor/EditorVegetationSystemComponent.cpp @@ -58,14 +58,11 @@ namespace Vegetation void EditorVegetationSystemComponent::Activate() { // This is necessary for the m_spawnerType in Descriptor.cpp to display properly as a ComboBox - m_propertyHandler = aznew AzToolsFramework::GenericComboBoxHandler(); - AzToolsFramework::PropertyTypeRegistrationMessages::Bus::Broadcast(&AzToolsFramework::PropertyTypeRegistrationMessages::RegisterPropertyType, m_propertyHandler); + AzToolsFramework::RegisterGenericComboBoxHandler(); } void EditorVegetationSystemComponent::Deactivate() { - AzToolsFramework::PropertyTypeRegistrationMessages::Bus::Broadcast(&AzToolsFramework::PropertyTypeRegistrationMessages::UnregisterPropertyType, m_propertyHandler); - delete m_propertyHandler; } } diff --git a/Gems/Vegetation/Code/Source/Editor/EditorVegetationSystemComponent.h b/Gems/Vegetation/Code/Source/Editor/EditorVegetationSystemComponent.h index 76ac881f7c..2b45303672 100644 --- a/Gems/Vegetation/Code/Source/Editor/EditorVegetationSystemComponent.h +++ b/Gems/Vegetation/Code/Source/Editor/EditorVegetationSystemComponent.h @@ -35,9 +35,6 @@ namespace Vegetation void Activate() override; void Deactivate() override; - - private: - AzToolsFramework::PropertyHandlerBase* m_propertyHandler{ nullptr }; }; } // namespace Vegetation