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.

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