Bug 922921 - Consistently check attrib accesses in WebGLVertexArray. Also rename a few things. - r=jgilbert, a=lsblakk
authorJeff Gilbert <jgilbert@mozilla.com>
Fri, 18 Oct 2013 18:33:59 -0700
changeset 160781 08e1cf38074c42021593f0a75356fa5e6d9cd636
parent 160780 50750097f7b3c19701471261c857485e74bdbb24
child 160782 9b997ce02039b4382d4a233ea910b36288650238
push id2961
push userlsblakk@mozilla.com
push dateMon, 28 Oct 2013 21:59:28 +0000
treeherdermozilla-beta@73ef4f13486f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjgilbert, lsblakk
bugs922921
milestone26.0a2
Bug 922921 - Consistently check attrib accesses in WebGLVertexArray. Also rename a few things. - r=jgilbert, a=lsblakk
content/canvas/src/WebGLContextBuffers.cpp
content/canvas/src/WebGLContextGL.cpp
content/canvas/src/WebGLContextValidate.cpp
content/canvas/src/WebGLContextVertices.cpp
content/canvas/src/WebGLVertexArray.cpp
content/canvas/src/WebGLVertexArray.h
--- a/content/canvas/src/WebGLContextBuffers.cpp
+++ b/content/canvas/src/WebGLContextBuffers.cpp
@@ -378,18 +378,18 @@ WebGLContext::DeleteBuffer(WebGLBuffer *
     }
 
     if (mBoundVertexArray->mBoundElementArrayBuffer == buffer) {
         BindBuffer(LOCAL_GL_ELEMENT_ARRAY_BUFFER,
                    static_cast<WebGLBuffer*>(nullptr));
     }
 
     for (int32_t i = 0; i < mGLMaxVertexAttribs; i++) {
-        if (mBoundVertexArray->mAttribBuffers[i].buf == buffer)
-            mBoundVertexArray->mAttribBuffers[i].buf = nullptr;
+        if (mBoundVertexArray->HasAttrib(i) && mBoundVertexArray->mAttribs[i].buf == buffer)
+            mBoundVertexArray->mAttribs[i].buf = nullptr;
     }
 
     buffer->RequestDelete();
 }
 
 bool
 WebGLContext::IsBuffer(WebGLBuffer *buffer)
 {
--- a/content/canvas/src/WebGLContextGL.cpp
+++ b/content/canvas/src/WebGLContextGL.cpp
@@ -801,24 +801,24 @@ WebGLContext::DepthRange(GLfloat zNear, 
 int
 WebGLContext::WhatDoesVertexAttrib0Need()
 {
   // here we may assume that mCurrentProgram != null
 
     // work around Mac OSX crash, see bug 631420
 #ifdef XP_MACOSX
     if (gl->WorkAroundDriverBugs() &&
-        mBoundVertexArray->mAttribBuffers[0].enabled &&
+        mBoundVertexArray->IsAttribArrayEnabled(0) &&
         !mCurrentProgram->IsAttribInUse(0))
     {
         return VertexAttrib0Status::EmulatedUninitializedArray;
     }
 #endif
 
-    return (gl->IsGLES2() || mBoundVertexArray->mAttribBuffers[0].enabled) ? VertexAttrib0Status::Default
+    return (gl->IsGLES2() || mBoundVertexArray->IsAttribArrayEnabled(0)) ? VertexAttrib0Status::Default
          : mCurrentProgram->IsAttribInUse(0)            ? VertexAttrib0Status::EmulatedInitializedArray
                                                         : VertexAttrib0Status::EmulatedUninitializedArray;
 }
 
 bool
 WebGLContext::DoFakeVertexAttrib0(GLuint vertexCount)
 {
     int whatDoesAttrib0Need = WhatDoesVertexAttrib0Need();
@@ -908,23 +908,28 @@ WebGLContext::DoFakeVertexAttrib0(GLuint
 void
 WebGLContext::UndoFakeVertexAttrib0()
 {
     int whatDoesAttrib0Need = WhatDoesVertexAttrib0Need();
 
     if (whatDoesAttrib0Need == VertexAttrib0Status::Default)
         return;
 
-    gl->fBindBuffer(LOCAL_GL_ARRAY_BUFFER, mBoundVertexArray->mAttribBuffers[0].buf ? mBoundVertexArray->mAttribBuffers[0].buf->GLName() : 0);
-    gl->fVertexAttribPointer(0,
-                             mBoundVertexArray->mAttribBuffers[0].size,
-                             mBoundVertexArray->mAttribBuffers[0].type,
-                             mBoundVertexArray->mAttribBuffers[0].normalized,
-                             mBoundVertexArray->mAttribBuffers[0].stride,
-                             reinterpret_cast<const GLvoid *>(mBoundVertexArray->mAttribBuffers[0].byteOffset));
+    if (mBoundVertexArray->HasAttrib(0) && mBoundVertexArray->mAttribs[0].buf) {
+        const WebGLVertexAttribData& attrib0 = mBoundVertexArray->mAttribs[0];
+        gl->fBindBuffer(LOCAL_GL_ARRAY_BUFFER, attrib0.buf->GLName());
+        gl->fVertexAttribPointer(0,
+                                 attrib0.size,
+                                 attrib0.type,
+                                 attrib0.normalized,
+                                 attrib0.stride,
+                                 reinterpret_cast<const GLvoid *>(attrib0.byteOffset));
+    } else {
+        gl->fBindBuffer(LOCAL_GL_ARRAY_BUFFER, 0);
+    }
 
     gl->fBindBuffer(LOCAL_GL_ARRAY_BUFFER, mBoundArrayBuffer ? mBoundArrayBuffer->GLName() : 0);
 }
 
 bool
 WebGLContext::NeedFakeBlack()
 {
     // handle this case first, it's the generic case
@@ -1930,28 +1935,28 @@ WebGLContext::IsTexture(WebGLTexture *te
     return ValidateObjectAllowDeleted("isTexture", tex) &&
         !tex->IsDeleted() &&
         tex->HasEverBeenBound();
 }
 
 // Try to bind an attribute that is an array to location 0:
 bool WebGLContext::BindArrayAttribToLocation0(WebGLProgram *program)
 {
-    if (mBoundVertexArray->mAttribBuffers[0].enabled) {
+    if (mBoundVertexArray->IsAttribArrayEnabled(0)) {
         return false;
     }
 
     GLint leastArrayLocation = -1;
 
     std::map<GLint, nsCString>::iterator itr;
     for (itr = program->mActiveAttribMap.begin();
          itr != program->mActiveAttribMap.end();
          itr++) {
         int32_t index = itr->first;
-        if (mBoundVertexArray->mAttribBuffers[index].enabled &&
+        if (mBoundVertexArray->IsAttribArrayEnabled(index) &&
             index < leastArrayLocation)
         {
             leastArrayLocation = index;
         }
     }
 
     if (leastArrayLocation > 0) {
         nsCString& attrName = program->mActiveAttribMap.find(leastArrayLocation)->second;
@@ -2283,17 +2288,17 @@ WebGLContext::ReadPixels(GLint x, GLint 
             subrect_width > width || subrect_height > height)
             return ErrorInvalidOperation("readPixels: integer overflow computing clipped rect size");
 
         // now we know that subrect_width is in the [0..width] interval, and same for heights.
 
         // now, same computation as above to find the size of the intermediate buffer to allocate for the subrect
         // no need to check again for integer overflow here, since we already know the sizes aren't greater than before
         uint32_t subrect_plainRowSize = subrect_width * bytesPerPixel;
-	// There are checks above to ensure that this doesn't overflow.
+    // There are checks above to ensure that this doesn't overflow.
         uint32_t subrect_alignedRowSize = 
             RoundedToNextMultipleOf(subrect_plainRowSize, mPixelStorePackAlignment).value();
         uint32_t subrect_byteLength = (subrect_height-1)*subrect_alignedRowSize + subrect_plainRowSize;
 
         // create subrect buffer, call glReadPixels, copy pixels into destination buffer, delete subrect buffer
         GLubyte *subrect_data = new GLubyte[subrect_byteLength];
         gl->fReadPixels(subrect_x, subrect_y, subrect_width, subrect_height, format, type, subrect_data);
 
--- a/content/canvas/src/WebGLContextValidate.cpp
+++ b/content/canvas/src/WebGLContextValidate.cpp
@@ -761,17 +761,17 @@ WebGLContext::ValidateUniformSetter(cons
     if (!ValidateUniformLocation(name, location_object))
         return false;
     location = location_object->Location();
     return true;
 }
 
 bool WebGLContext::ValidateAttribIndex(GLuint index, const char *info)
 {
-    return mBoundVertexArray->EnsureAttribIndex(index, info);
+    return mBoundVertexArray->EnsureAttrib(index, info);
 }
 
 bool WebGLContext::ValidateStencilParamsForDrawCall()
 {
   const char *msg = "%s set different front and back stencil %s. Drawing in this configuration is not allowed.";
   if (mStencilRefFront != mStencilRefBack) {
       ErrorInvalidOperation(msg, "stencilFuncSeparate", "reference values");
       return false;
@@ -993,13 +993,13 @@ WebGLContext::InitAndValidateGL()
         = mozilla::services::GetObserverService();
     if (observerService) {
         observerService->AddObserver(mMemoryPressureObserver,
                                      "memory-pressure",
                                      false);
     }
 
     mDefaultVertexArray = new WebGLVertexArray(this);
-    mDefaultVertexArray->mAttribBuffers.SetLength(mGLMaxVertexAttribs);
+    mDefaultVertexArray->mAttribs.SetLength(mGLMaxVertexAttribs);
     mBoundVertexArray = mDefaultVertexArray;
 
     return true;
 }
--- a/content/canvas/src/WebGLContextVertices.cpp
+++ b/content/canvas/src/WebGLContextVertices.cpp
@@ -191,17 +191,18 @@ WebGLContext::EnableVertexAttribArray(GL
 
     if (!ValidateAttribIndex(index, "enableVertexAttribArray"))
         return;
 
     MakeContextCurrent();
     InvalidateBufferFetching();
 
     gl->fEnableVertexAttribArray(index);
-    mBoundVertexArray->mAttribBuffers[index].enabled = true;
+    MOZ_ASSERT(mBoundVertexArray->HasAttrib(index)); // should have been validated earlier
+    mBoundVertexArray->mAttribs[index].enabled = true;
 }
 
 void
 WebGLContext::DisableVertexAttribArray(GLuint index)
 {
     if (IsContextLost())
         return;
 
@@ -209,49 +210,50 @@ WebGLContext::DisableVertexAttribArray(G
         return;
 
     MakeContextCurrent();
     InvalidateBufferFetching();
 
     if (index || gl->IsGLES2())
         gl->fDisableVertexAttribArray(index);
 
-    mBoundVertexArray->mAttribBuffers[index].enabled = false;
+    MOZ_ASSERT(mBoundVertexArray->HasAttrib(index)); // should have been validated earlier
+    mBoundVertexArray->mAttribs[index].enabled = false;
 }
 
 
 JS::Value
 WebGLContext::GetVertexAttrib(JSContext* cx, GLuint index, GLenum pname,
                               ErrorResult& rv)
 {
     if (IsContextLost())
         return JS::NullValue();
 
-    if (!mBoundVertexArray->EnsureAttribIndex(index, "getVertexAttrib"))
+    if (!ValidateAttribIndex(index, "getVertexAttrib"))
         return JS::NullValue();
 
     MakeContextCurrent();
 
     switch (pname) {
         case LOCAL_GL_VERTEX_ATTRIB_ARRAY_BUFFER_BINDING:
         {
-            return WebGLObjectAsJSValue(cx, mBoundVertexArray->mAttribBuffers[index].buf.get(), rv);
+            return WebGLObjectAsJSValue(cx, mBoundVertexArray->mAttribs[index].buf.get(), rv);
         }
 
         case LOCAL_GL_VERTEX_ATTRIB_ARRAY_STRIDE:
         {
-            return JS::Int32Value(mBoundVertexArray->mAttribBuffers[index].stride);
+            return JS::Int32Value(mBoundVertexArray->mAttribs[index].stride);
         }
 
         case LOCAL_GL_VERTEX_ATTRIB_ARRAY_SIZE:
         {
             if (!ValidateAttribIndex(index, "getVertexAttrib"))
                 return JS::NullValue();
             
-            if (!mBoundVertexArray->mAttribBuffers[index].enabled)
+            if (!mBoundVertexArray->mAttribs[index].enabled)
                 return JS::Int32Value(4);
             
             // Don't break; fall through.
         }
         case LOCAL_GL_VERTEX_ATTRIB_ARRAY_TYPE:
         {
             GLint i = 0;
             gl->fGetVertexAttribiv(index, pname, &i);
@@ -260,17 +262,17 @@ WebGLContext::GetVertexAttrib(JSContext*
             MOZ_ASSERT(pname == LOCAL_GL_VERTEX_ATTRIB_ARRAY_TYPE);
             return JS::NumberValue(uint32_t(i));
         }
 
         case LOCAL_GL_VERTEX_ATTRIB_ARRAY_DIVISOR:
         {
             if (IsExtensionEnabled(ANGLE_instanced_arrays))
             {
-                return JS::Int32Value(mBoundVertexArray->mAttribBuffers[index].divisor);
+                return JS::Int32Value(mBoundVertexArray->mAttribs[index].divisor);
             }
             break;
         }
 
         case LOCAL_GL_CURRENT_VERTEX_ATTRIB:
         {
             GLfloat vec[4] = {0, 0, 0, 1};
             if (index) {
@@ -285,22 +287,22 @@ WebGLContext::GetVertexAttrib(JSContext*
             if (!obj) {
                 rv.Throw(NS_ERROR_OUT_OF_MEMORY);
             }
             return JS::ObjectOrNullValue(obj);
         }
 
         case LOCAL_GL_VERTEX_ATTRIB_ARRAY_ENABLED:
         {
-            return JS::BooleanValue(mBoundVertexArray->mAttribBuffers[index].enabled);
+            return JS::BooleanValue(mBoundVertexArray->mAttribs[index].enabled);
         }
 
         case LOCAL_GL_VERTEX_ATTRIB_ARRAY_NORMALIZED:
         {
-            return JS::BooleanValue(mBoundVertexArray->mAttribBuffers[index].normalized);
+            return JS::BooleanValue(mBoundVertexArray->mAttribs[index].normalized);
         }
 
         default:
             break;
     }
 
     ErrorInvalidEnumInfo("getVertexAttrib: parameter", pname);
 
@@ -316,17 +318,17 @@ WebGLContext::GetVertexAttribOffset(GLui
     if (!ValidateAttribIndex(index, "getVertexAttribOffset"))
         return 0;
 
     if (pname != LOCAL_GL_VERTEX_ATTRIB_ARRAY_POINTER) {
         ErrorInvalidEnum("getVertexAttribOffset: bad parameter");
         return 0;
     }
 
-    return mBoundVertexArray->mAttribBuffers[index].byteOffset;
+    return mBoundVertexArray->mAttribs[index].byteOffset;
 }
 
 void
 WebGLContext::VertexAttribPointer(GLuint index, GLint size, GLenum type,
                                   WebGLboolean normalized, GLsizei stride,
                                   WebGLintptr byteOffset)
 {
     if (IsContextLost())
@@ -351,17 +353,17 @@ WebGLContext::VertexAttribPointer(GLuint
             break;
         default:
             return ErrorInvalidEnumInfo("vertexAttribPointer: type", type);
     }
 
     // requiredAlignment should always be a power of two.
     GLsizei requiredAlignmentMask = requiredAlignment - 1;
 
-    if ( !mBoundVertexArray->EnsureAttribIndex(index, "vertexAttribPointer") ) {
+    if (!ValidateAttribIndex(index, "vertexAttribPointer")) {
         return;
     }
 
     if (size < 1 || size > 4)
         return ErrorInvalidValue("vertexAttribPointer: invalid element size");
 
     if (stride < 0 || stride > 255) // see WebGL spec section 6.6 "Vertex Attribute Data Stride"
         return ErrorInvalidValue("vertexAttribPointer: negative or too large stride");
@@ -382,17 +384,17 @@ WebGLContext::VertexAttribPointer(GLuint
 
     InvalidateBufferFetching();
 
     /* XXX make work with bufferSubData & heterogeneous types
      if (type != mBoundArrayBuffer->GLType())
      return ErrorInvalidOperation("vertexAttribPointer: type must match bound VBO type: %d != %d", type, mBoundArrayBuffer->GLType());
      */
 
-    WebGLVertexAttribData &vd = mBoundVertexArray->mAttribBuffers[index];
+    WebGLVertexAttribData &vd = mBoundVertexArray->mAttribs[index];
 
     vd.buf = mBoundArrayBuffer;
     vd.stride = stride;
     vd.size = size;
     vd.byteOffset = byteOffset;
     vd.type = type;
     vd.normalized = normalized;
 
@@ -404,21 +406,21 @@ WebGLContext::VertexAttribPointer(GLuint
 }
 
 void
 WebGLContext::VertexAttribDivisor(GLuint index, GLuint divisor)
 {
     if (IsContextLost())
         return;
 
-    if ( !mBoundVertexArray->EnsureAttribIndex(index, "vertexAttribDivisor") ) {
+    if (!ValidateAttribIndex(index, "vertexAttribDivisor")) {
         return;
     }
 
-    WebGLVertexAttribData& vd = mBoundVertexArray->mAttribBuffers[index];
+    WebGLVertexAttribData& vd = mBoundVertexArray->mAttribs[index];
     vd.divisor = divisor;
 
     InvalidateBufferFetching();
 
     MakeContextCurrent();
 
     gl->fVertexAttribDivisor(index, divisor);
 }
@@ -741,20 +743,20 @@ WebGLContext::ValidateBufferFetching(con
 
     if (mBufferFetchingIsVerified) {
         return true;
     }
 
     bool hasPerVertex = false;
     uint32_t maxVertices = UINT32_MAX;
     uint32_t maxInstances = UINT32_MAX;
-    uint32_t attribs = mBoundVertexArray->mAttribBuffers.Length();
+    uint32_t attribs = mBoundVertexArray->mAttribs.Length();
 
     for (uint32_t i = 0; i < attribs; ++i) {
-        const WebGLVertexAttribData& vd = mBoundVertexArray->mAttribBuffers[i];
+        const WebGLVertexAttribData& vd = mBoundVertexArray->mAttribs[i];
 
         // If the attrib array isn't enabled, there's nothing to check;
         // it's a static value.
         if (!vd.enabled)
             continue;
 
         if (vd.buf == nullptr) {
             ErrorInvalidOperation("%s: no VBO bound to enabled vertex attrib index %d!", info, i);
--- a/content/canvas/src/WebGLVertexArray.cpp
+++ b/content/canvas/src/WebGLVertexArray.cpp
@@ -29,36 +29,36 @@ void WebGLVertexArray::Delete() {
         mBoundElementArrayBuffer = nullptr;
 
         mContext->MakeContextCurrent();
         mContext->gl->fDeleteVertexArrays(1, &mGLName);
         LinkedListElement<WebGLVertexArray>::removeFrom(mContext->mVertexArrays);
     }
 
     mBoundElementArrayBuffer = nullptr;
-    mAttribBuffers.Clear();
+    mAttribs.Clear();
 }
 
-bool WebGLVertexArray::EnsureAttribIndex(GLuint index, const char *info)
+bool WebGLVertexArray::EnsureAttrib(GLuint index, const char *info)
 {
     if (index >= GLuint(mContext->mGLMaxVertexAttribs)) {
         if (index == GLuint(-1)) {
             mContext->ErrorInvalidValue("%s: index -1 is invalid. That probably comes from a getAttribLocation() call, "
                                         "where this return value -1 means that the passed name didn't correspond to an active attribute in "
                                         "the specified program.", info);
         } else {
             mContext->ErrorInvalidValue("%s: index %d is out of range", info, index);
         }
         return false;
     }
-    else if (index >= mAttribBuffers.Length()) {
-        mAttribBuffers.SetLength(index + 1);
+    else if (index >= mAttribs.Length()) {
+        mAttribs.SetLength(index + 1);
     }
 
     return true;
 }
 
 NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_2(WebGLVertexArray,
-  mAttribBuffers,
+  mAttribs,
   mBoundElementArrayBuffer)
 
 NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(WebGLVertexArray, AddRef)
 NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(WebGLVertexArray, Release)
--- a/content/canvas/src/WebGLVertexArray.h
+++ b/content/canvas/src/WebGLVertexArray.h
@@ -54,29 +54,35 @@ public:
 
     // -------------------------------------------------------------------------
     // MEMBER FUNCTIONS
 
     bool HasEverBeenBound() { return mHasEverBeenBound; }
     void SetHasEverBeenBound(bool x) { mHasEverBeenBound = x; }
     GLuint GLName() const { return mGLName; }
 
-    bool EnsureAttribIndex(GLuint index, const char *info);
+    bool EnsureAttrib(GLuint index, const char *info);
+    bool HasAttrib(GLuint index) {
+        return index < mAttribs.Length();
+    }
+    bool IsAttribArrayEnabled(GLuint index) {
+        return HasAttrib(index) && mAttribs[index].enabled;
+    }
 
 
 // -----------------------------------------------------------------------------
 // PRIVATE
 private:
 
     // -------------------------------------------------------------------------
     // MEMBERS
 
     GLuint mGLName;
     bool mHasEverBeenBound;
-    nsTArray<WebGLVertexAttribData> mAttribBuffers;
+    nsTArray<WebGLVertexAttribData> mAttribs;
     WebGLRefPtr<WebGLBuffer> mBoundElementArrayBuffer;
 
 
     // -------------------------------------------------------------------------
     // FRIENDSHIPS
 
     friend class WebGLContext;
 };