Skip to content

Commit

Permalink
[ios] fix memory leak in WindowOverlay (#16700)
Browse files Browse the repository at this point in the history
Context: #16346

This addresses the memory leak discovered by:

    src/Core/src/WindowOverlay/WindowOverlay.iOS.cs(92,40): error MA0001: Event 'OnTouch' could cause memory leaks in an NSObject subclass. Remove the event or add the [UnconditionalSuppressMessage("Memory", "MA0001")] attribute with a justification as to why the event will not leak.

I could reproduce a leak by writing a custom test.

Solved the problem by:

* Removed the `PassthroughView.OnTouch` event completely. We could just
  call `WindowOverlay.OnTappedInternal()` directly instead of going
  through an intermediate event.

* `WindowOverlay overlay` is now a `WeakReference<WindowOverlay>`.
  • Loading branch information
jonathanpeppers authored and rmarinho committed Aug 19, 2023
1 parent c393ac2 commit df7bd4e
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 17 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
using System;
using System.Threading.Tasks;
using Microsoft.Maui.Controls;
using Microsoft.Maui.DeviceTests.Stubs;
using Xunit;

namespace Microsoft.Maui.DeviceTests.Memory;

[Category(TestCategory.WindowOverlay)]
public class WindowOverlayTests : ControlsHandlerTestBase
{
[Fact("Does Not Leak")]
public async Task DoesNotLeak()
{
WeakReference viewReference = null;

{
var window = new Window(new ContentPage());

await CreateHandlerAndAddToWindow<WindowHandlerStub>(window, _ =>
{
var overlay = new WindowOverlay(window);
viewReference = new(overlay);
window.AddOverlay(overlay);
window.RemoveOverlay(overlay);
});
}

await AssertionExtensions.WaitForGC(viewReference);
Assert.False(viewReference.IsAlive, "WindowOverlay should not be alive!");
}
}

1 change: 1 addition & 0 deletions src/Controls/tests/DeviceTests/TestCategory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -47,5 +47,6 @@ public static class TestCategory
public const string VisualElementTree = "VisualElementTree";
public const string WebView = "WebView";
public const string Window = "Window";
public const string WindowOverlay = "WindowOverlay";
}
}
28 changes: 11 additions & 17 deletions src/Core/src/WindowOverlay/WindowOverlay.iOS.cs
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,6 @@ public virtual bool Initialize()
platformWindow.RootViewController.View.AddSubview(_passthroughView);
platformWindow.RootViewController.View.BringSubviewToFront(_passthroughView);

// Any time the passthrough view is touched, handle it.
_passthroughView.OnTouch += UIViewOnTouch;
IsPlatformViewInitialized = true;
return IsPlatformViewInitialized;
}
Expand All @@ -75,9 +73,6 @@ void DeinitializePlatformDependencies()
IsPlatformViewInitialized = false;
}

void UIViewOnTouch(object? sender, CGPoint e) =>
OnTappedInternal(new Point(e.X, e.Y));

void FrameAction(Foundation.NSObservedChange obj)
{
HandleUIChange();
Expand All @@ -86,12 +81,7 @@ void FrameAction(Foundation.NSObservedChange obj)

class PassthroughView : UIView
{
/// <summary>
/// Event Handler for handling on touch events on the Passthrough View.
/// </summary>
public event EventHandler<CGPoint>? OnTouch;

WindowOverlay overlay;
readonly WeakReference<WindowOverlay> _overlay;

/// <summary>
/// Initializes a new instance of the <see cref="PassthroughView"/> class.
Expand All @@ -101,7 +91,7 @@ class PassthroughView : UIView
public PassthroughView(WindowOverlay windowOverlay, CGRect frame)
: base(frame)
{
overlay = windowOverlay;
_overlay = new(windowOverlay);
}

public override bool PointInside(CGPoint point, UIEvent? uievent)
Expand All @@ -119,12 +109,16 @@ public override bool PointInside(CGPoint point, UIEvent? uievent)

var disableTouchEvent = false;

if (overlay.DisableUITouchEventPassthrough)
disableTouchEvent = true;
else if (overlay.EnableDrawableTouchHandling)
disableTouchEvent = overlay.WindowElements.Any(n => n.Contains(new Point(point.X, point.Y)));
if (_overlay.TryGetTarget(out var overlay))
{
if (overlay.DisableUITouchEventPassthrough)
disableTouchEvent = true;
else if (overlay.EnableDrawableTouchHandling)
disableTouchEvent = overlay.WindowElements.Any(n => n.Contains(new Point(point.X, point.Y)));

overlay.OnTappedInternal(new Point(point.X, point.Y));
}

OnTouch?.Invoke(this, point);
return disableTouchEvent;
}
}
Expand Down

0 comments on commit df7bd4e

Please sign in to comment.