From 129355961e29c31ac1da215f33734c9fd232168a Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Thu, 8 Oct 2026 21:54:10 -0400 Subject: [PATCH] [Fix] (MG_IntegrationTest): each scenario starts with no program current and no GL error pending, so a case deleting its current program no longer breaks the next one in the same process. --- MobileGL/MG_IntegrationTest/CMakeLists.txt | 20 ++++ .../MG_IntegrationTest/Harness/HeadlessGL.cpp | 8 ++ .../MG_IntegrationTest/Harness/HeadlessGL.h | 4 + .../Harness/ScenarioFixture.h | 4 + .../Scenarios/ScenarioIsolationScenario.cpp | 108 ++++++++++++++++++ 5 files changed, 144 insertions(+) create mode 100644 MobileGL/MG_IntegrationTest/Scenarios/ScenarioIsolationScenario.cpp diff --git a/MobileGL/MG_IntegrationTest/CMakeLists.txt b/MobileGL/MG_IntegrationTest/CMakeLists.txt index 5c20995c..ac82595f 100644 --- a/MobileGL/MG_IntegrationTest/CMakeLists.txt +++ b/MobileGL/MG_IntegrationTest/CMakeLists.txt @@ -182,6 +182,7 @@ add_executable(MobileGLIntegrationTest Scenarios/TriangleScenario.cpp Scenarios/PersistentCoherentMapScenario.cpp Scenarios/DefaultFramebufferAcrossSwapScenario.cpp + Scenarios/ScenarioIsolationScenario.cpp ) target_include_directories(MobileGLIntegrationTest PRIVATE @@ -824,6 +825,25 @@ gtest_discover_tests(MobileGLIntegrationTest ENVIRONMENT "${MGL_ITEST_VULKAN_ENVIRONMENT}" ) +# ScenarioIsolationScenario's two cases in ONE process: the first leaves a deleted program current +# and an error unread, the second asserts ScenarioTest::SetUp cleared both. The entries above run +# one case per process, where both cases are green either way. add_test because +# gtest_discover_tests registers cases one at a time; a PLAIN list (see the arena pair below). +foreach(backend DirectGLES DirectVulkan) + if (backend STREQUAL "DirectGLES") + string(REPLACE "\\;" ";" mglItestIsolationEnv "${MGL_ITEST_GLES_ENVIRONMENT}") + else() + string(REPLACE "\\;" ";" mglItestIsolationEnv "${MGL_ITEST_VULKAN_ENVIRONMENT}") + endif() + add_test(NAME "${backend}.ScenarioIsolationScenario.CasesInOneProcess" + COMMAND MobileGLIntegrationTest "--gtest_filter=ScenarioIsolationScenario.*" + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}) + set_tests_properties("${backend}.ScenarioIsolationScenario.CasesInOneProcess" PROPERTIES + LABELS integration-gpu + TIMEOUT ${MGL_ITEST_TIMEOUT} + ENVIRONMENT "${mglItestIsolationEnv}") +endforeach() + foreach(backend DirectGLES DirectVulkan) if (backend STREQUAL "DirectGLES") set(mechanism_environment "${MGL_ITEST_GLES_ENVIRONMENT}") diff --git a/MobileGL/MG_IntegrationTest/Harness/HeadlessGL.cpp b/MobileGL/MG_IntegrationTest/Harness/HeadlessGL.cpp index 508f8990..6ca6343d 100644 --- a/MobileGL/MG_IntegrationTest/Harness/HeadlessGL.cpp +++ b/MobileGL/MG_IntegrationTest/Harness/HeadlessGL.cpp @@ -760,6 +760,14 @@ namespace MGITest { return first; } + void ResetStateBetweenScenarios() { + // A program deleted while current stays current, flagged (GL 4.6 core 7.3); the next + // case would read its name back from GL_CURRENT_PROGRAM and restore a dead one. + // Unbinding it here is what finally deletes it. + glUseProgram(0); + FirstGLError(); + } + const char* GLErrorName(unsigned int error) { switch (error) { case GL_NO_ERROR: diff --git a/MobileGL/MG_IntegrationTest/Harness/HeadlessGL.h b/MobileGL/MG_IntegrationTest/Harness/HeadlessGL.h index 46e5cf84..2dffce9c 100644 --- a/MobileGL/MG_IntegrationTest/Harness/HeadlessGL.h +++ b/MobileGL/MG_IntegrationTest/Harness/HeadlessGL.h @@ -200,6 +200,10 @@ namespace MGITest { unsigned int FirstGLError(); const char* GLErrorName(unsigned int error); + // What one case may leave behind in the shared context that the next case cannot be expected + // to clean up: no program current, no error pending. ScenarioTest::SetUp calls it. + void ResetStateBetweenScenarios(); + // ---- whole-region readback predicates ---------------------------------- // The scenarios used to assert on two or three individual pixels, which is // provably too weak: a draw in which 3 of a quad's 4 vertices carry stale diff --git a/MobileGL/MG_IntegrationTest/Harness/ScenarioFixture.h b/MobileGL/MG_IntegrationTest/Harness/ScenarioFixture.h index 460a392f..0176c5b2 100644 --- a/MobileGL/MG_IntegrationTest/Harness/ScenarioFixture.h +++ b/MobileGL/MG_IntegrationTest/Harness/ScenarioFixture.h @@ -127,6 +127,10 @@ namespace MGITest { FAIL() << "MOBILEGL_ITEST_REQUIRE_HARDWARE_GPU is set but the context landed on a software " << "rasterizer: " << gl.RendererString(); } + // One context per process: a case run after another in the same process inherits + // whatever that one left bound (ScenarioIsolationScenario). Before the split lane's + // baseline below, so these calls are not counted as the case's own records. + ResetStateBetweenScenarios(); // P5's DirectGLES.Split. lanes, in ONE place rather than in each scenario they point // at - the Split family also points at ClearThenReadPixelsScenario, which is target A // of the reduced path and predates P5, and any later Split lane gets the same diff --git a/MobileGL/MG_IntegrationTest/Scenarios/ScenarioIsolationScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/ScenarioIsolationScenario.cpp new file mode 100644 index 00000000..be53f796 --- /dev/null +++ b/MobileGL/MG_IntegrationTest/Scenarios/ScenarioIsolationScenario.cpp @@ -0,0 +1,108 @@ +// MobileGL - MobileGL/MG_IntegrationTest/Scenarios/ScenarioIsolationScenario.cpp +// Copyright (c) 2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header +// +// Scenario - A CASE STARTS FROM A CLEAN SLATE EVEN WHEN THE CASE BEFORE IT DID NOT CLEAN UP. +// +// The harness has ONE context per process, so whatever a case leaves bound is still bound when +// the next case starts. ctest runs one case per process, which hides that; a developer's +// `--gtest_filter=A:B` does not. +// +// The leak that bit: CrossFrameBufferScenario and StorageBufferRegrowScenario (among others) end +// with glDeleteProgram on the program that is still current. GL 4.6 core 7.3 defers that deletion +// until the program stops being current, so GL_CURRENT_PROGRAM still names it. GuiBatchScenario +// then replays AcceleratedRendering's save / switch / restore: it reads GL_CURRENT_PROGRAM, makes +// its compute program current - which is what finally deletes the old one - and restores the +// saved name, which no longer exists. GL_INVALID_VALUE, from MobileGL and from native Mesa alike: +// the implementation is right and the fixture's "clean slate" was not. +// +// The two cases below run in declaration order in ONE process (the +// ScenarioIsolationScenario.CasesInOneProcess registrations in CMakeLists.txt). The first one is +// the careless neighbour; the second asserts what ScenarioTest::SetUp owes every case. Each is +// green on its own, so the ordinary one-case-per-process entries cannot see the bug. + +#include + +#include "../Harness/HeadlessGL.h" +#include "../Harness/ScenarioFixture.h" + +#ifdef GLAPI +#undef GLAPI +#endif +#define GL_GLEXT_PROTOTYPES +#include +#include +#undef GL_GLEXT_PROTOTYPES + +namespace MGITest { + namespace { + + constexpr const char* kVertexSource = R"(#version 330 core +layout(location = 0) in vec2 aPos; +void main() { + gl_Position = vec4(aPos, 0.0, 1.0); +} +)"; + + constexpr const char* kFragmentSource = R"(#version 330 core +out vec4 fragColor; +void main() { + fragColor = vec4(1.0); +} +)"; + + class ScenarioIsolationScenario : public ScenarioTest {}; + + GLint CurrentProgram() { + GLint current = -1; + glGetIntegerv(GL_CURRENT_PROGRAM, ¤t); + return current; + } + + } // namespace + + // The careless neighbour: its program is deleted while current and never unbound, and it + // leaves an error in the queue that nobody reads. + TEST_F(ScenarioIsolationScenario, ACaseLeavesItsDeletedProgramCurrentAndAnErrorUnread) { + if (!Ready()) return; + std::string error; + const GLuint program = CompileProgram(kVertexSource, kFragmentSource, &error); + ASSERT_NE(program, 0u) << error; + glUseProgram(program); + glDeleteProgram(program); + // GL 4.6 core 7.3: still current, only flagged. + EXPECT_EQ(CurrentProgram(), static_cast(program)); + GLint deleteStatus = GL_FALSE; + glGetProgramiv(program, GL_DELETE_STATUS, &deleteStatus); + EXPECT_EQ(deleteStatus, GL_TRUE); + EXPECT_EQ(FirstGLError(), 0u); + + glBindBuffer(0x1234, 0); // GL_INVALID_ENUM, left in the queue + } + + TEST_F(ScenarioIsolationScenario, TheNextCaseStartsWithNoProgramCurrentAndNoErrorPending) { + if (!Ready()) return; + const unsigned int pending = FirstGLError(); + EXPECT_EQ(pending, 0u) << "an earlier case's " << GLErrorName(pending) << " reached this one"; + const GLint saved = CurrentProgram(); + EXPECT_EQ(saved, 0) << "an earlier case's program is still current"; + + // GuiBatchScenario's prepareBuffers(): save, switch, restore. Switching away is what + // deletes a flagged program, so restoring a leaked name is GL_INVALID_VALUE. + std::string error; + const GLuint program = CompileProgram(kVertexSource, kFragmentSource, &error); + ASSERT_NE(program, 0u) << error; + glUseProgram(program); + glUseProgram(0); + glUseProgram(static_cast(saved)); + const unsigned int restore = FirstGLError(); + EXPECT_EQ(restore, 0u) << "restoring GL_CURRENT_PROGRAM raised " << GLErrorName(restore); + glUseProgram(0); + glDeleteProgram(program); + } + +} // namespace MGITest