Issue #1354 - Fix another potential crashing scenario in WebGL.

(merge of gl-work branch)

10 files changed, 81 insertions, 95 deletions

diff --git a/dom/canvas/WebGLContextValidate.cpp b/dom/canvas/WebGLContextValidate.cpp
index ebf0aa8c2..30d4c6522 100644
--- a/dom/canvas/WebGLContextValidate.cpp
+++ b/dom/canvas/WebGLContextValidate.cpp
@@ -440,6 +440,13 @@ WebGLContext::InitAndValidateGL(FailureReason* const out_failReason)
 {
     MOZ_RELEASE_ASSERT(gl, "GFX: GL not initialized");
 
+    if (!gl->MakeCurrent(true)) {
+        MOZ_ASSERT(false);
+        *out_failReason = { "FEATURE_FAILURE_WEBGL_MAKECURRENT",
+                            "Failed to MakeCurrent for init." };
+        return false;
+    }
+
     // Unconditionally create a new format usage authority. This is
     // important when restoring contexts and extensions need to add
     // formats back into the authority.
diff --git a/gfx/gl/GLContext.cpp b/gfx/gl/GLContext.cpp
index 1515b8627..3fb87822d 100644
--- a/gfx/gl/GLContext.cpp
+++ b/gfx/gl/GLContext.cpp
@@ -578,6 +578,10 @@ GLContext::LoadFeatureSymbols(const char* prefix, bool trygl, const SymLoadStruc
 bool
 GLContext::InitWithPrefixImpl(const char* prefix, bool trygl)
 {
+    // wglGetProcAddress requires a current context.
+    if (!MakeCurrent(true))
+        return false;
+
     mWorkAroundDriverBugs = gfxPrefs::WorkAroundDriverBugs();
 
     const SymLoadStruct coreSymbols[] = {
@@ -714,7 +718,6 @@ GLContext::InitWithPrefixImpl(const char* prefix, bool trygl)
 
     ////////////////
 
-    MakeCurrent();
     MOZ_ASSERT(mProfile != ContextProfile::Unknown);
 
     uint32_t version = 0;
@@ -2253,13 +2256,11 @@ GLContext::MarkDestroyed()
     mBlitHelper = nullptr;
     mReadTexImageHelper = nullptr;
 
-    if (MakeCurrent()) {
+    mIsDestroyed = true;
+    mSymbols.Zero();
+    if (MakeCurrent(true)) {
         mTexGarbageBin->GLContextTeardown();
-    } else {
-        NS_WARNING("MakeCurrent() failed during MarkDestroyed! Skipping GL object teardown.");
     }
-
-    mSymbols.Zero();
 }
 
 #ifdef MOZ_GL_DEBUG
diff --git a/gfx/gl/GLContext.h b/gfx/gl/GLContext.h
index 6e3e22207..4c1f4f55c 100644
--- a/gfx/gl/GLContext.h
+++ b/gfx/gl/GLContext.h
@@ -353,6 +353,7 @@ public:
 protected:
     bool mIsOffscreen;
     bool mContextLost;
+    bool mIsDestroyed = false;
 
     /**
      * mVersion store the OpenGL's version, multiplied by 100. For example, if
@@ -3265,7 +3266,7 @@ public:
 #endif
         return MakeCurrentImpl(aForce);
     }
-
+    
     virtual bool Init() = 0;
 
     virtual bool SetupLookupFunction() = 0;
@@ -3273,8 +3274,7 @@ public:
     virtual void ReleaseSurface() {}
 
     bool IsDestroyed() {
-        // MarkDestroyed will mark all these as null.
-        return mSymbols.fUseProgram == nullptr;
+        return mIsDestroyed;
     }
 
     GLContext* GetSharedContext() { return mSharedContext; }
diff --git a/gfx/gl/GLContextCGL.h b/gfx/gl/GLContextCGL.h
index 1a29f3d15..23616d861 100644
--- 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)
diff --git a/gfx/gl/GLContextGLX.h b/gfx/gl/GLContextGLX.h
index 1f2cee08d..0463669e9 100644
--- a/gfx/gl/GLContextGLX.h
+++ b/gfx/gl/GLContextGLX.h
@@ -86,6 +86,7 @@ private:
     GLXContext mContext;
     Display* mDisplay;
     GLXDrawable mDrawable;
+    Maybe<GLXDrawable> mOverrideDrawable;
     bool mDeleteDrawable;
     bool mDoubleBuffered;
 
diff --git a/gfx/gl/GLContextProviderCGL.mm b/gfx/gl/GLContextProviderCGL.mm
index ceab3046c..7cc89eac9 100644
--- a/gfx/gl/GLContextProviderCGL.mm
+++ b/gfx/gl/GLContextProviderCGL.mm
@@ -83,26 +83,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
@@ -114,6 +101,11 @@ GLContextCGL::GetCGLContext() const
 bool
 GLContextCGL::MakeCurrentImpl(bool aForce)
 {
+    if (IsDestroyed()) {
+        [NSOpenGLContext clearCurrentContext];
+        return false;
+    }
+
     if (!aForce && [NSOpenGLContext currentContext] == mContext) {
         return true;
     }
diff --git a/gfx/gl/GLContextProviderEAGL.mm b/gfx/gl/GLContextProviderEAGL.mm
index 507616e2f..11c7cce77 100644
--- a/gfx/gl/GLContextProviderEAGL.mm
+++ b/gfx/gl/GLContextProviderEAGL.mm
@@ -36,35 +36,24 @@ GLContextEAGL::GLContextEAGL(CreateContextFlags flags, const SurfaceCaps& caps,
 
 GLContextEAGL::~GLContextEAGL()
 {
-    MakeCurrent();
-
-    if (mBackbufferFB) {
-        fDeleteFramebuffers(1, &mBackbufferFB);
-    }
-
-    if (mBackbufferRB) {
-        fDeleteRenderbuffers(1, &mBackbufferRB);
+    if (MakeCurrent()) {
+        if (mBackbufferFB) {
+            fDeleteFramebuffers(1, &mBackbufferFB);
+        }
+        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
@@ -120,12 +109,12 @@ GLContextEAGL::MakeCurrentImpl(bool aForce)
         return true;
     }
 
-    if (mContext) {
-        if(![EAGLContext setCurrentContext:mContext]) {
-            return false;
-        }
+    if (IsDestroyed()) {
+        [EAGLContext setCurrentContext:nil];
+        return false;
     }
-    return true;
+    
+    return [EAGLContext setCurrentContext:mContext];
 }
 
 bool
diff --git a/gfx/gl/GLContextProviderEGL.cpp b/gfx/gl/GLContextProviderEGL.cpp
index 7979f3bf0..23fc3472d 100644
--- a/gfx/gl/GLContextProviderEGL.cpp
+++ b/gfx/gl/GLContextProviderEGL.cpp
@@ -150,13 +150,12 @@ is_power_of_two(int v)
 }
 
 static void
-DestroySurface(EGLSurface oldSurface) {
-    if (oldSurface != EGL_NO_SURFACE) {
-        sEGLLibrary.fMakeCurrent(EGL_DISPLAY(),
-                                 EGL_NO_SURFACE, EGL_NO_SURFACE,
-                                 EGL_NO_CONTEXT);
-        sEGLLibrary.fDestroySurface(EGL_DISPLAY(), oldSurface);
+DestroySurface(const EGLSurface surf) {
+    if (!surf) {
+        // Nothing to do.
+        return;
     }
+    sEGLLibrary.fDestroySurface(EGL_DISPLAY(), surf);
 }
 
 static EGLSurface
@@ -223,7 +222,7 @@ GLContextEGL::~GLContextEGL()
     sEGLLibrary.fDestroyContext(EGL_DISPLAY(), mContext);
     sEGLLibrary.UnsetCachedCurrentContext();
 
-    mozilla::gl::DestroySurface(mSurface);
+    DestroySurface(mSurface);
 }
 
 bool
@@ -247,12 +246,7 @@ GLContextEGL::Init()
     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;
@@ -303,7 +297,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`,
@@ -314,12 +311,17 @@ GLContextEGL::SetEGLSurfaceOverride(EGLSurface surf) {
     }
 
     mSurfaceOverride = surf;
-    DebugOnly<bool> ok = MakeCurrent(true);
-    MOZ_ASSERT(ok);
+    MOZ_ALWAYS_TRUE(MakeCurrent(true));
 }
 
 bool
 GLContextEGL::MakeCurrentImpl(bool aForce) {
+    if (IsDestroyed()) {
+        //Clear and exit
+        sEGLLibrary.fMakeCurrent(EGL_DISPLAY(), nullptr, nullptr, nullptr);
+        return false;
+    }
+    
     bool succeeded = true;
 
     // Assume that EGL has the same problem as WGL does,
@@ -373,7 +375,7 @@ GLContextEGL::IsCurrent() {
 }
 
 bool
-GLContextEGL::RenewSurface(nsIWidget* aWidget) {
+GLContextEGL::RenewSurface(nsIWidget* const aWidget) {
     if (!mOwnsContext) {
         return false;
     }
@@ -389,13 +391,12 @@ GLContextEGL::RenewSurface(nsIWidget* aWidget) {
 
 void
 GLContextEGL::ReleaseSurface() {
-    if (mOwnsContext) {
-        mozilla::gl::DestroySurface(mSurface);
-    }
-    if (mSurface == mSurfaceOverride) {
-        mSurfaceOverride = EGL_NO_SURFACE;
+    if (!mOwnsContext) {
+        return;
     }
-    mSurface = EGL_NO_SURFACE;
+    
+    DestroySurface(mSurface);
+    mSurface = nullptr;
 }
 
 bool
diff --git a/gfx/gl/GLContextProviderGLX.cpp b/gfx/gl/GLContextProviderGLX.cpp
index 539520a8c..c44b1a9f0 100644
--- a/gfx/gl/GLContextProviderGLX.cpp
+++ b/gfx/gl/GLContextProviderGLX.cpp
@@ -894,20 +894,12 @@ GLContextGLX::~GLContextGLX()
         return;
     }
 
-    // see bug 659842 comment 76
-#ifdef DEBUG
-    bool success =
-#endif
-    mGLX->xMakeCurrent(mDisplay, X11None, nullptr);
-    MOZ_ASSERT(success,
-               "glXMakeCurrent failed to release GL context before we call "
-               "glXDestroyContext!");
-
     mGLX->xDestroyContext(mDisplay, mContext);
 
     if (mDeleteDrawable) {
         mGLX->xDestroyPixmap(mDisplay, mDrawable);
     }
+    MOZ_ASSERT(!mOverrideDrawable);
 }
 
 
@@ -944,6 +936,10 @@ GLContextGLX::MakeCurrentImpl(bool aForce)
           // DRI2InvalidateBuffers event before drawing. See bug 1280653.
           Unused << XPending(mDisplay);
         }
+        if (IsDestroyed()) {
+            MOZ_ALWAYS_TRUE(mGLX->xMakeCurrent(mDisplay, X11None, nullptr));
+            return false; // We did not MakeCurrent mContext, but that's what we wanted!
+        }
 
         succeeded = mGLX->xMakeCurrent(mDisplay, mDrawable, mContext);
         NS_ASSERTION(succeeded, "Failed to make GL context current!");
@@ -1017,16 +1013,18 @@ GLContextGLX::GetWSIInfo(nsCString* const out) const
 bool
 GLContextGLX::OverrideDrawable(GLXDrawable drawable)
 {
-    if (Screen())
+    if (Screen()) {
         Screen()->AssureBlitted();
-    Bool result = mGLX->xMakeCurrent(mDisplay, drawable, mContext);
-    return result;
+    }
+    mOverrideDrawable = Some(drawable);
+    return MakeCurrent(true);
 }
 
 bool
 GLContextGLX::RestoreDrawable()
 {
-    return mGLX->xMakeCurrent(mDisplay, mDrawable, mContext);
+    mOverrideDrawable = Nothing();
+    return MakeCurrent(true);
 }
 
 GLContextGLX::GLContextGLX(
diff --git a/gfx/gl/GLContextProviderWGL.cpp b/gfx/gl/GLContextProviderWGL.cpp
index 35957259d..da8c93d10 100644
--- a/gfx/gl/GLContextProviderWGL.cpp
+++ b/gfx/gl/GLContextProviderWGL.cpp
@@ -314,20 +314,17 @@ GLContextWGL::Init()
     if (!mDC || !mContext)
         return false;
 
-    // see bug 929506 comment 29. wglGetProcAddress requires a current context.
-    if (!sWGLLib.fMakeCurrent(mDC, mContext))
-        return false;
-
     SetupLookupFunction();
-    if (!InitWithPrefix("gl", true))
-        return false;
-
-    return true;
+    return InitWithPrefix("gl", true);
 }
 
 bool
 GLContextWGL::MakeCurrentImpl(bool aForce)
 {
+    if (IsDestroyed()) {
+        MOZ_ALWAYS_TRUE(sWGLLib.fMakeCurrent(0, 0));
+    }
+    
     BOOL succeeded = true;
 
     // wglGetCurrentContext seems to just pull the HGLRC out