[ios] fix memory leak in Frame, VisualElementRenderer#18552
Merged
jonathanpeppers merged 1 commit intodotnet:mainfrom Nov 13, 2023
Merged
[ios] fix memory leak in Frame, VisualElementRenderer#18552jonathanpeppers merged 1 commit intodotnet:mainfrom
Frame, VisualElementRenderer#18552jonathanpeppers merged 1 commit intodotnet:mainfrom
Conversation
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
21 tasks
jsuarezruiz
approved these changes
Nov 13, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context: #18365
I could reproduce a leak in
Frameby adding a parameterized test:FrameRendererhas a circular reference via its base type,VisualElementRenderer:Frame->FrameRenderer/VisualElementRenderer->Frame(viaVisualElementRenderer._virtualView)To solve this issue, I made
_virtualViewaWeakReference, 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 theElementproperty is accessed beforeSetVirtualView()returns. I had to create a_tempElementfield 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, onlyNSObject's. Will investigate further on this example here:jonathanpeppers/memory-analyzers#12