Skip to content

[ios] fix memory leak in SearchBar#16383

Merged
rmarinho merged 5 commits intodotnet:mainfrom
jonathanpeppers:SearchBarLeaks
Feb 27, 2024
Merged

[ios] fix memory leak in SearchBar#16383
rmarinho merged 5 commits intodotnet:mainfrom
jonathanpeppers:SearchBarLeaks

Conversation

@jonathanpeppers
Copy link
Copy Markdown
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
Copy Markdown
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 perf/memory-leak 💦 Memory usage grows / objects live forever (sub: perf) 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
Copy Markdown
Member Author

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

@rmarinho
Copy link
Copy Markdown
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
Copy Markdown
Member Author

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

@jonathanpeppers
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.

Copy link
Copy Markdown
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
@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 perf/general The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) 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 perf/general The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) perf/memory-leak 💦 Memory usage grows / objects live forever (sub: perf) platform/ios

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants