-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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.
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
fix(Windows): WindowsInputPane memory leak #16916
base: master
Are you sure you want to change the base?
fix(Windows): WindowsInputPane memory leak #16916
Conversation
You can test this PR using the following package version. |
{ | ||
lock (_lock) | ||
{ | ||
DestroyIfNeeded(); | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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;
}
}
}
}
Potentially all of the following classes are affected by the same problem
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
There was a problem hiding this comment.
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.
What does the pull request do?
Fix memory eak on windows platform
What is the current behavior?
see #16900
What is the updated/expected behavior with this PR?
How was the solution implemented (if it's not obvious)?
Checklist
Breaking changes
Obsoletions / Deprecations
Fixed issues
Closes #16900