[Fix] (Pipe): stop a make-current disarming the composite resolver's release path - the memo's freshness and the slot's release obligation were one flag, so after the first Reset no signature move ever spoke a delete

MobileGL/MG_Impl/Pipe/CompositeResolver.h

@@ -102,6 +102,13 @@ namespace MobileGL::MG_Pipe {
             Entry* entry = Find(key);
             if (entry != nullptr) {
                 if (entry->Signature == signature && entry->Handle == handle) {
+                    // THE SAME COMPOSITE. Not merely "the same signature": the handle is minted
+                    // off the composite ProgramObject's own lifetime id, so an identical handle
+                    // IS an identical object, and there is nothing to release whatever the
+                    // memo's freshness says. Re-arming Fresh here is what a Reset() costs - one
+                    // re-observation - and Live is deliberately NOT touched, because it is the
+                    // release obligation and it is still owed for exactly this handle.
+                    entry->Fresh = true;
                     ++m_counters.Reuses;
                     return handle;
                 }
@@ -115,35 +122,33 @@ namespace MobileGL::MG_Pipe {
             entry->Handle = handle;
             entry->CompositeLifetimeId = composite.GetLifetimeId();
             entry->Live = true;
+            entry->Fresh = true;
             ++m_counters.Mints;
             return handle;
         }
 
-        // The pipeline object itself is going away, or a test is tearing down. Speaks the same
-        // release for whatever it still holds.
-        void Forget(Uint pipelineName) {
-            for (SizeT i = 0; i < m_entries.size(); ++i) {
-                if (m_entries[i].PipelineName != pipelineName) continue;
-                ReleaseEntry(m_entries[i]);
-                m_entries[i] = m_entries.back();
-                m_entries.pop_back();
-                return;
-            }
-        }
-
         // A make-current. The entries name composites that belong to the frontend objects of
         // the context being left, and those objects outlive the switch, so the RECORDS are not
-        // released here - releasing them would emit a delete for a live program. Only the
+        // released here - releasing them would emit a delete for a live program.
-        // memo's freshness is dropped, which costs at most one re-observation.
+        //
+        // ONLY THE MEMO's FRESHNESS IS DROPPED, AND `Fresh` IS A SEPARATE FLAG FROM `Live` FOR
+        // EXACTLY THAT REASON. The two were one flag and the conflation was a real defect: the
+        // reuse branch above returns before anything could restore it, so after the first
+        // make-current every entry stayed at false for the life of the process and ReleaseEntry
+        // early-returned for ever - no delete_shader_state, no Free, no counter, and the old
+        // composite's handle simply overwritten out of the resolver. `Live` is a RELEASE
+        // OBLIGATION and nothing but ReleaseEntry may clear it; `Fresh` is the memo's own "does
+        // this entry describe the current context's pipeline of this name", which costs at most
+        // one re-observation when it is wrong.
         void Reset() {
-            for (Entry& entry : m_entries) entry.Live = false;
+            for (Entry& entry : m_entries) entry.Fresh = false;
         }
 
         void ResetCounters() { m_counters = Counters{}; }
 
         MGPipeHandle HandleFor(Uint pipelineName) const {
             for (const Entry& entry : m_entries) {
-                if (entry.PipelineName == pipelineName && entry.Live) return entry.Handle;
+                if (entry.PipelineName == pipelineName && entry.Live && entry.Fresh) return entry.Handle;
             }
             return kMGPipeNullHandle;
         }
@@ -156,11 +161,26 @@ namespace MobileGL::MG_Pipe {
             // choice: a static that held one would put a frontend destructor on an exit
             // handler's path into a torn-down pipe. A GL name, a signature of plain integers,
             // a handle and a lifetime id are all this needs.
+            // KEYED ON THE GL NAME, because a ProgramPipelineObject has no lifetime id -
+            // ComputeDrawProgramSignature reads the STAGE programs' ids and the pipeline itself
+            // carries none. glGenProgramPipelines recycles names, so a deleted-and-recreated
+            // pipeline can inherit its predecessor's entry; that is bounded and self-correcting
+            // rather than a hazard. The first Observe on the new object finds a signature and a
+            // handle that do not match and releases the old entry, and that release resolves
+            // NOTHING - the allocator erases the lifetime-id mapping on Free, so a stale
+            // CompositeLifetimeId emits no delete and frees no slot; all it costs is one
+            // redundant, idempotent death notice, which is the same shape the composite's own
+            // second release path already has. The vector is keyed by name, so it cannot grow
+            // past the highest pipeline name the process ever used.
             Uint PipelineName = 0;
             DrawProgramSignature Signature{};
             MGPipeHandle Handle = kMGPipeNullHandle;
             Uint64 CompositeLifetimeId = 0;
+            // THE RELEASE OBLIGATION. Set when this entry takes responsibility for a composite's
+            // slot, cleared ONLY by ReleaseEntry when that responsibility is discharged.
             Bool Live = false;
+            // THE MEMO's FRESHNESS, and deliberately not the same flag as Live - see Reset().
+            Bool Fresh = false;
         };
 
         Entry* Find(Uint pipelineName) {

MobileGL/MG_Test/Pipe/CompositeResolverTest.cpp

@@ -360,7 +360,8 @@ TEST(CompositeResolver, ASlotAtTheShaderCsoLimitIsRefusedWhileTheLastBandSlotIsN
     X(CompositeResolver, ASignatureThatHasNotMovedReusesOneComposite)                               \
     X(CompositeResolver, TwoPipelinesWithTheSameSignatureKeepTheirOwnComposite)                     \
     X(CompositeResolver, EvictionThenDestructionFreesTheSlotExactlyOnce)                            \
-    X(CompositeResolver, DestructionThenEvictionFreesTheSlotExactlyOnce)
+    X(CompositeResolver, DestructionThenEvictionFreesTheSlotExactlyOnce)                            \
+    X(CompositeResolver, ASignatureMoveAfterAMakeCurrentStillReleasesThroughTheResolver)
 
 #define MGL_DECLARE_PULL_SKIP(Suite, Name)                                                         \
     TEST(Suite, Name) { GTEST_SKIP() << "compiled only under MOBILEGL_PIPE_PUSH"; }
@@ -399,6 +400,13 @@ void main() { gl_Position = vec4(0.0, 0.0, 0.0, 1.0); }
     const char* kFs = R"(#version 430 core
 out vec4 o_color;
 void main() { o_color = vec4(1.0); }
+)";
+    // A SECOND fragment stage, so a pipeline's draw-program signature can be made to move for
+    // real: ComputeDrawProgramSignature is the per-stage {lifetimeId, GetLinkVersion()} array,
+    // and a different ProgramObject is a different lifetime id.
+    const char* kFs2 = R"(#version 430 core
+out vec4 o_color;
+void main() { o_color = vec4(0.5); }
 )";
 
     // Built by hand rather than through glCreateShaderProgramv, for ProgramPipelineCompositeTest's
@@ -555,6 +563,63 @@ void main() { o_color = vec4(1.0); }
         EXPECT_NE(recycled.Gen, cso.Gen);
         MGPipeSlots().Free(MGPipeKind::ShaderCso, recycled);
     }
+
+    // THE RELEASE PATH THE TWO CASES ABOVE DO NOT TOUCH. Both of them call the death helper
+    // directly, so the resolver is not in the picture at all and its own release - the
+    // pipeline-cache path, the one the header says exists precisely because "usually" is not a
+    // contract - had no case of its own. This drives it, and it drives it AFTER A
+    // MAKE-CURRENT, which is where it used to be permanently disarmed:
+    //
+    //   Reset() cleared the entry's one flag; the next Observe took the reuse branch and
+    //   returned before anything could restore it; from then on ReleaseEntry saw !Live and
+    //   returned immediately - no delete_shader_state, no Free, no counter movement - and the
+    //   old composite's handle was simply overwritten out of the resolver. Nothing leaked
+    //   today only because the frontend's one-slot pipeline cache drops the last SharedPtr on
+    //   the overwrite, which is exactly the "a client that only reacted to destructors" case
+    //   the design refuses to rely on.
+    //
+    // The release obligation now lives in its own flag and a make-current does not touch it.
+    TEST(CompositeResolver, ASignatureMoveAfterAMakeCurrentStillReleasesThroughTheResolver) {
+        ResolverScope scope;
+        const GLuint vs = MakeSeparableProgram(GL_VERTEX_SHADER, kVs);
+        const GLuint fs = MakeSeparableProgram(GL_FRAGMENT_SHADER, kFs);
+        const GLuint pipeline = MakeBoundPipeline(vs, fs);
+
+        ASSERT_GT(MGPipeProgramEmitterInstance().EmitShaderState(Ctx()), 0u);
+        const MGPipeHandle first = MGPipeProgramEmitterInstance().DrawCso();
+        ASSERT_FALSE(MGPipeHandleIsNull(first));
+        ASSERT_TRUE(MGPipeIsCompositeShaderSlot(first.Slot));
+        ASSERT_TRUE(MGPipeSlots().IsLive(MGPipeKind::ShaderCso, first));
+        ASSERT_EQ(MGPipeCompositeResolverInstance().GetCounters().Releases, 0u);
+
+        // THE MAKE-CURRENT. This is exactly what PipeFill's FreshlyPrimed arm does, and it
+        // reaches MGPipeCompositeResolver::Reset() through the program emitter's own Reset().
+        MGPipeProgramEmitterInstance().Reset();
+        // ... followed by a draw whose stage set has NOT moved, which is the reuse branch.
+        MGPipeProgramEmitterInstance().EmitShaderState(Ctx());
+        EXPECT_EQ(MGPipeProgramEmitterInstance().DrawCso(), first)
+            << "the same stage set is the same composite and the same handle";
+        EXPECT_GE(MGPipeCompositeResolverInstance().GetCounters().Reuses, 1u);
+        EXPECT_EQ(MGPipeCompositeResolverInstance().GetCounters().Releases, 0u)
+            << "a reuse releases nothing";
+
+        // NOW THE STAGE SET REALLY MOVES: a different fragment stage program is a different
+        // lifetime id, so ComputeDrawProgramSignature moves and the frontend builds a second
+        // composite. The resolver has to speak the release for the first one.
+        const GLuint fs2 = MakeSeparableProgram(GL_FRAGMENT_SHADER, kFs2);
+        GL::UseProgramStages(pipeline, GL_FRAGMENT_SHADER_BIT, fs2);
+        GL::UseProgram(0);
+
+        MGPipeProgramEmitterInstance().EmitShaderState(Ctx());
+        const MGPipeHandle second = MGPipeProgramEmitterInstance().DrawCso();
+        ASSERT_FALSE(MGPipeHandleIsNull(second));
+        EXPECT_NE(second, first) << "a moved signature is a second composite with its own handle";
+        EXPECT_EQ(MGPipeCompositeResolverInstance().GetCounters().Releases, 1u)
+            << "the resolver's own release path, spoken after a make-current";
+        EXPECT_FALSE(MGPipeSlots().IsLive(MGPipeKind::ShaderCso, first))
+            << "and the first composite's slot really went back, exactly once";
+        EXPECT_TRUE(MGPipeSlots().IsLive(MGPipeKind::ShaderCso, second));
+    }
 } // namespace
 #endif // MOBILEGL_PIPE_PUSH