Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ios] fix memory leak in SearchBar #16383

Merged
merged 5 commits into from
Feb 27, 2024
Merged

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Jul 26, 2023

Context: #16346

This addresses the memory leak discovered by:

src/Core/src/Platform/iOS/MauiSearchBar.cs(36,63): error MA0001: Event 'TextSetOrChanged' 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.
src/Core/src/Platform/iOS/MauiSearchBar.cs(54,32): error MA0001: Event 'OnMovedToWindow' 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.
src/Core/src/Platform/iOS/MauiSearchBar.cs(55,32): error MA0001: Event 'EditingChanged' 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.
src/Core/src/Platform/iOS/MauiSearchBar.cs(70,31): error MA0003: Subscribing to events with instance method 'OnEditingChanged' could cause memory leaks in an NSObject subclass. Remove the subscription or convert the method to a static method.

I could reproduce a leak in a test such as:

await InvokeOnMainThreadAsync(() =>
{
    var layout = new Grid();
    var searchBar = new SearchBar();
    layout.Add(searchBar);
    var handler = CreateHandler<LayoutHandler>(layout);
    viewReference = new WeakReference(searchBar);
    handlerReference = new WeakReference(searchBar.Handler);
    platformViewReference = new WeakReference(searchBar.Handler.PlatformView);
});

await AssertionExtensions.WaitForGC(viewReference, handlerReference, platformViewReference);
Assert.False(viewReference.IsAlive, "SearchBar should not be alive!");
Assert.False(handlerReference.IsAlive, "Handler should not be alive!");
Assert.False(platformViewReference.IsAlive, "PlatformView should not be alive!");

Solved the issues by making a non-NSObject MauiSearchBarProxy class.

I found & fixed a couple related issues:

  • SearchBarExtensions.GetSearchTextField() would have always thrown StackOverlowException if iOS was less than 13? It just called itself?!?

  • Removed MauiSearchBar.EditingChanged, as we could subscribe to the event from the UITextField directly instead.

Fixes - #20024

Comment on lines 12 to +15
if (OperatingSystem.IsIOSVersionAtLeast(13))
{
return searchBar.SearchTextField;
else
return searchBar.GetSearchTextField();
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If iOS < 13 before, this just threw StackOverflowException? Let me know if I'm missing something, I ported a Swift example to C# instead.

@jonathanpeppers jonathanpeppers added memory-leak 💦 Memory usage grows / objects live forever platform/iOS 🍎 labels Jul 26, 2023
@samhouts samhouts added this to the .NET 8 GA milestone Jul 31, 2023
@Eilon Eilon added the legacy-area-perf Startup / Runtime performance label Aug 1, 2023
@jonathanpeppers
Copy link
Member Author

This has UITest failures I haven't seen on other PRs, there must be a problem. Looking into it.

@rmarinho
Copy link
Member

@jonathanpeppers do you remember what was the test failure? now we have the uitests not working so can't trust these results.

@jonathanpeppers
Copy link
Member Author

@rmarinho I was just going to come back to this one when UITests are working.

@jonathanpeppers
Copy link
Member Author

This is green, but the UITests are disabled -- I am wondering if this actually breaks something, so will wait for UITests.

@rmarinho
Copy link
Member

@jonathanpeppers can you rebase main?! and i can run them for you to check. Main should be green for uitest right now, we will enable during this week

@samhouts samhouts added the stale Indicates a stale issue/pr and will be closed soon label Sep 11, 2023
@samhouts samhouts modified the milestones: .NET 8 GA, Under Consideration Sep 19, 2023
@samhouts samhouts modified the milestones: Under Consideration, .NET 8 GA Sep 28, 2023
@PureWeen PureWeen modified the milestones: .NET 8 GA, .NET 8 SR1 Sep 28, 2023
@jonathanpeppers jonathanpeppers removed the stale Indicates a stale issue/pr and will be closed soon label Nov 9, 2023
@Redth Redth removed this from the .NET 8 SR1 milestone Nov 30, 2023
@samhouts samhouts added the stale Indicates a stale issue/pr and will be closed soon label Dec 14, 2023
Context: dotnet#16346

This addresses the memory leak discovered by:

    src/Core/src/Platform/iOS/MauiSearchBar.cs(36,63): error MA0001: Event 'TextSetOrChanged' 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.
    src/Core/src/Platform/iOS/MauiSearchBar.cs(54,32): error MA0001: Event 'OnMovedToWindow' 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.
    src/Core/src/Platform/iOS/MauiSearchBar.cs(55,32): error MA0001: Event 'EditingChanged' 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.
    src/Core/src/Platform/iOS/MauiSearchBar.cs(70,31): error MA0003: Subscribing to events with instance method 'OnEditingChanged' could cause memory leaks in an NSObject subclass. Remove the subscription or convert the method to a static method.

I could reproduce a leak in a test such as:

    await InvokeOnMainThreadAsync(() =>
    {
        var layout = new Grid();
        var searchBar = new SearchBar();
        layout.Add(searchBar);
        var handler = CreateHandler<LayoutHandler>(layout);
        viewReference = new WeakReference(searchBar);
        handlerReference = new WeakReference(searchBar.Handler);
        platformViewReference = new WeakReference(searchBar.Handler.PlatformView);
    });

    await AssertionExtensions.WaitForGC(viewReference, handlerReference, platformViewReference);
    Assert.False(viewReference.IsAlive, "SearchBar should not be alive!");
    Assert.False(handlerReference.IsAlive, "Handler should not be alive!");
    Assert.False(platformViewReference.IsAlive, "PlatformView should not be alive!");

Solved the issues by making a non-NSObject `MauiSearchBarProxy` class.

I found & fixed a couple related issues:

* `SearchBarExtensions.GetSearchTextField()` would have always thrown
  `StackOverlowException` if iOS was less than 13? It just called itself?!?

* Removed `MauiSearchBar.EditingChanged`, as we could subscribe to the
  event from the `UITextField` directly instead.
Copy link
Contributor

@jsuarezruiz jsuarezruiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that the status is Draft, is there anything pending?

platformView.OnEditingStarted += OnEditingStarted;
platformView.EditingChanged += OnEditingChanged;
platformView.OnEditingStopped += OnEditingStopped;
_proxy.Connect(this, VirtualView, platformView);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR completely breaks all the UI tests, which accesses a SearchBar to find & run each test?

I did not figure out the cause, it seemed like SearchBar generally worked when I tried in other apps like the DeviceTests.

@samhouts samhouts removed the stale Indicates a stale issue/pr and will be closed soon label Feb 6, 2024
@jonathanpeppers jonathanpeppers marked this pull request as ready for review February 27, 2024 02:33
@jonathanpeppers jonathanpeppers requested a review from a team as a code owner February 27, 2024 02:33
@rmarinho rmarinho merged commit fdebf71 into dotnet:main Feb 27, 2024
43 of 47 checks passed
@jonathanpeppers jonathanpeppers deleted the SearchBarLeaks branch February 27, 2024 15:36
@github-actions github-actions bot locked and limited conversation to collaborators Mar 29, 2024
@Eilon Eilon added t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) area-controls-searchbar SearchBar control and removed legacy-area-perf Startup / Runtime performance labels May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-searchbar SearchBar control fixed-in-8.0.10 fixed-in-9.0.0-preview.2.10247 fixed-in-9.0.0-preview.2.10293 memory-leak 💦 Memory usage grows / objects live forever platform/iOS 🍎 t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants