fix(Win32): PopupImpl memory leak

[workgroupengineering]

What does the pull request do?

Fix memory leak when OverlayPopups is false and popup opens, PopupImpl is not released when popup closes.

What is the current behavior?

PopupImpl is not released when popup closes.

What is the updated/expected behavior with this PR?

How was the solution implemented (if it's not obvious)?

• Removed closures from TopLevel, WindowBase and Window that retention PopupImpl
• Avoid PopupImpl retention derived from cross-reference from Avalonia.Win32/WindowImpl and WindowsInputPane

Checklist

• Added unit tests (if possible)?
• Added XML documentation to any related classes?
• Consider submitting a PR to https://github.com/AvaloniaUI/avalonia-docs with user documentation

Breaking changes

Obsoletions / Deprecations

Fixed issues

[avaloniaui-bot]

You can test this PR using the following package version. 11.2.999-cibuild0051610-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[kekekeks]

I don't quite see the point of altering the callback signature. It's only possible to call CreatePlatformImplBinding from the same object instance, and any callbacks would be referencing this rather than the PlatformImpl value captured during callback creation.

[workgroupengineering]

The closure creates a cross reference that does not allow the PopupImpl to be collected

To test

1. Apply Sandbox Patch SandBox-Test-Patch.patch
2. Build Sandbox in Release mode
3. Run dotMemoery
4. Make memory Snapshot (Snapshot 1)
5. Click on Button e
6. Dismiss popup with click on window
7. Repeat steps 5, 6 ten times.
8. Make memory Snapshot (Snapshot 2)
9. Force GC Collect
10. Make memory Snapshot (Snapshot 3)
11. Kill Sandbox
12. Compare Snapshot 2 and 3
13. Filter for PopupImp
14. You can see there is only ManagedPopupPositionerPopupImplHelper+<>c
15. Revert some change with:

      git checkout 7041139ee0556c474dad7dc599c0b69bcdf5d1b7~ -- "src/Avalonia.Controls/WindowBase.cs"
      git checkout 7041139ee0556c474dad7dc599c0b69bcdf5d1b7~ -- "src/Avalonia.Controls/Window.cs"
      git checkout 7041139ee0556c474dad7dc599c0b69bcdf5d1b7~ -- "src/Avalonia.Controls/TopLevel.cs"

16. Repeat steps from 2 to 13
17. In in the list besides ManagedPopupPositionerPopupImplHelper there is a PopupImpl instance
18. If you step into you can see that retained from Avalonia.Controls.TopLevel+<>c__DisplayClass33_0.impl
    

[kekekeks]

It's the closure that's passed to CompositionTarget, not the one related to CreatePlatformImplBinding. The closures passed to CreatePlatformImplBinding are consuming PlatformImpl property and are capturing this.

[kekekeks]

BTW, the leak itself is about keeping a reference to a disposed PopupRoot, rather than about TopLevel keeping references to its internals after being disposed.

[workgroupengineering]

So I revert TopLevel except for line 222?

[kekekeks]

You can replace line 222 with Renderer = new CompositingRenderer(this, impl.Compositor, () => PlatformImpl.Surfaces ?? []);, it will capture this and use the property. The rest of the TopLevel changes seems to be not needed.

[avaloniaui-bot]

You can test this PR using the following package version. 11.2.999-cibuild0051621-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[workgroupengineering]

Do I need to make any other changes?

[maxkatz6]

We already dispose pane on WM_DESTROY. And it should be guaranteed to be called unless process was terminated (in which case we don't care about disposing).

Avalonia/src/Windows/Avalonia.Win32/WindowImpl.AppWndProc.cs

Line 112 in 41e86fb

_inputPane?.Dispose();

[maxkatz6]

Other than that lgtm

[avaloniaui-bot]

You can test this PR using the following package version. 11.2.999-cibuild0052254-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[avaloniaui-bot]

You can test this PR using the following package version. 11.2.999-cibuild0052256-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]