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

6 files changed, 60 insertions, 32 deletions

diff --git a/components/canvas/webgl_paint_thread.rs b/components/canvas/webgl_paint_thread.rs
index c88f2a378b7..30e47c06082 100644
--- a/components/canvas/webgl_paint_thread.rs
+++ b/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> {
-        let data = if let Some(sender) = webrender_api_sender {
+           webrender_api_sender: Option<webrender_traits::RenderApiSender>)
+        -> 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));
-            WebGLPaintTaskData::WebRender(webrender_api, id)
+            let (id, limits) = try!(webrender_api.request_webgl_context(&size, attrs));
+            (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) => {
-                    result_chan.send(Ok(())).unwrap();
+                Ok((thread, limits)) => {
+                    result_chan.send(Ok(limits)).unwrap();
                     thread
                 },
                 Err(e) => {
@@ -95,8 +99,7 @@ impl WebGLPaintThread {
             }
         });
 
-        try!(result_port.recv().unwrap());
-        Ok(sender)
+        result_port.recv().unwrap().map(|limits| (sender, limits))
     }
 
     fn send_data(&mut self, chan: IpcSender<CanvasData>) {
diff --git a/components/compositing/constellation.rs b/components/compositing/constellation.rs
index b52a1a22395..6c4cd0cdb95 100644
--- a/components/compositing/constellation.rs
+++ b/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))
     }
 
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
diff --git a/components/script_traits/script_msg.rs b/components/script_traits/script_msg.rs
index 7c82de027bf..10bb4864c4b 100644
--- a/components/script_traits/script_msg.rs
+++ b/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,
-                         IpcSender<Result<IpcSender<CanvasMsg>, String>>),
+                           GLContextAttributes,
+                           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.