Fixed NullReferenceException when using x:Name with SolidColorBrush#28805
Fixed NullReferenceException when using x:Name with SolidColorBrush#28805Vignesh-SF3580 wants to merge 10 commits intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
Files not reviewed (1)
- src/Controls/tests/TestCases.HostApp/Issues/Issue28711.xaml: Language not supported
Comments suppressed due to low confidence (1)
src/Controls/src/Core/SolidColorBrush.cs:56
- [nitpick] Consider adding a dedicated unit test for the GetHashCode method to explicitly validate its behavior when Color is null, ensuring that the fix works as expected in isolation from the UI tests.
return -1234567890 + (Color?.GetHashCode() ?? 0);
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/rebase |
80c8900 to
d5d882f
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
I’ve made the changes based on your suggestions. Let me know if you have any further feedback. |
not really, but that'll do provided you fix the GetHashCode implementation as suggested I'll look at what is expected of this code later on |
I have made the changes as per your suggestion regarding the GetHashCode implementation. Please let me know if you have any further feedback. |
| } | ||
|
|
||
| [Fact] | ||
| public void TestGetSolidColorBrushHashCode() |
There was a problem hiding this comment.
Could include more tests for the added methods to GradientStip and imageBrush?
There was a problem hiding this comment.
@jsuarezruiz Tests for GradientStop already exist. I’ve now added tests for ImageBrush as well.
|
@StephaneDelcroix Any news on this one ? |
Fixes #28711 When using x:Name on a SolidColorBrush in ContentPage.Resources, a NullReferenceException was thrown because GetHashCode was called (during NameScope registration) before Color was set. Changes: - SolidColorBrush.GetHashCode(): Add null check for Color - GradientStop.GetHashCode(): Use HashCode.Combine for proper hashing - ImageBrush.GetHashCode(): Use HashCode.Combine for proper hashing - Add XAML unit test for x:Name on resources Close #28805
Fixes #28711 When using x:Name on a SolidColorBrush in ContentPage.Resources, a NullReferenceException was thrown because GetHashCode was called (during NameScope registration) before Color was set. The fix uses base.GetHashCode() (identity-based) instead of hashing the mutable Color/ImageSource properties. This is correct because: 1. These are mutable reference types - their hash shouldn't change 2. The original implementation was incorrect anyway (hash based on mutable state breaks dictionary/hashset semantics) Changes: - SolidColorBrush.GetHashCode(): Use base.GetHashCode() - GradientStop.GetHashCode(): Use base.GetHashCode() - ImageBrush.GetHashCode(): Use base.GetHashCode() - Add XAML unit test for x:Name on resources Close #28805
Fixes #28711 When using x:Name on a SolidColorBrush in ContentPage.Resources, a NullReferenceException was thrown because GetHashCode was called (during NameScope registration) before Color was set. The fix uses base.GetHashCode() (identity-based) instead of hashing the mutable Color/ImageSource properties. This is correct because: 1. These are mutable reference types - their hash shouldn't change 2. The original implementation was incorrect anyway (hash based on mutable state breaks dictionary/hashset semantics) Changes: - SolidColorBrush.GetHashCode(): Use base.GetHashCode() - GradientStop.GetHashCode(): Use base.GetHashCode() - ImageBrush.GetHashCode(): Use base.GetHashCode() - Add XAML unit test for x:Name on resources Close #28805
<!-- Please let the below note in for people that find this PR --> > [!NOTE] > Are you waiting for the changes in this PR to be merged? > It would be very helpful if you could [test the resulting artifacts](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ## Description Fixes #28711 Close #28805 When using `x:Name` on a `SolidColorBrush` (or other brush types) in `ContentPage.Resources`, a `NullReferenceException` was thrown. This occurred because `GetHashCode()` was called during NameScope registration before the `Color` property was set. ## Root Cause When XAML elements with `x:Name` are registered in the NameScope, the registration calls `GetHashCode()` on the object (for dictionary storage). The original `SolidColorBrush.GetHashCode()` implementation directly called `Color.GetHashCode()` without null-checking, causing a crash when `Color` was still `null`. ## Changes - **`SolidColorBrush.GetHashCode()`**: Added null-safe operator (`Color?.GetHashCode() ?? 0`) and use `HashCode.Combine()` for proper hashing - **`GradientStop.GetHashCode()`**: Updated to use `HashCode.Combine()` for proper hashing (fixes same potential issue) - **`ImageBrush.GetHashCode()`**: Updated to use `HashCode.Combine()` for proper hashing (fixes same potential issue) The changes use `HashCode.Combine()` for non-NETSTANDARD targets and a properly implemented hash formula for NETSTANDARD compatibility. ## Test Added XAML unit test `Maui28711.xaml`/`.xaml.cs` that: - Creates a page with `x:Name="namedBrush"` on a `SolidColorBrush` in Resources - Verifies the page loads without crashing across all 3 XAML inflators (Runtime, XamlC, SourceGen) - Validates the named field exists and the Color is set correctly ## Comparison with PR #28805 This PR supersedes #28805 with the following improvements: - Test is properly placed in `Xaml.UnitTests` with `XamlInflator` parameter (as requested by reviewer @StephaneDelcroix) - Added `unchecked` to the NETSTANDARD path to properly handle potential overflow - Cleaner implementation following existing patterns in the codebase
Fixes #28711 When using x:Name on a SolidColorBrush in ContentPage.Resources, a NullReferenceException was thrown because GetHashCode was called (during NameScope registration) before Color was set. The fix uses base.GetHashCode() (identity-based) instead of hashing the mutable Color/ImageSource properties. This is correct because: 1. These are mutable reference types - their hash shouldn't change 2. The original implementation was incorrect anyway (hash based on mutable state breaks dictionary/hashset semantics) Changes: - SolidColorBrush.GetHashCode(): Use base.GetHashCode() - GradientStop.GetHashCode(): Use base.GetHashCode() - ImageBrush.GetHashCode(): Use base.GetHashCode() - Add XAML unit test for x:Name on resources Close #28805
Fixes #28711 When using x:Name on a SolidColorBrush in ContentPage.Resources, a NullReferenceException was thrown because GetHashCode was called (during NameScope registration) before Color was set. The fix uses base.GetHashCode() (identity-based) instead of hashing the mutable Color/ImageSource properties. This is correct because: 1. These are mutable reference types - their hash shouldn't change 2. The original implementation was incorrect anyway (hash based on mutable state breaks dictionary/hashset semantics) Changes: - SolidColorBrush.GetHashCode(): Use base.GetHashCode() - GradientStop.GetHashCode(): Use base.GetHashCode() - ImageBrush.GetHashCode(): Use base.GetHashCode() - Add XAML unit test for x:Name on resources Close #28805
Fixes #28711 When using x:Name on a SolidColorBrush in ContentPage.Resources, a NullReferenceException was thrown because GetHashCode was called (during NameScope registration) before Color was set. The fix uses base.GetHashCode() (identity-based) instead of hashing the mutable Color/ImageSource properties. This is correct because: 1. These are mutable reference types - their hash shouldn't change 2. The original implementation was incorrect anyway (hash based on mutable state breaks dictionary/hashset semantics) Changes: - SolidColorBrush.GetHashCode(): Use base.GetHashCode() - GradientStop.GetHashCode(): Use base.GetHashCode() - ImageBrush.GetHashCode(): Use base.GetHashCode() - Add XAML unit test for x:Name on resources Close #28805
Fixes #28711 When using x:Name on a SolidColorBrush in ContentPage.Resources, a NullReferenceException was thrown because GetHashCode was called (during NameScope registration) before Color was set. The fix uses base.GetHashCode() (identity-based) instead of hashing the mutable Color/ImageSource properties. This is correct because: 1. These are mutable reference types - their hash shouldn't change 2. The original implementation was incorrect anyway (hash based on mutable state breaks dictionary/hashset semantics) Changes: - SolidColorBrush.GetHashCode(): Use base.GetHashCode() - GradientStop.GetHashCode(): Use base.GetHashCode() - ImageBrush.GetHashCode(): Use base.GetHashCode() - Add XAML unit test for x:Name on resources Close #28805
Issue Detail
Null exception occurs when using x:Name for SolidColorBrush.
Root Cause:
When x:Name is assigned to SolidColorBrush, it triggers the RegisterName method in NameScope.cs, adding the value to a dictionary. It calls GetHashCode when setting x:Name. In that time, color was null and it throws null exception.
Description of change:
Added a null check for Color resolves the issue.
Reference:
https://github.com/dotnet/maui/blob/main/src/Controls/src/Core/GradientStop.cs#L53
https://github.com/dotnet/maui/blob/main/src/Controls/src/Core/ImageBrush.cs#L32
Issues Fixed
Fixes #28711
Screenshots
28711Before.mov
28711After.mov