Auto merge of #21284 - servo:webgl, r=emilio

Various fixes for the lifecycle of WebGL objects

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21284)
<!-- Reviewable:end -->

components/canvas/webgl_thread.rs

@@ -932,20 +932,6 @@ impl WebGLImpl {
                 }
                 sender.send(value[0]).unwrap()
             }
-            WebGLCommand::GetShaderParameterBool(shader, param, ref sender) => {
-                let mut value = [0];
-                unsafe {
-                    ctx.gl().get_shader_iv(shader.get(), param as u32, &mut value);
-                }
-                sender.send(value[0] != 0).unwrap()
-            }
-            WebGLCommand::GetShaderParameterInt(shader, param, ref sender) => {
-                let mut value = [0];
-                unsafe {
-                    ctx.gl().get_shader_iv(shader.get(), param as u32, &mut value);
-                }
-                sender.send(value[0]).unwrap()
-            }
             WebGLCommand::GetCurrentVertexAttrib(index, ref sender) => {
                 let mut value = [0.; 4];
                 unsafe {

components/canvas_traits/webgl.rs

@@ -273,8 +273,6 @@ pub enum WebGLCommand {
     GetParameterFloat4(ParameterFloat4, WebGLSender<[f32; 4]>),
     GetProgramValidateStatus(WebGLProgramId, WebGLSender<bool>),
     GetProgramActiveUniforms(WebGLProgramId, WebGLSender<i32>),
-    GetShaderParameterBool(WebGLShaderId, ShaderParameterBool, WebGLSender<bool>),
-    GetShaderParameterInt(WebGLShaderId, ShaderParameterInt, WebGLSender<i32>),
     GetCurrentVertexAttrib(u32, WebGLSender<[f32; 4]>),
     GetTexParameterFloat(u32, TexParameterFloat, WebGLSender<f32>),
     GetTexParameterInt(u32, TexParameterInt, WebGLSender<i32>),
@@ -583,18 +581,6 @@ parameters! {
     }
 }
 
-parameters! {
-    ShaderParameter {
-        Bool(ShaderParameterBool {
-            DeleteStatus = gl::DELETE_STATUS,
-            CompileStatus = gl::COMPILE_STATUS,
-        }),
-        Int(ShaderParameterInt {
-            ShaderType = gl::SHADER_TYPE,
-        }),
-    }
-}
-
 parameters! {
     TexParameter {
         Float(TexParameterFloat {

components/script/dom/webglprogram.rs

@@ -25,7 +25,8 @@ use std::cell::{Cell, Ref};
 pub struct WebGLProgram {
     webgl_object: WebGLObject,
     id: WebGLProgramId,
-    is_deleted: Cell<bool>,
+    is_in_use: Cell<bool>,
+    marked_for_deletion: Cell<bool>,
     link_called: Cell<bool>,
     linked: Cell<bool>,
     link_generation: Cell<u64>,
@@ -40,9 +41,10 @@ impl WebGLProgram {
         Self {
             webgl_object: WebGLObject::new_inherited(context),
             id: id,
-            is_deleted: Cell::new(false),
-            link_called: Cell::new(false),
-            linked: Cell::new(false),
+            is_in_use: Default::default(),
+            marked_for_deletion: Default::default(),
+            link_called: Default::default(),
+            linked: Default::default(),
             link_generation: Default::default(),
             fragment_shader: Default::default(),
             vertex_shader: Default::default(),
@@ -73,25 +75,51 @@ impl WebGLProgram {
     }
 
     /// glDeleteProgram
-    pub fn delete(&self) {
-        if !self.is_deleted.get() {
-            self.is_deleted.set(true);
-            self.upcast::<WebGLObject>()
-                .context()
-                .send_command(WebGLCommand::DeleteProgram(self.id));
-
-            if let Some(shader) = self.fragment_shader.get() {
-                shader.decrement_attached_counter();
-            }
-
-            if let Some(shader) = self.vertex_shader.get() {
-                shader.decrement_attached_counter();
-            }
+    pub fn mark_for_deletion(&self) {
+        if self.marked_for_deletion.get() {
+            return;
+        }
+        self.marked_for_deletion.set(true);
+        self.upcast::<WebGLObject>()
+            .context()
+            .send_command(WebGLCommand::DeleteProgram(self.id));
+        if self.is_deleted() {
+            self.detach_shaders();
         }
     }
 
+    pub fn in_use(&self, value: bool) {
+        if self.is_in_use.get() == value {
+            return;
+        }
+        self.is_in_use.set(value);
+        if self.is_deleted() {
+            self.detach_shaders();
+        }
+    }
+
+    fn detach_shaders(&self) {
+        assert!(self.is_deleted());
+        if let Some(shader) = self.fragment_shader.get() {
+            shader.decrement_attached_counter();
+            self.fragment_shader.set(None);
+        }
+        if let Some(shader) = self.vertex_shader.get() {
+            shader.decrement_attached_counter();
+            self.vertex_shader.set(None);
+        }
+    }
+
+    pub fn is_in_use(&self) -> bool {
+        self.is_in_use.get()
+    }
+
+    pub fn is_marked_for_deletion(&self) -> bool {
+        self.marked_for_deletion.get()
+    }
+
     pub fn is_deleted(&self) -> bool {
-        self.is_deleted.get()
+        self.marked_for_deletion.get() && !self.is_in_use.get()
     }
 
     pub fn is_linked(&self) -> bool {
@@ -99,10 +127,7 @@ impl WebGLProgram {
     }
 
     /// glLinkProgram
-    pub fn link(&self) -> WebGLResult<()>  {
-        if self.is_deleted() {
-            return Err(WebGLError::InvalidOperation);
-        }
+    pub fn link(&self) -> WebGLResult<()> {
         self.linked.set(false);
         self.link_generation.set(self.link_generation.get().checked_add(1).unwrap());
         *self.active_attribs.borrow_mut() = Box::new([]);
@@ -192,8 +217,6 @@ impl WebGLProgram {
             }
         };
 
-        // TODO(emilio): Differentiate between same shader already assigned and other previous
-        // shader.
         if shader_slot.get().is_some() {
             return Err(WebGLError::InvalidOperation);
         }
@@ -216,10 +239,7 @@ impl WebGLProgram {
         let shader_slot = match shader.gl_type() {
             constants::FRAGMENT_SHADER => &self.fragment_shader,
             constants::VERTEX_SHADER => &self.vertex_shader,
-            _ => {
-                error!("detachShader: Unexpected shader type");
-                return Err(WebGLError::InvalidValue);
-            }
+            _ => return Err(WebGLError::InvalidValue),
         };
 
         match shader_slot.get() {
@@ -391,7 +411,7 @@ impl WebGLProgram {
     }
 
     pub fn attached_shaders(&self) -> WebGLResult<Vec<DomRoot<WebGLShader>>> {
-        if self.is_deleted.get() {
+        if self.marked_for_deletion.get() {
             return Err(WebGLError::InvalidValue);
         }
         Ok(match (self.vertex_shader.get(), self.fragment_shader.get()) {
@@ -410,7 +430,8 @@ impl WebGLProgram {
 
 impl Drop for WebGLProgram {
     fn drop(&mut self) {
-        self.delete();
+        self.in_use(false);
+        self.mark_for_deletion();
     }
 }
 

components/script/dom/webglrenderingcontext.rs

@@ -5,8 +5,7 @@
 use byteorder::{NativeEndian, ReadBytesExt, WriteBytesExt};
 use canvas_traits::canvas::{byte_swap, multiply_u8_pixel};
 use canvas_traits::webgl::{ActiveAttribInfo, DOMToTextureCommand, Parameter};
-use canvas_traits::webgl::{ShaderParameter, TexParameter, WebGLCommand};
-use canvas_traits::webgl::{WebGLContextShareMode, WebGLError};
+use canvas_traits::webgl::{TexParameter, WebGLCommand, WebGLContextShareMode, WebGLError};
 use canvas_traits::webgl::{WebGLFramebufferBindingRequest, WebGLMsg, WebGLMsgSender};
 use canvas_traits::webgl::{WebGLProgramId, WebGLResult, WebGLSLVersion, WebGLSender};
 use canvas_traits::webgl::{WebGLVersion, WebVRCommand, webgl_channel};
@@ -2354,11 +2353,7 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
     fn DeleteProgram(&self, program: Option<&WebGLProgram>) {
         if let Some(program) = program {
             handle_potential_webgl_error!(self, self.validate_ownership(program), return);
-            // FIXME: We should call glUseProgram(0), but
-            // WebGLCommand::UseProgram() doesn't take an Option
-            // currently.  This is also a problem for useProgram(null)
-            handle_object_deletion!(self, self.current_program, program, None);
-            program.delete()
+            program.mark_for_deletion()
         }
     }
 
@@ -2366,7 +2361,7 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
     fn DeleteShader(&self, shader: Option<&WebGLShader>) {
         if let Some(shader) = shader {
             handle_potential_webgl_error!(self, self.validate_ownership(shader), return);
-            shader.delete()
+            shader.mark_for_deletion()
         }
     }
 
@@ -2700,8 +2695,12 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.9
     unsafe fn GetProgramParameter(&self, _: *mut JSContext, program: &WebGLProgram, param: u32) -> JSVal {
         handle_potential_webgl_error!(self, self.validate_ownership(program), return NullValue());
+        if program.is_deleted() {
+            self.webgl_error(InvalidOperation);
+            return NullValue();
+        }
         match param {
-            constants::DELETE_STATUS => BooleanValue(program.is_deleted()),
+            constants::DELETE_STATUS => BooleanValue(program.is_marked_for_deletion()),
             constants::LINK_STATUS => BooleanValue(program.is_linked()),
             constants::VALIDATE_STATUS => {
                 // FIXME(nox): This could be cached on the DOM side when we call validateProgram
@@ -2733,20 +2732,17 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.9
     unsafe fn GetShaderParameter(&self, _: *mut JSContext, shader: &WebGLShader, param: u32) -> JSVal {
         handle_potential_webgl_error!(self, self.validate_ownership(shader), return NullValue());
-        if shader.is_deleted() && !shader.is_attached() {
+        if shader.is_deleted() {
             self.webgl_error(InvalidValue);
             return NullValue();
         }
-        match handle_potential_webgl_error!(self, ShaderParameter::from_u32(param), return NullValue()) {
-            ShaderParameter::Bool(param) => {
-                let (sender, receiver) = webgl_channel().unwrap();
-                self.send_command(WebGLCommand::GetShaderParameterBool(shader.id(), param, sender));
-                BooleanValue(receiver.recv().unwrap())
-            }
-            ShaderParameter::Int(param) => {
-                let (sender, receiver) = webgl_channel().unwrap();
-                self.send_command(WebGLCommand::GetShaderParameterInt(shader.id(), param, sender));
-                Int32Value(receiver.recv().unwrap())
+        match param {
+            constants::DELETE_STATUS => BooleanValue(shader.is_marked_for_deletion()),
+            constants::COMPILE_STATUS => BooleanValue(shader.successfully_compiled()),
+            constants::SHADER_TYPE => UInt32Value(shader.gl_type()),
+            _ => {
+                self.webgl_error(InvalidEnum);
+                NullValue()
             }
         }
     }
@@ -2878,7 +2874,9 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
 
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.5
     fn IsBuffer(&self, buffer: Option<&WebGLBuffer>) -> bool {
-        buffer.map_or(false, |buf| buf.target().is_some() && !buf.is_deleted())
+        buffer.map_or(false, |buf| {
+            self.validate_ownership(buf).is_ok() && buf.target().is_some() && !buf.is_deleted()
+        })
     }
 
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.3
@@ -2888,27 +2886,33 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
 
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.6
     fn IsFramebuffer(&self, frame_buffer: Option<&WebGLFramebuffer>) -> bool {
-        frame_buffer.map_or(false, |buf| buf.target().is_some() && !buf.is_deleted())
+        frame_buffer.map_or(false, |buf| {
+            self.validate_ownership(buf).is_ok() && buf.target().is_some() && !buf.is_deleted()
+        })
     }
 
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.9
     fn IsProgram(&self, program: Option<&WebGLProgram>) -> bool {
-        program.map_or(false, |p| !p.is_deleted())
+        program.map_or(false, |p| self.validate_ownership(p).is_ok() && !p.is_deleted())
     }
 
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.7
     fn IsRenderbuffer(&self, render_buffer: Option<&WebGLRenderbuffer>) -> bool {
-        render_buffer.map_or(false, |buf| buf.ever_bound() && !buf.is_deleted())
+        render_buffer.map_or(false, |buf| {
+            self.validate_ownership(buf).is_ok() && buf.ever_bound() && !buf.is_deleted()
+        })
     }
 
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.9
     fn IsShader(&self, shader: Option<&WebGLShader>) -> bool {
-        shader.map_or(false, |s| !s.is_deleted() || s.is_attached())
+        shader.map_or(false, |s| self.validate_ownership(s).is_ok() && !s.is_deleted())
     }
 
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.8
     fn IsTexture(&self, texture: Option<&WebGLTexture>) -> bool {
-        texture.map_or(false, |tex| tex.target().is_some() && !tex.is_deleted())
+        texture.map_or(false, |tex| {
+            self.validate_ownership(tex).is_ok() && tex.target().is_some() && !tex.is_deleted()
+        })
     }
 
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.3
@@ -3166,6 +3170,9 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.9
     fn LinkProgram(&self, program: &WebGLProgram) {
         handle_potential_webgl_error!(self, self.validate_ownership(program), return);
+        if program.is_deleted() {
+            return self.webgl_error(InvalidValue);
+        }
         handle_potential_webgl_error!(self, program.link());
     }
 
@@ -3721,6 +3728,14 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
             if program.is_deleted() || !program.is_linked() {
                 return self.webgl_error(InvalidOperation);
             }
+            if program.is_in_use() {
+                return;
+            }
+            program.in_use(true);
+        }
+        match self.current_program.get() {
+            Some(ref current) if program != Some(&**current) => current.in_use(false),
+            _ => {}
         }
         self.send_command(WebGLCommand::UseProgram(program.map(|p| p.id())));
         self.current_program.set(program);

components/script/dom/webglshader.rs

@@ -38,7 +38,7 @@ pub struct WebGLShader {
     gl_type: u32,
     source: DomRefCell<DOMString>,
     info_log: DomRefCell<DOMString>,
-    is_deleted: Cell<bool>,
+    marked_for_deletion: Cell<bool>,
     attached_counter: Cell<u32>,
     compilation_status: Cell<ShaderCompilationStatus>,
 }
@@ -58,7 +58,7 @@ impl WebGLShader {
             gl_type: shader_type,
             source: Default::default(),
             info_log: Default::default(),
-            is_deleted: Cell::new(false),
+            marked_for_deletion: Cell::new(false),
             attached_counter: Cell::new(0),
             compilation_status: Cell::new(ShaderCompilationStatus::NotCompiled),
         }
@@ -101,7 +101,7 @@ impl WebGLShader {
         limits: &GLLimits,
         ext: &WebGLExtensions,
     ) -> WebGLResult<()> {
-        if self.is_deleted.get() && !self.is_attached() {
+        if self.marked_for_deletion.get() && !self.is_attached() {
             return Err(WebGLError::InvalidValue);
         }
         if self.compilation_status.get() != ShaderCompilationStatus::NotCompiled {
@@ -177,28 +177,27 @@ impl WebGLShader {
 
         *self.info_log.borrow_mut() = validator.info_log().into();
 
-        // 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)
     /// and delete it as if calling glDeleteShader.
     /// Currently does not check if shader is attached
-    pub fn delete(&self) {
-        if !self.is_deleted.get() {
-            self.is_deleted.set(true);
+    pub fn mark_for_deletion(&self) {
+        if !self.marked_for_deletion.get() {
+            self.marked_for_deletion.set(true);
             self.upcast::<WebGLObject>()
                 .context()
                 .send_command(WebGLCommand::DeleteShader(self.id));
         }
     }
 
+    pub fn is_marked_for_deletion(&self) -> bool {
+        self.marked_for_deletion.get()
+    }
+
     pub fn is_deleted(&self) -> bool {
-        self.is_deleted.get()
+        self.marked_for_deletion.get() && !self.is_attached()
     }
 
     pub fn is_attached(&self) -> bool {
@@ -236,7 +235,6 @@ impl WebGLShader {
 
 impl Drop for WebGLShader {
     fn drop(&mut self) {
-        assert_eq!(self.attached_counter.get(), 0);
-        self.delete();
+        self.mark_for_deletion();
     }
 }