Skip to content

Workaround for bad ImageList ownership management#3601

Merged
JeremyKuhne merged 1 commit intodotnet:masterfrom
weltkante:issue3358-workaround
Jul 22, 2020
Merged

Workaround for bad ImageList ownership management#3601
JeremyKuhne merged 1 commit intodotnet:masterfrom
weltkante:issue3358-workaround

Conversation

@weltkante
Copy link
Contributor

@weltkante weltkante commented Jul 17, 2020

Fixes #3358

Proposed changes

Disable sharing ImageList handles because they are implemented incorrectly and can double-free handles, which will crash if the handle was reused already (#3358). Instead of sharing ImageLists this PR creates an explicit duplicate when assigning an ImageList to the native control, and destroys the previous instance. This guarantees the handle used by the native control is always unique and can be safely destroyed. To avoid leaks all ImageLists are removed when the handle is destroyed.

#3531 is tracking proper implementation of ImageList ownership

For TreeViews the StateImageList is never shareable and it had similar bugs, so the correct solution is also to create unique handles and destroy them. Part of the logic already was there, but it was too eager and destroyed ImageLists it didn't own. By creating unique handles this problem is avoided.

Customer Impact

Regression?

  • No, the memory corruption and ownership issues were also present in Desktop Framework

Risk

  • This changes semantics of how ImageLists are managed, users which send window messages directly to the native control may not be prepared to handle this.

Before

  • ImageLists were destroyed while still in use
  • ImageLists were destroyed twice (risk of crashing if numeric handle value was reused)

After

  • ImageList destruction result is checked in debug builds to verify no double-free happened

Test methodology

  • Running the test suite in a debugger and ensure the asserts detected no double-frees
Microsoft Reviewers: Open in CodeFlow

@weltkante weltkante requested a review from a team as a code owner July 17, 2020 20:58
@ghost ghost assigned weltkante Jul 17, 2020
@weltkante
Copy link
Contributor Author

weltkante commented Jul 17, 2020

/cc @JeremyKuhne @RussKie your choice if you want to take this, or if you want to leave tests broken for now and fix ImageList ownership management properly later (#3531). There are obvious downsides to this workaround, using more memory and changing native control configuration (see PR description). I've no strong opinion, I don't think it happens much in practice outside of the unit tests.

@weltkante weltkante force-pushed the issue3358-workaround branch from 3d2e429 to 2a0cc20 Compare July 17, 2020 21:33
Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @weltkante !

@JeremyKuhne JeremyKuhne merged commit 03db3fb into dotnet:master Jul 22, 2020
@ghost ghost added this to the 5.0 RC1 milestone Jul 22, 2020
@weltkante weltkante deleted the issue3358-workaround branch July 27, 2020 20:28
RussKie added a commit to RussKie/winforms that referenced this pull request Nov 12, 2020
Changes in `ImageList` ownership model in dotnet#3601 means that very are now two
instances of imagelists - one instance is tracked by Windows Forms (i.e. managed)
side, and another one tracked by the underlying Win32 (unmanaged) side.
This was done due to an observed ownership disconnect between the managed and unmanged
code, that led to situations where the unmanaged code would effectively destroy
and instance of an imagelist, which the managed code was oblivious to.

However with the above change changes to images in an imagelist on the managed
side, i.e. a user adding or replacing an image in the imagelist, would not be reflected
in the imagelist on the unmanaged side, and thus would not be reflected in the UI
(which is drawn by the Win32).

The fix reuses the established infrastructure that notifies the managed imagelist
implementation of changes to the images collection, and once a notification of
a chance is received, the unmanaged imagelist is re-created, thus ensuring the UI
has all the correct images to display.

Fixes dotnet#4169
RussKie added a commit to RussKie/winforms that referenced this pull request Nov 27, 2020
Revert "Explicit ImageList ownership management. (dotnet#3601)"
This reverts commit 03db3fb.

Revert "Fix `ListView` no longer displays images (dotnet#4184)"
This reverts commit d0608e7.

We have observed an instability of tests under stress (and reasonably
high degree of concurrency) presumably caused by ImageList lifetime
handling (notably dotnet#3358).
The changes introduced in dotnet#3601 have helped with tests stability,
however resulted in a number of regressions, such as dotnet#4169 and dotnet#4275.

Restore the original implementation given it worked reasonably well for
the past so many years.
RussKie added a commit that referenced this pull request Dec 2, 2020
Revert "Explicit ImageList ownership management. (#3601)"
This reverts commit 03db3fb.

Revert "Fix `ListView` no longer displays images (#4184)"
This reverts commit d0608e7.

We have observed an instability of tests under stress (and reasonably
high degree of concurrency) presumably caused by ImageList lifetime
handling (notably #3358).
The changes introduced in #3601 have helped with tests stability,
however resulted in a number of regressions, such as #4169 and #4275.

Restore the original implementation given it worked reasonably well for
the past so many years.
RussKie added a commit to RussKie/winforms that referenced this pull request Dec 2, 2020
Revert "Explicit ImageList ownership management. (dotnet#3601)"
This reverts commit 03db3fb.

Revert "Fix `ListView` no longer displays images (dotnet#4184)"
This reverts commit d0608e7.

We have observed an instability of tests under stress (and reasonably
high degree of concurrency) presumably caused by ImageList lifetime
handling (notably dotnet#3358).
The changes introduced in dotnet#3601 have helped with tests stability,
however resulted in a number of regressions, such as dotnet#4169 and dotnet#4275.

Restore the original implementation given it worked reasonably well for
the past so many years.

(cherry picked from commit b0ee5da)
RussKie added a commit that referenced this pull request Dec 9, 2020
Revert "Explicit ImageList ownership management. (#3601)"
This reverts commit 03db3fb.

Revert "Fix `ListView` no longer displays images (#4184)"
This reverts commit d0608e7.

We have observed an instability of tests under stress (and reasonably
high degree of concurrency) presumably caused by ImageList lifetime
handling (notably #3358).
The changes introduced in #3601 have helped with tests stability,
however resulted in a number of regressions, such as #4169 and #4275.

Restore the original implementation given it worked reasonably well for
the past so many years.

(cherry picked from commit b0ee5da)
@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.

Flaky tests: ImageList randomly crashes tests and result in AccessViolationException

2 participants