Implement missing WebGLShader checks

Methods compileShader and getShaderParameter should emit an error when
the shader has been deleted.

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

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::{WebGLCommand, WebGLError, WebGLMsgSender};
-use canvas_traits::webgl::{webgl_channel, WebGLCommand, WebGLMsgSender, WebGLParameter, WebGLResult, WebGLShaderId};
+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 source = self.source.borrow();
-            let mut params = BuiltInResources::default();
+        let source = match source.as_ref() {
-            params.FragmentPrecisionHigh = 1;
+            Some(source) => source,
-            params.OES_standard_derivatives = ext.is_enabled::<OESStandardDerivatives>() as i32;
+            None => return Ok(()),
-            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,
-                                               &params).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,
-                                               &params).unwrap()
-                },
-            };
 
-            match validator.compile_and_translate(&[source]) {
+        let mut params = BuiltInResources::default();
-                Ok(translated_source) => {
+        params.FragmentPrecisionHigh = 1;
-                    debug!("Shader translated: {}", translated_source);
+        params.OES_standard_derivatives = ext.is_enabled::<OESStandardDerivatives>() as i32;
-                    // NOTE: At this point we should be pretty sure that the compilation in the paint thread
+        let validator = match webgl_version {
-                    // will succeed.
+            WebGLVersion::WebGL1 => {
-                    // It could be interesting to retrieve the info log from the paint thread though
+                let output_format = if cfg!(any(target_os = "android", target_os = "ios")) {
-                    let msg = WebGLCommand::CompileShader(self.id, translated_source);
+                    Output::Essl
-                    self.renderer.send(msg).unwrap();
+                } else {
-                    self.compilation_status.set(ShaderCompilationStatus::Succeeded);
+                    Output::Glsl
-                },
+                };
-                Err(error) => {
+                ShaderValidator::for_webgl(self.gl_type,
-                    self.compilation_status.set(ShaderCompilationStatus::Failed);
+                                            output_format,
-                    debug!("Shader {} compilation failed: {}", self.id, error);
+                                            &params).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,
+                                            &params).unwrap()
+            },
+        };
 
-            *self.info_log.borrow_mut() = Some(validator.info_log());
+        match validator.compile_and_translate(&[source]) {
-            // TODO(emilio): More data (like uniform data) should be collected
+            Ok(translated_source) => {
-            // here to properly validate uniforms.
+                debug!("Shader translated: {}", translated_source);
-            //
+                // NOTE: At this point we should be pretty sure that the compilation in the paint thread
-            // This requires a more complex interface with ANGLE, using C++
+                // will succeed.
-            // bindings and being extremely cautious about destructing things.
+                // 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<WebGLParameter> {
+        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()

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
+