From 20a309f037236afa1287da36a035b2e4122ed63c Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Sat, 24 Mar 2018 13:51:31 +0100 Subject: [PATCH] Implement missing WebGLShader checks Methods compileShader and getShaderParameter should emit an error when the shader has been deleted. --- .../script/dom/webglrenderingcontext.rs | 5 +- components/script/dom/webglshader.rs | 139 ++++++++++-------- .../programs/program-test.html.ini | 19 ++- 3 files changed, 98 insertions(+), 65 deletions(-) diff --git a/components/script/dom/webglrenderingcontext.rs b/components/script/dom/webglrenderingcontext.rs index 36f12577c8b..24c0c3df069 100644 --- a/components/script/dom/webglrenderingcontext.rs +++ b/components/script/dom/webglrenderingcontext.rs @@ -1966,7 +1966,10 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext { // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.9 fn CompileShader(&self, shader: &WebGLShader) { - shader.compile(self.webgl_version, self.glsl_version, &self.extension_manager) + handle_potential_webgl_error!( + self, + shader.compile(self.webgl_version, self.glsl_version, &self.extension_manager) + ) } // TODO(emilio): Probably in the future we should keep track of the diff --git a/components/script/dom/webglshader.rs b/components/script/dom/webglshader.rs index 2a6504be506..0e965b01b3b 100644 --- a/components/script/dom/webglshader.rs +++ b/components/script/dom/webglshader.rs @@ -3,8 +3,9 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ // https://www.khronos.org/registry/webgl/specs/latest/1.0/webgl.idl -use canvas_traits::webgl::{WebGLSLVersion, WebGLVersion}; -use canvas_traits::webgl::{webgl_channel, WebGLCommand, WebGLMsgSender, WebGLParameter, WebGLResult, WebGLShaderId}; +use canvas_traits::webgl::{WebGLCommand, WebGLError, WebGLMsgSender}; +use canvas_traits::webgl::{WebGLParameter, WebGLResult, WebGLSLVersion}; +use canvas_traits::webgl::{WebGLShaderId, WebGLVersion, webgl_channel}; use dom::bindings::cell::DomRefCell; use dom::bindings::codegen::Bindings::WebGLShaderBinding; use dom::bindings::reflector::reflect_dom_object; @@ -98,74 +99,83 @@ impl WebGLShader { &self, webgl_version: WebGLVersion, glsl_version: WebGLSLVersion, - ext: &WebGLExtensions - ) { + ext: &WebGLExtensions, + ) -> WebGLResult<()> { + if self.is_deleted.get() && !self.is_attached() { + return Err(WebGLError::InvalidValue); + } if self.compilation_status.get() != ShaderCompilationStatus::NotCompiled { debug!("Compiling already compiled shader {}", self.id); } - if let Some(ref source) = *self.source.borrow() { - let mut params = BuiltInResources::default(); - params.FragmentPrecisionHigh = 1; - params.OES_standard_derivatives = ext.is_enabled::() as i32; - let validator = match webgl_version { - WebGLVersion::WebGL1 => { - let output_format = if cfg!(any(target_os = "android", target_os = "ios")) { - Output::Essl - } else { - Output::Glsl - }; - ShaderValidator::for_webgl(self.gl_type, - output_format, - ¶ms).unwrap() - }, - WebGLVersion::WebGL2 => { - let output_format = if cfg!(any(target_os = "android", target_os = "ios")) { - Output::Essl - } else { - match (glsl_version.major, glsl_version.minor) { - (1, 30) => Output::Glsl130, - (1, 40) => Output::Glsl140, - (1, 50) => Output::Glsl150Core, - (3, 30) => Output::Glsl330Core, - (4, 0) => Output::Glsl400Core, - (4, 10) => Output::Glsl410Core, - (4, 20) => Output::Glsl420Core, - (4, 30) => Output::Glsl430Core, - (4, 40) => Output::Glsl440Core, - (4, _) => Output::Glsl450Core, - _ => Output::Glsl140 - } - }; - ShaderValidator::for_webgl2(self.gl_type, - output_format, - ¶ms).unwrap() - }, - }; + let source = self.source.borrow(); + let source = match source.as_ref() { + Some(source) => source, + None => return Ok(()), + }; - 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 - let msg = WebGLCommand::CompileShader(self.id, translated_source); - self.renderer.send(msg).unwrap(); - self.compilation_status.set(ShaderCompilationStatus::Succeeded); - }, - Err(error) => { - self.compilation_status.set(ShaderCompilationStatus::Failed); - debug!("Shader {} compilation failed: {}", self.id, error); - }, - } + let mut params = BuiltInResources::default(); + params.FragmentPrecisionHigh = 1; + params.OES_standard_derivatives = ext.is_enabled::() as i32; + let validator = match webgl_version { + WebGLVersion::WebGL1 => { + let output_format = if cfg!(any(target_os = "android", target_os = "ios")) { + Output::Essl + } else { + Output::Glsl + }; + ShaderValidator::for_webgl(self.gl_type, + output_format, + ¶ms).unwrap() + }, + WebGLVersion::WebGL2 => { + let output_format = if cfg!(any(target_os = "android", target_os = "ios")) { + Output::Essl + } else { + match (glsl_version.major, glsl_version.minor) { + (1, 30) => Output::Glsl130, + (1, 40) => Output::Glsl140, + (1, 50) => Output::Glsl150Core, + (3, 30) => Output::Glsl330Core, + (4, 0) => Output::Glsl400Core, + (4, 10) => Output::Glsl410Core, + (4, 20) => Output::Glsl420Core, + (4, 30) => Output::Glsl430Core, + (4, 40) => Output::Glsl440Core, + (4, _) => Output::Glsl450Core, + _ => Output::Glsl140 + } + }; + ShaderValidator::for_webgl2(self.gl_type, + output_format, + ¶ms).unwrap() + }, + }; - *self.info_log.borrow_mut() = Some(validator.info_log()); - // TODO(emilio): More data (like uniform data) should be collected - // here to properly validate uniforms. - // - // This requires a more complex interface with ANGLE, using C++ - // bindings and being extremely cautious about destructing things. + 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 + let msg = WebGLCommand::CompileShader(self.id, translated_source); + self.renderer.send(msg).unwrap(); + self.compilation_status.set(ShaderCompilationStatus::Succeeded); + }, + Err(error) => { + self.compilation_status.set(ShaderCompilationStatus::Failed); + debug!("Shader {} compilation failed: {}", self.id, error); + }, } + + *self.info_log.borrow_mut() = Some(validator.info_log()); + + // TODO(emilio): More data (like uniform data) should be collected + // here to properly validate uniforms. + // + // This requires a more complex interface with ANGLE, using C++ + // bindings and being extremely cautious about destructing things. + Ok(()) } /// Mark this shader as deleted (if it wasn't previously) @@ -202,6 +212,9 @@ impl WebGLShader { /// glGetParameter pub fn parameter(&self, param_id: u32) -> WebGLResult { + if self.is_deleted.get() && !self.is_attached() { + return Err(WebGLError::InvalidValue); + } let (sender, receiver) = webgl_channel().unwrap(); self.renderer.send(WebGLCommand::GetShaderParameter(self.id, param_id, sender)).unwrap(); receiver.recv().unwrap() diff --git a/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/programs/program-test.html.ini b/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/programs/program-test.html.ini index 48150cef19a..a5f39d42b57 100644 --- a/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/programs/program-test.html.ini +++ b/tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/programs/program-test.html.ini @@ -1,2 +1,19 @@ [program-test.html] - expected: CRASH + [WebGL test #53: getError expected: INVALID_OPERATION. Was NO_ERROR : drawing with a null program should generate INVALID_OPERATION] + expected: FAIL + + [WebGL test #58: linking should fail with in-use formerly good program, with new bad shader attached] + expected: FAIL + + [WebGL test #64: getError expected: NO_ERROR. Was INVALID_OPERATION : delete the current program shouldn't change the current rendering state] + expected: FAIL + + [WebGL test #65: getError expected: NO_ERROR. Was INVALID_OPERATION : The current program shouldn't be deleted] + expected: FAIL + + [WebGL test #69: an attached shader shouldn't be deleted] + expected: FAIL + + [WebGL test #70: getError expected: INVALID_VALUE. Was INVALID_OPERATION : a delete-marked program should be deleted once it's no longer the current program] + expected: FAIL +