Add _isReleasingDataSource to prevent unnecessary operations on CurrentCell when changing or releasing DataSource #13320

LeafShi1 · 2025-04-17T07:40:42Z

Root cause

Regression introduced in PR #4637

When closing the dialog in edit mode, DataSource is set to null, and then CurrentCell = null is set. Then unnecessary method EndEdit of the SetCurrentCellAddressCore is called.

Proposed changes

Add _isReleasingDataSource in DataGridView and set it to null in property DataSource before setting CurrentCell = null to prevent the EndEdit method of the SetCurrentCellAddressCore from being called

Customer Impact

When the DataGridView is in editing mode, its dialog box can be closed normally

Regression?

Yes

Risk

Minimal

Screenshots

Before

When a dialog with a DataGridView that has focus is closed, an System.InvalidOperationException: Operation is not valid because it results in a reentrant call to the SetCurrentCellAddressCore function. is thrown

DataGridViewCrash.mp4

After

The DataGridView dialog with focus can be closed successfully

Test methodology

Manually

Test environment(s)

.net 10.0.0-preview.4.25216.9

Microsoft Reviewers: Open in CodeFlow

…ntCell when changing or releasing DataSource

codecov · 2025-04-17T08:12:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.57350%. Comparing base (2f7ddc4) to head (59cbb81).
Report is 7 commits behind head on main.

Additional details and impacted files

@@                 Coverage Diff                 @@
##                main      #13320         +/-   ##
===================================================
+ Coverage   62.56333%   62.57350%   +0.01017%     
===================================================
  Files           1560        1560                 
  Lines         159675      159689         +14     
  Branches       14904       14906          +2     
===================================================
+ Hits           99898       99923         +25     
+ Misses         59008       58997         -11     
  Partials         769         769

Flag	Coverage Δ
Debug	`62.57350% <ø> (+0.01017%)`	⬆️
integration	`11.31854% <ø> (-0.00061%)`	⬇️
production	`40.75725% <ø> (+0.02004%)`	⬆️
test	`95.69057% <ø> (ø)`
unit	`38.15830% <ø> (+0.01937%)`	⬆️

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull Request Overview

This PR prevents unnecessary operations on CurrentCell when changing or releasing DataSource by introducing a new flag.

Added a private flag (_isReleasingDataSource) to signal when DataSource is being released.
Wrapped the CurrentCell update in a try-finally block to maintain flag integrity.
Updated the SetCurrentCellAddressCore method to check the new flag and prevent calling EndEdit unnecessarily.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
DataGridView.cs	Introduced _isReleasingDataSource and updated the DataSource setter to use a try-finally block when setting CurrentCell.
DataGridView.Methods.cs	Modified the condition in SetCurrentCellAddressCore to skip EndEdit when _isReleasingDataSource is true.

Tanya-Solyanik

Added a small comment, please sent to testing when done. Is it possible to add a unit test to the UI tests assembly?

src/System.Windows.Forms/System/Windows/Forms/Controls/DataGridView/DataGridView.cs

…ead of field _isReleasingDataSource

LeafShi1 · 2025-04-21T04:10:01Z

Added a small comment, please sent to testing when done. Is it possible to add a unit test to the UI tests assembly?

Currently this exception only occurs when the secondary form is closed. I have tried, but in unit test, it is not possible to open another form through an event on a main form page and perform operations on the secondary form.

LeafShi1 · 2025-04-21T06:34:32Z

/backport to release/8.0

LeafShi1 · 2025-04-21T06:37:50Z

/backport to release/9.0

Olina-Zhang · 2025-04-21T09:17:16Z

tested this PR change, GH issues: 13304, 4216 were fixed, no new issue found.

Tanya-Solyanik · 2025-04-21T18:13:31Z

/backport to release/9.0

LeafShi1 · 2025-04-23T00:46:08Z

/backport to release/8.0

github-actions · 2025-04-23T00:46:19Z

Started backporting to release/8.0: https://github.com/dotnet/winforms/actions/runs/14607450996

LeafShi1 · 2025-04-23T00:46:44Z

/backport to release/9.0

github-actions · 2025-04-23T00:46:55Z

Started backporting to release/9.0: https://github.com/dotnet/winforms/actions/runs/14607457364

github-actions · 2025-04-23T00:49:30Z

Started backporting to release/8.0: https://github.com/dotnet/winforms/actions/runs/14607450996

github-actions · 2025-04-23T00:49:56Z

@LeafShi1 backporting to release/8.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Add _isReleasingDataSource to prevent unnecessary operations on CurrentCell when changing or releasing DataSource
Using index info to reconstruct a base tree...
A	src/System.Windows.Forms/System/Windows/Forms/Controls/DataGridView/DataGridView.Methods.cs
A	src/System.Windows.Forms/System/Windows/Forms/Controls/DataGridView/DataGridView.cs
Falling back to patching base and 3-way merge...
Auto-merging src/System.Windows.Forms/src/System/Windows/Forms/DataGridView.cs
Auto-merging src/System.Windows.Forms/src/System/Windows/Forms/DataGridView.Methods.cs
CONFLICT (content): Merge conflict in src/System.Windows.Forms/src/System/Windows/Forms/DataGridView.Methods.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 Add _isReleasingDataSource to prevent unnecessary operations on CurrentCell when changing or releasing DataSource
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

…ntCell when changing or releasing DataSource (dotnet#13320) Fixes dotnet#13304 Root cause Regression introduced in PR dotnet#4637 When closing the dialog in edit mode, DataSource is set to null, and then CurrentCell = null is set. Then unnecessary method EndEdit of the SetCurrentCellAddressCore is called. Proposed changes Add _isReleasingDataSource in DataGridView and set it to null in property DataSource before setting CurrentCell = null to prevent the EndEdit method of the SetCurrentCellAddressCore from being called Customer Impact When the DataGridView is in editing mode, its dialog box can be closed normally Regression? Yes Risk Minimal

Tanya-Solyanik · 2025-04-23T23:57:03Z

src/System.Windows.Forms/System/Windows/Forms/Controls/DataGridView/DataGridView.cs

@@ -1921,7 +1922,17 @@ public object? DataSource
                    newDataSource.Disposed += OnDataSourceDisposed;
                }

-                CurrentCell = null;
+                _dataGridViewOper[OperationInReleasingDataSource] = true;


Should we actually set this value inside the OnDataSourceDisposed instead of on any invocation of DataSource setter property? The new flag skips code that commits the value being editted, we want to skip the commit only if the control is diaposed and editing had not been completed.

Yes, this makes more sense. CurrentCell = null; should also be placed in OnDataSourceDisposed, what do you think?

I submitted a new PR #13362

CurrentCell = null; should stay because it manages event handlers and that logic is needed when DataSource is changed for reasons other that disposal.

…cused DataGridView control (#13350) Backport of #13320 to release/8.0 Fixes #13304 ## Proposed changes - When the DataGridView is in edit mode and the parent form is being closed, the DataGridView's `EndEdit` triggers the `CurrentCell` setting logic, because the DGV's handle has been destroyed at this time, we are re-creating it and then re-enter logic that sets up the current cell causing an exception to be thrown. Checking `isHandleCreated` before calling `EndEdit` can avoid executing related logic when the control has been destroyed, thereby preventing exceptions from occurring. ## Customer Impact - When the DataGridView is in editing mode, its dialog box can be closed normally ## Regression? - Yes, introduced in #4637 ## Testing - Manual testing with the user-provided project ## Risk - Low ###### Microsoft Reviewers: [Open in CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/dotnet/winforms/pull/13350)

…cused DataGridView control (#13348) Backport of #13320 to release/9.0 Fixes #13304 ## Proposed changes - When the DataGridView is in edit mode and the parent form is being closed, the DataGridView's `EndEdit` triggers the `CurrentCell` setting logic, because the DGV's handle has been destroyed at this time, we are re-creating it and then re-enter logic that sets up the current cell causing an exception to be thrown. Checking `isHandleCreated` before calling `EndEdit` can avoid executing related logic when the control has been destroyed, thereby preventing exceptions from occurring. ## Customer Impact - When the DataGridView is in editing mode, its dialog box can be closed normally ## Regression? - Yes, introduced in #4637 ## Testing - Manual testing with the user-provided project ## Risk - Low ###### Microsoft Reviewers: [Open in CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/dotnet/winforms/pull/13348)

Add _isReleasingDataSource to prevent unnecessary operations on Curre…

ecf5b7f

…ntCell when changing or releasing DataSource

LeafShi1 requested a review from a team as a code owner April 17, 2025 07:40

github-actions bot added the area-controls-DataGridView label Apr 17, 2025

LeafShi1 requested review from Tanya-Solyanik, ricardobossan and Epica3055 April 17, 2025 07:41

dotnet-policy-service bot assigned LeafShi1 Apr 17, 2025

LeafShi1 requested a review from Copilot April 18, 2025 00:56

Copilot AI reviewed Apr 18, 2025

View reviewed changes

Tanya-Solyanik added the waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Apr 18, 2025

Tanya-Solyanik reviewed Apr 18, 2025

View reviewed changes

src/System.Windows.Forms/System/Windows/Forms/Controls/DataGridView/DataGridView.cs Outdated Show resolved Hide resolved

Tanya-Solyanik added the waiting-author-feedback The team requires more information from the author label Apr 18, 2025

Add a new flag _dataGridViewOper[OperationInReleasingDataSource] inst…

0d1418f

…ead of field _isReleasingDataSource

Removing field _isReleasingDataSource

59cbb81

Olina-Zhang removed the waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Apr 21, 2025

Tanya-Solyanik approved these changes Apr 21, 2025

View reviewed changes

Tanya-Solyanik merged commit 848bacb into dotnet:main Apr 21, 2025
8 checks passed

dotnet-policy-service bot added this to the 10.0 Preview4 milestone Apr 21, 2025

github-actions bot mentioned this pull request Apr 23, 2025

[release/9.0] InvalidOperationException when closing a dialog with focused DataGridView control #13348

Merged

LeafShi1 mentioned this pull request Apr 23, 2025

[release/8.0] InvalidOperationException when closing a dialog with focused DataGridView control #13350

Merged

LeafShi1 deleted the Issue_13304_fix_InvalidOperationException_DataGridView branch April 23, 2025 02:37

Tanya-Solyanik reviewed Apr 23, 2025

View reviewed changes

This was referenced Apr 24, 2025

Removing flag OperationInReleasingDataSource from DataGridView.cs #13362

Merged

Add test for PR 13320 #13342

Merged

github-actions bot locked and limited conversation to collaborators May 24, 2025

Add _isReleasingDataSource to prevent unnecessary operations on CurrentCell when changing or releasing DataSource #13320

Add _isReleasingDataSource to prevent unnecessary operations on CurrentCell when changing or releasing DataSource #13320

Uh oh!

Conversation

LeafShi1 commented Apr 17, 2025 • edited by Tanya-Solyanik Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Root cause

Proposed changes

Customer Impact

Regression?

Risk

Screenshots

Before

After

Test methodology

Test environment(s)

Microsoft Reviewers: Open in CodeFlow

Uh oh!

codecov bot commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Tanya-Solyanik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LeafShi1 commented Apr 21, 2025

Uh oh!

LeafShi1 commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LeafShi1 commented Apr 21, 2025

Uh oh!

Olina-Zhang commented Apr 21, 2025

Uh oh!

Uh oh!

Tanya-Solyanik commented Apr 21, 2025

Uh oh!

LeafShi1 commented Apr 23, 2025

Uh oh!

github-actions bot commented Apr 23, 2025

Uh oh!

LeafShi1 commented Apr 23, 2025

Uh oh!

github-actions bot commented Apr 23, 2025

Uh oh!

github-actions bot commented Apr 23, 2025

Uh oh!

github-actions bot commented Apr 23, 2025

Uh oh!

Tanya-Solyanik Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

LeafShi1 Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tanya-Solyanik Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LeafShi1 commented Apr 17, 2025 •

edited by Tanya-Solyanik

Loading

codecov bot commented Apr 17, 2025 •

edited

Loading

LeafShi1 commented Apr 21, 2025 •

edited

Loading

LeafShi1 Apr 24, 2025 •

edited

Loading