Skip to content

Commit

Permalink
[ios] fix memory leak in Frame, VisualElementRenderer (dotnet#18552)
Browse files Browse the repository at this point in the history 
Context: dotnet#18365

I could reproduce a leak in `Frame` by adding a parameterized test:

    [InlineData(typeof(Frame))]
    public async Task HandlerDoesNotLeak(Type type)

`FrameRenderer` has a circular reference via its base type,
`VisualElementRenderer`:

* `Frame` ->
* `FrameRenderer` / `VisualElementRenderer` ->
* `Frame` (via `VisualElementRenderer._virtualView`)

To solve this issue, I made `_virtualView` a `WeakReference`, but only
on iOS or MacCatalyst platforms. We don't necessarily need this
treatment for Windows or Android.

My initial attempt threw a `NullReferenceException`, because the
`Element` property is accessed before `SetVirtualView()` returns. I
had to create a `_tempElement` field to hold the value temporarily,
clearing it to avoid a circular reference.

The Roslyn analyzer didn't catch this cycle because it doesn't warn
about `IPlatformViewHandler`, only `NSObject`'s. Will investigate
further on this example here:

jonathanpeppers/memory-analyzers#12
  • Loading branch information
@jonathanpeppers
jonathanpeppers authored Nov 13, 2023
1 parent 2fb3603 commit 8bc2a2f
Show file tree
Hide file tree
Showing 3 changed files with 51 additions and 25 deletions.
54 changes: 38 additions & 16 deletions src/Controls/src/Core/Compatibility/Handlers/VisualElementRenderer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,22 @@ public abstract partial class VisualElementRenderer<TElement> : IPlatformViewHan

public static CommandMapper<TElement, IPlatformViewHandler> VisualElementRendererCommandMapper = new CommandMapper<TElement, IPlatformViewHandler>(ViewHandler.ViewCommandMapper);

#if IOS || MACCATALYST
WeakReference<TElement>? _virtualView;
TElement? _tempElement;
#else
TElement? _virtualView;
#endif
IMauiContext? _mauiContext;
internal IPropertyMapper _mapper;
internal readonly CommandMapper? _commandMapper;
internal readonly IPropertyMapper _defaultMapper;
protected IMauiContext MauiContext => _mauiContext ?? throw new InvalidOperationException("MauiContext not set");
#if IOS || MACCATALYST
public TElement? Element => _tempElement ?? (_virtualView is not null && _virtualView.TryGetTarget(out var target) ? target : null);
#else
public TElement? Element => _virtualView;
#endif
protected bool AutoPackage { get; set; } = true;

#if ANDROID
Expand Down Expand Up @@ -104,8 +113,8 @@ protected virtual void OnElementChanged(ElementChangedEventArgs<TElement> e)

protected virtual void OnElementPropertyChanged(object sender, PropertyChangedEventArgs e)
{
if (Element != null && e.PropertyName != null)
_mapper.UpdateProperty(this, Element, e.PropertyName);
if (Element is TElement element && e.PropertyName != null)
_mapper.UpdateProperty(this, element, e.PropertyName);

ElementPropertyChanged?.Invoke(sender, e);
ElementPropertyChangedPartial(sender, e);
Expand Down Expand Up @@ -172,8 +181,8 @@ protected virtual void SetBackgroundColor(Color? color)
protected virtual void UpdateBackgroundColor()
#endif
{
if (Element != null)
ViewHandler.MapBackground(this, Element);
if (Element is TElement element)
ViewHandler.MapBackground(this, element);
}

#if IOS
Expand All @@ -182,21 +191,21 @@ protected virtual void SetBackground(Brush brush)
protected virtual void UpdateBackground()
#endif
{
if (Element != null)
ViewHandler.MapBackground(this, Element);
if (Element is TElement element)
ViewHandler.MapBackground(this, element);
}


protected virtual void SetAutomationId(string id)
{
if (Element != null)
ViewHandler.MapAutomationId(this, Element);
if (Element is TElement element)
ViewHandler.MapAutomationId(this, element);
}

protected virtual void SetIsEnabled()
{
if (Element != null)
ViewHandler.MapIsEnabled(this, Element);
if (Element is TElement element)
ViewHandler.MapIsEnabled(this, element);
}

#if WINDOWS
Expand All @@ -205,8 +214,8 @@ protected virtual void SetAutomationPropertiesAccessibilityView()
protected virtual void SetImportantForAccessibility()
#endif
{
if (Element != null)
VisualElement.MapAutomationPropertiesIsInAccessibleTree(this, Element);
if (Element is TElement element)
VisualElement.MapAutomationPropertiesIsInAccessibleTree(this, element);
}


Expand Down Expand Up @@ -283,14 +292,27 @@ internal static void SetVirtualView(

static partial void ProcessAutoPackage(Maui.IElement element);

#if IOS || MACCATALYST
void IElementHandler.SetVirtualView(Maui.IElement view)
{
// _tempElement is used here, because the Element property is accessed before SetVirtualView() returns
_tempElement = Element;
SetVirtualView(view, this, OnElementChanged, ref _tempElement, ref _mapper, _defaultMapper, AutoPackage);

// We use _virtualView as a WeakReference, and clear _tempElement
_virtualView = _tempElement is null ? null : new(_tempElement);
_tempElement = null;
}
#else
void IElementHandler.SetVirtualView(Maui.IElement view) =>
SetVirtualView(view, this, OnElementChanged, ref _virtualView, ref _mapper, _defaultMapper, AutoPackage);
#endif

void IElementHandler.UpdateValue(string property)
{
if (Element != null)
if (Element is IElement element)
{
OnElementPropertyChanged(Element, new PropertyChangedEventArgs(property));
OnElementPropertyChanged(element, new PropertyChangedEventArgs(property));
}
}

Expand All @@ -302,8 +324,8 @@ void IElementHandler.Invoke(string command, object? args)
void IElementHandler.DisconnectHandler()
{
DisconnectHandlerCore();
if (Element != null && Element.Handler == (IPlatformViewHandler)this)
Element.Handler = null;
if (Element is IElement element && element.Handler == (IPlatformViewHandler)this)
element.Handler = null;

_virtualView = null;
}
Expand Down
20 changes: 11 additions & 9 deletions src/Controls/src/Core/Compatibility/Handlers/iOS/FrameRenderer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,30 +85,32 @@ public virtual void SetupLayer()
{
if (_actualView == null)
return;
if (Element is not Frame element)
return;

float cornerRadius = Element.CornerRadius;
float cornerRadius = element.CornerRadius;

if (cornerRadius == -1f)
cornerRadius = 5f; // default corner radius

_actualView.Layer.CornerRadius = cornerRadius;
_actualView.Layer.MasksToBounds = cornerRadius > 0;

if (Element.BackgroundColor == null)
if (element.BackgroundColor == null)
_actualView.Layer.BackgroundColor = Microsoft.Maui.Platform.ColorExtensions.BackgroundColor.CGColor;
else
{
// BackgroundColor gets set on the base class too which messes with
// the corner radius, shadow, etc. so override that behaviour here
BackgroundColor = UIColor.Clear;
_actualView.Layer.BackgroundColor = Element.BackgroundColor.ToCGColor();
_actualView.Layer.BackgroundColor = element.BackgroundColor.ToCGColor();
}

_actualView.Layer.RemoveBackgroundLayer();

if (!Brush.IsNullOrEmpty(Element.Background))
if (!Brush.IsNullOrEmpty(element.Background))
{
var backgroundLayer = this.GetBackgroundLayer(Element.Background);
var backgroundLayer = this.GetBackgroundLayer(element.Background);

if (backgroundLayer != null)
{
Expand All @@ -118,17 +120,17 @@ public virtual void SetupLayer()
}
}

if (Element.BorderColor == null)
if (element.BorderColor == null)
{
_actualView.Layer.BorderColor = UIColor.Clear.CGColor;
_actualView.Layer.BorderWidth = 0;
}
else
{
var borderWidth = (int)(Element is IBorderElement be ? be.BorderWidth : 1);
var borderWidth = (int)(element is IBorderElement be ? be.BorderWidth : 1);
borderWidth = Math.Max(1, borderWidth);

_actualView.Layer.BorderColor = Element.BorderColor.ToCGColor();
_actualView.Layer.BorderColor = element.BorderColor.ToCGColor();
_actualView.Layer.BorderWidth = borderWidth;
}

Expand All @@ -138,7 +140,7 @@ public virtual void SetupLayer()

_actualView.Layer.RasterizationScale = UIScreen.MainScreen.Scale;
_actualView.Layer.ShouldRasterize = true;
_actualView.Layer.MasksToBounds = Element.IsClippedToBoundsSet(true);
_actualView.Layer.MasksToBounds = element.IsClippedToBoundsSet(true);
}

void UpdateShadow()
Expand Down
2 changes: 2 additions & 0 deletions src/Controls/tests/DeviceTests/Memory/MemoryTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ void SetupBuilder()
handlers.AddHandler<DatePicker, DatePickerHandler>();
handlers.AddHandler<Entry, EntryHandler>();
handlers.AddHandler<Editor, EditorHandler>();
handlers.AddHandler<Frame, FrameRenderer>();
handlers.AddHandler<GraphicsView, GraphicsViewHandler>();
handlers.AddHandler<Label, LabelHandler>();
handlers.AddHandler<ListView, ListViewRenderer>();
Expand Down Expand Up @@ -61,6 +62,7 @@ void SetupBuilder()
[InlineData(typeof(DatePicker))]
[InlineData(typeof(Entry))]
[InlineData(typeof(Editor))]
[InlineData(typeof(Frame))]
[InlineData(typeof(GraphicsView))]
[InlineData(typeof(Image))]
[InlineData(typeof(ImageButton))]
Expand Down

0 comments on commit 8bc2a2f

Please sign in to comment.