Auto merge of #21072 - servo:webgl, r=emilio

Implement checks for vertex attribs enabled as arrays without a bound buffer

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21072)
<!-- Reviewable:end -->

9 files changed, 94 insertions, 114 deletions

diff --git a/components/script/dom/webgl_extensions/ext/oesvertexarrayobject.rs b/components/script/dom/webgl_extensions/ext/oesvertexarrayobject.rs
index f80cd7385e7..a6e8739862e 100644
--- a/components/script/dom/webgl_extensions/ext/oesvertexarrayobject.rs
+++ b/components/script/dom/webgl_extensions/ext/oesvertexarrayobject.rs
@@ -13,7 +13,6 @@ use dom_struct::dom_struct;
 use js::conversions::ToJSValConvertible;
 use js::jsapi::JSContext;
 use js::jsval::{JSVal, NullValue};
-use std::iter;
 use super::{WebGLExtension, WebGLExtensions, WebGLExtensionSpec};
 
 #[dom_struct]
@@ -70,9 +69,10 @@ impl OESVertexArrayObjectMethods for OESVertexArrayObject {
             }
 
             // Remove VAO references from buffers
-            let buffers = vao.bound_attrib_buffers();
-            for buffer in buffers {
-                buffer.remove_vao_reference(vao.id());
+            for (_, &(_, ref buffer)) in vao.bound_attrib_buffers().borrow().iter() {
+                if let Some(ref buffer) = *buffer {
+                    buffer.remove_vao_reference(vao.id());
+                }
             }
             if let Some(buffer) = vao.bound_buffer_element_array() {
                 buffer.remove_vao_reference(vao.id());
@@ -94,11 +94,12 @@ impl OESVertexArrayObjectMethods for OESVertexArrayObject {
     fn BindVertexArrayOES(&self, vao: Option<&WebGLVertexArrayObjectOES>) {
         if let Some(bound_vao) = self.bound_vao.get() {
             // Store buffers attached to attrib pointers
-            let buffers = self.ctx.borrow_bound_attrib_buffers();
-            bound_vao.set_bound_attrib_buffers(buffers.iter().map(|(key, buffer)| {
-                (*buffer).add_vao_reference(bound_vao.id());
-                (*key, &**buffer)
-            }));
+            bound_vao.bound_attrib_buffers().set_from(&self.ctx.bound_attrib_buffers());
+            for (_, (_, ref buffer)) in bound_vao.bound_attrib_buffers().borrow().iter() {
+                if let Some(ref buffer) = *buffer {
+                    buffer.add_vao_reference(bound_vao.id());
+                }
+            }
             // Store element array buffer
             let element_array = self.ctx.bound_buffer_element_array();
             bound_vao.set_bound_buffer_element_array(element_array.as_ref().map(|buffer| {
@@ -118,14 +119,13 @@ impl OESVertexArrayObjectMethods for OESVertexArrayObject {
             self.bound_vao.set(Some(&vao));
 
             // Restore WebGLRenderingContext current bindings
-            let buffers = vao.borrow_bound_attrib_buffers();
-            self.ctx.set_bound_attrib_buffers(buffers.iter().map(|(k, v)| (*k, &**v)));
+            self.ctx.bound_attrib_buffers().set_from(&vao.bound_attrib_buffers());
             let element_array = vao.bound_buffer_element_array();
             self.ctx.set_bound_buffer_element_array(element_array.as_ref().map(|buffer| &**buffer));
         } else {
             self.ctx.send_command(WebGLCommand::BindVertexArray(None));
             self.bound_vao.set(None);
-            self.ctx.set_bound_attrib_buffers(iter::empty());
+            self.ctx.bound_attrib_buffers().clear();
         }
     }
 }
diff --git a/components/script/dom/webglrenderingcontext.rs b/components/script/dom/webglrenderingcontext.rs
index 60474753eb3..d712b8cc58a 100644
--- a/components/script/dom/webglrenderingcontext.rs
+++ b/components/script/dom/webglrenderingcontext.rs
@@ -57,11 +57,11 @@ use js::typedarray::ArrayBufferView;
 use net_traits::image::base::PixelFormat;
 use net_traits::image_cache::ImageResponse;
 use offscreen_gl_context::{GLContextAttributes, GLLimits};
+use ref_filter_map::ref_filter_map;
 use script_layout_interface::HTMLCanvasDataSource;
 use servo_config::prefs::PREFS;
 use std::cell::{Cell, Ref};
 use std::cmp;
-use std::iter::FromIterator;
 use std::ptr::NonNull;
 use webrender_api;
 
@@ -192,7 +192,7 @@ pub struct WebGLRenderingContext {
     bound_texture_unit: Cell<u32>,
     bound_buffer_array: MutNullableDom<WebGLBuffer>,
     bound_buffer_element_array: MutNullableDom<WebGLBuffer>,
-    bound_attrib_buffers: DomRefCell<FnvHashMap<u32, Dom<WebGLBuffer>>>,
+    bound_attrib_buffers: BoundAttribBuffers,
     current_program: MutNullableDom<WebGLProgram>,
     #[ignore_malloc_size_of = "Because it's small"]
     current_vertex_attrib_0: Cell<(f32, f32, f32, f32)>,
@@ -243,7 +243,7 @@ impl WebGLRenderingContext {
                 bound_texture_unit: Cell::new(constants::TEXTURE0),
                 bound_buffer_array: MutNullableDom::new(None),
                 bound_buffer_element_array: MutNullableDom::new(None),
-                bound_attrib_buffers: DomRefCell::new(Default::default()),
+                bound_attrib_buffers: Default::default(),
                 bound_renderbuffer: MutNullableDom::new(None),
                 current_program: MutNullableDom::new(None),
                 current_vertex_attrib_0: Cell::new((0f32, 0f32, 0f32, 1f32)),
@@ -311,12 +311,8 @@ impl WebGLRenderingContext {
         })
     }
 
-    pub fn borrow_bound_attrib_buffers(&self) -> Ref<FnvHashMap<u32, Dom<WebGLBuffer>>> {
-        self.bound_attrib_buffers.borrow()
-    }
-
-    pub fn set_bound_attrib_buffers<'a, T>(&self, iter: T) where T: Iterator<Item=(u32, &'a WebGLBuffer)> {
-        *self.bound_attrib_buffers.borrow_mut() = FnvHashMap::from_iter(iter.map(|(k,v)| (k, Dom::from_ref(v))));
+    pub fn bound_attrib_buffers(&self) -> &BoundAttribBuffers {
+        &self.bound_attrib_buffers
     }
 
     pub fn bound_buffer_element_array(&self) -> Option<DomRoot<WebGLBuffer>> {
@@ -2079,13 +2075,7 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
             }
 
             // Remove deleted buffer from bound attrib buffers.
-            let attrib_ids: Vec<_> = self.bound_attrib_buffers.borrow().iter()
-                                                              .filter(|&(_, v)| v.id() == buffer.id())
-                                                              .map(|(&k, _)| k)
-                                                              .collect();
-            for id in attrib_ids {
-                self.bound_attrib_buffers.borrow_mut().remove(&id);
-            }
+            self.bound_attrib_buffers.remove_buffer(buffer);
 
             // Delete buffer.
             handle_object_deletion!(self, self.bound_buffer_array, buffer,
@@ -2211,6 +2201,15 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
                 return self.webgl_error(InvalidOperation);
             }
         }
+
+        {
+            // https://www.khronos.org/registry/webgl/specs/latest/1.0/#6.2
+            let buffers = self.bound_attrib_buffers.borrow();
+            if buffers.iter().any(|(_, &(enabled, ref buffer))| enabled && buffer.is_none()) {
+                return self.webgl_error(InvalidOperation);
+            }
+        }
+
         if !self.validate_framebuffer_complete() {
             return;
         }
@@ -2278,6 +2277,14 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
             }
         }
 
+        {
+            // https://www.khronos.org/registry/webgl/specs/latest/1.0/#6.2
+            let buffers = self.bound_attrib_buffers.borrow();
+            if buffers.iter().any(|(_, &(enabled, ref buffer))| enabled && buffer.is_none()) {
+                return self.webgl_error(InvalidOperation);
+            }
+        }
+
         if !self.validate_framebuffer_complete() {
             return;
         }
@@ -2292,6 +2299,7 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
             return self.webgl_error(InvalidValue);
         }
 
+        self.bound_attrib_buffers.enabled(attrib_id, true);
         self.send_command(WebGLCommand::EnableVertexAttribArray(attrib_id));
     }
 
@@ -2301,6 +2309,7 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
             return self.webgl_error(InvalidValue);
         }
 
+        self.bound_attrib_buffers.enabled(attrib_id, false);
         self.send_command(WebGLCommand::DisableVertexAttribArray(attrib_id));
     }
 
@@ -2592,7 +2601,7 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
 
         if param == constants::VERTEX_ATTRIB_ARRAY_BUFFER_BINDING {
             rooted!(in(cx) let mut jsval = NullValue());
-            if let Some(buffer) =  self.bound_attrib_buffers.borrow().get(&index) {
+            if let Some(buffer) = self.bound_attrib_buffers.get(index) {
                 buffer.to_jsval(cx, jsval.handle_mut());
             }
             return jsval.get();
@@ -3358,7 +3367,7 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
 
         }
 
-        self.bound_attrib_buffers.borrow_mut().insert(attrib_id, Dom::from_ref(&*buffer_array));
+        self.bound_attrib_buffers.bind_buffer(attrib_id, &buffer_array);
 
         let msg = WebGLCommand::VertexAttribPointer(attrib_id, size, data_type, normalized, stride, offset as u32);
         self.send_command(msg);
@@ -3795,3 +3804,49 @@ impl UniformSetterType {
         }
     }
 }
+
+#[derive(Default, JSTraceable, MallocSizeOf)]
+#[must_root]
+pub struct BoundAttribBuffers {
+    elements: DomRefCell<FnvHashMap<u32, (bool, Option<Dom<WebGLBuffer>>)>>,
+}
+
+impl BoundAttribBuffers {
+    pub fn clear(&self) {
+        self.elements.borrow_mut().clear()
+    }
+
+    pub fn set_from(&self, other: &BoundAttribBuffers) {
+        *self.elements.borrow_mut() = other.elements.borrow().clone();
+    }
+
+    pub fn borrow(&self) -> Ref<FnvHashMap<u32, (bool, Option<Dom<WebGLBuffer>>)>> {
+        self.elements.borrow()
+    }
+
+    fn remove_buffer(&self, buffer: &WebGLBuffer) {
+        self.elements.borrow_mut().retain(|_, v| {
+            v.1.as_ref().map_or(true, |b| b.id() != buffer.id())
+        })
+    }
+
+    fn get(&self, index: u32) -> Option<Ref<WebGLBuffer>> {
+        ref_filter_map(self.elements.borrow(), |elements| {
+            elements.get(&index).and_then(|&(_, ref buffer)| {
+                buffer.as_ref().map(|b| &**b)
+            })
+        })
+    }
+
+    fn enabled(&self, index: u32, value: bool) {
+        let mut elements = self.elements.borrow_mut();
+        let pair = elements.entry(index).or_insert((false, None));
+        pair.0 = value;
+    }
+
+    fn bind_buffer(&self, index: u32, buffer: &WebGLBuffer) {
+        let mut elements = self.elements.borrow_mut();
+        let pair = elements.entry(index).or_insert((false, None));
+        pair.1 = Some(Dom::from_ref(buffer));
+    }
+}
diff --git a/components/script/dom/webglvertexarrayobjectoes.rs b/components/script/dom/webglvertexarrayobjectoes.rs
index 4ca8c7b7add..49babf3095d 100644
--- a/components/script/dom/webglvertexarrayobjectoes.rs
+++ b/components/script/dom/webglvertexarrayobjectoes.rs
@@ -3,17 +3,15 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use canvas_traits::webgl::WebGLVertexArrayId;
-use dom::bindings::cell::DomRefCell;
 use dom::bindings::codegen::Bindings::WebGLVertexArrayObjectOESBinding;
 use dom::bindings::reflector::reflect_dom_object;
-use dom::bindings::root::{Dom, DomRoot, MutNullableDom};
+use dom::bindings::root::{DomRoot, MutNullableDom};
 use dom::globalscope::GlobalScope;
 use dom::webglbuffer::WebGLBuffer;
 use dom::webglobject::WebGLObject;
+use dom::webglrenderingcontext::BoundAttribBuffers;
 use dom_struct::dom_struct;
-use std::cell::{Cell, Ref};
-use std::collections::HashMap;
-use std::iter::FromIterator;
+use std::cell::Cell;
 
 #[dom_struct]
 pub struct WebGLVertexArrayObjectOES {
@@ -21,7 +19,7 @@ pub struct WebGLVertexArrayObjectOES {
     id: WebGLVertexArrayId,
     ever_bound: Cell<bool>,
     is_deleted: Cell<bool>,
-    bound_attrib_buffers: DomRefCell<HashMap<u32, Dom<WebGLBuffer>>>,
+    bound_attrib_buffers: BoundAttribBuffers,
     bound_buffer_element_array: MutNullableDom<WebGLBuffer>,
 }
 
@@ -32,7 +30,7 @@ impl WebGLVertexArrayObjectOES {
             id: id,
             ever_bound: Cell::new(false),
             is_deleted: Cell::new(false),
-            bound_attrib_buffers: DomRefCell::new(HashMap::new()),
+            bound_attrib_buffers: Default::default(),
             bound_buffer_element_array: MutNullableDom::new(None),
         }
     }
@@ -43,6 +41,10 @@ impl WebGLVertexArrayObjectOES {
                            WebGLVertexArrayObjectOESBinding::Wrap)
     }
 
+    pub fn bound_attrib_buffers(&self) -> &BoundAttribBuffers {
+        &self.bound_attrib_buffers
+    }
+
     pub fn id(&self) -> WebGLVertexArrayId {
         self.id
     }
@@ -63,18 +65,6 @@ impl WebGLVertexArrayObjectOES {
         self.ever_bound.set(true);
     }
 
-    pub fn borrow_bound_attrib_buffers(&self) -> Ref<HashMap<u32, Dom<WebGLBuffer>>> {
-        self.bound_attrib_buffers.borrow()
-    }
-
-    pub fn bound_attrib_buffers(&self) -> Vec<DomRoot<WebGLBuffer>> {
-        self.bound_attrib_buffers.borrow().iter().map(|(_, b)| DomRoot::from_ref(&**b)).collect()
-    }
-
-    pub fn set_bound_attrib_buffers<'a, T>(&self, iter: T) where T: Iterator<Item=(u32, &'a WebGLBuffer)> {
-        *self.bound_attrib_buffers.borrow_mut() = HashMap::from_iter(iter.map(|(k,v)| (k, Dom::from_ref(v))));
-    }
-
     pub fn bound_buffer_element_array(&self) -> Option<DomRoot<WebGLBuffer>> {
         self.bound_buffer_element_array.get()
     }
diff --git a/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/buffers/index-validation-copies-indices.html.ini b/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/buffers/index-validation-copies-indices.html.ini
deleted file mode 100644
index 89582a558e6..00000000000
--- a/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/buffers/index-validation-copies-indices.html.ini
+++ /dev/null
@@ -1,14 +0,0 @@
-[index-validation-copies-indices.html]
-  type: testharness
-  [WebGL test #1: getError expected: INVALID_OPERATION. Was NO_ERROR : after evaluating: context.drawElements(context.TRIANGLE_STRIP, 4, context.UNSIGNED_SHORT, 0)]
-    expected: FAIL
-
-  [WebGL test #2: getError expected: INVALID_OPERATION. Was NO_ERROR : after evaluating: context.drawElements(context.TRIANGLE_STRIP, 4, context.UNSIGNED_SHORT, 4)]
-    expected: FAIL
-
-  [WebGL test #4: getError expected: INVALID_OPERATION. Was NO_ERROR : after evaluating: context.drawElements(context.TRIANGLE_STRIP, 4, context.UNSIGNED_SHORT, 0)]
-    expected: FAIL
-
-  [WebGL test #5: getError expected: INVALID_OPERATION. Was NO_ERROR : after evaluating: context.drawElements(context.TRIANGLE_STRIP, 4, context.UNSIGNED_SHORT, 4)]
-    expected: FAIL
-
diff --git a/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/buffers/index-validation-verifies-too-many-indices.html.ini b/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/buffers/index-validation-verifies-too-many-indices.html.ini
deleted file mode 100644
index 808b4986afc..00000000000
--- a/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/buffers/index-validation-verifies-too-many-indices.html.ini
+++ /dev/null
@@ -1,8 +0,0 @@
-[index-validation-verifies-too-many-indices.html]
-  type: testharness
-  [WebGL test #1: getError expected: INVALID_OPERATION. Was NO_ERROR : after evaluating: context.drawElements(context.TRIANGLE_STRIP, 4, context.UNSIGNED_SHORT, 0)]
-    expected: FAIL
-
-  [WebGL test #2: getError expected: INVALID_OPERATION. Was NO_ERROR : after evaluating: context.drawElements(context.TRIANGLE_STRIP, 4, context.UNSIGNED_SHORT, 4)]
-    expected: FAIL
-
diff --git a/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/buffers/index-validation.html.ini b/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/buffers/index-validation.html.ini
index a23bc4c3ebb..7c161d80677 100644
--- a/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/buffers/index-validation.html.ini
+++ b/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/buffers/index-validation.html.ini
@@ -1,8 +1,4 @@
 [index-validation.html]
-  type: testharness
-  [WebGL test #9: getError expected: INVALID_OPERATION. Was NO_ERROR : ]
-    expected: FAIL
-
   [WebGL test #12: getError expected: INVALID_OPERATION. Was NO_ERROR : ]
     expected: FAIL
 
diff --git a/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/extensions/oes-element-index-uint.html.ini b/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/extensions/oes-element-index-uint.html.ini
deleted file mode 100644
index f56e13215da..00000000000
--- a/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/extensions/oes-element-index-uint.html.ini
+++ /dev/null
@@ -1,7 +0,0 @@
-[oes-element-index-uint.html]
-  [WebGL test #20: getError expected: INVALID_OPERATION. Was NO_ERROR : ]
-    expected: FAIL
-
-  [WebGL test #64: getError expected: INVALID_OPERATION. Was NO_ERROR : ]
-    expected: FAIL
-
diff --git a/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/rendering/draw-elements-out-of-bounds.html.ini b/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/rendering/draw-elements-out-of-bounds.html.ini
deleted file mode 100644
index b31392cfec8..00000000000
--- a/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/rendering/draw-elements-out-of-bounds.html.ini
+++ /dev/null
@@ -1,20 +0,0 @@
-[draw-elements-out-of-bounds.html]
-  type: testharness
-  [WebGL test #19: getError expected: NO_ERROR. Was INVALID_OPERATION : after evaluating: gl.drawElements(gl.TRIANGLES, 0, gl.UNSIGNED_BYTE, 4)]
-    expected: FAIL
-
-  [WebGL test #25: getError expected: INVALID_OPERATION. Was NO_ERROR : after evaluating: gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_BYTE, 0)]
-    expected: FAIL
-
-  [WebGL test #31: getError expected: INVALID_OPERATION. Was NO_ERROR : after evaluating: gl.drawElements(gl.TRIANGLES, 12, gl.UNSIGNED_SHORT, 0)]
-    expected: FAIL
-
-  [WebGL test #32: getError expected: INVALID_OPERATION. Was NO_ERROR : after evaluating: gl.drawElements(gl.TRIANGLES, 15, gl.UNSIGNED_SHORT, 0)]
-    expected: FAIL
-
-  [WebGL test #33: getError expected: INVALID_OPERATION. Was NO_ERROR : after evaluating: gl.drawElements(gl.TRIANGLES, 18, gl.UNSIGNED_SHORT, 0)]
-    expected: FAIL
-
-  [WebGL test #34: getError expected: INVALID_OPERATION. Was NO_ERROR : after evaluating: gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_SHORT, 30)]
-    expected: FAIL
-
diff --git a/tests/wpt/mozilla/meta/webgl/conformance-2.0.0/conformance2/rendering/element-index-uint.html.ini b/tests/wpt/mozilla/meta/webgl/conformance-2.0.0/conformance2/rendering/element-index-uint.html.ini
index c7c6e8afc5d..98632c5c59b 100644
--- a/tests/wpt/mozilla/meta/webgl/conformance-2.0.0/conformance2/rendering/element-index-uint.html.ini
+++ b/tests/wpt/mozilla/meta/webgl/conformance-2.0.0/conformance2/rendering/element-index-uint.html.ini
@@ -1,16 +1,7 @@
 [element-index-uint.html]
-  [WebGL test #1: Draw 0 failed pixel test]
-    expected: FAIL
-
-  [WebGL test #2: successfullyParsed should be true (of type boolean). Was undefined (of type undefined).]
-    expected: FAIL
-
   [WebGL test #11: getError expected: INVALID_OPERATION. Was NO_ERROR : ]
     expected: FAIL
 
-  [WebGL test #14: getError expected: INVALID_OPERATION. Was NO_ERROR : ]
-    expected: FAIL
-
   [WebGL test #20: getError expected: INVALID_OPERATION. Was NO_ERROR : after evaluating: gl.drawElements(gl.TRIANGLE_STRIP, 4, gl.UNSIGNED_INT, 0)]
     expected: FAIL
 
@@ -32,9 +23,6 @@
   [WebGL test #49: getError expected: INVALID_OPERATION. Was NO_ERROR : ]
     expected: FAIL
 
-  [WebGL test #52: getError expected: INVALID_OPERATION. Was NO_ERROR : ]
-    expected: FAIL
-
   [WebGL test #58: getError expected: INVALID_OPERATION. Was NO_ERROR : after evaluating: gl.drawElements(gl.TRIANGLE_STRIP, 4, gl.UNSIGNED_INT, 0)]
     expected: FAIL