Reference count context to fix ASAN issues

src/libANGLE/Context.cpp

@@ -326,6 +326,7 @@ Context::Context(egl::Display *display,
       mProgramPipelineObserverBinding(this, kProgramPipelineSubjectIndex),
       mThreadPool(nullptr),
       mFrameCapture(new angle::FrameCapture),
+      mRefCount(0),
       mOverlay(mImplementation.get())
 {
     for (angle::SubjectIndex uboIndex = kUniformBuffer0SubjectIndex;
@@ -691,10 +692,10 @@ egl::Error Context::makeCurrent(egl::Display *display,
 
 egl::Error Context::unMakeCurrent(const egl::Display *display)
 {
-    ANGLE_TRY(unsetDefaultFramebuffer());
-
     ANGLE_TRY(angle::ResultToEGL(mImplementation->onUnMakeCurrent(this)));
 
+    ANGLE_TRY(unsetDefaultFramebuffer());
+
     // Return the scratch buffers to the display so they can be shared with other contexts while
     // this one is not current.
     if (mScratchBuffer.valid())

src/libANGLE/Context.h

@@ -628,6 +628,10 @@ class Context final : public egl::LabeledObject, angle::NonCopyable, public angl
     bool isClearBufferMaskedOut(GLenum buffer, GLint drawbuffer) const;
     bool noopClearBuffer(GLenum buffer, GLint drawbuffer) const;
 
+    void addRef() const { mRefCount++; }
+    void release() const { mRefCount--; }
+    size_t getRefCount() const { return mRefCount; }
+
   private:
     void initialize();
 
@@ -780,6 +784,8 @@ class Context final : public egl::LabeledObject, angle::NonCopyable, public angl
     // Note: we use a raw pointer here so we can exclude frame capture sources from the build.
     std::unique_ptr<angle::FrameCapture> mFrameCapture;
 
+    mutable size_t mRefCount;
+
     OverlayType mOverlay;
 };
 }  // namespace gl

src/libANGLE/Display.cpp

@@ -877,7 +877,7 @@ Error Display::terminate(const Thread *thread)
         ANGLE_TRY(destroyContext(thread, *mContextSet.begin()));
     }
 
-    ANGLE_TRY(makeCurrent(thread, nullptr, nullptr, nullptr));
+    ANGLE_TRY(makeCurrent(thread->getContext(), nullptr, nullptr, nullptr));
 
     // The global texture and semaphore managers should be deleted with the last context that uses
     // it.
@@ -1194,6 +1194,8 @@ Error Display::createContext(const Config *configuration,
     ASSERT(context != nullptr);
     mContextSet.insert(context);
 
+    context->addRef();
+
     ASSERT(outContext != nullptr);
     *outContext = context;
     return NoError();
@@ -1225,7 +1227,7 @@ Error Display::createSync(const gl::Context *currentContext,
     return NoError();
 }
 
-Error Display::makeCurrent(const Thread *thread,
+Error Display::makeCurrent(gl::Context *previousContext,
                            egl::Surface *drawSurface,
                            egl::Surface *readSurface,
                            gl::Context *context)
@@ -1235,10 +1237,17 @@ Error Display::makeCurrent(const Thread *thread,
         return NoError();
     }
 
-    gl::Context *previousContext = thread->getContext();
-    if (previousContext)
+    // If the context is changing we need to update the reference counts. If it's not, e.g. just
+    // changing the surfaces leave the reference count alone. Otherwise the reference count might go
+    // to zero even though we know we are not done with the context.
+    bool updateRefCount = context != previousContext;
+    if (previousContext != nullptr)
     {
         ANGLE_TRY(previousContext->unMakeCurrent(this));
+        if (updateRefCount)
+        {
+            ANGLE_TRY(releaseContext(previousContext));
+        }
     }
 
     ANGLE_TRY(mImplementation->makeCurrent(drawSurface, readSurface, context));
@@ -1246,6 +1255,10 @@ Error Display::makeCurrent(const Thread *thread,
     if (context != nullptr)
     {
         ANGLE_TRY(context->makeCurrent(this, drawSurface, readSurface));
+        if (updateRefCount)
+        {
+            context->addRef();
+        }
     }
 
     // Tick all the scratch buffers to make sure they get cleaned up eventually if they stop being
@@ -1322,18 +1335,17 @@ void Display::destroyStream(egl::Stream *stream)
     SafeDelete(stream);
 }
 
-Error Display::destroyContext(const Thread *thread, gl::Context *context)
+// releaseContext must be called with the context being deleted as current.
+// To do that we can only call this in two places, Display::makeCurrent at the point where this
+// context is being made uncurrent and in Display::destroyContext where we make the context current
+// as part of destruction.
+Error Display::releaseContext(gl::Context *context)
 {
-    gl::Context *currentContext   = thread->getContext();
-    Surface *currentDrawSurface   = thread->getCurrentDrawSurface();
-    Surface *currentReadSurface   = thread->getCurrentReadSurface();
-    bool changeContextForDeletion = context != currentContext;
-
-    // Make the context being deleted current during it's deletion.  This allows it to delete
-    // any resources it's holding.
-    if (changeContextForDeletion)
+    context->release();
+    size_t refCount = context->getRefCount();
+    if (refCount > 0)
     {
-        ANGLE_TRY(makeCurrent(thread, nullptr, nullptr, context));
+        return NoError();
     }
 
     if (context->usingDisplayTextureShareGroup())
@@ -1368,10 +1380,37 @@ Error Display::destroyContext(const Thread *thread, gl::Context *context)
     mContextSet.erase(context);
     SafeDelete(context);
 
+    return NoError();
+}
+
+Error Display::destroyContext(const Thread *thread, gl::Context *context)
+{
+    size_t refCount = context->getRefCount();
+    if (refCount > 1)
+    {
+        context->release();
+        return NoError();
+    }
+
+    // This is the last reference for this context, so we can destroy it now.
+    gl::Context *currentContext   = thread->getContext();
+    Surface *currentDrawSurface   = thread->getCurrentDrawSurface();
+    Surface *currentReadSurface   = thread->getCurrentReadSurface();
+    bool changeContextForDeletion = context != currentContext;
+
+    // Make the context being deleted current during its deletion.  This allows it to delete
+    // any resources it's holding.
+    if (changeContextForDeletion)
+    {
+        ANGLE_TRY(makeCurrent(currentContext, nullptr, nullptr, context));
+    }
+
+    ANGLE_TRY(releaseContext(context));
+
     // Set the previous context back to current
     if (changeContextForDeletion)
     {
-        ANGLE_TRY(makeCurrent(thread, currentDrawSurface, currentReadSurface, currentContext));
+        ANGLE_TRY(makeCurrent(context, currentDrawSurface, currentReadSurface, currentContext));
     }
 
     return NoError();

src/libANGLE/Display.h

@@ -151,7 +151,7 @@ class Display final : public LabeledObject,
                      const AttributeMap &attribs,
                      Sync **outSync);
 
-    Error makeCurrent(const Thread *thread,
+    Error makeCurrent(gl::Context *previousContext,
                       Surface *drawSurface,
                       Surface *readSurface,
                       gl::Context *context);
@@ -259,6 +259,7 @@ class Display final : public LabeledObject,
     void updateAttribsFromEnvironment(const AttributeMap &attribMap);
 
     Error restoreLostDevice();
+    Error releaseContext(gl::Context *context);
 
     void initDisplayExtensions();
     void initVendorString();

src/libANGLE/Shader.h

@@ -22,6 +22,7 @@
 
 #include "common/Optional.h"
 #include "common/angleutils.h"
+#include "libANGLE/Caps.h"
 #include "libANGLE/Compiler.h"
 #include "libANGLE/Debug.h"
 #include "libANGLE/angletypes.h"
@@ -44,7 +45,6 @@ namespace gl
 {
 class CompileTask;
 class Context;
-struct Limitations;
 class ShaderProgramManager;
 class State;
 
@@ -242,7 +242,7 @@ class Shader final : angle::NonCopyable, public LabeledObject
 
     ShaderState mState;
     std::unique_ptr<rx::ShaderImpl> mImplementation;
-    const gl::Limitations &mRendererLimitations;
+    const gl::Limitations mRendererLimitations;
     const ShaderProgramID mHandle;
     const ShaderType mType;
     unsigned int mRefCount;  // Number of program objects this shader is attached to

src/libGL/entry_points_wgl.cpp

@@ -265,9 +265,9 @@ BOOL GL_APIENTRY wglMakeCurrent(HDC hDc, HGLRC newContext)
 
     if (previousDraw != surface || previousRead != surface || previousContext != context)
     {
-        ANGLE_EGL_TRY_RETURN(
-            thread,
-            display->makeCurrent(thread, surface, surface, const_cast<gl::Context *>(context)),
+        ANGLE_EGL_TRY_RETURN(thread,
+                             display->makeCurrent(previousContext, surface, surface,
+                                                  const_cast<gl::Context *>(context)),
                              "wglMakeCurrent", GetContextIfValid(display, context), EGL_FALSE);
 
         SetContextCurrent(thread, const_cast<gl::Context *>(context));

src/libGLESv2/entry_points_egl.cpp

@@ -106,7 +106,8 @@ EGLBoolean EGLAPIENTRY EGL_Terminate(EGLDisplay dpy)
     ANGLE_EGL_TRY_RETURN(thread, ValidateTerminate(display), "eglTerminate",
                          GetDisplayIfValid(display), EGL_FALSE);
 
-    ANGLE_EGL_TRY_RETURN(thread, display->makeCurrent(thread, nullptr, nullptr, nullptr),
+    ANGLE_EGL_TRY_RETURN(thread,
+                         display->makeCurrent(thread->getContext(), nullptr, nullptr, nullptr),
                          "eglTerminate", GetDisplayIfValid(display), EGL_FALSE);
     SetContextCurrent(thread, nullptr);
     ANGLE_EGL_TRY_RETURN(thread, display->terminate(thread), "eglTerminate",
@@ -416,6 +417,8 @@ EGLBoolean EGLAPIENTRY EGL_DestroyContext(EGLDisplay dpy, EGLContext ctx)
 
     if (contextWasCurrent)
     {
+        ANGLE_EGL_TRY_RETURN(thread, display->makeCurrent(context, nullptr, nullptr, nullptr),
+                             "eglDestroyContext", GetContextIfValid(display, context), EGL_FALSE);
         SetContextCurrent(thread, nullptr);
     }
 
@@ -451,8 +454,8 @@ EGLBoolean EGLAPIENTRY EGL_MakeCurrent(EGLDisplay dpy,
     // Only call makeCurrent if the context or surfaces have changed.
     if (previousDraw != drawSurface || previousRead != readSurface || previousContext != context)
     {
-        ANGLE_EGL_TRY_RETURN(thread,
-                             display->makeCurrent(thread, drawSurface, readSurface, context),
+        ANGLE_EGL_TRY_RETURN(
+            thread, display->makeCurrent(previousContext, drawSurface, readSurface, context),
             "eglMakeCurrent", GetContextIfValid(display, context), EGL_FALSE);
 
         SetContextCurrent(thread, context);
@@ -787,8 +790,8 @@ EGLBoolean EGLAPIENTRY EGL_ReleaseThread(void)
     {
         if (previousDisplay != EGL_NO_DISPLAY)
         {
-            ANGLE_EGL_TRY_RETURN(thread,
-                                 previousDisplay->makeCurrent(thread, nullptr, nullptr, nullptr),
+            ANGLE_EGL_TRY_RETURN(
+                thread, previousDisplay->makeCurrent(previousContext, nullptr, nullptr, nullptr),
                 "eglReleaseThread", nullptr, EGL_FALSE);
         }
 

src/tests/angle_end2end_tests.gni

@@ -7,6 +7,7 @@ angle_end2end_tests_sources = [
   "egl_tests/EGLBackwardsCompatibleContextTest.cpp",
   "egl_tests/EGLBlobCacheTest.cpp",
   "egl_tests/EGLChooseConfigTest.cpp",
+  "egl_tests/EGLContextASANTest.cpp",
   "egl_tests/EGLContextCompatibilityTest.cpp",
   "egl_tests/EGLContextSharingTest.cpp",
   "egl_tests/EGLCreateContextAttribsTest.cpp",

src/tests/egl_tests/EGLContextASANTest.cpp

+//
+// Copyright 2020 The ANGLE Project Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// EGLContextASANTest.cpp:
+//   Tests relating to ASAN errors regarding context.
+
+#include <gtest/gtest.h>
+
+#include "test_utils/ANGLETest.h"
+#include "test_utils/angle_test_configs.h"
+#include "test_utils/gl_raii.h"
+#include "util/EGLWindow.h"
+
+#include <condition_variable>
+#include <mutex>
+#include <thread>
+
+using namespace angle;
+
+namespace
+{
+
+EGLBoolean SafeDestroyContext(EGLDisplay display, EGLContext &context)
+{
+    EGLBoolean result = EGL_TRUE;
+    if (context != EGL_NO_CONTEXT)
+    {
+        result  = eglDestroyContext(display, context);
+        context = EGL_NO_CONTEXT;
+    }
+    return result;
+}
+
+class EGLContextASANTest : public ANGLETest
+{
+  public:
+    EGLContextASANTest() {}
+};
+
+// Tests that creating resources works after freeing the share context.
+TEST_P(EGLContextASANTest, DestroyContextInUse)
+{
+    ANGLE_SKIP_TEST_IF(!platformSupportsMultithreading());
+
+    EGLDisplay display = getEGLWindow()->getDisplay();
+    EGLConfig config   = getEGLWindow()->getConfig();
+    EGLSurface surface = getEGLWindow()->getSurface();
+
+    const EGLint contextAttribs[] = {EGL_CONTEXT_CLIENT_VERSION,
+                                     getEGLWindow()->getClientMajorVersion(), EGL_NONE};
+
+    EGLContext context = eglCreateContext(display, config, nullptr, contextAttribs);
+    ASSERT_EGL_SUCCESS();
+    ASSERT_TRUE(context != EGL_NO_CONTEXT);
+
+    // Synchronization tools to ensure the two threads are interleaved as designed by this test.
+    std::mutex mutex;
+    std::condition_variable condVar;
+
+    enum class Step
+    {
+        Start,
+        Thread1Draw,
+        Thread0Delete,
+        Thread1Draw2,
+        Finish,
+        Abort,
+    };
+    Step currentStep = Step::Start;
+
+    // Helper functions to synchronize the threads so that the operations are executed in the
+    // specific order the test is written for.
+    auto waitForStep = [&](Step waitStep) -> bool {
+        std::unique_lock<std::mutex> lock(mutex);
+        while (currentStep != waitStep)
+        {
+            // If necessary, abort execution as the other thread has encountered a GL error.
+            if (currentStep == Step::Abort)
+            {
+                return false;
+            }
+            condVar.wait(lock);
+        }
+
+        return true;
+    };
+    auto nextStep = [&](Step newStep) {
+        {
+            std::unique_lock<std::mutex> lock(mutex);
+            currentStep = newStep;
+        }
+        condVar.notify_one();
+    };
+
+    class AbortOnFailure
+    {
+      public:
+        AbortOnFailure(Step *currentStep, std::mutex *mutex, std::condition_variable *condVar)
+            : mCurrentStep(currentStep), mMutex(mutex), mCondVar(condVar)
+        {}
+
+        ~AbortOnFailure()
+        {
+            bool isAborting = false;
+            {
+                std::unique_lock<std::mutex> lock(*mMutex);
+                isAborting = *mCurrentStep != Step::Finish;
+
+                if (isAborting)
+                {
+                    *mCurrentStep = Step::Abort;
+                }
+            }
+            mCondVar->notify_all();
+        }
+
+      private:
+        Step *mCurrentStep;
+        std::mutex *mMutex;
+        std::condition_variable *mCondVar;
+    };
+
+    std::thread deletingThread = std::thread([&]() {
+        AbortOnFailure abortOnFailure(&currentStep, &mutex, &condVar);
+
+        EXPECT_EGL_TRUE(eglMakeCurrent(display, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT));
+        EXPECT_EGL_SUCCESS();
+        EXPECT_GL_NO_ERROR();
+
+        // Wait for other thread to draw
+        ASSERT_TRUE(waitForStep(Step::Thread1Draw));
+
+        // Delete the context, if implemented properly this is a no-op because the context is
+        // current in another thread.
+        SafeDestroyContext(display, context);
+
+        // Wait for the other thread to use context again
+        nextStep(Step::Thread0Delete);
+        ASSERT_TRUE(waitForStep(Step::Finish));
+    });
+
+    std::thread continuingThread = std::thread([&]() {
+        EGLContext localContext = context;
+        AbortOnFailure abortOnFailure(&currentStep, &mutex, &condVar);
+
+        EXPECT_EGL_TRUE(eglMakeCurrent(display, surface, surface, localContext));
+        EXPECT_EGL_SUCCESS();
+
+        constexpr GLsizei kTexSize = 1;
+        const GLColor kTexData     = GLColor::red;
+
+        GLTexture tex;
+        glBindTexture(GL_TEXTURE_2D, tex);
+        glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, kTexSize, kTexSize, 0, GL_RGBA, GL_UNSIGNED_BYTE,
+                     &kTexData);
+        glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
+        glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
+
+        GLProgram program;
+        program.makeRaster(essl1_shaders::vs::Texture2D(), essl1_shaders::fs::Texture2D());
+        ASSERT_TRUE(program.valid());
+
+        // Draw using the texture.
+        drawQuad(program.get(), essl1_shaders::PositionAttrib(), 0.5f);
+
+        EXPECT_EGL_TRUE(eglMakeCurrent(display, EGL_NO_SURFACE, EGL_NO_SURFACE, localContext));
+        EXPECT_EGL_SUCCESS();
+
+        // Wait for the other thread to delete the context.
+        nextStep(Step::Thread1Draw);
+        ASSERT_TRUE(waitForStep(Step::Thread0Delete));
+
+        EXPECT_EGL_TRUE(eglMakeCurrent(display, surface, surface, localContext));
+        EXPECT_EGL_SUCCESS();
+
+        // Draw again. If the context has been inappropriately deleted in thread0 this will cause a
+        // use-after-free error.
+        drawQuad(program.get(), essl1_shaders::PositionAttrib(), 0.5f);
+
+        nextStep(Step::Finish);
+
+        EXPECT_EGL_TRUE(eglMakeCurrent(display, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT));
+        EXPECT_EGL_SUCCESS();
+    });
+
+    deletingThread.join();
+    continuingThread.join();
+
+    ASSERT_NE(currentStep, Step::Abort);
+
+    // cleanup
+    ASSERT_GL_NO_ERROR();
+}
+}  // anonymous namespace
+
+ANGLE_INSTANTIATE_TEST(EGLContextASANTest,
+                       ES2_D3D9(),
+                       ES2_D3D11(),
+                       ES3_D3D11(),
+                       ES2_OPENGL(),
+                       ES3_OPENGL(),
+                       ES2_VULKAN(),
+                       ES3_VULKAN());