From 785c4440f3e3fc2ba684d6dd8d6eb1e714247161 Mon Sep 17 00:00:00 2001 From: Kenn Jacobsen Date: Thu, 25 Aug 2022 15:30:56 +0200 Subject: [PATCH] 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 10b8f63052c3c69c24c1d81d8e166003b3fc2db8) --- .../Collections/DeepCloneableList.cs | 24 ++----------------- .../EventClearingObservableCollection.cs | 11 ++++++++- src/Umbraco.Core/Models/ContentBase.cs | 8 ------- src/Umbraco.Core/Models/DeepCloneHelper.cs | 12 ++++++++++ .../Models/Entities/BeingDirtyBase.cs | 2 ++ src/Umbraco.Core/Models/PropertyType.cs | 7 +----- src/Umbraco.Core/Models/PublicAccessEntry.cs | 16 +++++++------ 7 files changed, 36 insertions(+), 44 deletions(-) diff --git a/src/Umbraco.Core/Collections/DeepCloneableList.cs b/src/Umbraco.Core/Collections/DeepCloneableList.cs index 301795281c..4f22ac094e 100644 --- a/src/Umbraco.Core/Collections/DeepCloneableList.cs +++ b/src/Umbraco.Core/Collections/DeepCloneableList.cs @@ -41,17 +41,7 @@ public class DeepCloneableList : List, 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(ListCloneBehavior.None); - foreach (T item in this) - { - if (item is IDeepCloneable dc) - { - newList.Add((T)dc.DeepClone()); - } - else - { - newList.Add(item); - } - } + DeepCloneHelper.CloneListItems, T>(this, newList); return newList; case ListCloneBehavior.None: @@ -60,17 +50,7 @@ public class DeepCloneableList : List, IDeepCloneable, IRememberBeingDirty case ListCloneBehavior.Always: // always clone to new list var newList2 = new DeepCloneableList(ListCloneBehavior.Always); - foreach (T item in this) - { - if (item is IDeepCloneable dc) - { - newList2.Add((T)dc.DeepClone()); - } - else - { - newList2.Add(item); - } - } + DeepCloneHelper.CloneListItems, T>(this, newList2); return newList2; default: diff --git a/src/Umbraco.Core/Collections/EventClearingObservableCollection.cs b/src/Umbraco.Core/Collections/EventClearingObservableCollection.cs index 579716456b..baf131ca80 100644 --- a/src/Umbraco.Core/Collections/EventClearingObservableCollection.cs +++ b/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 /// /// -public class EventClearingObservableCollection : ObservableCollection, INotifyCollectionChanged +public class EventClearingObservableCollection : ObservableCollection, 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 : ObservableCollection event /// public void ClearCollectionChangedEvents() => _changed = null; + + public object DeepClone() + { + var clone = new EventClearingObservableCollection(); + DeepCloneHelper.CloneListItems, TValue>(this, clone); + + return clone; + } } diff --git a/src/Umbraco.Core/Models/ContentBase.cs b/src/Umbraco.Core/Models/ContentBase.cs index e9fcc61e7c..8ecf0bfc8f 100644 --- a/src/Umbraco.Core/Models/ContentBase.cs +++ b/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(contentType.CompositionPropertyTypes); } - internal IReadOnlyList 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; diff --git a/src/Umbraco.Core/Models/DeepCloneHelper.cs b/src/Umbraco.Core/Models/DeepCloneHelper.cs index ce34dab6f1..7b9110f432 100644 --- a/src/Umbraco.Core/Models/DeepCloneHelper.cs +++ b/src/Umbraco.Core/Models/DeepCloneHelper.cs @@ -212,4 +212,16 @@ public static class DeepCloneHelper public bool IsList => GenericListType != null; } + + public static void CloneListItems(TList source, TList target) + where TList : ICollection + { + target.Clear(); + foreach (TEntity entity in source) + { + target.Add(entity is IDeepCloneable deepCloneableEntity + ? (TEntity)deepCloneableEntity.DeepClone() + : entity); + } + } } diff --git a/src/Umbraco.Core/Models/Entities/BeingDirtyBase.cs b/src/Umbraco.Core/Models/Entities/BeingDirtyBase.cs index 887477c743..18bc984853 100644 --- a/src/Umbraco.Core/Models/Entities/BeingDirtyBase.cs +++ b/src/Umbraco.Core/Models/Entities/BeingDirtyBase.cs @@ -85,6 +85,8 @@ public abstract class BeingDirtyBase : IRememberBeingDirty /// public event PropertyChangedEventHandler? PropertyChanged; + protected void ClearPropertyChangedEvents() => PropertyChanged = null; + /// /// Registers that a property has changed. /// diff --git a/src/Umbraco.Core/Models/PropertyType.cs b/src/Umbraco.Core/Models/PropertyType.cs index 0699ecbc0d..8fe28f7751 100644 --- a/src/Umbraco.Core/Models/PropertyType.cs +++ b/src/Umbraco.Core/Models/PropertyType.cs @@ -283,12 +283,7 @@ public class PropertyType : EntityBase, IPropertyType, IEquatable 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(() => PropertyGroupId.Value); - } + clonedEntity.ClearPropertyChangedEvents(); } /// diff --git a/src/Umbraco.Core/Models/PublicAccessEntry.cs b/src/Umbraco.Core/Models/PublicAccessEntry.cs index 8789ef5052..fdf3761366 100644 --- a/src/Umbraco.Core/Models/PublicAccessEntry.cs +++ b/src/Umbraco.Core/Models/PublicAccessEntry.cs @@ -10,7 +10,7 @@ namespace Umbraco.Cms.Core.Models; public class PublicAccessEntry : EntityBase { private readonly List _removedRules = new(); - private readonly EventClearingObservableCollection _ruleCollection; + private EventClearingObservableCollection _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)_ruleCollection.DeepClone(); + + // re-assign correct event handler + cloneEntity._ruleCollection.CollectionChanged += cloneEntity.RuleCollection_CollectionChanged; } }