[controls] fix memory leak in Window

[jonathanpeppers]

Fixes #10029

I could reproduce a memory leak by doing:

App.Current.OpenWindow(new Window { Page = new ContentPage() });

I found that App.Current._requestedWindows just held onto every Window object forever.

Note that we can't use ConditionalWeakTable here:

https://learn.microsoft.com/dotnet/api/system.runtime.compilerservices.conditionalweaktable-2

After initially trying this, I added a test showing not to use it. The problem is if the string is GC'd the Window will be lost.

We can use Dictionary<string, WeakReference<Window>> instead.

The only other change I made was to use Guid.ToString("n") as it removes {, }, and - characters from the string.

After these changes, I still found Window objects hanging around that appeared to be held onto by many other objects.

Then I reviewed:

void IWindow.Destroying()
{
    SendWindowDisppearing();
    Destroying?.Invoke(this, EventArgs.Empty);
    OnDestroying();

    Application?.RemoveWindow(this);
    // This wasn't here!
    Handler?.DisconnectHandler();
}

It appears that upon Window's closing, we didn't have any code that "disconnected" the MAUI handler. After this change, I could see Window objects properly go away.

I added a unit test as well.

[rmarinho]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[jonathanpeppers]

This might cause a test failure on iOS:

[jonathanpeppers]

Can repro the test failure locally, I can say that I am a bit confused:

There might be an issue in this test, will look into it.

[jonathanpeppers]

Ok the test uses async void to do work:

maui/src/Controls/tests/DeviceTests/Elements/Window/WindowTests.cs

Line 125 in 53f6e39

await CreateHandlerAndAddToWindow<WindowHandlerStub>(window, async (handler) =>

xunit is moving on as if the test passed, but test code is continuing to run.

Will review if we have a better way to do this.

[jonathanpeppers]

Will come back to this after this one lands:

#13536

[PureWeen]

I'm testing this on Android and the TryGetValue here seems like it's always evaluating to false.

[jonathanpeppers]

I added a test for this issue, and changed it to a Dictionary<string, WeakReference<Window>> which won't cause this behavior.

I think you were getting a new string instance and ConditionalWeakTable doesn't work in that case.

maui/src/Controls/tests/Core.UnitTests/WindowsTests.cs

Lines 664 to 670 in 2e7c3ff

	// A "cloned" key should still work
	string key;
	{
	var originalKey = table.Keys.OfType<string>().Single();
	key = new string(originalKey);
	Assert.NotSame(originalKey, key);
	}

[PureWeen]

/backport to net7.0

[github-actions]

Started backporting to net7.0: https://github.com/dotnet/maui/actions/runs/4315741361

[github-actions]

@PureWeen backporting to net7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: [controls] fix memory leak in `Window`
Using index info to reconstruct a base tree...
M	src/Controls/src/Core/HandlerImpl/Application/Application.Impl.cs
M	src/Controls/src/Core/HandlerImpl/Window/Window.Impl.cs
M	src/Controls/tests/Core.UnitTests/WindowsTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Controls/tests/Core.UnitTests/WindowsTests.cs
CONFLICT (content): Merge conflict in src/Controls/tests/Core.UnitTests/WindowsTests.cs
Auto-merging src/Controls/src/Core/HandlerImpl/Window/Window.Impl.cs
Auto-merging src/Controls/src/Core/HandlerImpl/Application/Application.Impl.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 [controls] fix memory leak in `Window`
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

[github-actions]

@PureWeen an error occurred while backporting to net7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.