Bug 1286459 - Be very careful about nulling WebGLContext::gl. - r=jerry
authorJeff Gilbert <jgilbert@mozilla.com>
Thu, 21 Jul 2016 01:12:15 -0700
changeset 331857 7d231bdcb67ad93f3b00bd667022ff4a53fe5112
parent 331856 8c4e144e372bac0d3ebef156e32109a57b47f47c
child 331858 1aa5261093ae8eaa7ea2eecd5b5bd1179fdfad66
push id9858
push userjlund@mozilla.com
push dateMon, 01 Aug 2016 14:37:10 +0000
treeherdermozilla-aurora@203106ef6cb6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjerry
bugs1286459
milestone50.0a1
Bug 1286459 - Be very careful about nulling WebGLContext::gl. - r=jerry MozReview-Commit-ID: 3evG45zLT5j
dom/canvas/WebGLContext.cpp
dom/canvas/WebGLContextUnchecked.cpp
dom/canvas/WebGLContextUnchecked.h
--- a/dom/canvas/WebGLContext.cpp
+++ b/dom/canvas/WebGLContext.cpp
@@ -280,18 +280,20 @@ WebGLContext::DestroyResourcesAndContext
     mFakeBlack_2D_0001       = nullptr;
     mFakeBlack_CubeMap_0000  = nullptr;
     mFakeBlack_CubeMap_0001  = nullptr;
     mFakeBlack_3D_0000       = nullptr;
     mFakeBlack_3D_0001       = nullptr;
     mFakeBlack_2D_Array_0000 = nullptr;
     mFakeBlack_2D_Array_0001 = nullptr;
 
-    if (mFakeVertexAttrib0BufferObject)
+    if (mFakeVertexAttrib0BufferObject) {
         gl->fDeleteBuffers(1, &mFakeVertexAttrib0BufferObject);
+        mFakeVertexAttrib0BufferObject = 0;
+    }
 
     // disable all extensions except "WEBGL_lose_context". see bug #927969
     // spec: http://www.khronos.org/registry/webgl/specs/latest/1.0/#5.15.2
     for (size_t i = 0; i < size_t(WebGLExtensionID::Max); ++i) {
         WebGLExtensionID extension = WebGLExtensionID(i);
 
         if (!IsExtensionEnabled(extension) || (extension == WebGLExtensionID::WEBGL_lose_context))
             continue;
@@ -301,17 +303,19 @@ WebGLContext::DestroyResourcesAndContext
     }
 
     // We just got rid of everything, so the context had better
     // have been going away.
     if (GLContext::ShouldSpew()) {
         printf_stderr("--- WebGL context destroyed: %p\n", gl.get());
     }
 
-    gl = nullptr;
+    MOZ_ASSERT(gl);
+    mGL_OnlyClearInDestroyResourcesAndContext = nullptr;
+    MOZ_ASSERT(!gl);
 }
 
 void
 WebGLContext::Invalidate()
 {
     if (!mCanvasElement)
         return;
 
@@ -664,32 +668,37 @@ WebGLContext::CreateAndInitGLWith(FnCrea
                                   const gl::SurfaceCaps& baseCaps,
                                   gl::CreateContextFlags flags,
                                   std::vector<FailureReason>* const out_failReasons)
 {
     std::queue<gl::SurfaceCaps> fallbackCaps;
     PopulateCapFallbackQueue(baseCaps, &fallbackCaps);
 
     MOZ_RELEASE_ASSERT(!gl, "GFX: Already have a context.");
-    gl = nullptr;
+    RefPtr<gl::GLContext> potentialGL;
     while (!fallbackCaps.empty()) {
         const gl::SurfaceCaps& caps = fallbackCaps.front();
-        gl = fnCreateGL(caps, flags, this, out_failReasons);
-        if (gl)
+        potentialGL = fnCreateGL(caps, flags, this, out_failReasons);
+        if (potentialGL)
             break;
 
         fallbackCaps.pop();
     }
-    if (!gl)
+    if (!potentialGL)
         return false;
 
     FailureReason reason;
+
+    mGL_OnlyClearInDestroyResourcesAndContext = potentialGL;
+    MOZ_RELEASE_ASSERT(gl);
     if (!InitAndValidateGL(&reason)) {
+        DestroyResourcesAndContext();
+        MOZ_RELEASE_ASSERT(!gl);
+
         // The fail reason here should be specific enough for now.
-        gl = nullptr;
         out_failReasons->push_back(reason);
         return false;
     }
 
     return true;
 }
 
 bool
@@ -978,31 +987,33 @@ WebGLContext::SetDimensions(int32_t sign
         ThrowEvent_WebGLContextCreationError(text);
         return NS_ERROR_FAILURE;
     }
     MOZ_ASSERT(gl);
     MOZ_ASSERT_IF(mOptions.alpha, gl->Caps().alpha);
 
     if (mOptions.failIfMajorPerformanceCaveat) {
         if (gl->IsWARP()) {
-            gl = nullptr;
+            DestroyResourcesAndContext();
+            MOZ_ASSERT(!gl);
 
             Telemetry::Accumulate(Telemetry::CANVAS_WEBGL_FAILURE_ID,
                                   NS_LITERAL_CSTRING("FEATURE_FAILURE_PERF_WARP"));
             const nsLiteralCString text("failIfMajorPerformanceCaveat: Driver is not"
                                         " hardware-accelerated.");
             ThrowEvent_WebGLContextCreationError(text);
             return NS_ERROR_FAILURE;
         }
 
 #ifdef XP_WIN
         if (gl->GetContextType() == gl::GLContextType::WGL &&
             !gl::sWGLLib.HasDXInterop2())
         {
-            gl = nullptr;
+            DestroyResourcesAndContext();
+            MOZ_ASSERT(!gl);
 
             const nsLiteralCString text("Caveat: WGL without DXGLInterop2.");
             ThrowEvent_WebGLContextCreationError(text);
             return NS_ERROR_FAILURE;
         }
 #endif
     }
 
--- a/dom/canvas/WebGLContextUnchecked.cpp
+++ b/dom/canvas/WebGLContextUnchecked.cpp
@@ -7,18 +7,19 @@
 #include "WebGLContextUnchecked.h"
 
 #include "GLContext.h"
 #include "WebGLBuffer.h"
 #include "WebGLSampler.h"
 
 namespace mozilla {
 
-WebGLContextUnchecked::WebGLContextUnchecked(gl::GLContext* gl)
-    : gl(gl)
+WebGLContextUnchecked::WebGLContextUnchecked(gl::GLContext* _gl)
+    : mGL_NeverTouchDirectly(_gl)
+    , gl(mGL_NeverTouchDirectly) // const reference
 { }
 
 
 // -----------------------------------------------------------------------------
 // Buffer Objects
 
 void
 WebGLContextUnchecked::BindBuffer(GLenum target, WebGLBuffer* buffer)
--- a/dom/canvas/WebGLContextUnchecked.h
+++ b/dom/canvas/WebGLContextUnchecked.h
@@ -34,15 +34,21 @@ public:
     GLint   GetSamplerParameteriv(WebGLSampler* sampler, GLenum pname);
     GLfloat GetSamplerParameterfv(WebGLSampler* sampler, GLenum pname);
 
     void SamplerParameteri(WebGLSampler* sampler, GLenum pname, GLint param);
     void SamplerParameteriv(WebGLSampler* sampler, GLenum pname, const GLint* param);
     void SamplerParameterf(WebGLSampler* sampler, GLenum pname, GLfloat param);
     void SamplerParameterfv(WebGLSampler* sampler, GLenum pname, const GLfloat* param);
 
-protected: // data
-    RefPtr<gl::GLContext> gl;
+protected:
+    // We've had issues in the past with nulling `gl` without actually releasing
+    // all of our resources. This construction ensures that we are aware that we
+    // should only null `gl` in DestroyResourcesAndContext.
+    RefPtr<gl::GLContext> mGL_OnlyClearInDestroyResourcesAndContext;
+public:
+    // Grab a const reference so we can see changes, but can't make changes.
+    const decltype(mGL_OnlyClearInDestroyResourcesAndContext)& gl;
 };
 
 } // namespace mozilla
 
 #endif // !WEBGLCONTEXTUNCHECKED_H