Fix program and shader lifetime cycle

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),
+            is_in_use: Default::default(),
-            link_called: Cell::new(false),
+            marked_for_deletion: Default::default(),
-            linked: Cell::new(false),
+            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) {
+    pub fn mark_for_deletion(&self) {
-        if !self.is_deleted.get() {
+        if self.marked_for_deletion.get() {
-            self.is_deleted.set(true);
+            return;
-            self.upcast::<WebGLObject>()
+        }
-                .context()
+        self.marked_for_deletion.set(true);
-                .send_command(WebGLCommand::DeleteProgram(self.id));
+        self.upcast::<WebGLObject>()
-
+            .context()
-            if let Some(shader) = self.fragment_shader.get() {
+            .send_command(WebGLCommand::DeleteProgram(self.id));
-                shader.decrement_attached_counter();
+        if self.is_deleted() {
-            }
+            self.detach_shaders();
-
-            if let Some(shader) = self.vertex_shader.get() {
-                shader.decrement_attached_counter();
-            }
         }
     }
 
+    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<()>  {
+    pub fn link(&self) -> WebGLResult<()> {
-        if self.is_deleted() {
-            return Err(WebGLError::InvalidOperation);
-        }
         self.linked.set(false);
         self.link_generation.set(self.link_generation.get().checked_add(1).unwrap());
         *self.active_attribs.borrow_mut() = Box::new([]);
@@ -214,10 +239,7 @@ impl WebGLProgram {
         let shader_slot = match shader.gl_type() {
             constants::FRAGMENT_SHADER => &self.fragment_shader,
             constants::VERTEX_SHADER => &self.vertex_shader,
-            _ => {
+            _ => return Err(WebGLError::InvalidValue),
-                error!("detachShader: Unexpected shader type");
-                return Err(WebGLError::InvalidValue);
-            }
         };
 
         match shader_slot.get() {
@@ -389,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()) {
@@ -408,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

@@ -2354,11 +2354,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
+            program.mark_for_deletion()
-            // 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()
         }
     }
 
@@ -2366,7 +2362,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 +2696,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,7 +2733,7 @@ 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();
         }
@@ -2903,7 +2903,7 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
 
     // 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| !s.is_deleted())
     }
 
     // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.8
@@ -3166,6 +3166,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 +3724,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 {
@@ -183,9 +183,9 @@ impl WebGLShader {
     /// 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) {
+    pub fn mark_for_deletion(&self) {
-        if !self.is_deleted.get() {
+        if !self.marked_for_deletion.get() {
-            self.is_deleted.set(true);
+            self.marked_for_deletion.set(true);
             self.upcast::<WebGLObject>()
                 .context()
                 .send_command(WebGLCommand::DeleteShader(self.id));
@@ -193,7 +193,7 @@ impl WebGLShader {
     }
 
     pub fn is_deleted(&self) -> bool {
-        self.is_deleted.get()
+        self.marked_for_deletion.get() && !self.is_attached()
     }
 
     pub fn is_attached(&self) -> bool {
@@ -231,7 +231,6 @@ impl WebGLShader {
 
 impl Drop for WebGLShader {
     fn drop(&mut self) {
-        assert_eq!(self.attached_counter.get(), 0);
+        self.mark_for_deletion();
-        self.delete();
     }
 }

tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/misc/object-deletion-behaviour.html.ini

@@ -1,4 +0,0 @@
-[object-deletion-behaviour.html]
-  [WebGL test #17: gl.isProgram(program) should be true. Was false.]
-    expected: FAIL
-

tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/programs/program-test.html.ini

@@ -1,13 +0,0 @@
-[program-test.html]
-  [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
-