Skip to content

Solve image list woes#3526

Merged
RussKie merged 5 commits intodotnet:masterfrom
RussKie:Solve_ImageList_woes
Jul 2, 2020
Merged

Solve image list woes#3526
RussKie merged 5 commits intodotnet:masterfrom
RussKie:Solve_ImageList_woes

Conversation

@RussKie
Copy link
Contributor

@RussKie RussKie commented Jun 30, 2020

Proposed changes

  • Centralise lifetime management of native ImageList
    Move calls to Create, Duplicate and Destroy into NativeImageList to reduce a chance of interleaved calls to Win32 API.
  • Track disposal of NativeImageList
  • Dispose and do not reuse ImageList instances in tests.
    Quite likely the observe AVE were a result of use reusing a shared instance of an ImageList across theory runs. Quite likely xUnit would dispose the instance while another test was run, leading to a corrupt heap. Resolves Flaky tests: ImageList randomly crashes tests and result in AccessViolationException #3358
  • Prevent ImageList allocation disposing PropertyGrid. Resolves PropertyGrid allocates ImageList handles while disposing #3485
  • Refactor ToolStripItem.Animate and ToolStripItem.set_Image methods to reduce nesting
Microsoft Reviewers: Open in CodeFlow

RussKie added 2 commits July 2, 2020 11:02
Move calls to Create, Duplicate and Destroy into NativeImageList to
reduce a chance of interleaved calls to Win32 API.
@RussKie RussKie force-pushed the Solve_ImageList_woes branch 2 times, most recently from 6acb995 to b020ffc Compare July 2, 2020 01:15
@RussKie RussKie marked this pull request as ready for review July 2, 2020 01:16
@RussKie RussKie requested a review from a team as a code owner July 2, 2020 01:16
@RussKie RussKie requested a review from JeremyKuhne July 2, 2020 03:17
@RussKie
Copy link
Contributor Author

RussKie commented Jul 2, 2020

/cc: @weltkante @hughbe

@dotnet dotnet deleted a comment from azure-pipelines bot Jul 2, 2020
@codecov
Copy link

codecov bot commented Jul 2, 2020

Codecov Report

Merging #3526 into master will decrease coverage by 31.43013%.
The diff coverage is 84.61538%.

@@                 Coverage Diff                  @@
##              master       #3526          +/-   ##
====================================================
- Coverage   67.03175%   35.60162%   -31.43014%     
====================================================
  Files           1352         900         -452     
  Lines         505814      253025      -252789     
  Branches       40899       36734        -4165     
====================================================
- Hits          339056       90081      -248975     
+ Misses        161191      158092        -3099     
+ Partials        5567        4852         -715     
Flag Coverage Δ
#Debug 35.60162% <84.61538%> (-31.43014%) ⬇️
#production 35.60162% <84.61538%> (+0.00544%) ⬆️
#test ?

RussKie added 3 commits July 2, 2020 13:31
Quite likely the observe AVE were a result of use reusing a shared instance
of an `ImageList` across theory runs. Quite likely xUnit would dispose the
instance while another test was run, leading to a corrupt heap.

Resolves dotnet#3358
@RussKie RussKie force-pushed the Solve_ImageList_woes branch from 9a01f39 to abe892e Compare July 2, 2020 03:32
@RussKie RussKie merged commit e510dfb into dotnet:master Jul 2, 2020
@RussKie RussKie deleted the Solve_ImageList_woes branch July 2, 2020 04:06
@ghost ghost added this to the 5.0 Preview8 milestone Jul 2, 2020
@RussKie RussKie mentioned this pull request May 21, 2021
RussKie added a commit to RussKie/winforms that referenced this pull request May 21, 2021
There are certain code paths where we are unable to track the lifetime of the object,
for example in the following scenarios:

     this.imageList1.ImageStream = (System.Windows.Forms.ImageListStreamer)(resources.GetObject("imageList1.ImageStream"));
or
     resources.ApplyResources(this.listView1, "listView1");

In those cases the loose instances will be collected by the GC.

Undo the regression introduced in  dotnet#3526.

Fixes dotnet#3567
Closes dotnet#4928
RussKie added a commit to RussKie/winforms that referenced this pull request May 25, 2021
There are certain code paths where we are unable to track the lifetime of the object,
for example in the following scenarios:

     this.imageList1.ImageStream = (System.Windows.Forms.ImageListStreamer)(resources.GetObject("imageList1.ImageStream"));

or

     resources.ApplyResources(this.listView1, "listView1");

In those cases the loose instances will be collected by the GC.

Undo the regression introduced in  dotnet#3526.
RussKie added a commit to RussKie/winforms that referenced this pull request May 25, 2021
There are certain code paths where we are unable to track the lifetime of the object,
for example in the following scenarios:

     this.imageList1.ImageStream = (System.Windows.Forms.ImageListStreamer)(resources.GetObject("imageList1.ImageStream"));

or

     resources.ApplyResources(this.listView1, "listView1");

In those cases the loose instances will be collected by the GC.

Undo the regression introduced in  dotnet#3526.
RussKie added a commit that referenced this pull request May 26, 2021
There are certain code paths where we are unable to track the lifetime of the object,
for example in the following scenarios:

     this.imageList1.ImageStream = (System.Windows.Forms.ImageListStreamer)(resources.GetObject("imageList1.ImageStream"));

or

     resources.ApplyResources(this.listView1, "listView1");

In those cases the loose instances will be collected by the GC.

Undo the regression introduced in  #3526.
@ghost ghost locked as resolved and limited conversation to collaborators Jan 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PropertyGrid allocates ImageList handles while disposing Flaky tests: ImageList randomly crashes tests and result in AccessViolationException

2 participants