Skip to content
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

fix: Prevent GC'on of native callback #4280

Merged
merged 1 commit into from
Nov 25, 2020

Conversation

RussKie
Copy link
Member

@RussKie RussKie commented Nov 25, 2020

Fixes #4264

Proposed changes

The native callback can be garbage collected even in a simple app, which leads to an app crash with ExecutionEngineException.
The issue can be observed even in an app as simple as:

namespace repro_listview
{
    static class Program
    {
        [STAThread]
        static void Main()
        {
            Application.SetHighDpiMode(HighDpiMode.SystemAware);
            Application.EnableVisualStyles();
            Application.SetCompatibleTextRenderingDefault(false);

            var f = new Form();
            var listView = new ListView();

            f.Controls.Add(listView);

            listView.Items.Add("A");
            listView.Items.Add("Z");
            listView.Items.Add("X");
            listView.ListViewItemSorter = new TestComparer();

            Application.Run(f);
        }

        class TestComparer : Comparer<ListViewItem>
        {
            public override int Compare(ListViewItem x, ListViewItem y)
            {
                GC.Collect();
                Thread.Sleep(10);
                return 0;
            }
        }
    }
}

Customer Impact

  • Interacting with a ListView with sort enabled won't be crashing an app due to a GC'ed native callback.

Regression?

  • Yes

Risk

  • Minimal

Test methodology

  • manual, by replacing binaries in C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App\6.0.0-alpha.1.20570.2
  • no automation test could be devised as the problem couldn't be reproduced from the Windows Forms codebase, either 5.0 or 6.0 branches
Microsoft Reviewers: Open in CodeFlow

The native callback can be garbage collected even in a simple app, which
leads to an app crash with ExecutionEngineException.

Fixes dotnet#4264
@RussKie RussKie requested a review from a team as a code owner November 25, 2020 06:56
@ghost ghost assigned RussKie Nov 25, 2020
@codecov
Copy link

codecov bot commented Nov 25, 2020

Codecov Report

Merging #4280 (4b0e22e) into master (12b8059) will increase coverage by 0.00773%.
The diff coverage is n/a.

@@                 Coverage Diff                 @@
##              master       #4280         +/-   ##
===================================================
+ Coverage   97.96185%   97.96958%   +0.00772%     
===================================================
  Files            499         499                 
  Lines         259010      259010                 
  Branches        4569        4569                 
===================================================
+ Hits          253731      253751         +20     
+ Misses          4473        4457         -16     
+ Partials         806         802          -4     
Flag Coverage Δ
Debug 97.96958% <ø> (+0.00772%) ⬆️
production 100.00000% <ø> (ø)
test 97.96958% <ø> (+0.00772%) ⬆️

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

@RussKie RussKie merged commit 9b140c0 into dotnet:master Nov 25, 2020
@RussKie RussKie deleted the fix_4264_collected_callback branch November 25, 2020 09:52
@ghost ghost added this to the 6.0 Preview1 milestone Nov 25, 2020
@RussKie
Copy link
Member Author

RussKie commented Nov 25, 2020

Thank you @kpreisser for your help repro'ing the issue.

madewokherd pushed a commit to madewokherd/winforms that referenced this pull request Nov 30, 2020
The native callback can be garbage collected even in a simple app, which
leads to an app crash with ExecutionEngineException.

Fixes dotnet#4264

(cherry picked from commit 9b140c0)
@ghost ghost locked as resolved and limited conversation to collaborators Jan 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
1 participant