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

fix(Windows): WindowsInputPane memory leak #16916

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
+50 −12
Draft

fix(Windows): WindowsInputPane memory leak #16916

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 50 additions & 12 deletions src/Windows/Avalonia.Win32/Input/WindowsInputPane.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
using System;
using Avalonia.Controls.Platform;
using Avalonia.MicroCom;
using Avalonia.Win32.Interop;
using Avalonia.Win32.Win32Com;
using Avalonia.Win32.WinRT;
Expand All @@ -11,16 +10,17 @@ namespace Avalonia.Win32.Input;
internal unsafe class WindowsInputPane : InputPaneBase, IDisposable
{
private static readonly Lazy<bool> s_inputPaneSupported = new(() =>
WinRTApiInformation.IsTypePresent("Windows.UI.ViewManagement.InputPane"));
WinRTApiInformation.IsTypePresent("Windows.UI.ViewManagement.InputPane"));

// GUID: D5120AA3-46BA-44C5-822D-CA8092C1FC72
private static readonly Guid CLSID_FrameworkInputPane = new(0xD5120AA3, 0x46BA, 0x44C5, 0x82, 0x2D, 0xCA, 0x80, 0x92, 0xC1, 0xFC, 0x72);
// GUID: 5752238B-24F0-495A-82F1-2FD593056796
private static readonly Guid SID_IFrameworkInputPane = new(0x5752238B, 0x24F0, 0x495A, 0x82, 0xF1, 0x2F, 0xD5, 0x93, 0x05, 0x67, 0x96);
private static readonly Guid SID_IFrameworkInputPane = new(0x5752238B, 0x24F0, 0x495A, 0x82, 0xF1, 0x2F, 0xD5, 0x93, 0x05, 0x67, 0x96);

private readonly WindowImpl _windowImpl;
private WindowImpl _windowImpl;
private IFrameworkInputPane? _inputPane;
private readonly uint _cookie;
private Handler _eventHandler;

private WindowsInputPane(WindowImpl windowImpl)
{
Expand All @@ -31,12 +31,11 @@ private WindowsInputPane(WindowImpl windowImpl)
_inputPane = inputPane.CloneReference();
}

using (var handler = new Handler(this))
{
uint cookie = 0;
_inputPane.AdviseWithHWND(windowImpl.Handle.Handle, handler, &cookie);
_cookie = cookie;
}
_eventHandler = new Handler(this);
uint cookie = 0;
_inputPane.AdviseWithHWND(windowImpl.Handle.Handle, _eventHandler, &cookie);
_cookie = cookie;

}

public static WindowsInputPane? TryCreate(WindowImpl windowImpl)
Expand Down Expand Up @@ -72,6 +71,9 @@ private Rect ScreenRectToClient(UnmanagedMethods.RECT screenRect)

public void Dispose()
{
_windowImpl = null!;
_eventHandler?.Dispose();
_eventHandler = null!;
if (_inputPane is not null)
{
if (_cookie != 0)
Expand All @@ -84,9 +86,45 @@ public void Dispose()
}
}

private class Handler : CallbackBase, IFrameworkInputPaneHandler
private class Handler : IUnknown, IMicroComShadowContainer, IFrameworkInputPaneHandler
{
private readonly WindowsInputPane _pane;
private WindowsInputPane _pane;

private readonly object _lock = new();
private bool _destroyed;
public bool IsDestroyed => _destroyed;

public void Dispose()
{
lock (_lock)
{
DestroyIfNeeded();
}
Comment on lines +98 to +102
Copy link
Member

Choose a reason for hiding this comment

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

It might be problematic to completely destroy this object, as it still might be used by native side.
To avoid Window memory leak, it will be enough to nullify "_pane" reference on disposal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did as you suggest, WindowsInputPane is collected by GC but WindowsInputPane+Handler is reatain because Shadow keeps reference to it.

immagine

immagine

I tried clear both references but WindowsInputPane+Handler is still active

    private class Handler : CallbackBase, IFrameworkInputPaneHandler
    {
        private WindowsInputPane _pane;

        public Handler(WindowsInputPane pane) => _pane = pane;
        public void Showing(UnmanagedMethods.RECT* rect, int _) => _pane.OnStateChanged(true, *rect);
        public void Hiding(int fEnsureFocusedElementInView) => _pane.OnStateChanged(false, null);

        public void Clear()
        {
            lock (this)
            {
                _pane = null!;
                Shadow = null;
            }
        }
    }
}

immagine

Potentially all of the following classes are affected by the same problem

immagine

I think at this point that the best thing to do is to do two PRs

  • one to change the behavior of CallbackBase
  • the other to clear the reference to WindowsInputPane

@maxkatz6 , @kekekeks What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Potentially all of the following classes are affected by the same problem

It needs to be confirmed whether it's MicroCOM holds these references when it shouldn't, or native side haven't released them. And I feel like it's the second case.
Either way, I don't think it's in the scope of this PR.

Just the fact that Handler doesn't leak whole Window with it is enough for this PR.

}

void DestroyIfNeeded()
{
if (_destroyed)
return;
_destroyed = true;
Shadow?.Dispose();
Shadow = null;
_pane = null!;
}

public MicroComShadow? Shadow { get; set; }
public void OnReferencedFromNative()
{

}

public void OnUnreferencedFromNative()
{
lock (_lock)
{
DestroyIfNeeded();
}
}

public Handler(WindowsInputPane pane) => _pane = pane;
public void Showing(UnmanagedMethods.RECT* rect, int _) => _pane.OnStateChanged(true, *rect);
Expand Down
Loading