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.

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

CommandBarFlyout crash fix #5937

Merged
merged 1 commit into from
Sep 21, 2021
Merged

CommandBarFlyout crash fix #5937

merged 1 commit into from
Sep 21, 2021

Conversation

RBrid
Copy link
Contributor

@RBrid RBrid commented Sep 21, 2021

CommandBarFlyout fix for internal bug 35782367.

Crash dumps indicated that the Hide() call was causing a null ref exception. The call occurs when the closing animation completes.
My assumption is that in rare cases the 'this' pointer is no longer valid by the time the closing animation completed and using a weak reference to the CommandBarFlyout instead solves the issue. I have not been able to repro the crash though after trying to get the CommandBarFlyout to be garbage-collected before the Hide call. So I was not able to confirm my assumption.

@RBrid RBrid self-assigned this Sep 21, 2021
@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Sep 21, 2021
@RBrid
Copy link
Contributor Author

RBrid commented Sep 21, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ranjeshj ranjeshj added team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Sep 21, 2021
Copy link
Contributor

@ranjeshj ranjeshj left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@marcelwgn marcelwgn left a comment

Choose a reason for hiding this comment

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

I assume we can't realistically add a test for this can we? Changes look good to me 🙂

@RBrid
Copy link
Contributor Author

RBrid commented Sep 21, 2021

@chingucoding, no repro no test :-(

@marcelwgn
Copy link
Contributor

@chingucoding, no repro no test :-(

Fair enough.

@RBrid RBrid merged commit 03a091b into main Sep 21, 2021
@RBrid RBrid deleted the user/regisb/CommandBarFlyoutFix2 branch September 21, 2021 21:14
RBrid added a commit that referenced this pull request Oct 14, 2021
This reverts commit 03a091b.

Undoing CommandBarFlyout changes because of crash regressions.
RBrid added a commit that referenced this pull request Oct 14, 2021
This reverts commit 03a091b.

Undoing CommandBarFlyout changes because of crash regressions.
@ghost
Copy link

ghost commented Apr 14, 2022

🎉Microsoft.UI.Xaml v2.8.0-prerelease.220413001 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Jul 13, 2022

🎉Microsoft.UI.Xaml v2.8.0-prerelease.220712001 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Jul 18, 2022

🎉Microsoft.UI.Xaml v2.8.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Jan 3, 2023

🎉Microsoft.UI.Xaml v2.8.2 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-CommandBarFlyout team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants