Fix memory leaks (#12900)

* Fix leak for PublicAccessEntry

* Fix memory leak for PropertyTypeCollection

* Don't clone the lazy property group ID when caching property types, it is explicitly assigned at runtime

(cherry picked from commit 10b8f63052)

src/Umbraco.Core/Collections/DeepCloneableList.cs

@@ -41,17 +41,7 @@ public class DeepCloneableList<T> : List<T>, IDeepCloneable, IRememberBeingDirty
                 // we are cloning once, so create a new list in none mode
                 // and deep clone all items into it
                 var newList = new DeepCloneableList<T>(ListCloneBehavior.None);
-                foreach (T item in this)
-                {
-                    if (item is IDeepCloneable dc)
-                    {
-                        newList.Add((T)dc.DeepClone());
-                    }
-                    else
-                    {
-                        newList.Add(item);
-                    }
-                }
+                DeepCloneHelper.CloneListItems<DeepCloneableList<T>, T>(this, newList);
 
                 return newList;
             case ListCloneBehavior.None:
@@ -60,17 +50,7 @@ public class DeepCloneableList<T> : List<T>, IDeepCloneable, IRememberBeingDirty
             case ListCloneBehavior.Always:
                 // always clone to new list
                 var newList2 = new DeepCloneableList<T>(ListCloneBehavior.Always);
-                foreach (T item in this)
-                {
-                    if (item is IDeepCloneable dc)
-                    {
-                        newList2.Add((T)dc.DeepClone());
-                    }
-                    else
-                    {
-                        newList2.Add(item);
-                    }
-                }
+                DeepCloneHelper.CloneListItems<DeepCloneableList<T>, T>(this, newList2);
 
                 return newList2;
             default:

src/Umbraco.Core/Collections/EventClearingObservableCollection.cs

@@ -1,5 +1,6 @@
 using System.Collections.ObjectModel;
 using System.Collections.Specialized;
+using Umbraco.Cms.Core.Models;
 
 namespace Umbraco.Cms.Core.Collections;
 
@@ -7,7 +8,7 @@ namespace Umbraco.Cms.Core.Collections;
 ///     Allows clearing all event handlers
 /// </summary>
 /// <typeparam name="TValue"></typeparam>
-public class EventClearingObservableCollection<TValue> : ObservableCollection<TValue>, INotifyCollectionChanged
+public class EventClearingObservableCollection<TValue> : ObservableCollection<TValue>, INotifyCollectionChanged, IDeepCloneable
 {
     // need to explicitly implement with event accessor syntax in order to override in order to to clear
     // c# events are weird, they do not behave the same way as other c# things that are 'virtual',
@@ -39,4 +40,12 @@ public class EventClearingObservableCollection<TValue> : ObservableCollection<TV
     ///     Clears all event handlers for the <see cref="CollectionChanged" /> event
     /// </summary>
     public void ClearCollectionChangedEvents() => _changed = null;
+
+    public object DeepClone()
+    {
+        var clone = new EventClearingObservableCollection<TValue>();
+        DeepCloneHelper.CloneListItems<EventClearingObservableCollection<TValue>, TValue>(this, clone);
+
+        return clone;
+    }
 }

src/Umbraco.Core/Models/ContentBase.cs

@@ -60,15 +60,8 @@ public abstract class ContentBase : TreeEntityBase, IContentBase
         _contentTypeId = contentType.Id;
         _properties = properties ?? throw new ArgumentNullException(nameof(properties));
         _properties.EnsurePropertyTypes(contentType.CompositionPropertyTypes);
-
-        // track all property types on this content type, these can never change during the lifetime of this single instance
-        // there is no real extra memory overhead of doing this since these property types are already cached on this object via the
-        // properties already.
-        AllPropertyTypes = new List<IPropertyType>(contentType.CompositionPropertyTypes);
     }
 
-    internal IReadOnlyList<IPropertyType> AllPropertyTypes { get; }
-
     [IgnoreDataMember]
     public ISimpleContentType ContentType { get; private set; }
 
@@ -146,7 +139,6 @@ public abstract class ContentBase : TreeEntityBase, IContentBase
         base.PerformDeepClone(clone);
 
         var clonedContent = (ContentBase)clone;
-
         // Need to manually clone this since it's not settable
         clonedContent.ContentType = ContentType;
 

src/Umbraco.Core/Models/DeepCloneHelper.cs

@@ -212,4 +212,16 @@ public static class DeepCloneHelper
 
         public bool IsList => GenericListType != null;
     }
+
+    public static void CloneListItems<TList, TEntity>(TList source, TList target)
+        where TList : ICollection<TEntity>
+    {
+        target.Clear();
+        foreach (TEntity entity in source)
+        {
+            target.Add(entity is IDeepCloneable deepCloneableEntity
+                ? (TEntity)deepCloneableEntity.DeepClone()
+                : entity);
+        }
+    }
 }

src/Umbraco.Core/Models/Entities/BeingDirtyBase.cs

@@ -85,6 +85,8 @@ public abstract class BeingDirtyBase : IRememberBeingDirty
     /// </summary>
     public event PropertyChangedEventHandler? PropertyChanged;
 
+    protected void ClearPropertyChangedEvents() => PropertyChanged = null;
+
     /// <summary>
     ///     Registers that a property has changed.
     /// </summary>

src/Umbraco.Core/Models/PropertyType.cs

@@ -283,12 +283,7 @@ public class PropertyType : EntityBase, IPropertyType, IEquatable<PropertyType>
         base.PerformDeepClone(clone);
 
         var clonedEntity = (PropertyType)clone;
-
-        // need to manually assign the Lazy value as it will not be automatically mapped
-        if (PropertyGroupId != null)
-        {
-            clonedEntity._propertyGroupId = new Lazy<int>(() => PropertyGroupId.Value);
-        }
+        clonedEntity.ClearPropertyChangedEvents();
     }
 
     /// <summary>

src/Umbraco.Core/Models/PublicAccessEntry.cs

@@ -10,7 +10,7 @@ namespace Umbraco.Cms.Core.Models;
 public class PublicAccessEntry : EntityBase
 {
     private readonly List<Guid> _removedRules = new();
-    private readonly EventClearingObservableCollection<PublicAccessRule> _ruleCollection;
+    private EventClearingObservableCollection<PublicAccessRule> _ruleCollection;
     private int _loginNodeId;
     private int _noAccessNodeId;
     private int _protectedNodeId;
@@ -144,11 +144,13 @@ public class PublicAccessEntry : EntityBase
 
         var cloneEntity = (PublicAccessEntry)clone;
 
-        if (cloneEntity._ruleCollection != null)
-        {
-            cloneEntity._ruleCollection.ClearCollectionChangedEvents(); // clear this event handler if any
-            cloneEntity._ruleCollection.CollectionChanged +=
-                cloneEntity.RuleCollection_CollectionChanged; // re-assign correct event handler
-        }
+        // clear this event handler if any
+        cloneEntity._ruleCollection.ClearCollectionChangedEvents();
+
+        // clone the rule collection explicitly
+        cloneEntity._ruleCollection = (EventClearingObservableCollection<PublicAccessRule>)_ruleCollection.DeepClone();
+
+        // re-assign correct event handler
+        cloneEntity._ruleCollection.CollectionChanged += cloneEntity.RuleCollection_CollectionChanged;
     }
 }