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 -->

components/canvas/webgl_paint_thread.rs

@@ -6,7 +6,7 @@ use canvas_traits::{CanvasCommonMsg, CanvasMsg, CanvasPixelData, CanvasData, Fro
 use euclid::size::Size2D;
 use gleam::gl;
 use ipc_channel::ipc::{self, IpcSender, IpcSharedMemory};
-use offscreen_gl_context::{ColorAttachmentType, GLContext, GLContextAttributes, NativeGLContext};
+use offscreen_gl_context::{ColorAttachmentType, GLContext, GLLimits, GLContextAttributes, NativeGLContext};
 use std::borrow::ToOwned;
 use std::sync::mpsc::channel;
 use util::thread::spawn_named;
@@ -26,20 +26,24 @@ pub struct WebGLPaintThread {
 impl WebGLPaintThread {
     fn new(size: Size2D<i32>,
            attrs: GLContextAttributes,
-           webrender_api_sender: Option<webrender_traits::RenderApiSender>) -> Result<WebGLPaintThread, String> {
+           webrender_api_sender: Option<webrender_traits::RenderApiSender>)
-        let data = if let Some(sender) = webrender_api_sender {
+        -> Result<(WebGLPaintThread, GLLimits), String> {
+        let (data, limits) = if let Some(sender) = webrender_api_sender {
             let webrender_api = sender.create_api();
-            let (id, _) = try!(webrender_api.request_webgl_context(&size, attrs));
+            let (id, limits) = try!(webrender_api.request_webgl_context(&size, attrs));
-            WebGLPaintTaskData::WebRender(webrender_api, id)
+            (WebGLPaintTaskData::WebRender(webrender_api, id), limits)
         } else {
             let context = try!(GLContext::<NativeGLContext>::new(size, attrs, ColorAttachmentType::Texture, None));
-            WebGLPaintTaskData::Servo(context)
+            let limits = context.borrow_limits().clone();
+            (WebGLPaintTaskData::Servo(context), limits)
         };
 
-        Ok(WebGLPaintThread {
+        let painter_object = WebGLPaintThread {
             size: size,
             data: data,
-        })
+        };
+
+        Ok((painter_object, limits))
     }
 
     pub fn handle_webgl_message(&self, message: webrender_traits::WebGLCommand) {
@@ -59,13 +63,13 @@ impl WebGLPaintThread {
     pub fn start(size: Size2D<i32>,
                  attrs: GLContextAttributes,
                  webrender_api_sender: Option<webrender_traits::RenderApiSender>)
-                 -> Result<IpcSender<CanvasMsg>, String> {
+                 -> Result<(IpcSender<CanvasMsg>, GLLimits), String> {
         let (sender, receiver) = ipc::channel::<CanvasMsg>().unwrap();
         let (result_chan, result_port) = channel();
         spawn_named("WebGLThread".to_owned(), move || {
             let mut painter = match WebGLPaintThread::new(size, attrs, webrender_api_sender) {
-                Ok(thread) => {
+                Ok((thread, limits)) => {
-                    result_chan.send(Ok(())).unwrap();
+                    result_chan.send(Ok(limits)).unwrap();
                     thread
                 },
                 Err(e) => {
@@ -95,8 +99,7 @@ impl WebGLPaintThread {
             }
         });
 
-        try!(result_port.recv().unwrap());
+        result_port.recv().unwrap().map(|limits| (sender, limits))
-        Ok(sender)
     }
 
     fn send_data(&mut self, chan: IpcSender<CanvasData>) {

components/compositing/constellation.rs

@@ -39,7 +39,7 @@ use msg::webdriver_msg;
 use net_traits::image_cache_thread::ImageCacheThread;
 use net_traits::storage_thread::{StorageThread, StorageThreadMsg};
 use net_traits::{self, ResourceThread};
-use offscreen_gl_context::GLContextAttributes;
+use offscreen_gl_context::{GLContextAttributes, GLLimits};
 use pipeline::{CompositionPipeline, InitialPipelineState, Pipeline, UnprivilegedPipelineContent};
 use profile_traits::mem;
 use profile_traits::time;
@@ -1349,11 +1349,11 @@ impl<LTF: LayoutThreadFactory, STF: ScriptThreadFactory> Constellation<LTF, STF>
             &mut self,
             size: &Size2D<i32>,
             attributes: GLContextAttributes,
-            response_sender: IpcSender<Result<IpcSender<CanvasMsg>, String>>) {
+            response_sender: IpcSender<Result<(IpcSender<CanvasMsg>, GLLimits), String>>) {
         let webrender_api = self.webrender_api_sender.clone();
-        let sender = WebGLPaintThread::start(*size, attributes, webrender_api);
+        let response = WebGLPaintThread::start(*size, attributes, webrender_api);
 
-        response_sender.send(sender)
+        response_sender.send(response)
             .unwrap_or_else(|e| debug!("Create WebGL paint thread response failed ({})", e))
     }
 

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);

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);
+                    return self.webgl_error(InvalidValue);
-                } else {
-                    self.ipc_renderer
-                        .send(CanvasMsg::WebGL(WebGLCommand::DrawArrays(mode, first, count)))
-                        .unwrap();
-                    self.mark_as_dirty();
                 }
+
+                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));

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

components/script_traits/script_msg.rs

@@ -14,7 +14,7 @@ use euclid::size::Size2D;
 use ipc_channel::ipc::IpcSender;
 use msg::constellation_msg::{Failure, NavigationDirection, PipelineId};
 use msg::constellation_msg::{LoadData, SubpageId};
-use offscreen_gl_context::GLContextAttributes;
+use offscreen_gl_context::{GLContextAttributes, GLLimits};
 use style_traits::cursor::Cursor;
 use style_traits::viewport::ViewportConstraints;
 use url::Url;
@@ -43,8 +43,8 @@ pub enum ScriptMsg {
     /// Requests that a new WebGL thread be created. (This is done in the constellation because
     /// WebGL uses the GPU and we don't want to give untrusted content access to the GPU.)
     CreateWebGLPaintThread(Size2D<i32>,
-                         GLContextAttributes,
+                           GLContextAttributes,
-                         IpcSender<Result<IpcSender<CanvasMsg>, String>>),
+                           IpcSender<Result<(IpcSender<CanvasMsg>, GLLimits), String>>),
     /// Dispatched after the DOM load event has fired on a document
     /// Causes a `load` event to be dispatched to any enclosing frame context element
     /// for the given pipeline.

tests/html/test_webgl_triangle.html

@@ -9,23 +9,25 @@
    <canvas id="canvas" width="512" height="512"></canvas>
 </div>
 <script id="vertexshader" type="x-shader">
-   attribute vec2 aVertexPosition;
+precision mediump float;
-   attribute vec4 aColour;
-   varying vec4 aVertexColor;
 
-   void main() {
+attribute vec2 aVertexPosition;
-      aVertexColor = aColour;
+attribute vec4 aColour;
-      gl_Position = vec4(aVertexPosition, 0.0, 1.0);
+varying vec4 aVertexColor;
-   }
+
+void main() {
+  aVertexColor = aColour;
+  gl_Position = vec4(aVertexPosition, 0.0, 1.0);
+}
 </script>
 <script id="fragmentshader" type="x-shader">
-   precision mediump float;
+precision mediump float;
 
-   varying vec4 aVertexColor;
+varying vec4 aVertexColor;
 
-   void main() {
+void main() {
-      gl_FragColor = aVertexColor;
+  gl_FragColor = aVertexColor;
-   }
+}
 </script>
 <script type="text/javascript">
 
@@ -44,6 +46,9 @@
    var v = document.getElementById("vertexshader").firstChild.nodeValue;
    var f = document.getElementById("fragmentshader").firstChild.nodeValue;
 
+   console.log("vertex source: ", v);
+   console.log("fragment source: ", f);
+
    var vs = gl.createShader(gl.VERTEX_SHADER);
    gl.shaderSource(vs, v);
    gl.compileShader(vs);
@@ -96,6 +101,7 @@
    gl.vertexAttribPointer(program.aVertexPosition, itemSize, gl.FLOAT, false, 0, 0);
 
    program.aColour = gl.getAttribLocation(program, "aColour");
+   console.log("aColour: " + program.aColour);
    gl.enableVertexAttribArray(program.aColour);
    gl.vertexAttribPointer(program.aColour, 4, gl.FLOAT, false, 0, 24);
 

tests/wpt/mozilla/meta/MANIFEST.json

@@ -6477,6 +6477,12 @@
             "url": "/_mozilla/mozilla/webgl/get_supported_extensions.html"
           }
         ],
+        "mozilla/webgl/invalid_vertex_attributes.html": [
+          {
+            "path": "mozilla/webgl/invalid_vertex_attributes.html",
+            "url": "/_mozilla/mozilla/webgl/invalid_vertex_attributes.html"
+          }
+        ],
         "mozilla/websocket_connection_fail.html": [
           {
             "path": "mozilla/websocket_connection_fail.html",

tests/wpt/mozilla/tests/mozilla/webgl/invalid_vertex_attributes.html

@@ -0,0 +1,60 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>Validation of out-of-bounds vertex attributes</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script id="shader-fs" type="x-shader/x-fragment">
+precision mediump float;
+void main() {
+  gl_FragColor = vec4(0.0, 0.0, 1.0, 1.0);
+}
+</script>
+<script id="shader-vs" type="x-shader/x-vertex">
+void main() {
+
+}
+</script>
+<script type="application/x-javascript">
+  function getShader(gl, id) {
+      var shaderScript = document.getElementById(id);
+      if (!shaderScript)
+          return null;
+      var type = (shaderScript.getAttribute('type') == "x-shader/x-fragment") ?
+                    gl.FRAGMENT_SHADER : gl.VERTEX_SHADER;
+      var shader = gl.createShader(type);
+      gl.shaderSource(shader, shaderScript.textContent);
+      gl.compileShader(shader);
+      assert_true(!!gl.getShaderParameter(shader, gl.COMPILE_STATUS), shaderScript.textContent + '\n' + gl.getShaderInfoLog(shader));
+      return shader;
+  }
+  </script>
+  <canvas id="canvas"></canvas>
+  <script>
+    test(function() {
+      var canvas = document.getElementById("canvas");
+      var gl = canvas.getContext("experimental-webgl");
+      assert_true(!!gl);
+
+      var shaderProgram = gl.createProgram();
+      var vertexShader = getShader(gl, "shader-vs");
+      gl.attachShader(shaderProgram, vertexShader);
+      var fragmentShader = getShader(gl, "shader-fs");
+      gl.attachShader(shaderProgram, fragmentShader);
+      gl.linkProgram(shaderProgram);
+      gl.useProgram(shaderProgram);
+      assert_true(gl.getError() == gl.NO_ERROR);
+
+      // -1 is what it's returned from a not found attribute.
+      gl.enableVertexAttribArray(-1);
+      assert_true(gl.getError() == gl.INVALID_VALUE);
+      assert_true(gl.getError() == gl.NO_ERROR);
+
+      gl.vertexAttribPointer(-1, 2, gl.FLOAT, false, 0, 0);
+      assert_true(gl.getError() == gl.INVALID_VALUE);
+      assert_true(gl.getError() == gl.NO_ERROR);
+
+      gl.vertexAttrib1f(-1, 3.0);
+      assert_true(gl.getError() == gl.INVALID_VALUE);
+    });
+  </script>
+</html>