From 240ac7cfe2c5ce87a87ce1bf38d7a62a3c32aaa2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 27 Mar 2016 14:18:13 +0200 Subject: [PATCH 1/6] webgl: Validate shader type parameter to CreateShader. --- components/script/dom/webglrenderingcontext.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/components/script/dom/webglrenderingcontext.rs b/components/script/dom/webglrenderingcontext.rs index d08a9456624..6cbba2e3c29 100644 --- a/components/script/dom/webglrenderingcontext.rs +++ b/components/script/dom/webglrenderingcontext.rs @@ -679,6 +679,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> { + 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) } From 466c8881de22faaf21e01c1e95e904e36ee5947d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 27 Mar 2016 14:21:09 +0200 Subject: [PATCH 2/6] webgl: Use early return in DrawArrays --- components/script/dom/webglrenderingcontext.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/components/script/dom/webglrenderingcontext.rs b/components/script/dom/webglrenderingcontext.rs index 6cbba2e3c29..7bba7300976 100644 --- a/components/script/dom/webglrenderingcontext.rs +++ b/components/script/dom/webglrenderingcontext.rs @@ -743,13 +743,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), } From 8d7ee15acee6dba661880ba93551537cdc80bdaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 27 Mar 2016 14:36:50 +0200 Subject: [PATCH 3/6] webgl: Remove unneeded return value. --- components/script/dom/webglrenderingcontext.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/components/script/dom/webglrenderingcontext.rs b/components/script/dom/webglrenderingcontext.rs index 7bba7300976..3cfb4bab816 100644 --- a/components/script/dom/webglrenderingcontext.rs +++ b/components/script/dom/webglrenderingcontext.rs @@ -138,6 +138,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() { @@ -154,7 +157,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) } } From 6fcc03c9659c7140b02ac9039d1646142161e234 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 27 Mar 2016 16:08:52 +0200 Subject: [PATCH 4/6] webgl: Make the api return the context limits and use them for validations This allows keeping the VertexAttrib* calls asynchronous. Another option would be to do the validation in the apply() function, but that'd require us passing an unnecessary channel around and add extra synchronization. The counterpart of this is that it has to be updated when the context changes, but that's less problem. --- components/canvas/webgl_paint_thread.rs | 29 ++++++++++--------- components/compositing/constellation.rs | 8 ++--- components/script/dom/bindings/trace.rs | 3 +- .../script/dom/webglrenderingcontext.rs | 21 +++++++++++--- components/script/dom/webglshader.rs | 1 + components/script_traits/script_msg.rs | 6 ++-- tests/html/test_webgl_triangle.html | 1 + 7 files changed, 44 insertions(+), 25 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, attrs: GLContextAttributes, - webrender_api_sender: Option) -> Result { - let data = if let Some(sender) = webrender_api_sender { + webrender_api_sender: Option) + -> 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::::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, attrs: GLContextAttributes, webrender_api_sender: Option) - -> Result, String> { + -> Result<(IpcSender, GLLimits), String> { let (sender, receiver) = ipc::channel::().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) { diff --git a/components/compositing/constellation.rs b/components/compositing/constellation.rs index b41f75c7b6e..154b0dbc81c 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 Constellation &mut self, size: &Size2D, attributes: GLContextAttributes, - response_sender: IpcSender, String>>) { + response_sender: IpcSender, 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 2491058750c..70f609ebe20 100644 --- a/components/script/dom/bindings/trace.rs +++ b/components/script/dom/bindings/trace.rs @@ -61,6 +61,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_thread::ScriptChan; @@ -307,7 +308,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 3cfb4bab816..e4e6fbd315e 100644 --- a/components/script/dom/webglrenderingcontext.rs +++ b/components/script/dom/webglrenderingcontext.rs @@ -30,7 +30,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; @@ -70,6 +70,8 @@ pub struct WebGLRenderingContext { reflector_: Reflector, #[ignore_heap_size_of = "Defined in ipc-channel"] ipc_renderer: IpcSender, + #[ignore_heap_size_of = "Defined in offscreen_gl_context"] + limits: GLLimits, canvas: JS, #[ignore_heap_size_of = "Defined in webrender_traits"] last_error: Cell>, @@ -94,10 +96,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), @@ -166,6 +169,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(); @@ -799,6 +806,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() @@ -1084,7 +1095,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::(data) { @@ -1102,7 +1112,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::(data) { @@ -1153,6 +1162,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, - GLContextAttributes, - IpcSender, String>>), + GLContextAttributes, + IpcSender, 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. diff --git a/tests/html/test_webgl_triangle.html b/tests/html/test_webgl_triangle.html index 980049f4466..cf8b1f5251b 100644 --- a/tests/html/test_webgl_triangle.html +++ b/tests/html/test_webgl_triangle.html @@ -96,6 +96,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); From ee8ecd929945cbe1ee4f06c07c90eac9bb9de67f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 27 Mar 2016 16:25:46 +0200 Subject: [PATCH 5/6] webgl: tests: Fix webgl triangle test It was giving a ton of whitespace errors, and I don't know if it's due to nodeValue or ANGLE... --- tests/html/test_webgl_triangle.html | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/tests/html/test_webgl_triangle.html b/tests/html/test_webgl_triangle.html index cf8b1f5251b..2386f3fa1bf 100644 --- a/tests/html/test_webgl_triangle.html +++ b/tests/html/test_webgl_triangle.html @@ -9,23 +9,25 @@ + + + + + + +