-
Notifications
You must be signed in to change notification settings - Fork 997
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
base: main
Are you sure you want to change the base?
Conversation
… function OnMouseUpInternal
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #12701 +/- ##
===================================================
+ Coverage 76.00638% 76.03495% +0.02857%
===================================================
Files 3181 3181
Lines 639670 639690 +20
Branches 47215 47215
===================================================
+ Hits 486190 486388 +198
+ Misses 149968 149775 -193
- Partials 3512 3527 +15
Flags with carried forward coverage won't be shown. Click here to find out more. |
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?
@@ -3040,7 +3040,7 @@ internal void OnMouseUpInternal(DataGridViewCellMouseEventArgs e) | |||
DataGridView.OnCommonCellContentClick(e.ColumnIndex, e.RowIndex, e.Clicks > 1); | |||
} | |||
|
|||
if (e.ColumnIndex < DataGridView.Columns.Count && e.RowIndex < DataGridView.Rows.Count) | |||
if (DataGridView is not null && e.ColumnIndex < DataGridView.Columns.Count && e.RowIndex < DataGridView.Rows.Count) |
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 change looks confusing because at the top of this method we tested that DGV is not null already. Please add a comment explaining that the custom Click event handler might delete the cell and the parent DGV.
|
||
using DataGridView dataGridView = new(); | ||
dataGridView.Columns.Add(column); | ||
SubDataGridViewCheckBoxCell cell = (SubDataGridViewCheckBoxCell)dataGridView.Rows[0].Cells[0]; |
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.
Consider:
SubDataGridViewCheckBoxCell cell = (SubDataGridViewCheckBoxCell)dataGridView.Rows[0].Cells[0]; | |
var cell = (SubDataGridViewCheckBoxCell)dataGridView.Rows[0].Cells[0]; |
act.Should().NotThrow(); | ||
} | ||
|
||
private void dataGridView_CellContentClick(object sender, DataGridViewCellEventArgs e) |
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.
Could you please inline this event handler to make the test more case self-contained.
dataGridView.CellContentClick += new DataGridViewCellEventHandler((s, e) =>
{
DataGridView dataGridView = (DataGridView)s;
dataGridView.Rows.Clear();
dataGridView.Rows.Add();
}
);
Fixes #12692
Root Cause
OnMouseUp
in file DataGridViewCell.csProposed changes
DataGridView is not null
before executingOnMouseUp
in 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_CellContentClick
contains 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