-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add a null-check for "DataGridView" property before executing OnMouseUp in function OnMouseUpInternal #12701
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
Add a null-check for "DataGridView" property before executing OnMouseUp in function OnMouseUpInternal #12701
Conversation
… function OnMouseUpInternal
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12701 +/- ##
===================================================
+ Coverage 76.00638% 76.12895% +0.12257%
===================================================
Files 3181 3188 +7
Lines 639670 640106 +436
Branches 47215 47233 +18
===================================================
+ Hits 486190 487306 +1116
+ Misses 149968 149269 -699
- Partials 3512 3531 +19
Flags with carried forward coverage won't be shown. Click here to find out more. |
Tanya-Solyanik
left a comment
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.
Thank you for jumping on this issue! Looks good, I added only minor comments.
Could you please also review the rootcause PR for similar cases, for example other instances of invocations of user-provided event handlers that are followed by an access to the parent DGV?
src/System.Windows.Forms/src/System/Windows/Forms/Controls/DataGridView/DataGridViewCell.cs
Show resolved
Hide resolved
src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataGridViewCellTests.cs
Outdated
Show resolved
Hide resolved
src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataGridViewCellTests.cs
Outdated
Show resolved
Hide resolved
Will do that and share my findings once the review is complete. |
src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataGridViewCellTests.cs
Show resolved
Hide resolved
src/System.Windows.Forms/src/System/Windows/Forms/Controls/DataGridView/DataGridViewCell.cs
Outdated
Show resolved
Hide resolved
src/System.Windows.Forms/src/System/Windows/Forms/Controls/DataGridView/DataGridViewCell.cs
Outdated
Show resolved
Hide resolved
|
@LeafShi1 - review is practically complete, the only remaining step is the investigation of similar cases. We would want to service all duplicates at once. |
|
/backport to release/9.0 |
|
Started backporting to release/9.0: https://github.com/dotnet/winforms/actions/runs/12684423877 |
src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataGridViewCellTests.cs
Show resolved
Hide resolved
…cuting OnMouseUp in function OnMouseUpInternal (#12742) Backport of #12701 to release/9.0 Customer Impact Application crashes when the customer clicks on a DataGridView cell, where previously the content was selected. Root Cause This issue is caused by PR Enable nullability in remaining DataGridViewCell members #10406. which removed the check whether the parent control is null before executing OnMouseUp when handling a MouseUp event. That fix moved the null check to the top of our MouseUp event handler. However, reference to the parent control can be set to null when this cell (or a row containing it) is cleared in the user-provided click event handler which is invoked in the middle of this method. We should restore the null check before accessing the parent DataGridView.

Fixes #12692
Root Cause
OnMouseUpin file DataGridViewCell.csThat fix moved the null check for the parent DataGridView to the top of the method. However, the parent can be set to null when this cell (or a row containing it) is deleted. In the user's case the row is deleted when the DGV cell is clicked.
Proposed changes
DataGridView is not nullbefore executingOnMouseUpin functionOnMouseUpInternal, because the Parent DataGridView might have been disconnected in the user event handler that is executed before this call, for example if the user clears the DataGridView row.Customer Impact
Regression?
Risk
Screenshots
Before
Object reference not set to an instance of an object exception pops up after clicked the DataGridViewCell content when the function
DataGridView1_CellContentClickcontains contentDataGridView1.Rows.Clear();DataGridView1.Rows.Add(1, 0, "ABCD");After
DataGridView Rows can be cleaned and re-added normally

Test methodology
Test environment(s)
Microsoft Reviewers: Open in CodeFlow