better handle invalid programs in release builds (#7389)

* better handle invalid programs in release builds

Until now invalid program would basically be undefined behavior,
which in practice was a crash via a null pointer dereference.
With this change, invalid programs cause drawing ops to become no-ops.

Additionally fixed an unsynchronized access of a the variable containing
the program id. I don't think it would have caused issues though.

FIXES=[311775564]

Co-authored-by: Powei Feng <[email protected]>

filament/backend/src/opengl/OpenGLDriver.cpp

@@ -262,7 +262,12 @@ void OpenGLDriver::bindTexture(GLuint unit, GLTexture const* t) noexcept {
     mContext.bindTexture(unit, t->gl.target, t->gl.id, t->gl.targetIndex);
 }
 
-void OpenGLDriver::useProgram(OpenGLProgram* p) noexcept {
+bool OpenGLDriver::useProgram(OpenGLProgram* p) noexcept {
+    if (UTILS_UNLIKELY(!p->isValid())) {
+        // If the program is not valid, we can't call use().
+        return false;
+    }
+
     // set-up textures and samplers in the proper TMUs (as specified in setSamplers)
     p->use(this, mContext);
 
@@ -277,6 +282,7 @@ void OpenGLDriver::useProgram(OpenGLProgram* p) noexcept {
         // when mPlatform.isSRGBSwapChainSupported() is false (no need to check though).
         p->setRec709ColorSpace(mRec709OutputColorspace);
     }
+    return true;
 }
 
 
@@ -3531,18 +3537,17 @@ void OpenGLDriver::draw(PipelineState state, Handle<HwRenderPrimitive> rph, uint
     DEBUG_MARKER()
     auto& gl = mContext;
 
-    OpenGLProgram* p = handle_cast<OpenGLProgram*>(state.program);
+    OpenGLProgram* const p = handle_cast<OpenGLProgram*>(state.program);
 
-    // If the material debugger is enabled, avoid fatal (or cascading) errors and that can occur
-    // during the draw call when the program is invalid. The shader compile error has already been
-    // dumped to the console at this point, so it's fine to simply return early.
-    if (FILAMENT_ENABLE_MATDBG && UTILS_UNLIKELY(!p->isValid())) {
+    bool const success = useProgram(p);
+    if (UTILS_UNLIKELY(!success)) {
+        // Avoid fatal (or cascading) errors that can occur during the draw call when the program
+        // is invalid. The shader compile error has already been dumped to the console at this
+        // point, so it's fine to simply return early.
         return;
     }
 
-    useProgram(p);
-
-    GLRenderPrimitive* rp = handle_cast<GLRenderPrimitive *>(rph);
+    GLRenderPrimitive* const rp = handle_cast<GLRenderPrimitive*>(rph);
 
     // Gracefully do nothing if the render primitive has not been set up.
     VertexBufferHandle vb = rp->gl.vertexBufferWithObjects;
@@ -3553,7 +3558,7 @@ void OpenGLDriver::draw(PipelineState state, Handle<HwRenderPrimitive> rph, uint
     gl.bindVertexArray(&rp->gl);
 
     // If necessary, mutate the bindings in the VAO.
-    const GLVertexBuffer* glvb = handle_cast<GLVertexBuffer*>(vb);
+    GLVertexBuffer const* const glvb = handle_cast<GLVertexBuffer*>(vb);
     if (UTILS_UNLIKELY(rp->gl.vertexBufferVersion != glvb->bufferObjectsVersion)) {
         updateVertexArrayObject(rp, glvb);
     }
@@ -3587,17 +3592,16 @@ void OpenGLDriver::draw(PipelineState state, Handle<HwRenderPrimitive> rph, uint
 void OpenGLDriver::dispatchCompute(Handle<HwProgram> program, math::uint3 workGroupCount) {
     getShaderCompilerService().tick();
 
-    OpenGLProgram* p = handle_cast<OpenGLProgram*>(program);
+    OpenGLProgram* const p = handle_cast<OpenGLProgram*>(program);
 
-    // If the material debugger is enabled, avoid fatal (or cascading) errors and that can occur
-    // during the draw call when the program is invalid. The shader compile error has already been
-    // dumped to the console at this point, so it's fine to simply return early.
-    if (FILAMENT_ENABLE_MATDBG && UTILS_UNLIKELY(!p->isValid())) {
+    bool const success = useProgram(p);
+    if (UTILS_UNLIKELY(!success)) {
+        // Avoid fatal (or cascading) errors that can occur during the draw call when the program
+        // is invalid. The shader compile error has already been dumped to the console at this
+        // point, so it's fine to simply return early.
         return;
     }
 
-    useProgram(p);
-
 #if defined(BACKEND_OPENGL_LEVEL_GLES31)
 
 #if defined(__ANDROID__)

filament/backend/src/opengl/OpenGLDriver.h

@@ -318,7 +318,7 @@ class OpenGLDriver final : public DriverBase {
 
            void bindTexture(GLuint unit, GLTexture const* t) noexcept;
            void bindSampler(GLuint unit, GLuint sampler) noexcept;
-    inline void useProgram(OpenGLProgram* p) noexcept;
+    inline bool useProgram(OpenGLProgram* p) noexcept;
 
     enum class ResolveAction { LOAD, STORE };
     void resolvePass(ResolveAction action, GLRenderTarget const* rt,

filament/backend/src/opengl/OpenGLProgram.cpp

@@ -129,7 +129,7 @@ void OpenGLProgram::initializeProgramState(OpenGLContext& context, GLuint progra
 #endif
     {
         // ES2 initialization of (fake) UBOs
-        UniformsRecord* const uniformsRecords = new UniformsRecord[Program::UNIFORM_BINDING_COUNT];
+        UniformsRecord* const uniformsRecords = new(std::nothrow) UniformsRecord[Program::UNIFORM_BINDING_COUNT];
         UTILS_NOUNROLL
         for (GLuint binding = 0, n = Program::UNIFORM_BINDING_COUNT; binding < n; binding++) {
             Program::UniformInfo& uniforms = lazyInitializationData.bindingUniformInfo[binding];

filament/backend/src/opengl/OpenGLProgram.h

@@ -59,7 +59,7 @@ class OpenGLProgram : public HwProgram {
             // - the content of any bound sampler buffer has changed
             // ... since last time we used this program
 
-            // turns out the former might be relatively cheap to check, the later requires
+            // Turns out the former might be relatively cheap to check, the latter requires
             // a bit less. Compared to what updateSamplers() actually does, which is
             // pretty little, I'm not sure if we'll get ahead.
 

filament/backend/src/opengl/ShaderCompilerService.cpp

@@ -22,7 +22,6 @@
 
 #include <private/backend/BackendUtils.h>
 
-#include <backend/platforms/OpenGLPlatform.h>
 #include <backend/Program.h>
 
 #include <utils/compiler.h>
@@ -254,8 +253,6 @@ ShaderCompilerService::program_token_t ShaderCompilerService::createProgram(
                     glGetProgramiv(glProgram, GL_LINK_STATUS, &status);
                     programData.program = glProgram;
 
-                    token->gl.program = programData.program;
-
                     // we don't need to check for success here, it'll be done on the
                     // main thread side.
                     token->set(programData);
@@ -337,12 +334,12 @@ GLuint ShaderCompilerService::getProgram(ShaderCompilerService::program_token_t&
     return program;
 }
 
-/* static*/ void ShaderCompilerService::terminate(program_token_t& token) {
+void ShaderCompilerService::terminate(program_token_t& token) {
     assert_invariant(token);
 
     token->canceled = true;
 
-    bool canceled = token->compiler.cancelTickOp(token);
+    bool const canceled = token->compiler.cancelTickOp(token);
 
     if (token->compiler.mShaderCompilerThreadCount) {
         auto job = token->compiler.mCompilerThreadPool.dequeue(token);