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 346823 7d231bdcb67ad93f3b00bd667022ff4a53fe5112
parent 346822 8c4e144e372bac0d3ebef156e32109a57b47f47c
child 346824 1aa5261093ae8eaa7ea2eecd5b5bd1179fdfad66
push id6389
push userraliiev@mozilla.com
push dateMon, 19 Sep 2016 13:38:22 +0000
treeherdermozilla-beta@01d67bfe6c81 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjerry
bugs1286459
milestone50.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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