From 6b5adcce6f6241dcd610a980aa27cb2228d7fbca Mon Sep 17 00:00:00 2001
From: Tzarial <[email protected]>
Date: Wed, 08 Jul 2026 18:18:31 -0700
Subject: [PATCH] [agy][gpu] Fix active transform feedback buffer tracking

An active (even if paused/unbound) transform feedback object continues
to reference its attached buffers in the driver until
glEndTransformFeedback. If a different transform feedback object is
bound, the previous one becomes unbound but remains active, and its
attached buffers should still be considered busy and locked.

This CL refactors IndexedBufferBindingHost and TransformFeedback to use
AreBuffersBound() instead of is_bound_ directly. For
TransformFeedback, AreBuffersBound() is defined as (is_bound_ ||
active_). This preserves symmetric SetIsBound(true/false) calls on
TransformFeedback while correctly tracking buffer bindings on active
(but paused/unbound) transform feedback objects.

Additionally, this CL addresses destruction edge cases by:
1. Adding ForceUnbindBuffers() to prevent a buffer binding state leak
   on destruction of an active but unbound transform feedback object.
2. Restricting glEndTransformFeedback() inside the destructor to only
   run when the object is currently bound, matching GL requirements.

Fixed: 523750584
Test: gpu_unittests --gtest_filter=TransformFeedbackManagerTest.*
Change-Id: I4d9f0f133e18e54f57a14e15b9c5d70a36a099a7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8015317
Reviewed-by: Kai Ninomiya <[email protected]>
Commit-Queue: Kai Ninomiya <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1659202}
---

diff --git a/gpu/command_buffer/service/indexed_buffer_binding_host.cc b/gpu/command_buffer/service/indexed_buffer_binding_host.cc
index f605cb28..73c9fcc6 100644
--- a/gpu/command_buffer/service/indexed_buffer_binding_host.cc
+++ b/gpu/command_buffer/service/indexed_buffer_binding_host.cc
@@ -99,11 +99,14 @@
   GLuint service_id = buffer ? buffer->service_id() : 0;
   glBindBufferBase(target_, index, service_id);
 
-  if (buffer_bindings_[index].buffer && is_bound_) {
+  // AttachedBuffersAreLocked() is checked here (and in DoBindBufferRange)
+  // rather than DCHECK-ing because these methods are also called during
+  // RestoreBindings() when the host is not yet marked as bound.
+  if (buffer_bindings_[index].buffer && AttachedBuffersAreLocked()) {
     buffer_bindings_[index].buffer->OnUnbind(target_, true);
   }
   buffer_bindings_[index].SetBindBufferBase(buffer);
-  if (buffer && is_bound_) {
+  if (buffer && AttachedBuffersAreLocked()) {
     buffer->OnBind(target_, true);
   }
   UpdateMaxNonNullBindingIndex(index);
@@ -123,11 +126,14 @@
     glBindBufferRange(target_, index, service_id, offset, size);
   }
 
-  if (buffer_bindings_[index].buffer && is_bound_) {
+  // AttachedBuffersAreLocked() is checked here rather than DCHECK-ing because
+  // these methods are also called during RestoreBindings() when the host is not
+  // yet marked as bound.
+  if (buffer_bindings_[index].buffer && AttachedBuffersAreLocked()) {
     buffer_bindings_[index].buffer->OnUnbind(target_, true);
   }
   buffer_bindings_[index].SetBindBufferRange(buffer, offset, size);
-  if (buffer && is_bound_) {
+  if (buffer && AttachedBuffersAreLocked()) {
     buffer->OnBind(target_, true);
   }
   UpdateMaxNonNullBindingIndex(index);
@@ -244,11 +250,14 @@
     }
   }
 
-  if (is_bound != is_bound_) {
-    is_bound_ = is_bound;
+  bool was_bound = AttachedBuffersAreLocked();
+  is_bound_ = is_bound;
+  bool is_now_bound = AttachedBuffersAreLocked();
+
+  if (was_bound != is_now_bound) {
     for (auto& bb : buffer_bindings_) {
       if (bb.buffer) {
-        if (is_bound_) {
+        if (is_now_bound) {
           bb.buffer->OnBind(target_, true);
         } else {
           bb.buffer->OnUnbind(target_, true);
@@ -258,6 +267,18 @@
   }
 }
 
+bool IndexedBufferBindingHost::AttachedBuffersAreLocked() const {
+  return is_bound_;
+}
+
+void IndexedBufferBindingHost::ForceUnbindBuffers() {
+  for (auto& bb : buffer_bindings_) {
+    if (bb.buffer) {
+      bb.buffer->OnUnbind(target_, true);
+    }
+  }
+}
+
 Buffer* IndexedBufferBindingHost::GetBufferBinding(GLuint index) const {
   DCHECK_LT(index, buffer_bindings_.size());
   return buffer_bindings_[index].buffer.get();
diff --git a/gpu/command_buffer/service/indexed_buffer_binding_host.h b/gpu/command_buffer/service/indexed_buffer_binding_host.h
index fc89215..57e0356 100644
--- a/gpu/command_buffer/service/indexed_buffer_binding_host.h
+++ b/gpu/command_buffer/service/indexed_buffer_binding_host.h
@@ -50,6 +50,8 @@
 
   void SetIsBound(bool bound);
 
+  virtual bool AttachedBuffersAreLocked() const;
+
   Buffer* GetBufferBinding(GLuint index) const;
   // Returns |size| set by glBindBufferRange; 0 if set by glBindBufferBase.
   GLsizeiptr GetBufferSize(GLuint index) const;
@@ -72,6 +74,8 @@
 
   virtual ~IndexedBufferBindingHost();
 
+  void ForceUnbindBuffers();
+
   // Whether this object is currently bound into the context.
   bool is_bound_;
 
diff --git a/gpu/command_buffer/service/transform_feedback_manager.cc b/gpu/command_buffer/service/transform_feedback_manager.cc
index 4dc64183..df67034c 100644
--- a/gpu/command_buffer/service/transform_feedback_manager.cc
+++ b/gpu/command_buffer/service/transform_feedback_manager.cc
@@ -36,10 +36,20 @@
 
 TransformFeedback::~TransformFeedback() {
   if (!manager_->lost_context()) {
-    if (active_)
+    if (active_ && is_bound_) {
       glEndTransformFeedback();
+    }
     glDeleteTransformFeedbacks(1, &service_id_);
   }
+  // ForceUnbindBuffers() manages purely CPU-side state tracking/refcounting of
+  // the attached buffers. We must run it even if the context is lost (unlike
+  // the GL driver calls above) to ensure that the buffer reference counts are
+  // correctly decremented, preventing state leaks or triggering BufferManager
+  // DCHECKs upon destruction of an active but unbound transform feedback
+  // object.
+  if (active_ && !is_bound_) {
+    ForceUnbindBuffers();
+  }
 }
 
 void TransformFeedback::DoBindTransformFeedback(
@@ -73,6 +83,10 @@
   }
 }
 
+bool TransformFeedback::AttachedBuffersAreLocked() const {
+  return is_bound_ || active_;
+}
+
 void TransformFeedback::SetActiveProgram(Program* program) {
   CHECK(!active_program_);
   CHECK(program);
diff --git a/gpu/command_buffer/service/transform_feedback_manager.h b/gpu/command_buffer/service/transform_feedback_manager.h
index d779049..f36bbe4 100644
--- a/gpu/command_buffer/service/transform_feedback_manager.h
+++ b/gpu/command_buffer/service/transform_feedback_manager.h
@@ -59,6 +59,8 @@
     return paused_;
   }
 
+  bool AttachedBuffersAreLocked() const override;
+
   void SetActiveProgram(Program* program);
   void ClearActiveProgram();
 
diff --git a/gpu/command_buffer/service/transform_feedback_manager_unittest.cc b/gpu/command_buffer/service/transform_feedback_manager_unittest.cc
index 406407da..440c53c 100644
--- a/gpu/command_buffer/service/transform_feedback_manager_unittest.cc
+++ b/gpu/command_buffer/service/transform_feedback_manager_unittest.cc
@@ -2,15 +2,18 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "gpu/command_buffer/service/transform_feedback_manager.h"
+
 #include <memory>
 
+#include "gpu/command_buffer/service/buffer_manager.h"
 #include "gpu/command_buffer/service/gpu_service_test.h"
 #include "gpu/command_buffer/service/test_helper.h"
-#include "gpu/command_buffer/service/transform_feedback_manager.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/gl/gl_mock.h"
 
 using ::testing::_;

diff --git a/gpu/command_buffer/service/transform_feedback_manager_unittest.cc b/gpu/command_buffer/service/transform_feedback_manager_unittest.cc
index 406407da..440c53c 100644
--- a/gpu/command_buffer/service/transform_feedback_manager_unittest.cc
+++ b/gpu/command_buffer/service/transform_feedback_manager_unittest.cc
@@ -2,15 +2,18 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "gpu/command_buffer/service/transform_feedback_manager.h"
+
 #include <memory>
 
+#include "gpu/command_buffer/service/buffer_manager.h"
 #include "gpu/command_buffer/service/gpu_service_test.h"
 #include "gpu/command_buffer/service/test_helper.h"
-#include "gpu/command_buffer/service/transform_feedback_manager.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/gl/gl_mock.h"
 
 using ::testing::_;
+using ::testing::AnyNumber;
 
 namespace gpu {
 namespace gles2 {
@@ -58,5 +61,117 @@
   transform_feedback = nullptr;
 }
 
+TEST_F(TransformFeedbackManagerTest, BufferBindingTrackedWhilePaused) {
+  const GLuint kBufferClientId = 11;
+  const GLuint kBufferServiceId = 1011;
+  const GLuint kClientId2 = 77;
+  const GLuint kServiceId2 = 1077;
+
+  EXPECT_CALL(*gl_, BindTransformFeedback(_, _)).Times(AnyNumber());
+  EXPECT_CALL(*gl_, BindBuffer(_, _)).Times(AnyNumber());
+  EXPECT_CALL(*gl_, BindBufferBase(_, _, _)).Times(AnyNumber());
+  EXPECT_CALL(*gl_, BeginTransformFeedback(_)).Times(AnyNumber());
+  EXPECT_CALL(*gl_, PauseTransformFeedback()).Times(AnyNumber());
+  EXPECT_CALL(*gl_, EndTransformFeedback()).Times(AnyNumber());
+  EXPECT_CALL(*gl_, DeleteTransformFeedbacks(1, _)).Times(AnyNumber());
+
+  BufferManager buffer_manager(nullptr, nullptr);
+  buffer_manager.CreateBuffer(kBufferClientId, kBufferServiceId);
+  scoped_refptr<Buffer> buffer = buffer_manager.GetBuffer(kBufferClientId);
+  ASSERT_TRUE(buffer.get());
+
+  scoped_refptr<TransformFeedback> tf1 = manager_->CreateTransformFeedback(
+      kTransformFeedbackClientId, kTransformFeedbackServiceId);
+  scoped_refptr<TransformFeedback> tf2 =
+      manager_->CreateTransformFeedback(kClientId2, kServiceId2);
+
+  tf1->DoBindTransformFeedback(GL_TRANSFORM_FEEDBACK, nullptr, nullptr);
+  tf1->DoBindBufferBase(0, buffer.get());
+  EXPECT_TRUE(buffer->IsBoundForTransformFeedback());
+
+  tf1->DoBeginTransformFeedback(GL_POINTS);
+  tf1->DoPauseTransformFeedback();
+  EXPECT_TRUE(buffer->IsBoundForTransformFeedback());
+
+  // Switching to another transform feedback object while the previous one is
+  // still active (paused) must not drop the buffer's transform feedback
+  // binding state, otherwise it could be re-specified before it is resumed.
+  tf2->DoBindTransformFeedback(GL_TRANSFORM_FEEDBACK, tf1.get(), nullptr);
+  EXPECT_TRUE(buffer->IsBoundForTransformFeedback());
+
+  tf1->DoBindTransformFeedback(GL_TRANSFORM_FEEDBACK, tf2.get(), nullptr);
+  EXPECT_TRUE(buffer->IsBoundForTransformFeedback());
+  EXPECT_FALSE(buffer->IsDoubleBoundForTransformFeedback());
+
+  tf1->DoEndTransformFeedback();
+  EXPECT_TRUE(buffer->IsBoundForTransformFeedback());
+
+  tf2->DoBindTransformFeedback(GL_TRANSFORM_FEEDBACK, tf1.get(), nullptr);
+  EXPECT_FALSE(buffer->IsBoundForTransformFeedback());
+
+  manager_->Destroy();
+  tf1 = nullptr;
+  tf2 = nullptr;
+  buffer = nullptr;
+  buffer_manager.MarkContextLost();
+  buffer_manager.Destroy();
+}
+
+TEST_F(TransformFeedbackManagerTest,
+       BufferBindingTrackedWhenActiveTfDestroyedUnbound) {
+  const GLuint kBufferClientId = 11;
+  const GLuint kBufferServiceId = 1011;
+  const GLuint kClientId2 = 77;
+  const GLuint kServiceId2 = 1077;
+
+  // Expect glDeleteTransformFeedbacks when destroying the unbound active TF
+  // tf1. Importantly, we do NOT expect glEndTransformFeedback to be called on
+  // tf1 since it was unbound when destroyed.
+  EXPECT_CALL(*gl_, BindTransformFeedback(_, _)).Times(AnyNumber());
+  EXPECT_CALL(*gl_, BindBuffer(_, _)).Times(AnyNumber());
+  EXPECT_CALL(*gl_, BindBufferBase(_, _, _)).Times(AnyNumber());
+  EXPECT_CALL(*gl_, BeginTransformFeedback(_)).Times(AnyNumber());
+  EXPECT_CALL(*gl_, PauseTransformFeedback()).Times(AnyNumber());
+  EXPECT_CALL(*gl_, DeleteTransformFeedbacks(1, _)).Times(AnyNumber());
+  // glEndTransformFeedback must not be called!
+  EXPECT_CALL(*gl_, EndTransformFeedback()).Times(0);
+
+  BufferManager buffer_manager(nullptr, nullptr);
+  buffer_manager.CreateBuffer(kBufferClientId, kBufferServiceId);
+  scoped_refptr<Buffer> buffer = buffer_manager.GetBuffer(kBufferClientId);
+  ASSERT_TRUE(buffer.get());
+
+  scoped_refptr<TransformFeedback> tf1 = manager_->CreateTransformFeedback(
+      kTransformFeedbackClientId, kTransformFeedbackServiceId);
+  scoped_refptr<TransformFeedback> tf2 =
+      manager_->CreateTransformFeedback(kClientId2, kServiceId2);
+
+  tf1->DoBindTransformFeedback(GL_TRANSFORM_FEEDBACK, nullptr, nullptr);
+  tf1->DoBindBufferBase(0, buffer.get());
+  EXPECT_TRUE(buffer->IsBoundForTransformFeedback());
+
+  tf1->DoBeginTransformFeedback(GL_POINTS);
+  tf1->DoPauseTransformFeedback();
+  EXPECT_TRUE(buffer->IsBoundForTransformFeedback());
+
+  // Bind to tf2. Now tf1 is unbound but remains active (paused).
+  tf2->DoBindTransformFeedback(GL_TRANSFORM_FEEDBACK, tf1.get(), nullptr);
+  EXPECT_TRUE(buffer->IsBoundForTransformFeedback());
+
+  // Destroy tf1 while active and unbound.
+  // This must unbind the buffers attached to it (OnUnbind should be called),
+  // reducing transform_feedback_indexed_binding_count_ to 0.
+  // In addition, glEndTransformFeedback must not be called in the driver.
+  manager_->RemoveTransformFeedback(kTransformFeedbackClientId);
+  tf1 = nullptr;
+  EXPECT_FALSE(buffer->IsBoundForTransformFeedback());
+
+  manager_->Destroy();
+  tf2 = nullptr;
+  buffer = nullptr;
+  buffer_manager.MarkContextLost();
+  buffer_manager.Destroy();
+}
+
 }  // namespace gles2
 }  // namespace gpu