[Peek] Fix memory leak caused by unmanaged bitmaps not being freed
Summary of the Pull Request
Fixes the Peek PowerToy leaking unmanaged resources associated with bitmaps when using the ImagePreviewer. Also ensures any Previewer implementing IDisposable will be correctly disposed.
PR Checklist
- [x] Closes: #33593
- [ ] Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
- [ ] Tests: Added/updated and all pass
- [ ] Localization: All end user facing strings can be localized
- [ ] Dev docs: Added/updated
- [ ] New binaries: Added on the required places
- [ ] JSON for signing for new binaries
- [ ] WXS for installer for new binaries and localization folder
- [ ] YML for CI pipeline for new test projects
- [ ] YML for signed pipeline
- [ ] Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx
Detailed Description of the Pull Request / Additional comments
The Peek PowerToy creates a Previewer for each item as the user navigates a folder. For the ImagePreviewer, a Clear method is provided which cleans up unmanaged resources, including bitmaps created for preview thumbnails. Unfortunately, Clear is not called on the old Previewer before a new instance is created, leading to unreachable resources which are never freed. This PR ensures Dispose is called on a Previewer if it implements IDisposable. ImagePreviewer calls Clear within its Dispose, which fixes the leak.
Validation Steps Performed
Manual validation:
- Attached to the
PowerToys.Peek.UIexe in VS, where the memory leak manifests. - Created a Heap Snapshot.
- Manually navigated through a test folder, which contains 90% image files, and also zip files for several minutes.
- Created another Heap Snapshot.
- Confirmed that the Process Memory used was stable.
- Confirmed that only a small number of pinned
Byte[]instances were present in the heap snapshot. - Left the program running for more than an hour as I did other work. Confirmed that it still ran without issue and memory issues had not resurfaced.
Before Fix
Despite garbage collection, unmanaged resources are not freed, leading to an ever-increasing amount of memory used - nearly 2GB in this example.
After Fix
@microsoft-github-policy-service agree
Thank you for approving the merge, @drawbyperpetual !
I can speak to the strange behaviour you were seeing previously, where the increase in private bytes used was not as prevalent when the same files were previewed again: this could be because of the thumbnail retrieval code in Peek and the relationship with the Windows thumbnail cache. You are 'priming' the cache by previewing the images, as Peek always calls the Shell thumbnail retrieval/creation routine for every image, even when the full size image loads correctly. This was the fundamental 'tell' of the memory leak issue, as the thumbnail bitmaps are unmanaged and never freed. I've recently raised an issue about the unnecessary thumbnail generation as #34490 . In the majority of cases in my testing, the bitmaps are never used, so the #34490 fix actually saves a considerable amount of memory use and subsequent garbage collection (75%+ GC reduction).
I totally agree about the existing Dispose code, too. I'm happy to do that update in a separate PR to match the recommended pattern and ensure idempotency.
@daverayment as FYI, we've been heads down trying to get ship .84 and stabilize based on feedback. We appreciate the PRs here!
@crutkas Thanks! It's a privilege to be able to contribute, and there's certainly no rush. Best of luck with the upcoming release 😀
Thanks a lot for the contribution! Merging this in!