Bug 1399501 - !MakeCurrent() should clear the current context. - r=kvark

Make MarkDestroyed call MakeCurrent(force=true) to clear the current context also. Differential Revision: https://phabricator.services.mozilla.com/D9289
2018-10-11 17:18:12 -07:00 · 2018-10-11 17:18:12 -07:00 · cb11c4842a
--- a/gfx/gl/GLContext.cpp
+++ b/gfx/gl/GLContext.cpp
@ -381,6 +381,10 @@ GLContext::LoadFeatureSymbols(const char* prefix, bool trygl, const SymLoadStruc
 bool
 GLContext::InitWithPrefixImpl(const char* prefix, bool trygl)
 {
+    // see bug 929506 comment 29. wglGetProcAddress requires a current context.
+    if (!MakeCurrent(true))
+        return false;
+
    mWorkAroundDriverBugs = gfxPrefs::WorkAroundDriverBugs();

    const SymLoadStruct coreSymbols[] = {
@ -517,10 +521,6 @@ GLContext::InitWithPrefixImpl(const char* prefix, bool trygl)

    ////////////////

-    if (!MakeCurrent()) {
-        return false;
-    }
-
    const std::string versionStr = (const char*)fGetString(LOCAL_GL_VERSION);
    if (versionStr.find("OpenGL ES") == 0) {
        mProfile = ContextProfile::OpenGLES;
@ -2054,11 +2054,9 @@ GLContext::MarkDestroyed()
    mBlitHelper = nullptr;
    mReadTexImageHelper = nullptr;

-    if (!MakeCurrent()) {
-        NS_WARNING("MakeCurrent() failed during MarkDestroyed! Skipping GL object teardown.");
-    }
-
+    mIsDestroyed = true;
    mSymbols = {};
+    (void)MakeCurrent(true); // Clear current context.
 }

 #ifdef MOZ_GL_DEBUG
@ -2944,16 +2942,14 @@ GetBytesPerTexel(GLenum format, GLenum type)
 }

 bool
-GLContext::MakeCurrent(bool aForce) const
+GLContext::MakeCurrent(const bool aForce) const
 {
-    if (MOZ_UNLIKELY( IsDestroyed() ))
-        return false;
-
-    if (MOZ_LIKELY( !aForce )) {
-        bool isCurrent;
+    if (MOZ_LIKELY( !aForce & !IsDestroyed() )) {
+        bool isCurrent = false;
        if (mUseTLSIsCurrent) {
            isCurrent = (sCurrentContext.get() == reinterpret_cast<uintptr_t>(this));
-        } else {
+        }
+        if (MOZ_UNLIKELY( !isCurrent )) {
            isCurrent = IsCurrentImpl();
        }
        if (MOZ_LIKELY( isCurrent )) {
@ -2962,8 +2958,10 @@ GLContext::MakeCurrent(bool aForce) const
        }
    }

-    if (!MakeCurrentImpl())
+    if (MOZ_UNLIKELY( !MakeCurrentImpl() )) {
+        ClearGetCurrentContextTLS();
        return false;
+    }

    sCurrentContext.set(reinterpret_cast<uintptr_t>(this));
    return true;
--- a/gfx/gl/GLContext.h
+++ b/gfx/gl/GLContext.h
@ -201,6 +201,10 @@ public:
    MOZ_DECLARE_WEAKREFERENCE_TYPENAME(GLContext)
    static MOZ_THREAD_LOCAL(uintptr_t) sCurrentContext;

+    static void ClearGetCurrentContextTLS() {
+        sCurrentContext.set(0);
+    }
+
    bool mImplicitMakeCurrent = false;
    bool mUseTLSIsCurrent;

@ -342,6 +346,7 @@ public:
 protected:
    bool mIsOffscreen;
    mutable bool mContextLost = false;
+    bool mIsDestroyed = false;

    /**
     * mVersion store the OpenGL's version, multiplied by 100. For example, if
@ -3378,8 +3383,7 @@ public:
    virtual void ReleaseSurface() {}

    bool IsDestroyed() const {
-        // MarkDestroyed will mark all these as null.
-        return mSymbols.fUseProgram == nullptr;
+        return mIsDestroyed;
    }

    GLContext* GetSharedContext() { return mSharedContext; }
--- a/gfx/gl/GLContextCGL.h
+++ b/gfx/gl/GLContextCGL.h
@ -24,7 +24,7 @@ class GLContextCGL : public GLContext
 {
    friend class GLContextProviderCGL;

-    NSOpenGLContext* mContext;
+    NSOpenGLContext* const mContext;

 public:
    MOZ_DECLARE_REFCOUNTED_VIRTUAL_TYPENAME(GLContextCGL, override)
--- a/gfx/gl/GLContextGLX.h
+++ b/gfx/gl/GLContextGLX.h
@ -87,6 +87,7 @@ private:
    GLXContext mContext;
    Display* mDisplay;
    GLXDrawable mDrawable;
+    Maybe<GLXDrawable> mOverrideDrawable;
    bool mDeleteDrawable;
    bool mDoubleBuffered;

--- a/gfx/gl/GLContextProviderCGL.mm
+++ b/gfx/gl/GLContextProviderCGL.mm
@ -76,25 +76,13 @@ GLContextCGL::GLContextCGL(CreateContextFlags flags, const SurfaceCaps& caps,
 GLContextCGL::~GLContextCGL()
 {
    MarkDestroyed();
-
-    if (mContext) {
-        if ([NSOpenGLContext currentContext] == mContext) {
-            // Clear the current context before releasing. If we don't do
-            // this, the next time we call [NSOpenGLContext currentContext],
-            // "invalid context" will be printed to the console.
-            [NSOpenGLContext clearCurrentContext];
-        }
-        [mContext release];
-    }
+    [mContext release];
 }

 bool
 GLContextCGL::Init()
 {
-    if (!InitWithPrefix("gl", true))
-        return false;
-
-    return true;
+    return InitWithPrefix("gl", true);
 }

 CGLContextObj
@ -106,6 +94,11 @@ GLContextCGL::GetCGLContext() const
 bool
 GLContextCGL::MakeCurrentImpl() const
 {
+    if (IsDestroyed()) {
+        [NSOpenGLContext clearCurrentContext];
+        return false;
+    }
+
    if (mContext) {
        [mContext makeCurrentContext];
        MOZ_ASSERT(IsCurrentImpl());
--- a/gfx/gl/GLContextProviderEAGL.mm
+++ b/gfx/gl/GLContextProviderEAGL.mm
@ -32,35 +32,26 @@ GLContextEAGL::GLContextEAGL(CreateContextFlags flags, const SurfaceCaps& caps,

 GLContextEAGL::~GLContextEAGL()
 {
-    MakeCurrent();
+    if (MakeCurrent()) {
+        if (mBackbufferFB) {
+            fDeleteFramebuffers(1, &mBackbufferFB);
+        }

-    if (mBackbufferFB) {
-        fDeleteFramebuffers(1, &mBackbufferFB);
+        if (mBackbufferRB) {
+            fDeleteRenderbuffers(1, &mBackbufferRB);
+        }
    }

-    if (mBackbufferRB) {
-        fDeleteRenderbuffers(1, &mBackbufferRB);
-    }
+    mLayer = nil;

    MarkDestroyed();
-
-    if (mLayer) {
-      mLayer = nil;
-    }
-
-    if (mContext) {
-      [EAGLContext setCurrentContext:nil];
-      [mContext release];
-    }
+    [mContext release];
 }

 bool
 GLContextEAGL::Init()
 {
-    if (!InitWithPrefix("gl", true))
-        return false;
-
-    return true;
+    return InitWithPrefix("gl", true);
 }

 bool
@ -112,12 +103,11 @@ GLContextEAGL::RecreateRB()
 bool
 GLContextEAGL::MakeCurrentImpl() const
 {
-    if (mContext) {
-        if(![EAGLContext setCurrentContext:mContext]) {
-            return false;
-        }
+    if (IsDestroyed()) {
+        [EAGLContext setCurrentContext:nil];
+        return false;
    }
-    return true;
+    return [EAGLContext setCurrentContext:mContext];
 }

 bool
--- a/gfx/gl/GLContextProviderEGL.cpp
+++ b/gfx/gl/GLContextProviderEGL.cpp
@ -158,19 +158,16 @@ is_power_of_two(int v)
 }

 static void
-DestroySurface(EGLSurface oldSurface) {
-    auto* egl = gl::GLLibraryEGL::Get();
+DestroySurface(const EGLSurface surf)
+{
+    MOZ_ASSERT(surf);
+    const auto& egl = gl::GLLibraryEGL::Get();

-    if (oldSurface != EGL_NO_SURFACE) {
-        // TODO: This breaks TLS MakeCurrent caching.
-        egl->fMakeCurrent(EGL_DISPLAY(),
-                          EGL_NO_SURFACE, EGL_NO_SURFACE,
-                          EGL_NO_CONTEXT);
-        egl->fDestroySurface(EGL_DISPLAY(), oldSurface);
+    // TODO: This breaks TLS MakeCurrent caching.
+    MOZ_ALWAYS_TRUE( egl->fDestroySurface(EGL_DISPLAY(), surf) );
 #if defined(MOZ_WAYLAND)
-        DeleteWaylandGLSurface(oldSurface);
+    DeleteWaylandGLSurface(surf);
 #endif
-    }
 }

 static EGLSurface
@ -284,7 +281,7 @@ GLContextEGLFactory::Create(EGLNativeWindowType aWindow,
                                                            surface, &discardFailureId);
    if (!gl) {
        gfxCriticalNote << "Failed to create EGLContext!";
-        mozilla::gl::DestroySurface(surface);
+        DestroySurface(surface);
        return nullptr;
    }

@ -326,9 +323,12 @@ GLContextEGL::~GLContextEGL()
 #endif

    mEgl->fDestroyContext(EGL_DISPLAY(), mContext);
-
-    mozilla::gl::DestroySurface(mSurface);
-    mozilla::gl::DestroySurface(mFallbackSurface);
+    if (mSurface) {
+        DestroySurface(mSurface);
+    }
+    if (mFallbackSurface) {
+        DestroySurface(mFallbackSurface);
+    }
 }

 bool
@ -351,13 +351,7 @@ GLContextEGL::Init()
    SetupLookupFunction();
    if (!InitWithPrefix("gl", true))
        return false;
-
-    bool current = MakeCurrent();
-    if (!current) {
-        gfx::LogFailure(NS_LITERAL_CSTRING(
-            "Couldn't get device attachments for device."));
-        return false;
-    }
+    MOZ_ASSERT(IsCurrent());

    static_assert(sizeof(GLint) >= sizeof(int32_t), "GLint is smaller than int32_t");
    mMaxTextureImageSize = INT32_MAX;
@ -408,7 +402,10 @@ GLContextEGL::ReleaseTexImage()
 }

 void
-GLContextEGL::SetEGLSurfaceOverride(EGLSurface surf) {
+GLContextEGL::SetEGLSurfaceOverride(const EGLSurface surf)
+{
+    MOZ_ASSERT(!surf || surf != mSurface);
+
    if (Screen()) {
        /* Blit `draw` to `read` if we need to, before we potentially juggle
          * `read` around. If we don't, we might attach a different `read`,
@ -419,15 +416,21 @@ GLContextEGL::SetEGLSurfaceOverride(EGLSurface surf) {
    }

    mSurfaceOverride = surf;
-    DebugOnly<bool> ok = MakeCurrent(true);
-    MOZ_ASSERT(ok);
+    MOZ_ALWAYS_TRUE( MakeCurrent(true) );
 }

 bool
 GLContextEGL::MakeCurrentImpl() const
 {
-    EGLSurface surface = (mSurfaceOverride != EGL_NO_SURFACE) ? mSurfaceOverride
-                                                              : mSurface;
+    if (IsDestroyed()) {
+        MOZ_ALWAYS_TRUE( mEgl->fMakeCurrent(EGL_DISPLAY(), nullptr, nullptr, nullptr) );
+        return false;
+    }
+
+    auto surface = mSurface;
+    if (mSurfaceOverride) {
+        surface = mSurfaceOverride;
+    }
    if (!surface) {
        surface = mFallbackSurface;
    }
@ -457,7 +460,8 @@ GLContextEGL::IsCurrentImpl() const
 }

 bool
-GLContextEGL::RenewSurface(CompositorWidget* aWidget) {
+GLContextEGL::RenewSurface(CompositorWidget* const aWidget)
+{
    if (!mOwnsContext) {
        return false;
    }
@ -479,14 +483,13 @@ GLContextEGL::RenewSurface(CompositorWidget* aWidget) {
 }

 void
-GLContextEGL::ReleaseSurface() {
-    if (mOwnsContext) {
-        mozilla::gl::DestroySurface(mSurface);
-    }
-    if (mSurface == mSurfaceOverride) {
-        mSurfaceOverride = EGL_NO_SURFACE;
-    }
-    mSurface = EGL_NO_SURFACE;
+GLContextEGL::ReleaseSurface()
+{
+    if (!mOwnsContext)
+        return;
+
+    mozilla::gl::DestroySurface(mSurface);
+    mSurface = nullptr;
 }

 bool
--- a/gfx/gl/GLContextProviderGLX.cpp
+++ b/gfx/gl/GLContextProviderGLX.cpp
@ -587,20 +587,12 @@ GLContextGLX::~GLContextGLX()
        return;
    }

-    // see bug 659842 comment 76
-#ifdef DEBUG
-    bool success =
-#endif
-    mGLX->fMakeCurrent(mDisplay, X11None, nullptr);
-    MOZ_ASSERT(success,
-               "glXMakeCurrent failed to release GL context before we call "
-               "glXDestroyContext!");
-
    mGLX->fDestroyContext(mDisplay, mContext);

    if (mDeleteDrawable) {
        mGLX->fDestroyPixmap(mDisplay, mDrawable);
    }
+    MOZ_ASSERT(!mOverrideDrawable);
 }


@ -629,7 +621,16 @@ GLContextGLX::MakeCurrentImpl() const
        Unused << XPending(mDisplay);
    }

-    const bool succeeded = mGLX->fMakeCurrent(mDisplay, mDrawable, mContext);
+    if (IsDestroyed()) {
+        MOZ_ALWAYS_TRUE( mGLX->fMakeCurrent(mDisplay, X11None, nullptr) );
+        return false; // Did not MakeCurrent mContext, but that's what we wanted!
+    }
+
+    auto drawable = mDrawable;
+    if (mOverrideDrawable) {
+        drawable = mOverrideDrawable.ref();
+    }
+    const bool succeeded = mGLX->fMakeCurrent(mDisplay, drawable, mContext);
    NS_ASSERTION(succeeded, "Failed to make GL context current!");

    if (!IsOffscreen() && mGLX->SupportsSwapControl()) {
@ -694,16 +695,18 @@ GLContextGLX::GetWSIInfo(nsCString* const out) const
 bool
 GLContextGLX::OverrideDrawable(GLXDrawable drawable)
 {
-    if (Screen())
+    if (Screen()) {
        Screen()->AssureBlitted();
-    Bool result = mGLX->fMakeCurrent(mDisplay, drawable, mContext);
-    return result;
+    }
+    mOverrideDrawable = Some(drawable);
+    return MakeCurrent(true);
 }

 bool
 GLContextGLX::RestoreDrawable()
 {
-    return mGLX->fMakeCurrent(mDisplay, mDrawable, mContext);
+    mOverrideDrawable = Nothing();
+    return MakeCurrent(true);
 }

 GLContextGLX::GLContextGLX(
--- a/gfx/gl/GLContextProviderWGL.cpp
+++ b/gfx/gl/GLContextProviderWGL.cpp
@ -163,7 +163,6 @@ WGLLibrary::EnsureInitialized()

    const auto curCtx = mSymbols.fGetCurrentContext();
    const auto curDC = mSymbols.fGetCurrentDC();
-
    if (!mSymbols.fMakeCurrent(mRootDc, mDummyGlrc)) {
        NS_WARNING("wglMakeCurrent failed");
        return false;
@ -299,7 +298,6 @@ GLContextWGL::GLContextWGL(CreateContextFlags flags, const SurfaceCaps& caps,
 GLContextWGL::~GLContextWGL()
 {
    MarkDestroyed();
-
    (void)sWGLLib.mSymbols.fDeleteContext(mContext);

    if (mPBuffer) {
@ -318,20 +316,18 @@ GLContextWGL::Init()
    if (!mDC || !mContext)
        return false;

-    // see bug 929506 comment 29. wglGetProcAddress requires a current context.
-    if (!sWGLLib.mSymbols.fMakeCurrent(mDC, mContext))
-        return false;
-
    SetupLookupFunction();
-    if (!InitWithPrefix("gl", true))
-        return false;
-
-    return true;
+    return InitWithPrefix("gl", true);
 }

 bool
 GLContextWGL::MakeCurrentImpl() const
 {
+    if (IsDestroyed()) {
+        MOZ_ALWAYS_TRUE( sWGLLib.mSymbols.fMakeCurrent(0, 0) );
+        return false;
+    }
+
    const bool succeeded = sWGLLib.mSymbols.fMakeCurrent(mDC, mContext);
    NS_ASSERTION(succeeded, "Failed to make GL context current!");
    return succeeded;