-
Notifications
You must be signed in to change notification settings - Fork 686
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
Avoid heap corruption issue by setting the property via the DP instead of the property setter #5781
Avoid heap corruption issue by setting the property via the DP instead of the property setter #5781
Conversation
…d of the property setter.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/AZP RUN |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -63,6 +64,7 @@ class SharedHelpers | |||
|
|||
// Actual OS version checks | |||
static bool IsAPIContractV14Available(); // 21H1 | |||
static bool IsAPIContractV10Available(); // 20H1 |
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.
static bool IsAPIContractV10Available(); // 20H1 | |
static bool IsAPIContractV10Available(); // 20H1 |
{ | ||
// On 19h1 and 19h2 there is a heap corruption bug that occurs when setting the TintLuminosityOpacity via the property, | ||
// So we'll set it by the DP instead. | ||
brush.SetValue(winrt::Windows::UI::Xaml::Media::AcrylicBrush::TintLuminosityOpacityProperty(), box_value(luminosityValue)); |
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.
IIRC the only difference is that this doesn't trigger the "notify on property change" handlers, which is probably ok for this prop but would be worth adding a comment for
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.
This matches my understanding of the issue and the proposal that I had made in the internal email investigation thread we had. Thank you for doing this! Hopefully we can get a release with this fix very soon!
…d of the property setter (#5781) * Avoid heap corruption issue by setting the property via the DP instead of the property setter. * Disable setting the property on 19h1 and 19h2 * reenable set by DP to test with repo project
🎉 Handy links: |
An OS bug was fixed in 20h1 that resolved the heap corruption bug our code is hitting on 19h1 and 19h2. Fixes #5435