[CP-stable][Impeller] Ensure that the TextureGLES destructor cleans up all objects that it holds including the sync fence#187403
Conversation
…p all objects that it holds including the sync fence TextureGLES owns a texture and may be given ownership of cached FBO and sync fence objects. Use UniqueHandleGLES to manage the lifecycle of these objects. Fixes flutter#186899
|
This pull request was opened from and to a release candidate branch. This should only be done as part of the official Flutter release process. If you are attempting to make a regular contribution to the Flutter project, please close this PR and follow the instructions at Tree Hygiene for detailed instructions on contributing to Flutter. Reviewers: Use caution before merging pull requests to release branches. Ensure the proper procedure has been followed. |
There was a problem hiding this comment.
Code Review
This pull request refactors TextureGLES to manage its GLES handles (handle_, fence_, and cached_fbo_) using UniqueHandleGLES instead of raw HandleGLES, enabling automated cleanup and the removal of the explicit TextureGLES destructor. It also adds a unit test to verify that the texture destructor deletes its associated fence. The review feedback recommends marking the move constructor and move assignment operator of UniqueHandleGLES as noexcept and refactoring them to use member initializer lists instead of std::swap to optimize performance and adhere to the Google C++ Style Guide.
gaaclarke
left a comment
There was a problem hiding this comment.
lgtm, tested and fixes problems on stable
407abe9
into
flutter:flutter-3.44-candidate.0
TextureGLES owns a texture and may be given ownership of cached FBO and sync fence objects. Use UniqueHandleGLES to manage the lifecycle of these objects.
Fixes #186899
Issue Link:
#186899
Impact Description:
This can cause a resource leak in some scenarios (particularly animated images).
Changelog Description:
The PR ensures that a GLES fence sync object held by a texture will be deleted if the texture is destructed before the fence was used.
Workaround:
No workaround.
Risk:
What is the risk level of this cherry-pick?
Test Coverage:
Are you confident that your fix is well-tested by automated tests?
Validation Steps:
Run the sample app in #186899 (comment) using Impeller/GLES.
Look at the count of file descriptors in
/proc/[pid]/fdfor the app process. Without the fix, the count will increase quickly. With the fix, the count will be stable.The exact behavior of the resource leak may vary across devices, and some devices may not show it as a leak of file descriptors. I was able to reproduce the file descriptor leak on a Pixel 3 with an Adreno GPU.