From 4a3525ece39b095af8820b40d41e7a2af0e75451 Mon Sep 17 00:00:00 2001 From: Shannon Date: Tue, 12 Jan 2021 13:41:50 +1100 Subject: [PATCH] Fixes a memory leak caused by deep cloning There was a memory leak with PublicAccessEntry during even unassignment which was not clearing the correct handler. This goes a step further and adds a new ClearCollectionChangedEvents method for all observable collections used in umbraco which allows fully clearing ALL event handlers instead of having to track specific ones. This will ensure there are no unintended memory leaks in case end-users have assigned event handlers to the collection changed event which would not be unassigned during deep cloning. --- .../EventClearingObservableCollection.cs | 33 +++++++++++++++++++ .../Collections/ObservableDictionary.cs | 11 +++++++ src/Umbraco.Core/Models/Content.cs | 21 +++++++++--- src/Umbraco.Core/Models/ContentBase.cs | 17 +++++++--- .../Models/ContentCultureInfosCollection.cs | 2 +- .../Models/ContentScheduleCollection.cs | 5 +++ src/Umbraco.Core/Models/ContentTypeBase.cs | 9 +++-- .../Models/Identity/BackOfficeIdentityUser.cs | 7 ++-- src/Umbraco.Core/Models/Media.cs | 2 +- src/Umbraco.Core/Models/PropertyCollection.cs | 2 ++ src/Umbraco.Core/Models/PropertyGroup.cs | 7 ++-- .../Models/PropertyGroupCollection.cs | 5 +++ .../Models/PropertyTypeCollection.cs | 5 +++ src/Umbraco.Core/Models/PublicAccessEntry.cs | 9 ++--- src/Umbraco.Core/Umbraco.Core.csproj | 1 + 15 files changed, 113 insertions(+), 23 deletions(-) create mode 100644 src/Umbraco.Core/Collections/EventClearingObservableCollection.cs diff --git a/src/Umbraco.Core/Collections/EventClearingObservableCollection.cs b/src/Umbraco.Core/Collections/EventClearingObservableCollection.cs new file mode 100644 index 0000000000..724261ed8f --- /dev/null +++ b/src/Umbraco.Core/Collections/EventClearingObservableCollection.cs @@ -0,0 +1,33 @@ +using System.Collections.Generic; +using System.Collections.ObjectModel; +using System.Collections.Specialized; + +namespace Umbraco.Core.Collections +{ + /// + /// Allows clearing all event handlers + /// + /// + public class EventClearingObservableCollection : ObservableCollection + { + public EventClearingObservableCollection() + { + } + + public EventClearingObservableCollection(List list) : base(list) + { + } + + public EventClearingObservableCollection(IEnumerable collection) : base(collection) + { + } + + // need to override to clear + public override event NotifyCollectionChangedEventHandler CollectionChanged; + + /// + /// Clears all event handlers for the event + /// + public void ClearCollectionChangedEvents() => CollectionChanged = null; + } +} diff --git a/src/Umbraco.Core/Collections/ObservableDictionary.cs b/src/Umbraco.Core/Collections/ObservableDictionary.cs index c6aedab377..c7d1908d45 100644 --- a/src/Umbraco.Core/Collections/ObservableDictionary.cs +++ b/src/Umbraco.Core/Collections/ObservableDictionary.cs @@ -1,11 +1,14 @@ using System; using System.Collections.Generic; using System.Collections.ObjectModel; +using System.Collections.Specialized; +using System.ComponentModel; using System.Linq; using System.Runtime.Serialization; namespace Umbraco.Core.Collections { + /// /// An ObservableDictionary /// @@ -74,6 +77,14 @@ namespace Umbraco.Core.Collections #endregion + // need to override to clear + public override event NotifyCollectionChangedEventHandler CollectionChanged; + + /// + /// Clears all event handlers + /// + public void ClearCollectionChangedEvents() => CollectionChanged = null; + public bool ContainsKey(TKey key) { return Indecies.ContainsKey(key); diff --git a/src/Umbraco.Core/Models/Content.cs b/src/Umbraco.Core/Models/Content.cs index c5930bf998..04f49e704e 100644 --- a/src/Umbraco.Core/Models/Content.cs +++ b/src/Umbraco.Core/Models/Content.cs @@ -98,10 +98,15 @@ namespace Umbraco.Core.Models set { if (_schedule != null) - _schedule.CollectionChanged -= ScheduleCollectionChanged; + { + _schedule.ClearCollectionChangedEvents(); + } + SetPropertyValueAndDetectChanges(value, ref _schedule, nameof(ContentSchedule)); if (_schedule != null) + { _schedule.CollectionChanged += ScheduleCollectionChanged; + } } } @@ -223,10 +228,16 @@ namespace Umbraco.Core.Models } set { - if (_publishInfos != null) _publishInfos.CollectionChanged -= PublishNamesCollectionChanged; + if (_publishInfos != null) + { + _publishInfos.ClearCollectionChangedEvents(); + } + _publishInfos = value; if (_publishInfos != null) + { _publishInfos.CollectionChanged += PublishNamesCollectionChanged; + } } } @@ -321,7 +332,7 @@ namespace Umbraco.Core.Models else Properties.EnsurePropertyTypes(contentType.CompositionPropertyTypes); - Properties.CollectionChanged -= PropertiesChanged; // be sure not to double add + Properties.ClearCollectionChangedEvents(); // be sure not to double add Properties.CollectionChanged += PropertiesChanged; } @@ -438,7 +449,7 @@ namespace Umbraco.Core.Models //if culture infos exist then deal with event bindings if (clonedContent._publishInfos != null) { - clonedContent._publishInfos.CollectionChanged -= PublishNamesCollectionChanged; //clear this event handler if any + clonedContent._publishInfos.ClearCollectionChangedEvents(); //clear this event handler if any clonedContent._publishInfos = (ContentCultureInfosCollection)_publishInfos.DeepClone(); //manually deep clone clonedContent._publishInfos.CollectionChanged += clonedContent.PublishNamesCollectionChanged; //re-assign correct event handler } @@ -446,7 +457,7 @@ namespace Umbraco.Core.Models //if properties exist then deal with event bindings if (clonedContent._schedule != null) { - clonedContent._schedule.CollectionChanged -= ScheduleCollectionChanged; //clear this event handler if any + clonedContent._schedule.ClearCollectionChangedEvents(); //clear this event handler if any clonedContent._schedule = (ContentScheduleCollection)_schedule.DeepClone(); //manually deep clone clonedContent._schedule.CollectionChanged += clonedContent.ScheduleCollectionChanged; //re-assign correct event handler } diff --git a/src/Umbraco.Core/Models/ContentBase.cs b/src/Umbraco.Core/Models/ContentBase.cs index d02ea82012..cbdee479fe 100644 --- a/src/Umbraco.Core/Models/ContentBase.cs +++ b/src/Umbraco.Core/Models/ContentBase.cs @@ -138,7 +138,11 @@ namespace Umbraco.Core.Models get => _properties; set { - if (_properties != null) _properties.CollectionChanged -= PropertiesChanged; + if (_properties != null) + { + _properties.ClearCollectionChangedEvents(); + } + _properties = value; _properties.CollectionChanged += PropertiesChanged; } @@ -173,10 +177,15 @@ namespace Umbraco.Core.Models } set { - if (_cultureInfos != null) _cultureInfos.CollectionChanged -= CultureInfosCollectionChanged; + if (_cultureInfos != null) + { + _cultureInfos.ClearCollectionChangedEvents(); + } _cultureInfos = value; if (_cultureInfos != null) + { _cultureInfos.CollectionChanged += CultureInfosCollectionChanged; + } } } @@ -479,7 +488,7 @@ namespace Umbraco.Core.Models //if culture infos exist then deal with event bindings if (clonedContent._cultureInfos != null) { - clonedContent._cultureInfos.CollectionChanged -= CultureInfosCollectionChanged; //clear this event handler if any + clonedContent._cultureInfos.ClearCollectionChangedEvents(); //clear this event handler if any clonedContent._cultureInfos = (ContentCultureInfosCollection)_cultureInfos.DeepClone(); //manually deep clone clonedContent._cultureInfos.CollectionChanged += clonedContent.CultureInfosCollectionChanged; //re-assign correct event handler } @@ -487,7 +496,7 @@ namespace Umbraco.Core.Models //if properties exist then deal with event bindings if (clonedContent._properties != null) { - clonedContent._properties.CollectionChanged -= PropertiesChanged; //clear this event handler if any + clonedContent._properties.ClearCollectionChangedEvents(); //clear this event handler if any clonedContent._properties = (PropertyCollection)_properties.DeepClone(); //manually deep clone clonedContent._properties.CollectionChanged += clonedContent.PropertiesChanged; //re-assign correct event handler } diff --git a/src/Umbraco.Core/Models/ContentCultureInfosCollection.cs b/src/Umbraco.Core/Models/ContentCultureInfosCollection.cs index 9bc78fc56d..07ee661497 100644 --- a/src/Umbraco.Core/Models/ContentCultureInfosCollection.cs +++ b/src/Umbraco.Core/Models/ContentCultureInfosCollection.cs @@ -15,7 +15,7 @@ namespace Umbraco.Core.Models public ContentCultureInfosCollection() : base(x => x.Culture, StringComparer.InvariantCultureIgnoreCase) { } - + /// /// Adds or updates a instance. /// diff --git a/src/Umbraco.Core/Models/ContentScheduleCollection.cs b/src/Umbraco.Core/Models/ContentScheduleCollection.cs index 6c7dd79312..0ebcc0fe4b 100644 --- a/src/Umbraco.Core/Models/ContentScheduleCollection.cs +++ b/src/Umbraco.Core/Models/ContentScheduleCollection.cs @@ -14,6 +14,11 @@ namespace Umbraco.Core.Models public event NotifyCollectionChangedEventHandler CollectionChanged; + /// + /// Clears all event handlers + /// + public void ClearCollectionChangedEvents() => CollectionChanged = null; + private void OnCollectionChanged(NotifyCollectionChangedEventArgs args) { CollectionChanged?.Invoke(this, args); diff --git a/src/Umbraco.Core/Models/ContentTypeBase.cs b/src/Umbraco.Core/Models/ContentTypeBase.cs index 04bcb7424a..4865db428a 100644 --- a/src/Umbraco.Core/Models/ContentTypeBase.cs +++ b/src/Umbraco.Core/Models/ContentTypeBase.cs @@ -262,7 +262,10 @@ namespace Umbraco.Core.Models set { if (_noGroupPropertyTypes != null) - _noGroupPropertyTypes.CollectionChanged -= PropertyTypesChanged; + { + _noGroupPropertyTypes.ClearCollectionChangedEvents(); + } + _noGroupPropertyTypes = new PropertyTypeCollection(SupportsPublishing, value); _noGroupPropertyTypes.CollectionChanged += PropertyTypesChanged; PropertyTypesChanged(_noGroupPropertyTypes, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset)); @@ -480,14 +483,14 @@ namespace Umbraco.Core.Models // its ignored from the auto-clone process because its return values are unions, not raw and // we end up with duplicates, see: http://issues.umbraco.org/issue/U4-4842 - clonedEntity._noGroupPropertyTypes.CollectionChanged -= PropertyTypesChanged; //clear this event handler if any + clonedEntity._noGroupPropertyTypes.ClearCollectionChangedEvents(); //clear this event handler if any clonedEntity._noGroupPropertyTypes = (PropertyTypeCollection) _noGroupPropertyTypes.DeepClone(); //manually deep clone clonedEntity._noGroupPropertyTypes.CollectionChanged += clonedEntity.PropertyTypesChanged; //re-assign correct event handler } if (clonedEntity._propertyGroups != null) { - clonedEntity._propertyGroups.CollectionChanged -= PropertyGroupsChanged; //clear this event handler if any + clonedEntity._propertyGroups.ClearCollectionChangedEvents(); //clear this event handler if any clonedEntity._propertyGroups = (PropertyGroupCollection) _propertyGroups.DeepClone(); //manually deep clone clonedEntity._propertyGroups.CollectionChanged += clonedEntity.PropertyGroupsChanged; //re-assign correct event handler } diff --git a/src/Umbraco.Core/Models/Identity/BackOfficeIdentityUser.cs b/src/Umbraco.Core/Models/Identity/BackOfficeIdentityUser.cs index b7bde26fae..d2899caccc 100644 --- a/src/Umbraco.Core/Models/Identity/BackOfficeIdentityUser.cs +++ b/src/Umbraco.Core/Models/Identity/BackOfficeIdentityUser.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Collections.ObjectModel; using System.Collections.Specialized; using System.Linq; +using Umbraco.Core.Collections; using Umbraco.Core.Composing; using Umbraco.Core.Models.Entities; using Umbraco.Core.Models.Membership; @@ -62,7 +63,7 @@ namespace Umbraco.Core.Models.Identity _culture = Current.Configs.Global().DefaultUILanguage; // TODO: inject // must initialize before setting groups - _roles = new ObservableCollection>(); + _roles = new EventClearingObservableCollection>(); _roles.CollectionChanged += _roles_CollectionChanged; // use the property setters - they do more than just setting a field @@ -223,7 +224,7 @@ namespace Umbraco.Core.Models.Identity _groups = value; //now clear all roles and re-add them - _roles.CollectionChanged -= _roles_CollectionChanged; + _roles.ClearCollectionChangedEvents(); _roles.Clear(); foreach (var identityUserRole in _groups.Select(x => new IdentityUserRole { @@ -306,7 +307,7 @@ namespace Umbraco.Core.Models.Identity _beingDirty.OnPropertyChanged(nameof(Roles)); } - private readonly ObservableCollection> _roles; + private readonly EventClearingObservableCollection> _roles; /// /// helper method to easily add a role without having to deal with IdentityUserRole{T} diff --git a/src/Umbraco.Core/Models/Media.cs b/src/Umbraco.Core/Models/Media.cs index 002611c09c..87ec9196d6 100644 --- a/src/Umbraco.Core/Models/Media.cs +++ b/src/Umbraco.Core/Models/Media.cs @@ -77,7 +77,7 @@ namespace Umbraco.Core.Models else Properties.EnsurePropertyTypes(contentType.CompositionPropertyTypes); - Properties.CollectionChanged -= PropertiesChanged; // be sure not to double add + Properties.ClearCollectionChangedEvents(); // be sure not to double add Properties.CollectionChanged += PropertiesChanged; } } diff --git a/src/Umbraco.Core/Models/PropertyCollection.cs b/src/Umbraco.Core/Models/PropertyCollection.cs index 13fbf949d6..a73df6a095 100644 --- a/src/Umbraco.Core/Models/PropertyCollection.cs +++ b/src/Umbraco.Core/Models/PropertyCollection.cs @@ -169,6 +169,8 @@ namespace Umbraco.Core.Models /// public event NotifyCollectionChangedEventHandler CollectionChanged; + public void ClearCollectionChangedEvents() => CollectionChanged = null; + protected virtual void OnCollectionChanged(NotifyCollectionChangedEventArgs args) { CollectionChanged?.Invoke(this, args); diff --git a/src/Umbraco.Core/Models/PropertyGroup.cs b/src/Umbraco.Core/Models/PropertyGroup.cs index 2e6da5d837..022fb6ed03 100644 --- a/src/Umbraco.Core/Models/PropertyGroup.cs +++ b/src/Umbraco.Core/Models/PropertyGroup.cs @@ -66,7 +66,10 @@ namespace Umbraco.Core.Models set { if (_propertyTypes != null) - _propertyTypes.CollectionChanged -= PropertyTypesChanged; + { + _propertyTypes.ClearCollectionChangedEvents(); + } + _propertyTypes = value; // since we're adding this collection to this group, @@ -100,7 +103,7 @@ namespace Umbraco.Core.Models if (clonedEntity._propertyTypes != null) { - clonedEntity._propertyTypes.CollectionChanged -= PropertyTypesChanged; //clear this event handler if any + clonedEntity._propertyTypes.ClearCollectionChangedEvents(); //clear this event handler if any clonedEntity._propertyTypes = (PropertyTypeCollection) _propertyTypes.DeepClone(); //manually deep clone clonedEntity._propertyTypes.CollectionChanged += clonedEntity.PropertyTypesChanged; //re-assign correct event handler } diff --git a/src/Umbraco.Core/Models/PropertyGroupCollection.cs b/src/Umbraco.Core/Models/PropertyGroupCollection.cs index 5422dfb792..973147b3fb 100644 --- a/src/Umbraco.Core/Models/PropertyGroupCollection.cs +++ b/src/Umbraco.Core/Models/PropertyGroupCollection.cs @@ -160,6 +160,11 @@ namespace Umbraco.Core.Models public event NotifyCollectionChangedEventHandler CollectionChanged; + /// + /// Clears all event handlers + /// + public void ClearCollectionChangedEvents() => CollectionChanged = null; + protected virtual void OnCollectionChanged(NotifyCollectionChangedEventArgs args) { CollectionChanged?.Invoke(this, args); diff --git a/src/Umbraco.Core/Models/PropertyTypeCollection.cs b/src/Umbraco.Core/Models/PropertyTypeCollection.cs index 6e41f0d12b..c512630983 100644 --- a/src/Umbraco.Core/Models/PropertyTypeCollection.cs +++ b/src/Umbraco.Core/Models/PropertyTypeCollection.cs @@ -161,8 +161,13 @@ namespace Umbraco.Core.Models return item.Alias; } + /// + /// Clears all event handlers + /// public event NotifyCollectionChangedEventHandler CollectionChanged; + public void ClearCollectionChangedEvents() => CollectionChanged = null; + protected virtual void OnCollectionChanged(NotifyCollectionChangedEventArgs args) { CollectionChanged?.Invoke(this, args); diff --git a/src/Umbraco.Core/Models/PublicAccessEntry.cs b/src/Umbraco.Core/Models/PublicAccessEntry.cs index cfb30de147..cc86b797cb 100644 --- a/src/Umbraco.Core/Models/PublicAccessEntry.cs +++ b/src/Umbraco.Core/Models/PublicAccessEntry.cs @@ -4,6 +4,7 @@ using System.Collections.ObjectModel; using System.Collections.Specialized; using System.Linq; using System.Runtime.Serialization; +using Umbraco.Core.Collections; using Umbraco.Core.Models.Entities; namespace Umbraco.Core.Models @@ -12,7 +13,7 @@ namespace Umbraco.Core.Models [DataContract(IsReference = true)] public class PublicAccessEntry : EntityBase { - private readonly ObservableCollection _ruleCollection; + private readonly EventClearingObservableCollection _ruleCollection; private int _protectedNodeId; private int _noAccessNodeId; private int _loginNodeId; @@ -28,7 +29,7 @@ namespace Umbraco.Core.Models NoAccessNodeId = noAccessNode.Id; _protectedNodeId = protectedNode.Id; - _ruleCollection = new ObservableCollection(ruleCollection); + _ruleCollection = new EventClearingObservableCollection(ruleCollection); _ruleCollection.CollectionChanged += _ruleCollection_CollectionChanged; foreach (var rule in _ruleCollection) @@ -44,7 +45,7 @@ namespace Umbraco.Core.Models NoAccessNodeId = noAccessNodeId; _protectedNodeId = protectedNodeId; - _ruleCollection = new ObservableCollection(ruleCollection); + _ruleCollection = new EventClearingObservableCollection(ruleCollection); _ruleCollection.CollectionChanged += _ruleCollection_CollectionChanged; foreach (var rule in _ruleCollection) @@ -148,7 +149,7 @@ namespace Umbraco.Core.Models if (cloneEntity._ruleCollection != null) { - cloneEntity._ruleCollection.CollectionChanged -= _ruleCollection_CollectionChanged; //clear this event handler if any + cloneEntity._ruleCollection.ClearCollectionChangedEvents(); //clear this event handler if any cloneEntity._ruleCollection.CollectionChanged += cloneEntity._ruleCollection_CollectionChanged; //re-assign correct event handler } } diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index 69ff94e482..c09f45557a 100755 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -128,6 +128,7 @@ --> +