[Fix] (Espryt): close four review minors on the twin table - a walk that can outlive its vector, a null call that is not arm-equivalent, and two comments that claimed more than the code does

- ForEachLive walked with a range-for and handed fn a reference INTO m_slots, so a callee
  that reached GetOrCreate on the same table would resize the vector under both. Index loop
  and a copied twin, the shape ReclaimDeadSlots already uses. The one caller today happens
  not to insert; that is not a property the walk should depend on.
- GetOrCreate(nullptr) reset the parking twin on EVERY call, so a second null call destroyed
  what the first was handed. The map arm kept its null-keyed entry until a sweep, so this
  was an arm difference in the one path (SyncTextureObjectToBackend) that documents relying
  on the tolerance. It now keeps the parked twin, and the case makes a second call.
- The one-entry memo's comment claimed the three per-draw resolution paths ask for the same
  object every draw. Two of them do not: BindCurrentFBO resolves both targets in a frame and
  ResolveUnitSamplerBackend asks per texture unit, so both thrash a single-entry memo and
  pay a probe P1 did not. The comment now says so and names the fix (per-unit / per-target)
  and the gate that would price it (G11, device-side, owed).
- HandleOf caches a NULL answer too - deliberate, because a bound-but-never-synced object
  would otherwise re-probe every draw - and what makes it safe is that GetOrCreate refreshes
  the memo. Nothing pinned that; RepeatedLookupsOfALiveObjectKeepOneHandle now does.
- Removed the dead #if MOBILEGL_PIPE_PUSH nested inside #if MOBILEGL_PIPE_PUSH in
  ScopedDetachedTextureFramebufferAttachments.

MobileGL/MG_Backend/DirectGLES/DirectGLES.cpp

@@ -6706,7 +6706,8 @@ namespace MobileGL::MG_Backend::DirectGLES {
                 }
             };
 
-#if MOBILEGL_PIPE_PUSH
+            // Already inside #if MOBILEGL_PIPE_PUSH, so no second guard here: the arm choice
+            // below is the RUNTIME one.
             if (EsprytSlotTablesEnabled()) {
                 FramebufferImpl::g_backendFramebufferObjects.ForEachLive(
                     [&](const SharedPtr<MG_State::GLState::FramebufferObject>& stateFBO,
@@ -6715,7 +6716,6 @@ namespace MobileGL::MG_Backend::DirectGLES {
                     });
                 return;
             }
-#endif
 #if MOBILEGL_PIPE_LEGACY_MEMOS
             for (auto it = FramebufferImpl::g_backendFramebufferObjects.begin();
                  it != FramebufferImpl::g_backendFramebufferObjects.end(); ++it) {

MobileGL/MG_Backend/DirectGLES/SlotTables.h

@@ -137,12 +137,15 @@ namespace MobileGL::MG_Backend::DirectGLES {
             // No assert on null here, unlike the map arm: null is TOLERATED, so a DEBUG build
             // must not trap where the release build quietly does the documented thing.
             if (stateObj == nullptr) {
-                // The registry this replaces inserted a null key and handed back ITS twin slot
-                // (DirectGLES.cpp's SyncTextureObjectToBackend documents relying on exactly
-                // that tolerance), so a release build never dereferenced null here. Keep the
-                // shape: one per-table parking slot, never live, never swept, never handed a
-                // handle. A null object has no identity and therefore cannot have a twin.
-                m_nullTwin.reset();
+                // The registry this replaces inserted a null KEY and handed back that entry's
+                // twin (DirectGLES.cpp's SyncTextureObjectToBackend documents relying on
+                // exactly that tolerance), so a release build never dereferenced null here.
+                // Keep the shape exactly, INCLUDING across calls: the map kept its null-keyed
+                // entry, so a second null call was handed the same twin the first one got.
+                // Resetting here instead would have destroyed it - an arm difference in the one
+                // path that documents relying on this. One per-table parking slot, never live,
+                // never swept, never handed a handle, because a null object has no identity and
+                // therefore cannot have a {slot, gen}.
                 return m_nullTwin;
             }
 
@@ -285,11 +288,18 @@ namespace MobileGL::MG_Backend::DirectGLES {
         // a dangling key the way the old iteration could.
         template <typename Fn>
         void ForEachLive(Fn&& fn) const {
-            for (const Entry& entry : m_slots) {
+            // Index loop and a COPIED twin, not a range-for over references: fn is arbitrary
+            // backend code, and a nested GetOrCreate on this table would resize m_slots and
+            // invalidate both the iterator and any reference into the vector that outlives the
+            // call. ReclaimDeadSlots walks by index for the same reason. The one caller today
+            // happens not to insert; that is not a property the walk should depend on.
+            for (SizeT slot = 0; slot < m_slots.size(); ++slot) {
+                const Entry& entry = m_slots[slot];
                 if (!entry.Live || !entry.backend) continue;
                 const StatePtr state = entry.stateRef.lock();
                 if (!state) continue;
-                fn(state, entry.backend);
+                const BackendPtr twin = entry.backend;
+                fn(state, twin);
             }
         }
 
@@ -322,12 +332,18 @@ namespace MobileGL::MG_Backend::DirectGLES {
         // Handed back by GetOrCreate for a null state object. Never live, never swept.
         BackendPtr m_nullTwin;
 
-        // ONE-entry resolution memo, lifetimeId -> handle. The three per-draw resolution paths
-        // (ResolveVaoTwin, SyncCurrentProgram, BindCurrentFBO) ask the SAME table for the SAME
-        // object every draw, so this turns the steady state back into an integer compare plus
-        // one array index - which is what the deleted TwinLookupMemos bought and what D13
-        // promises ("direct slot indexing - the memo existed only to avoid the hash probe").
-        // Without it every resolution went through the allocator's ByLifetimeId hash.
+        // ONE-entry resolution memo, lifetimeId -> handle. It exists because without it every
+        // resolution goes through the allocator's ByLifetimeId hash, which the deleted
+        // TwinLookupMemos existed to avoid and which D13 promises to replace with "direct slot
+        // indexing".
+        //
+        // It is one entry and therefore only helps a caller that asks for the SAME object twice
+        // running - ResolveVaoTwin and SyncCurrentProgram do, once per draw each. Two callers
+        // it does NOT help, recorded rather than claimed away: BindCurrentFBO resolves BOTH
+        // targets in a frame, and ResolveUnitSamplerBackend asks for a different sampler per
+        // texture unit, so both thrash a single-entry memo and pay the probe P1 did not (P1 had
+        // a per-unit memo and a direct-mapped 6-slot array there). Making the memo per-unit /
+        // per-target is the fix, and G11 - the device-side gate that would price it - is owed.
         //
         // It cannot serve a stale answer, by two independent arguments:
         //   * the key is a lifetime id, which MG_State never hands out twice, so a recycled

MobileGL/MG_Test/SanityTest.cpp

@@ -3263,8 +3263,16 @@ TEST(DirectGLESSlotTable, RepeatedLookupsOfALiveObjectKeepOneHandle) {
 
     FakeSlotTable table;
     auto object = MakeShared<FakeStateObject>(0xB1u);
+    // HandleOf caches whatever the allocator answered, INCLUDING the null handle - an object
+    // that is bound but never synced has no twin, and re-probing the hash for it every draw is
+    // exactly what the memo exists to avoid. What makes that safe is that GetOrCreate refreshes
+    // the memo, so a cached "no handle" can never outlive the twin's creation. Nothing pinned
+    // that; this does.
+    EXPECT_TRUE(MG_Pipe::MGPipeHandleIsNull(table.HandleOf(object.get())));
     table.GetOrCreate(object) = MakeShared<FakeBackendObject>();
     const MG_Pipe::MGPipeHandle handle = table.HandleOf(object.get());
+    ASSERT_FALSE(MG_Pipe::MGPipeHandleIsNull(handle))
+        << "the memo went on answering the null handle it cached before the twin existed";
 
     for (int i = 0; i < 8; ++i) {
         auto* slot = table.GetOrCreate(object) ? table.Find(object.get()) : nullptr;
@@ -3413,7 +3421,11 @@ TEST(DirectGLESSlotTable, AnnouncedDeathKeepsObjectChurnFromAccumulatingWithoutA
     EXPECT_EQ(table.LiveCount(), 0u);
     EXPECT_EQ(slots.LiveCount(MG_Pipe::MGPipeKind::Query), liveBefore)
         << "the churn leaked slots the announced deaths should have returned";
-    EXPECT_LE(slots.HighWater(MG_Pipe::MGPipeKind::Query) - highWaterBefore, 1u)
+    // 2 and not 1: on a cold allocator the high-water mark counts the RESERVED slot 0
+    // (kMGPipeFirstAllocatableSlot is 1) as well as the one slot this loop recycles, and ctest
+    // runs every case in its own process, so this case sees a cold allocator. What the bound
+    // rules out is the thing that matters - 256 churned objects growing the space by 256.
+    EXPECT_LE(slots.HighWater(MG_Pipe::MGPipeKind::Query) - highWaterBefore, 2u)
         << "the slot space grew with the churn instead of being recycled";
 }
 
@@ -3430,6 +3442,18 @@ TEST(DirectGLESSlotTable, GetOrCreateToleratesANullStateObject) {
     EXPECT_EQ(table.LiveCount(), 0u) << "a null object took a slot";
     EXPECT_EQ(table.Find(nullptr), nullptr);
     EXPECT_TRUE(MG_Pipe::MGPipeHandleIsNull(table.HandleOf(nullptr)));
+
+    // ...and a SECOND null call is handed the same parking slot rather than destroying what the
+    // first one was given. The map arm kept its null-keyed entry until a sweep, so a table that
+    // reset here would answer differently on the two arms in the one path that documents
+    // relying on this tolerance.
+    twin = MakeShared<FakeBackendObject>();
+    twin->marker = 5;
+    auto& again = table.GetOrCreate(none);
+    ASSERT_NE(again, nullptr) << "the second null call destroyed the first one's parked twin";
+    EXPECT_EQ(again->marker, 5);
+    EXPECT_EQ(&again, &twin);
+    EXPECT_EQ(table.LiveCount(), 0u);
 }
 // P2 step e2, the backend half. A sweep is a stand-in for a death notice; this is the notice.
 // Nothing below calls CollectGarbage*: the slot comes back, and the twin goes, at the moment