Auto merge of #10224 - emilio:shader-type-validations, r=jdm

webgl: Add attribute validations and other nits

Fixes https://github.com/servo/servo/issues/9958

Depends on a bunch of prs, and needs a test.

r? @jdm

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

3 files changed, 37 insertions, 12 deletions

diff --git a/components/script/dom/bindings/trace.rs b/components/script/dom/bindings/trace.rs
index 9cc2d13fb77..16138b33641 100644
--- a/components/script/dom/bindings/trace.rs
+++ b/components/script/dom/bindings/trace.rs
@@ -62,6 +62,7 @@ use net_traits::image::base::{Image, ImageMetadata};
 use net_traits::image_cache_thread::{ImageCacheChan, ImageCacheThread};
 use net_traits::response::HttpsState;
 use net_traits::storage_thread::StorageType;
+use offscreen_gl_context::GLLimits;
 use profile_traits::mem::ProfilerChan as MemProfilerChan;
 use profile_traits::time::ProfilerChan as TimeProfilerChan;
 use script_runtime::ScriptChan;
@@ -308,7 +309,7 @@ no_jsmanaged_fields!(StorageType);
 no_jsmanaged_fields!(CanvasGradientStop, LinearGradientStyle, RadialGradientStyle);
 no_jsmanaged_fields!(LineCapStyle, LineJoinStyle, CompositionOrBlending);
 no_jsmanaged_fields!(RepetitionStyle);
-no_jsmanaged_fields!(WebGLError);
+no_jsmanaged_fields!(WebGLError, GLLimits);
 no_jsmanaged_fields!(TimeProfilerChan);
 no_jsmanaged_fields!(MemProfilerChan);
 no_jsmanaged_fields!(PseudoElement);
diff --git a/components/script/dom/webglrenderingcontext.rs b/components/script/dom/webglrenderingcontext.rs
index 920fd91bbad..ac75e84efc8 100644
--- a/components/script/dom/webglrenderingcontext.rs
+++ b/components/script/dom/webglrenderingcontext.rs
@@ -31,7 +31,7 @@ use js::jsapi::{JSContext, JSObject, RootedValue};
 use js::jsval::{BooleanValue, DoubleValue, Int32Value, JSVal, NullValue, UndefinedValue};
 use net_traits::image::base::PixelFormat;
 use net_traits::image_cache_thread::ImageResponse;
-use offscreen_gl_context::GLContextAttributes;
+use offscreen_gl_context::{GLContextAttributes, GLLimits};
 use script_traits::ScriptMsg as ConstellationMsg;
 use std::cell::Cell;
 use util::str::DOMString;
@@ -71,6 +71,8 @@ pub struct WebGLRenderingContext {
     reflector_: Reflector,
     #[ignore_heap_size_of = "Defined in ipc-channel"]
     ipc_renderer: IpcSender<CanvasMsg>,
+    #[ignore_heap_size_of = "Defined in offscreen_gl_context"]
+    limits: GLLimits,
     canvas: JS<HTMLCanvasElement>,
     #[ignore_heap_size_of = "Defined in webrender_traits"]
     last_error: Cell<Option<WebGLError>>,
@@ -95,10 +97,11 @@ impl WebGLRenderingContext {
                           .unwrap();
         let result = receiver.recv().unwrap();
 
-        result.map(|ipc_renderer| {
+        result.map(|(ipc_renderer, context_limits)| {
             WebGLRenderingContext {
                 reflector_: Reflector::new(),
                 ipc_renderer: ipc_renderer,
+                limits: context_limits,
                 canvas: JS::from_ref(canvas),
                 last_error: Cell::new(None),
                 texture_unpacking_settings: Cell::new(CONVERT_COLORSPACE),
@@ -139,6 +142,9 @@ impl WebGLRenderingContext {
     }
 
     pub fn webgl_error(&self, err: WebGLError) {
+        // TODO(emilio): Add useful debug messages to this
+        warn!("WebGL error: {:?}, previous error was {:?}", err, self.last_error.get());
+
         // If an error has been detected no further errors must be
         // recorded until `getError` has been called
         if self.last_error.get().is_none() {
@@ -155,7 +161,7 @@ impl WebGLRenderingContext {
         if let Some(texture) = texture {
             handle_potential_webgl_error!(self, texture.tex_parameter(target, name, value));
         } else {
-            return self.webgl_error(InvalidOperation);
+            self.webgl_error(InvalidOperation)
         }
     }
 
@@ -164,6 +170,10 @@ impl WebGLRenderingContext {
     }
 
     fn vertex_attrib(&self, indx: u32, x: f32, y: f32, z: f32, w: f32) {
+        if indx > self.limits.max_vertex_attribs {
+            return self.webgl_error(InvalidValue);
+        }
+
         self.ipc_renderer
             .send(CanvasMsg::WebGL(WebGLCommand::VertexAttrib(indx, x, y, z, w)))
             .unwrap();
@@ -680,6 +690,13 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
 
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.9
     fn CreateShader(&self, shader_type: u32) -> Option<Root<WebGLShader>> {
+        match shader_type {
+            constants::VERTEX_SHADER | constants::FRAGMENT_SHADER => {},
+            _ => {
+                self.webgl_error(InvalidEnum);
+                return None;
+            }
+        }
         WebGLShader::maybe_new(self.global().r(), self.ipc_renderer.clone(), shader_type)
     }
 
@@ -737,13 +754,13 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
                 }
 
                 if first < 0 || count < 0 {
-                    self.webgl_error(InvalidValue);
-                } else {
-                    self.ipc_renderer
-                        .send(CanvasMsg::WebGL(WebGLCommand::DrawArrays(mode, first, count)))
-                        .unwrap();
-                    self.mark_as_dirty();
+                    return self.webgl_error(InvalidValue);
                 }
+
+                self.ipc_renderer
+                    .send(CanvasMsg::WebGL(WebGLCommand::DrawArrays(mode, first, count)))
+                    .unwrap();
+                self.mark_as_dirty();
             },
             _ => self.webgl_error(InvalidEnum),
         }
@@ -790,6 +807,10 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
 
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.10
     fn EnableVertexAttribArray(&self, attrib_id: u32) {
+        if attrib_id > self.limits.max_vertex_attribs {
+            return self.webgl_error(InvalidValue);
+        }
+
         self.ipc_renderer
             .send(CanvasMsg::WebGL(WebGLCommand::EnableVertexAttribArray(attrib_id)))
             .unwrap()
@@ -1188,7 +1209,6 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
         self.vertex_attrib(indx, x, 0f32, 0f32, 1f32)
     }
 
-    #[allow(unsafe_code)]
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.10
     fn VertexAttrib1fv(&self, _cx: *mut JSContext, indx: u32, data: *mut JSObject) {
         if let Some(data_vec) = array_buffer_view_to_vec_checked::<f32>(data) {
@@ -1206,7 +1226,6 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
         self.vertex_attrib(indx, x, y, 0f32, 1f32)
     }
 
-    #[allow(unsafe_code)]
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.10
     fn VertexAttrib2fv(&self, _cx: *mut JSContext, indx: u32, data: *mut JSObject) {
         if let Some(data_vec) = array_buffer_view_to_vec_checked::<f32>(data) {
@@ -1257,6 +1276,10 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.10
     fn VertexAttribPointer(&self, attrib_id: u32, size: i32, data_type: u32,
                            normalized: bool, stride: i32, offset: i64) {
+        if attrib_id > self.limits.max_vertex_attribs {
+            return self.webgl_error(InvalidValue);
+        }
+
         if let constants::FLOAT = data_type {
            let msg = CanvasMsg::WebGL(
                WebGLCommand::VertexAttribPointer2f(attrib_id, size, normalized, stride, offset as u32));
diff --git a/components/script/dom/webglshader.rs b/components/script/dom/webglshader.rs
index 804dc3cdcf8..495417065d7 100644
--- a/components/script/dom/webglshader.rs
+++ b/components/script/dom/webglshader.rs
@@ -101,6 +101,7 @@ impl WebGLShader {
                                                        &BuiltInResources::default()).unwrap();
             match validator.compile_and_translate(&[source]) {
                 Ok(translated_source) => {
+                    debug!("Shader translated: {}", translated_source);
                     // NOTE: At this point we should be pretty sure that the compilation in the paint thread
                     // will succeed.
                     // It could be interesting to retrieve the info log from the paint thread though