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.

Sign up for GitHub

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

Jump to bottom

Consider warning about IPlatformViewHandler #12

Open
jonathanpeppers opened this issue Nov 6, 2023 · 2 comments
Open

Consider warning about IPlatformViewHandler #12

jonathanpeppers opened this issue Nov 6, 2023 · 2 comments

Comments

@jonathanpeppers
Copy link
Owner

jonathanpeppers commented Nov 6, 2023 •
edited

This problem came up with MAUI's Frame class, as FrameRenderer is not an NSObject but somehow produced a cycle that I could cause problems with at runtime.

Examples:
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Nov 6, 2023
@jonathanpeppers
[ios] fix memory leak in Frame, VisualElementRenderer
6c6a3ae 
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
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Nov 6, 2023
@jonathanpeppers
[ios] fix memory leak in Frame, VisualElementRenderer
d03c7f1 
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
@jonathanpeppers jonathanpeppers mentioned this issue Nov 6, 2023
Merged
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Nov 7, 2023
@jonathanpeppers
[ios] fix memory leak in ImageButton
6445ac7 
Context: dotnet#18365

Adding a parameter to the test:

    [Theory("Handler Does Not Leak")]
    [InlineData(typeof(ImageButton))]
    public async Task HandlerDoesNotLeak(Type type)

Shows a memory leak in `ImageButton`, caused by the cycle

* `ImageButtonHandler` ->
* `UIButton` events like `TouchUpInside` ->
* `ImageButtonHandler`

I could solve this problem by creating a `ImageButtonProxy` class -- the
same pattern I've used in other PRs to avoid cycles. This makes an
intermediate type to handle the events and breaks the cycle.

Still thinking if the analyzer could have caught this, issue filed at:

jonathanpeppers/memory-analyzers#12
@jonathanpeppers jonathanpeppers mentioned this issue Nov 7, 2023
Merged
@jonathanpeppers
Copy link
Owner Author

jonathanpeppers commented Nov 7, 2023 •
edited

Ok, I think I see the pattern that isn't currently caught by the analyzer:

class Foo
{
    UIButton bar = new();

    public Foo()
    {
        bar.TouchUpInside += OnTouch;
    }

    void OnTouch(object sender, EventArgs e) { }
}

Foo is a non-NSObject, but causes a cycle: Foo -> UIButton -> Foo.

I'll need to test in isolation to verify this leaks every time. It definitely leaks if Foo was a .NET MAUI handler class.

No idea how the analyzer could check for this yet... I guess warn on any event? That would warn on all usage of TouchUpInside!

@jonathanpeppers
Copy link
Owner Author

Ok, the simple example above, Foo, does not appear to leak in isolation:

https://github.com/jonathanpeppers/MemoryLeaksOniOS/compare/UIButtonEvents

So, this must only happen with MAUI IPlatformViewHandler types? There must be something that causes a cycle there.

jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Nov 9, 2023
@jonathanpeppers
[ios] fix memory leak in ImageButton
a12261c 
Context: dotnet#18365

Adding a parameter to the test:

    [Theory("Handler Does Not Leak")]
    [InlineData(typeof(ImageButton))]
    public async Task HandlerDoesNotLeak(Type type)

Shows a memory leak in `ImageButton`, caused by the cycle

* `ImageButtonHandler` ->
* `UIButton` events like `TouchUpInside` ->
* `ImageButtonHandler`

I could solve this problem by creating a `ImageButtonProxy` class -- the
same pattern I've used in other PRs to avoid cycles. This makes an
intermediate type to handle the events and breaks the cycle.

Still thinking if the analyzer could have caught this, issue filed at:

jonathanpeppers/memory-analyzers#12
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Nov 9, 2023
@jonathanpeppers
[ios] fix memory leak in Stepper
52124a1 
Context: dotnet#18365

Adding a parameter to the test:

    [Theory("Handler Does Not Leak")]
    [InlineData(typeof(Stepper))]
    public async Task HandlerDoesNotLeak(Type type)

Shows a memory leak in `Stepper`, caused by the cycle

* `StepperHandler` ->
* `UIStepper.ValueChanged` event ->
* `StepperHandler`

I could solve this problem by creating a `StepperProxy` class -- the
same pattern I've used in other PRs to avoid cycles. This makes an
intermediate type to handle the events and breaks the cycle.

Still thinking if the analyzer could have caught this, issue filed at:

jonathanpeppers/memory-analyzers#12
@jonathanpeppers jonathanpeppers mentioned this issue Nov 9, 2023
Merged
rmarinho pushed a commit to dotnet/maui that referenced this issue Nov 10, 2023
@jonathanpeppers
[ios] fix memory leak in Stepper (#18663)
71b591c 
Context: #18365

Adding a parameter to the test:

    [Theory("Handler Does Not Leak")]
    [InlineData(typeof(Stepper))]
    public async Task HandlerDoesNotLeak(Type type)

Shows a memory leak in `Stepper`, caused by the cycle

* `StepperHandler` ->
* `UIStepper.ValueChanged` event ->
* `StepperHandler`

I could solve this problem by creating a `StepperProxy` class -- the
same pattern I've used in other PRs to avoid cycles. This makes an
intermediate type to handle the events and breaks the cycle.

Still thinking if the analyzer could have caught this, issue filed at:

jonathanpeppers/memory-analyzers#12
jonathanpeppers added a commit to dotnet/maui that referenced this issue Nov 10, 2023
@jonathanpeppers
[ios/android] fix memory leak in ImageButton (#18602)
fabfc55 
* [ios] fix memory leak in `ImageButton`

Context: #18365

Adding a parameter to the test:

    [Theory("Handler Does Not Leak")]
    [InlineData(typeof(ImageButton))]
    public async Task HandlerDoesNotLeak(Type type)

Shows a memory leak in `ImageButton`, caused by the cycle

* `ImageButtonHandler` ->
* `UIButton` events like `TouchUpInside` ->
* `ImageButtonHandler`

I could solve this problem by creating a `ImageButtonProxy` class -- the
same pattern I've used in other PRs to avoid cycles. This makes an
intermediate type to handle the events and breaks the cycle.

Still thinking if the analyzer could have caught this, issue filed at:

jonathanpeppers/memory-analyzers#12

* [android] fix memory leak in `ImageButton`

Context: a270ebd
Context: 1bbe79d
Context: material-components/material-components-android#2063

Reviewing a GC dump of the device tests, I noticed a `System.Action`
keeping the `ImageButton` alive:

    Microsoft.Maui.Controls.ImageButton
        System.Action
            Java.Lang.Thread.RunnableImplementor

So next, I looked for `System.Action` and found the path on the
`ReferencedTypes` tab:

    System.Action
        Microsoft.Maui.Platform.ImageButtonExtensions.[]c__DisplayClass4_0
            Google.Android.Material.ImageView.ShapeableImageView
            Microsoft.Maui.Controls.ImageButton

Which led me to the code:

    public static async void UpdatePadding(this ShapeableImageView platformButton, IImageButton imageButton)
    {
        platformButton.SetContentPadding(imageButton);
        platformButton.Post(() =>
        {
            platformButton.SetContentPadding(imageButton);
        });
        platformButton.SetContentPadding(imageButton);
    }

?!? Why is this code calling `SetContentPadding` three times?

Reviewing the commit history:

* a270ebd
* 1bbe79d
* material-components/material-components-android#2063

I could comment out the code and the leak is solved, but I found I could
also change the code to use `await Task.Yield()` for the same result.
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Nov 10, 2023
@jonathanpeppers
[ios] fix memory leak in Slider
c122cbf 
Context: dotnet#18365

Adding a parameter to the test:

    [Theory("Handler Does Not Leak")]
    [InlineData(typeof(Slider))]
    public async Task HandlerDoesNotLeak(Type type)

Shows a memory leak in `Slider`, caused by the cycle

* `SliderHandler` ->
* `UISlider` events ->
* `SliderHandler`

I could solve this problem by creating a `SliderProxy` class -- the
same pattern I've used in other PRs to avoid cycles. This makes an
intermediate type to handle the events and breaks the cycle.

Still thinking if the analyzer could have caught this, issue filed at:

jonathanpeppers/memory-analyzers#12
@jonathanpeppers jonathanpeppers mentioned this issue Nov 10, 2023
Merged
jonathanpeppers added a commit to jonathanpeppers/maui that referenced this issue Nov 10, 2023
@jonathanpeppers
[ios] fix memory leak in Switch
735f03f 
Context: dotnet#18365

Adding a parameter to the test:

    [Theory("Handler Does Not Leak")]
    [InlineData(typeof(Switch))]
    public async Task HandlerDoesNotLeak(Type type)

Shows a memory leak in `Switch`, caused by the cycle

* `SwitchHandler` ->
* `UISwitch.ValueChanged` event ->
* `SwitchHandler`

I could solve this problem by creating a `SwitchProxy` class -- the same
pattern I've used in other PRs to avoid cycles. This makes an
intermediate type to handle the events and breaks the cycle.

Still thinking if the analyzer could have caught this, issue filed at:

jonathanpeppers/memory-analyzers#12
@jonathanpeppers jonathanpeppers mentioned this issue Nov 10, 2023
Merged
rmarinho pushed a commit to dotnet/maui that referenced this issue Nov 13, 2023
@jonathanpeppers
[ios] fix memory leak in Switch (#18682)
2e286bf 
Context: #18365

Adding a parameter to the test:

    [Theory("Handler Does Not Leak")]
    [InlineData(typeof(Switch))]
    public async Task HandlerDoesNotLeak(Type type)

Shows a memory leak in `Switch`, caused by the cycle

* `SwitchHandler` ->
* `UISwitch.ValueChanged` event ->
* `SwitchHandler`

I could solve this problem by creating a `SwitchProxy` class -- the same
pattern I've used in other PRs to avoid cycles. This makes an
intermediate type to handle the events and breaks the cycle.

Still thinking if the analyzer could have caught this, issue filed at:

jonathanpeppers/memory-analyzers#12
rmarinho pushed a commit to dotnet/maui that referenced this issue Nov 13, 2023
@jonathanpeppers
[ios] fix memory leak in Slider (#18681)
8d029d4 
Context: #18365

Adding a parameter to the test:

    [Theory("Handler Does Not Leak")]
    [InlineData(typeof(Slider))]
    public async Task HandlerDoesNotLeak(Type type)

Shows a memory leak in `Slider`, caused by the cycle

* `SliderHandler` ->
* `UISlider` events ->
* `SliderHandler`

I could solve this problem by creating a `SliderProxy` class -- the
same pattern I've used in other PRs to avoid cycles. This makes an
intermediate type to handle the events and breaks the cycle.

Still thinking if the analyzer could have caught this, issue filed at:

jonathanpeppers/memory-analyzers#12
jonathanpeppers added a commit to dotnet/maui that referenced this issue Nov 13, 2023
@jonathanpeppers
[ios] fix memory leak in Frame, VisualElementRenderer (#18552)
8bc2a2f 
Context: #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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@jonathanpeppers