Skip to content

Conversation

@bijington
Copy link
Contributor

Description of Change

Linked Issues

PR Checklist

Additional information

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug where ShowPopupAsync would not return correctly under heavy garbage collection by updating the popup closing mechanism and adding tests to simulate GC pressure.

  • Replaced the WeakEventManager based PopupClosed event with a standard event invocation.
  • Introduced a new popup subclass (GarbageCollectionHeavySelfClosingPopup) to simulate heavy GC scenarios during popup closure.
  • Updated tests to include explicit GC.Collect calls around CloseAsync to validate the fix.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/CommunityToolkit.Maui/Views/Popup/PopupPage.shared.cs Replaced weak event manager with a standard event for PopupClosed
src/CommunityToolkit.Maui.UnitTests/Services/PopupServiceTests.cs Added GC.Collect calls and a new popup subclass for heavy GC testing
src/CommunityToolkit.Maui.UnitTests/Extensions/PopupExtensionsTests.cs Added a test to verify that ShowPopupAsync returns the correct result under heavy GC
src/CommunityToolkit.Maui.UnitTests/BaseHandlerTest.cs Registered the new GarbageCollectionHeavySelfClosingPopup service
Comments suppressed due to low confidence (3)

src/CommunityToolkit.Maui/Views/Popup/PopupPage.shared.cs:65

  • Replacing the WeakEventManager with a standard event might lead to memory retention if subscribers are not properly unsubscribed. Ensure that consumers of PopupClosed unsubscribe when appropriate.
	public event EventHandler<IPopupResult>? PopupClosed;

src/CommunityToolkit.Maui.UnitTests/Services/PopupServiceTests.cs:558

  • Consider adding a comment explaining the intent behind invoking GC.Collect here, to improve clarity for future maintainers of this test.
				GC.Collect();

src/CommunityToolkit.Maui.UnitTests/Services/PopupServiceTests.cs:560

  • Consider adding a comment explaining the intent behind invoking GC.Collect here, to improve clarity for future maintainers of this test.
				GC.Collect();

@bijington
Copy link
Contributor Author

This will likely conflict with #2749 but I am happy to deal with that

Copy link
Collaborator

@TheCodeTraveler TheCodeTraveler left a comment

Choose a reason for hiding this comment

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

Thanks Shaun! Good catch!

@bijington bijington enabled auto-merge (squash) June 20, 2025 21:27
@bijington
Copy link
Contributor Author

Full credit to @mfeingol for the investigation into this. It was easy once he worked out that it was garbage collection related

@TheCodeTraveler TheCodeTraveler disabled auto-merge June 20, 2025 21:36
@TheCodeTraveler TheCodeTraveler merged commit 47c5535 into main Jun 20, 2025
10 checks passed
@TheCodeTraveler TheCodeTraveler deleted the feature/sl-indefinite-show-popup-task branch June 20, 2025 21:36
@github-actions github-actions bot locked and limited conversation to collaborators Jun 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Popup V2 - Android - ShowPopupAsync(INavigation) returns a Task that never completes

2 participants