[ios] fix memory leak in SearchBar#16383
Conversation
| if (OperatingSystem.IsIOSVersionAtLeast(13)) | ||
| { | ||
| return searchBar.SearchTextField; | ||
| else | ||
| return searchBar.GetSearchTextField(); | ||
| } |
There was a problem hiding this comment.
If iOS < 13 before, this just threw StackOverflowException? Let me know if I'm missing something, I ported a Swift example to C# instead.
6393d33 to
1c1851b
Compare
|
This has UITest failures I haven't seen on other PRs, there must be a problem. Looking into it. |
1bd2039 to
3d91b35
Compare
|
@jonathanpeppers do you remember what was the test failure? now we have the uitests not working so can't trust these results. |
|
@rmarinho I was just going to come back to this one when UITests are working. |
3d91b35 to
911ea75
Compare
|
This is green, but the UITests are disabled -- I am wondering if this actually breaks something, so will wait for UITests. |
|
@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 |
911ea75 to
9af950d
Compare
9af950d to
45f9b1f
Compare
45f9b1f to
63e5f5d
Compare
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.
cdf2ffb to
639d42c
Compare
jsuarezruiz
left a comment
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
Context: #16346
This addresses the memory leak discovered by:
I could reproduce a leak in a test such as:
Solved the issues by making a non-NSObject
MauiSearchBarProxyclass.I found & fixed a couple related issues:
SearchBarExtensions.GetSearchTextField()would have always thrownStackOverlowExceptionif iOS was less than 13? It just called itself?!?Removed
MauiSearchBar.EditingChanged, as we could subscribe to the event from theUITextFielddirectly instead.Fixes - #20024