From 2d30ce56feb7fe1c71be699024a56482dca61a7e Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Tue, 10 Jun 2025 09:25:12 -0700 Subject: [PATCH] [engine] dont double free surface texture interop objects. (#170284) Fixes https://github.com/flutter/flutter/issues/152459 I suspect the double free is related to https://github.com/flutter/flutter/issues/162147 . With the Impeller backend, the second free is performed by the reactor and comes only after the application has returned to the foreground. This increaases the likelyhood that we nuke a handle that is in active use by another part of the system. Since SurfaceTexture:: detachFromGLContext is documented as releasing the texture, we don't need to free it at all and can "leak" it on our side. --- .../renderer/backend/gles/reactor_gles.cc | 3 +++ .../gles/test/texture_gles_unittests.cc | 23 +++++++++++++++++++ .../renderer/backend/gles/texture_gles.cc | 5 ++++ .../renderer/backend/gles/texture_gles.h | 4 ++++ ...ce_texture_external_texture_gl_impeller.cc | 3 +++ ...urface_texture_external_texture_gl_skia.cc | 7 +++--- 6 files changed, 41 insertions(+), 4 deletions(-) diff --git a/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc b/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc index 2cb47560a74..c4a35799a7a 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/reactor_gles.cc @@ -233,6 +233,9 @@ HandleGLES ReactorGLES::CreateHandle(HandleType type, GLuint external_handle) { } void ReactorGLES::CollectHandle(HandleGLES handle) { + if (handle.IsDead()) { + return; + } if (handle.untracked_id_.has_value()) { LiveHandle live_handle(GLStorage{.integer = handle.untracked_id_.value()}); live_handle.pending_collection = true; diff --git a/engine/src/flutter/impeller/renderer/backend/gles/test/texture_gles_unittests.cc b/engine/src/flutter/impeller/renderer/backend/gles/test/texture_gles_unittests.cc index a0b52590117..8ed499fc373 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/test/texture_gles_unittests.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/test/texture_gles_unittests.cc @@ -7,6 +7,7 @@ #include "flutter/impeller/renderer/backend/gles/texture_gles.h" #include "flutter/testing/testing.h" #include "gtest/gtest.h" +#include "impeller/base/validation.h" #include "impeller/core/formats.h" #include "impeller/core/texture_descriptor.h" #include "impeller/renderer/backend/gles/handle_gles.h" @@ -93,4 +94,26 @@ TEST_P(TextureGLESTest, Binds2DTexture) { } } +TEST_P(TextureGLESTest, Leak) { + TextureDescriptor desc; + desc.storage_mode = StorageMode::kDevicePrivate; + desc.size = {100, 100}; + desc.format = PixelFormat::kR8G8B8A8UNormInt; + desc.type = TextureType::kTexture2D; + desc.sample_count = SampleCount::kCount1; + + auto texture = GetContext()->GetResourceAllocator()->CreateTexture(desc); + + ASSERT_TRUE(texture); + + std::optional handle = TextureGLES::Cast(*texture).GetGLHandle(); + EXPECT_TRUE(handle.has_value()); + + TextureGLES::Cast(*texture).Leak(); + + ScopedValidationDisable disable_validation; + handle = TextureGLES::Cast(*texture).GetGLHandle(); + EXPECT_FALSE(handle.has_value()); +} + } // namespace impeller::testing diff --git a/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.cc b/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.cc index 15292f3e7e7..402fb220daa 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.cc +++ b/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.cc @@ -16,6 +16,7 @@ #include "impeller/core/formats.h" #include "impeller/core/texture_descriptor.h" #include "impeller/renderer/backend/gles/formats_gles.h" +#include "impeller/renderer/backend/gles/handle_gles.h" namespace impeller { @@ -229,6 +230,10 @@ TextureGLES::~TextureGLES() { } } +void TextureGLES::Leak() { + handle_ = HandleGLES::DeadHandle(); +} + // |Texture| bool TextureGLES::IsValid() const { return is_valid_; diff --git a/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.h b/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.h index c1feb9030e9..84deb194403 100644 --- a/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.h +++ b/engine/src/flutter/impeller/renderer/backend/gles/texture_gles.h @@ -102,6 +102,10 @@ class TextureGLES final : public Texture, bool IsWrapped() const; + /// @brief Reset the internal texture state so that the reactor will not free + /// the associated handle. + void Leak(); + std::optional GetFBO() const; //---------------------------------------------------------------------------- diff --git a/engine/src/flutter/shell/platform/android/surface_texture_external_texture_gl_impeller.cc b/engine/src/flutter/shell/platform/android/surface_texture_external_texture_gl_impeller.cc index 94eda0f6a2c..5cdbf875d94 100644 --- a/engine/src/flutter/shell/platform/android/surface_texture_external_texture_gl_impeller.cc +++ b/engine/src/flutter/shell/platform/android/surface_texture_external_texture_gl_impeller.cc @@ -55,6 +55,9 @@ void SurfaceTextureExternalTextureGLImpeller::ProcessFrame( void SurfaceTextureExternalTextureGLImpeller::Detach() { SurfaceTextureExternalTexture::Detach(); + // Detach will collect the texture handle. + // See also: https://github.com/flutter/flutter/issues/152459 + texture_->Leak(); texture_.reset(); } diff --git a/engine/src/flutter/shell/platform/android/surface_texture_external_texture_gl_skia.cc b/engine/src/flutter/shell/platform/android/surface_texture_external_texture_gl_skia.cc index dfa5797e50c..829ffb9d515 100644 --- a/engine/src/flutter/shell/platform/android/surface_texture_external_texture_gl_skia.cc +++ b/engine/src/flutter/shell/platform/android/surface_texture_external_texture_gl_skia.cc @@ -52,10 +52,9 @@ void SurfaceTextureExternalTextureGLSkia::ProcessFrame(PaintContext& context, void SurfaceTextureExternalTextureGLSkia::Detach() { SurfaceTextureExternalTexture::Detach(); - if (texture_name_ != 0) { - glDeleteTextures(1, &texture_name_); - texture_name_ = 0; - } + // Detach will collect the texture handle. + // See also: https://github.com/flutter/flutter/issues/152459 + texture_name_ = 0; } } // namespace flutter