yv01p/Umbraco-CMS
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fixes a memory leak caused by deep cloning

Browse Source 
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.
This commit is contained in:
Shannon
2021-01-12 13:41:50 +11:00
parent 94b1c63cb3
commit 4a3525ece3
15 changed files with 113 additions and 23 deletions
Download Patch File Download Diff File Expand all files Collapse all files

33
src/Umbraco.Core/Collections/EventClearingObservableCollection.cs Normal file
View File

@@ -0,0 +1,33 @@
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Collections.Specialized;
namespace Umbraco.Core.Collections
{
/// <summary>
/// Allows clearing all event handlers
/// </summary>
/// <typeparam name="TValue"></typeparam>
public class EventClearingObservableCollection<TValue> : ObservableCollection<TValue>
{
public EventClearingObservableCollection()
{
}
public EventClearingObservableCollection(List<TValue> list) : base(list)
{
}
public EventClearingObservableCollection(IEnumerable<TValue> collection) : base(collection)
{
}
// need to override to clear
public override event NotifyCollectionChangedEventHandler CollectionChanged;
/// <summary>
/// Clears all event handlers for the <see cref="CollectionChanged"/> event
/// </summary>
public void ClearCollectionChangedEvents() => CollectionChanged = null;
}
}

11
src/Umbraco.Core/Collections/ObservableDictionary.cs
View File

@@ -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
{
/// <summary>
/// An ObservableDictionary
/// </summary>
@@ -74,6 +77,14 @@ namespace Umbraco.Core.Collections
#endregion
// need to override to clear
public override event NotifyCollectionChangedEventHandler CollectionChanged;
/// <summary>
/// Clears all <see cref="CollectionChanged"/> event handlers
/// </summary>
public void ClearCollectionChangedEvents() => CollectionChanged = null;
public bool ContainsKey(TKey key)
{
return Indecies.ContainsKey(key);

21
src/Umbraco.Core/Models/Content.cs
View File

@@ -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
}

17
src/Umbraco.Core/Models/ContentBase.cs
View File

@@ -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
}

2
src/Umbraco.Core/Models/ContentCultureInfosCollection.cs
View File

@@ -15,7 +15,7 @@ namespace Umbraco.Core.Models
public ContentCultureInfosCollection()
: base(x => x.Culture, StringComparer.InvariantCultureIgnoreCase)
{ }
/// <summary>
/// Adds or updates a <see cref="ContentCultureInfos"/> instance.
/// </summary>

5
src/Umbraco.Core/Models/ContentScheduleCollection.cs
View File

@@ -14,6 +14,11 @@ namespace Umbraco.Core.Models
public event NotifyCollectionChangedEventHandler CollectionChanged;
/// <summary>
/// Clears all <see cref="CollectionChanged"/> event handlers
/// </summary>
public void ClearCollectionChangedEvents() => CollectionChanged = null;
private void OnCollectionChanged(NotifyCollectionChangedEventArgs args)
{
CollectionChanged?.Invoke(this, args);

9
src/Umbraco.Core/Models/ContentTypeBase.cs
View File

@@ -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
}

7
src/Umbraco.Core/Models/Identity/BackOfficeIdentityUser.cs
View File

@@ -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<IdentityUserRole<string>>();
_roles = new EventClearingObservableCollection<IdentityUserRole<string>>();
_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<string>
{
@@ -306,7 +307,7 @@ namespace Umbraco.Core.Models.Identity
_beingDirty.OnPropertyChanged(nameof(Roles));
}
private readonly ObservableCollection<IdentityUserRole<string>> _roles;
private readonly EventClearingObservableCollection<IdentityUserRole<string>> _roles;
/// <summary>
/// helper method to easily add a role without having to deal with IdentityUserRole{T}

2
src/Umbraco.Core/Models/Media.cs
View File

@@ -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;
}
}

2
src/Umbraco.Core/Models/PropertyCollection.cs
View File

@@ -169,6 +169,8 @@ namespace Umbraco.Core.Models
/// </summary>
public event NotifyCollectionChangedEventHandler CollectionChanged;
public void ClearCollectionChangedEvents() => CollectionChanged = null;
protected virtual void OnCollectionChanged(NotifyCollectionChangedEventArgs args)
{
CollectionChanged?.Invoke(this, args);

7
src/Umbraco.Core/Models/PropertyGroup.cs
View File

@@ -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
}

5
src/Umbraco.Core/Models/PropertyGroupCollection.cs
View File

@@ -160,6 +160,11 @@ namespace Umbraco.Core.Models
public event NotifyCollectionChangedEventHandler CollectionChanged;
/// <summary>
/// Clears all <see cref="CollectionChanged"/> event handlers
/// </summary>
public void ClearCollectionChangedEvents() => CollectionChanged = null;
protected virtual void OnCollectionChanged(NotifyCollectionChangedEventArgs args)
{
CollectionChanged?.Invoke(this, args);

5
src/Umbraco.Core/Models/PropertyTypeCollection.cs
View File

@@ -161,8 +161,13 @@ namespace Umbraco.Core.Models
return item.Alias;
}
/// <summary>
/// Clears all <see cref="CollectionChanged"/> event handlers
/// </summary>
public event NotifyCollectionChangedEventHandler CollectionChanged;
public void ClearCollectionChangedEvents() => CollectionChanged = null;
protected virtual void OnCollectionChanged(NotifyCollectionChangedEventArgs args)
{
CollectionChanged?.Invoke(this, args);

9
src/Umbraco.Core/Models/PublicAccessEntry.cs
View File

@@ -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<PublicAccessRule> _ruleCollection;
private readonly EventClearingObservableCollection<PublicAccessRule> _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<PublicAccessRule>(ruleCollection);
_ruleCollection = new EventClearingObservableCollection<PublicAccessRule>(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<PublicAccessRule>(ruleCollection);
_ruleCollection = new EventClearingObservableCollection<PublicAccessRule>(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
}
}

1
src/Umbraco.Core/Umbraco.Core.csproj
View File

@@ -128,6 +128,7 @@
</Compile>
-->
<Compile Include="AssemblyExtensions.cs" />
<Compile Include="Collections\EventClearingObservableCollection.cs" />
<Compile Include="Constants-SqlTemplates.cs" />
<Compile Include="Exceptions\UnattendedInstallException.cs" />
<Compile Include="Migrations\Upgrade\V_8_0_0\Models\ContentTypeDto80.cs" />