From 0f2db02289ed2c9ef67cc1f8852086be39ada8cb Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Tue, 12 Jan 2021 11:46:29 +0100 Subject: [PATCH] Merge pull request #9640 from umbraco/v8/bugfix/memory-leak-9348 Fixes a memory leak caused by deep cloning (cherry picked from commit 824349ae0d8987a6028f4653f7869f3a8abf2c9e) --- .../EventClearingObservableCollection.cs | 41 +++++++++++++++++++ .../Collections/ObservableDictionary.cs | 21 +++++++++- 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 | 6 ++- src/Umbraco.Core/Models/PublicAccessEntry.cs | 9 ++-- src/Umbraco.Core/Umbraco.Core.csproj | 1 + 15 files changed, 130 insertions(+), 25 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..af25cc1b4a --- /dev/null +++ b/src/Umbraco.Core/Collections/EventClearingObservableCollection.cs @@ -0,0 +1,41 @@ +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, INotifyCollectionChanged + { + public EventClearingObservableCollection() + { + } + + public EventClearingObservableCollection(List list) : base(list) + { + } + + public EventClearingObservableCollection(IEnumerable collection) : base(collection) + { + } + + // 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', + // a good article is here: https://medium.com/@unicorn_dev/virtual-events-in-c-something-went-wrong-c6f6f5fbe252 + // and https://stackoverflow.com/questions/2268065/c-sharp-language-design-explicit-interface-implementation-of-an-event + private NotifyCollectionChangedEventHandler _changed; + event NotifyCollectionChangedEventHandler INotifyCollectionChanged.CollectionChanged + { + add { _changed += value; } + remove { _changed -= value; } + } + + /// + /// Clears all event handlers for the event + /// + public void ClearCollectionChangedEvents() => _changed = null; + } +} diff --git a/src/Umbraco.Core/Collections/ObservableDictionary.cs b/src/Umbraco.Core/Collections/ObservableDictionary.cs index c6aedab377..fd9e469f07 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 /// @@ -15,7 +18,7 @@ namespace Umbraco.Core.Collections /// /// The type of elements contained in the BindableCollection /// The type of the indexing key - public class ObservableDictionary : ObservableCollection, IReadOnlyDictionary, IDictionary + public class ObservableDictionary : ObservableCollection, IReadOnlyDictionary, IDictionary, INotifyCollectionChanged { protected Dictionary Indecies { get; } protected Func KeySelector { get; } @@ -74,6 +77,22 @@ namespace Umbraco.Core.Collections #endregion + // 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', + // a good article is here: https://medium.com/@unicorn_dev/virtual-events-in-c-something-went-wrong-c6f6f5fbe252 + // and https://stackoverflow.com/questions/2268065/c-sharp-language-design-explicit-interface-implementation-of-an-event + private NotifyCollectionChangedEventHandler _changed; + event NotifyCollectionChangedEventHandler INotifyCollectionChanged.CollectionChanged + { + add { _changed += value; } + remove { _changed -= value; } + } + + /// + /// Clears all event handlers + /// + public void ClearCollectionChangedEvents() => _changed = 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 8dc056d555..63404bb38b 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 { @@ -299,7 +300,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 c587a45424..1097a2af93 100644 --- a/src/Umbraco.Core/Models/PropertyCollection.cs +++ b/src/Umbraco.Core/Models/PropertyCollection.cs @@ -168,6 +168,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..cc9d00fa5d 100644 --- a/src/Umbraco.Core/Models/PropertyTypeCollection.cs +++ b/src/Umbraco.Core/Models/PropertyTypeCollection.cs @@ -162,7 +162,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/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 6b77ae83f6..affafe313b 100755 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -128,6 +128,7 @@ --> +