Make toImageSync collection safe #34931
Conversation
common/graphics/texture.h
Outdated
|
|
||
| private: | ||
| std::map<int64_t, std::shared_ptr<Texture>> mapping_; | ||
| std::map<uint32_t, ContextDestroyedListener*> images_; |
There was a problem hiding this comment.
Hold the reference to the image via a smart pointer here.
The raw pointer makes it unclear how the listener object's lifetime is managed and creates the potential for leaks.
For example, TextureRegistry::UnregisterContextDestroyedListener is not dropping the reference added by Picture::RasterizeToImageSync.
| DlDeferredImageGPU::~DlDeferredImageGPU() { | ||
| fml::TaskRunner::RunNowOrPostTask( | ||
| raster_task_runner_, | ||
| [image = std::move(image_), unref_queue = std::move(unref_queue_), |
There was a problem hiding this comment.
image_ is an sk_sp member that is supposed to be accessed only from the raster thread. But here the std::move will be reading and writing it on the UI thread without synchronization. This is potentially a race.
The image should be accessed via something wrapped in a std::atomic.
This is a preexisting issue in the current DlDeferredImageGPU implementation and can be addressed in a separate PR. However, it may make sense to handle it here.
Given the issues related to the TextureRegistry/DlDeferredImageGPU circular reference, one option would be to hold the SkImage here via a std::atomic<shared_ptr> to a wrapper object. The TextureRegistry could then hold a weak_ptr to the wrapper.
There was a problem hiding this comment.
Done - because the shard_ptr specialization is C++20 only I'm using the std::atomic_load/store directly, which only requires C++11.
| texture_registry = std::move(texture_registry_)]() { | ||
| if (image) { | ||
| if (texture_registry) { | ||
| texture_registry->UnregisterContextDestroyedListener( |
There was a problem hiding this comment.
The TextureRegistry needs to hold a reference to each DlDeferredImageGPU in order to ensure that the entries in images_ have not been deleted. But holding that reference will prevent the DlDeferredImageGPU from being destructed (at least until OnGrContextDestroyed is called and the TextureRegistry drops its reference).
The TextureRegistry may need to hold the images via some kind of weak pointer.
| fml::RefPtr<fml::TaskRunner> raster_task_runner_; | ||
| fml::RefPtr<SkiaUnrefQueue> unref_queue_; | ||
| sk_sp<SkImage> image_; | ||
| TextureRegistry* texture_registry_ = nullptr; |
There was a problem hiding this comment.
This raw pointer must be cleared before the TextureRegistry is destroyed during rasterizer teardown.
Is OnGrContextDestroyed guaranteed to run before that happens? Possibly that can clear this pointer.
There was a problem hiding this comment.
I changed this to a std::shared_ptr here and everywhere else.
|
I'm meeting with some Skia folks on Monday to discuss this further - @jason-simmons added you to that meeting as well. Need some more input from them about whether the linked Skia patch is safe enough or we need something else. |
|
I'm going to abandon this in favor of a new patch that doesn't require any new Skia API and also incorporates the feedback from Jason. |
2 participants
Relies on currently unimplemented Skia API.
See flutter/flutter#108389