Skip to content

Conversation

@LeafShi1
Copy link
Member

@LeafShi1 LeafShi1 commented Jan 3, 2025

Fixes #12692

Root Cause

  • This issue is caused by PR Enable nullability in remaining DataGridViewCell members #10406,which removes the check whether the control is null before executing OnMouseUp in file DataGridViewCell.cs
    That 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

  • Add a check if DataGridView is not null before executing OnMouseUp in function OnMouseUpInternal, 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

  • Application crashes when the customer clicks on a DataGridView cell, where previously the content was selected.

Regression?

Risk

  • Minimal, we are restoring the previous code, i.e. re-adding a null-check

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 content
DataGridView1.Rows.Clear();
DataGridView1.Rows.Add(1, 0, "ABCD");

Image

After

DataGridView Rows can be cleaned and re-added normally
AfterChange

Test methodology

  • Manually, added a unit test

Test environment(s)

  • .net 10.0.0-alpha.1.24631.2
Microsoft Reviewers: Open in CodeFlow

@LeafShi1 LeafShi1 requested a review from a team as a code owner January 3, 2025 03:07
@codecov
Copy link

codecov bot commented Jan 3, 2025

Codecov Report

Attention: Patch coverage is 95.83333% with 1 line in your changes missing coverage. Please review.

Project coverage is 76.12895%. Comparing base (9007f33) to head (0b9a711).
Report is 31 commits behind head on main.

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     
Flag Coverage Δ
Debug 76.12895% <95.83333%> (+0.12257%) ⬆️
integration 18.16321% <0.00000%> (+0.11730%) ⬆️
production 50.01476% <75.00000%> (+0.24972%) ⬆️
test 97.02618% <100.00000%> (-0.00270%) ⬇️
unit 47.24991% <75.00000%> (+0.19892%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@LeafShi1 LeafShi1 added the waiting-review This item is waiting on review by one or more members of team label Jan 3, 2025
@ricardobossan
Copy link
Member

I tested the PR code, and the issue appears to be resolved:

12701

LGTM!

Copy link
Contributor

@Tanya-Solyanik Tanya-Solyanik left a 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?

@Tanya-Solyanik Tanya-Solyanik added waiting-author-feedback The team requires more information from the author and removed waiting-review This item is waiting on review by one or more members of team labels Jan 6, 2025
@Tanya-Solyanik Tanya-Solyanik changed the title Add judgment "DataGridView is not null" before executing OnMouseUp in function OnMouseUpInternal Add a null-check for "DataGridView" property before executing OnMouseUp in function OnMouseUpInternal Jan 6, 2025
@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback The team requires more information from the author label Jan 7, 2025
@LeafShi1
Copy link
Member Author

LeafShi1 commented Jan 7, 2025

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?

Will do that and share my findings once the review is complete.

@Tanya-Solyanik
Copy link
Contributor

@LeafShi1 - review is practically complete, the only remaining step is the investigation of similar cases. We would want to service all duplicates at once.

@Tanya-Solyanik Tanya-Solyanik added the waiting-author-feedback The team requires more information from the author label Jan 7, 2025
@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback The team requires more information from the author label Jan 8, 2025
@Tanya-Solyanik Tanya-Solyanik added the 💥 regression-release Regression from a public release label Jan 8, 2025
@LeafShi1 LeafShi1 merged commit b33827a into dotnet:main Jan 9, 2025
8 checks passed
@LeafShi1 LeafShi1 deleted the Fix_12692_Add_conditional_judgment_in_OnMouseUpInternal branch January 9, 2025 05:51
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0 Preview1 milestone Jan 9, 2025
@LeafShi1
Copy link
Member Author

LeafShi1 commented Jan 9, 2025

/backport to release/9.0

@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2025

Tanya-Solyanik pushed a commit that referenced this pull request Jan 9, 2025
…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.
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

💥 regression-release Regression from a public release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Object reference not set to an instance of an object" exception pops up after clicked the DataGridViewCell content

3 participants