Merge pull request #694 from aws-lumberyard-dev/mbalfour/slice-prefab-converter

Fix up incorrect autoDelete use and handling.

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.");
+            }
         }
 
 

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.
     }
 
     //

Gems/GradientSignal/Code/Source/UI/GradientPreviewDataWidget.h

@@ -76,8 +76,5 @@ namespace GradientSignal
 
         static void Register();
         static void Unregister();
-
-    private:
-        static GradientPreviewDataWidgetHandler* s_instance;
     };
 }

Gems/ScriptCanvas/Code/Editor/SystemComponent.cpp

@@ -134,7 +134,7 @@ namespace ScriptCanvasEditor
 
         PopulateEditorCreatableTypes();
 
-        m_propertyHandlers.emplace_back(AzToolsFramework::RegisterGenericComboBoxHandler<ScriptCanvas::VariableId>());
+        AzToolsFramework::RegisterGenericComboBoxHandler<ScriptCanvas::VariableId>();
 
         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();

Gems/ScriptCanvas/Code/Editor/SystemComponent.h

@@ -147,7 +147,6 @@ namespace ScriptCanvasEditor
         AZStd::unique_ptr<AZ::JobManager> m_jobManager;
         AZStd::unique_ptr<AZ::JobContext> m_jobContext;
 
-        AZStd::vector<AZStd::unique_ptr<AzToolsFramework::PropertyHandlerBase>> m_propertyHandlers;
         AZStd::unordered_set<ScriptCanvas::Data::Type> m_creatableTypes;
 
         AssetTracker m_assetTracker;

Gems/ScriptEvents/Code/Source/Editor/ScriptEventsSystemEditorComponent.cpp

@@ -236,17 +236,11 @@ namespace ScriptEventsEditor
             moduleConfiguration->RegisterAssetHandler();
         }
 
-        m_propertyHandlers.emplace_back(AzToolsFramework::RegisterGenericComboBoxHandler<ScriptEventData::VersionedProperty>());
+        AzToolsFramework::RegisterGenericComboBoxHandler<ScriptEventData::VersionedProperty>();
     }
 
     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);

Gems/ScriptEvents/Code/Source/Editor/ScriptEventsSystemEditorComponent.h

@@ -90,8 +90,6 @@ namespace ScriptEventsEditor
         void Deactivate() override;
         ////////////////////////////////////////////////////////////////////////
 
-        AZStd::vector<AZStd::unique_ptr<AzToolsFramework::PropertyHandlerBase>> m_propertyHandlers;
-
         // Script Event Assets
         AZStd::unordered_map<ScriptEvents::ScriptEventKey, AZStd::intrusive_ptr<ScriptEvents::Internal::ScriptEventRegistration>> m_scriptEvents;
 

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<AZ::TypeId>();
-        AzToolsFramework::PropertyTypeRegistrationMessages::Bus::Broadcast(&AzToolsFramework::PropertyTypeRegistrationMessages::RegisterPropertyType, m_propertyHandler);
+        AzToolsFramework::RegisterGenericComboBoxHandler<AZ::TypeId>();
     }
 
     void EditorVegetationSystemComponent::Deactivate()
     {
-        AzToolsFramework::PropertyTypeRegistrationMessages::Bus::Broadcast(&AzToolsFramework::PropertyTypeRegistrationMessages::UnregisterPropertyType, m_propertyHandler);
-        delete m_propertyHandler;
     }
 
 }

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