Beware of the leaked image list when using the TVS_CHECKBOXES style


The TVS_CHECK­BOXES tree view style is quirky, which is a nice way of saying that it's crazy.

As we noted last time, the support for check boxes was migrated in from external code, and it followed the pattern for external code. In particular, the state image list for the check boxes needs to be manually destroyed, because when you created the check boxes manually, you also needed to clean them up.

Yes, this goes against the general principle that things which were created by the control will be destroyed by the control. Like I said, the TVS_CHECK­BOXES tree view style is quirky. And if you fail to accommodate this quirk, you end up with a resource leak.

MSDN suggests that you use the TVM_GET­IMAGE­LIST message to retrieve the state image list, and then use Image­List_Destroy to destroy it. I prefer to exchange the image list out by setting the state image list to null, then destroying the returned image list (which is the previous image list). This avoids dangling references to a destroyed image list, and it also means that if somehow you try to clean up the image lists twice, the second one will simply not do anything since it won't see anything to clean up.

ImageList_Destroy(TreeView_SetImageList(hwndTV, nullptr, TVSIL_STATE));

We take advantage of the fact that the HIMAGELIST parameter to the Image­List_Destroy function is marked _In_opt_, which means that it is permissible to pass nullptr.

Okay, with these two common errors out of the way, I'll continue next time by beginning our exploration of tree view check boxes from the ground up.

Comments (6)
  1. jon says:

    I’m curious how code can just be migrated in to an operating system without any sort of QA associated with it?

    At no stage no one reviewed this feature and suggested maybe it should be fixed to get rid of all these quirks before it shipped? Or is it just seen as easier/cheaper to make that the responsibility of every third-party developer in the world, forever?

    1. Devin says:

      @jon

      That’s easy to say, but Windows has many millions of lines of code. It’s impossible to review everything for each release. Even for small products, it’s impractical to review everything whenever you want to release. Even then, once you see an issue, is it possible to fix it and be absolutely certain that your fix won’t have any side effects?

      This gets compounded when you are talking about an API surface that is consumed by an unknown number of applications. If a quirk is known by developers, then they will have worked around it, or could even be relying on that quirk. Fixing that quirk could possibly cause more issues than it solves.

      My experience in this industry is that developers are a cantankerous bunch, and hate anything new or different. They need to be coddled. This means years of deprecation warnings, gigabytes of documentation on how to update, and a personal visit from Raymond wielding a large stick. All of this will be ignored. Finally, when things stop working the developers will blame Microsoft for “randomly” breaking things.

      1. voo says:

        You’re misunderstanding Jon. He’s asking why there weren’t any code reviews in place when the code was first added internally.

        And the answer that 640k gives seems rather logical – it was an internal API first which gets much less oversight and nobody reviewed the code when it was made public.

        Also this happened ages ago, I doubt there was a standardized process in place with regards to testing and code reviews. We shouldn’t forget that even 10 years ago writing unit tests was still anything but common.

    2. 640k says:

      ItThe imported code was taken from the Office code base, which was thought to be stable and tested.

  2. Aged .Net Guy says:

    The reference in the third paragraph is to a blog post that tells us of a checkbox-related resource leak in the .Net implementation of TreeView. Following refs back, it seems the original bug report behind the blog post came from roughly 2008.

    Does anyone know which .Net framework version or service pack update resolved this problem?

    To make this question actionable as is Raymond’s wont(https://blogs.msdn.microsoft.com/oldnewthing/20160329-00/?p=93214) let me rephrase it as: How far back into our codebase vs. supported configs do we need to go (or not) to find places we ought to implement that blog’s suggested shim?

  3. Alex Guteniev says:

    I recall other cases of stock state images for controls that were added at about the same time or later.
    They are
    1. LVS_EX_CHECKBOXES
    2. HDF_SORTDOWN, HDF_SORTUP, HDF_CHECKBOX, etc in HDITEM::fmt

    Any similar issues with them?

Comments are closed.

Skip to main content