script: Move WebGLProgram Drop implementation to a member type by willypuzzle · Pull Request #37622 · servo/servo

willypuzzle · 2025-06-22T22:02:31Z

Testing: No new tests introduced
Fixes: Partially #26488

jdm · 2025-06-22T23:00:31Z

components/script/dom/webgl/webglprogram.rs

    #[no_trace]
    id: WebGLProgramId,
    is_in_use: Cell<bool>,
    marked_for_deletion: Cell<bool>,
+    fragment_shader: MutNullableDom<WebGLShader>,


Any Dom value like this is JS-owned and unsafe to use from the destructor. We'll need to look more closely at this.

I don't know how to use WeakRef instead of it...

Could the solution be creating a WeakNullableRef struct or trait?

Does RefCell<Option<WeakRef<WebGLShader>>> work? Sorry for missing this question; I was reminded of this PR by #40655.

How can I solve this?

error[E0277]: the trait bound `dom::webgl::webglshader::WebGLShader: script_bindings::weakref::WeakReferenceable` is not satisfied --> components\script\dom\webgl\webglprogram.rs:38:37 | 38 | fragment_shader: RefCell<Option<WeakRef<WebGLShader>>>, | ^^^^^^^^^^^^^^^^^^^^ unsatisfied trait bound

Add a new entry like https://github.com/willypuzzle/servo/blob/af27c801b1db0c0da93f380f46bdeb957828eb56/components/script_bindings/codegen/Bindings.conf#L678 for the WebGLShader interface.

…0725) Many WebGL objects refer to a `WebGLRenderingContext` and rely on it for messaging to the `WebGLThread`. This poses a problem, because WebGL objects often need to send a message to the `WebGLThread` during their `Drop` implementation. If the `Drop` is triggered as part of garbage collection, references to the `WebGLRenderingContext` might be invalid, if they were garbage collected first as part of the same harvest. This change makes it so that all of these objects store a `WeakRef` instead of a `Dom<>`. The `WeakRef` is only used if it can be rooted, otherwise a `ContextLost` error is given. In cases where only messaging is needed, a cloned `WebGLMsgSender` is used to perform messages regardless of whether the context is garbage collected or not. This isn't a replacement for #37622, but should make it easier to implement as the `WebGLMsgSender` and the `WeakRef` could be stored in the droppable portion of the DOM object. Testing: This fixes a use-after-free issue which is mainly detectable via ASAN builds. Since we do not run ASAN on CI, this is a bit hard to create automated tests for. I verified that this fixed the issue manually. Fixes: #40655. Signed-off-by: Martin Robinson <[email protected]>

willypuzzle · 2025-11-21T13:30:16Z

@jdm I pulled the new modifications of repository and I found this:

self.upcast().send_with_fallibility(WebGLCommand::DeleteProgram(self.id()), operation_fallibility);

Inside a routine called from drop() method.

I haven't found a way to call that from a Droppable struct (that is the struct that should implement Drop trait instead of WebGLProgram)

willypuzzle · 2025-11-21T14:15:41Z

components/script/dom/webgl/webglprogram.rs

+            return;
+        }
+        self.marked_for_deletion.set(true);
+        self.upcast().send_with_fallibility(WebGLCommand::DeleteProgram(self.id()), operation_fallibility);


This is the new code of the mark_for_deletion method, but there is not way to call something of similar to upcast() here @jdm

The droppable struct will need a webgl_sender to do its own communication instead of calling self.upcast().send_with_fallibility.

Where do I get a webgl_sender? I instance a new one?

Clone it from the WebGLObject in the WebGLProgram constructor.

I modified it, tell me if the solution is good for you and after I'll try to replace MutNullableDom<*> with RefCell<Option<WeakRef<*>>>

codecov-commenter · 2025-11-21T14:25:22Z

⚠️ JUnit XML file not found

The CLI was unable to find any JUnit XML files to upload.
For more help, visit our troubleshooting guide.

jdm · 2025-11-22T00:20:03Z

Yeah, that looks like it should work!

willypuzzle · 2025-11-22T15:44:59Z

@jdm the PR is formally correct, that is, it compiles. I pass the ball to you.

jdm

Looking pretty good! I think we can cut down on some of the simple getters and setters to make the code more clear.

components/script/dom/webgl/webglprogram.rs

willypuzzle · 2025-11-23T16:56:20Z

@jdm I applied your suggestions, please take a look :)

jdm

Thank you!

willypuzzle · 2025-12-04T15:36:35Z

@jdm see, if you like my implementation, in my local machine tests seem to pass.

willypuzzle · 2025-12-04T16:11:10Z

@jdm how can I remove this error? https://github.com/servo/servo/actions/runs/19935448487/job/57158859243?pr=37622 I don't rimember how and can't figure out how

components/script/dom/webgl/webglprogram.rs

jdm

Really close now! Thanks for sticking with this.

components/script/dom/webgl/webglprogram.rs

Signed-off-by: Domenico Rizzo <[email protected]>

The handwritten Drop implementation was removed in #37622, so we can now remove the exception for it. Testing: Compile-time generated code can't be meaningfully tested. Fixes: part of #26488 Signed-off-by: Josh Matthews <[email protected]>

willypuzzle · 2025-12-08T15:53:28Z

Thanks God! Finally!

willypuzzle requested a review from gterzian as a code owner June 22, 2025 22:02

jdm reviewed Jun 22, 2025

View reviewed changes

jdm mentioned this pull request Nov 15, 2025

ASAN: heap-use-after-free in canvas_traits::webgl::WebGLMsgSender::send: on jingdong.com #40655

Closed

servo-highfive added the S-needs-rebase There are merge conflict errors. label Nov 15, 2025

mrobinson mentioned this pull request Nov 18, 2025

script: Use a WeakRef for references to WebGLRenderingContext #40725

Merged

willypuzzle force-pushed the 26488-Drop-impls-for-DOM-types-should-be-forbidden-WebGLProgram branch from 3af944a to a00f290 Compare November 21, 2025 14:14

servo-highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-rebase There are merge conflict errors. labels Nov 21, 2025

willypuzzle commented Nov 21, 2025

View reviewed changes

willypuzzle force-pushed the 26488-Drop-impls-for-DOM-types-should-be-forbidden-WebGLProgram branch from a00f290 to af27c80 Compare November 21, 2025 22:50

willypuzzle force-pushed the 26488-Drop-impls-for-DOM-types-should-be-forbidden-WebGLProgram branch 2 times, most recently from db6451d to a3650ca Compare November 22, 2025 15:43

jdm requested changes Nov 23, 2025

View reviewed changes

components/script/dom/webgl/webglprogram.rs Outdated Show resolved Hide resolved

components/script/dom/webgl/webglprogram.rs Outdated Show resolved Hide resolved

components/script/dom/webgl/webglprogram.rs Outdated Show resolved Hide resolved

servo-highfive added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Nov 23, 2025

willypuzzle force-pushed the 26488-Drop-impls-for-DOM-types-should-be-forbidden-WebGLProgram branch from a3650ca to 6a7a6b5 Compare November 23, 2025 16:55

servo-highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 23, 2025

willypuzzle force-pushed the 26488-Drop-impls-for-DOM-types-should-be-forbidden-WebGLProgram branch from 6a7a6b5 to 61bc92d Compare November 24, 2025 13:44

jdm approved these changes Nov 25, 2025

View reviewed changes

servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Nov 25, 2025

jdm added this pull request to the merge queue Nov 25, 2025

servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 25, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 25, 2025

willypuzzle force-pushed the 26488-Drop-impls-for-DOM-types-should-be-forbidden-WebGLProgram branch 2 times, most recently from 1062720 to b871007 Compare December 4, 2025 15:35

willypuzzle force-pushed the 26488-Drop-impls-for-DOM-types-should-be-forbidden-WebGLProgram branch 2 times, most recently from 459a234 to 14939de Compare December 4, 2025 16:04

Taym95 reviewed Dec 4, 2025

View reviewed changes

components/script/dom/webgl/webglprogram.rs Outdated Show resolved Hide resolved

willypuzzle force-pushed the 26488-Drop-impls-for-DOM-types-should-be-forbidden-WebGLProgram branch 2 times, most recently from 8a0dce9 to 3f96680 Compare December 5, 2025 11:32

jdm requested changes Dec 6, 2025

View reviewed changes

servo-highfive added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Dec 6, 2025

willypuzzle force-pushed the 26488-Drop-impls-for-DOM-types-should-be-forbidden-WebGLProgram branch from 3f96680 to 9b5a69a Compare December 6, 2025 15:13

servo-highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Dec 6, 2025

willypuzzle force-pushed the 26488-Drop-impls-for-DOM-types-should-be-forbidden-WebGLProgram branch from 9b5a69a to 4b90663 Compare December 6, 2025 15:19

Refactors WebGLProgram for resource management

df2096e

Signed-off-by: Domenico Rizzo <[email protected]>

willypuzzle force-pushed the 26488-Drop-impls-for-DOM-types-should-be-forbidden-WebGLProgram branch from 4b90663 to df2096e Compare December 7, 2025 21:56

jdm changed the title ~~[#26488] Refactors WebGLProgram for resource management~~ script: Move WebGLProgram Drop implementation to a member type Dec 7, 2025

jdm approved these changes Dec 7, 2025

View reviewed changes

servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Dec 7, 2025

jdm added this pull request to the merge queue Dec 7, 2025

servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Dec 7, 2025

Merged via the queue into servo:main with commit 34160f9 Dec 7, 2025
30 checks passed

servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Dec 7, 2025

jdm mentioned this pull request Dec 8, 2025

script_bindings: Remove Drop impl exception for WebGLProgram. #41124

Merged

willypuzzle deleted the 26488-Drop-impls-for-DOM-types-should-be-forbidden-WebGLProgram branch December 8, 2025 11:32

Uh oh!

Conversation

willypuzzle commented Jun 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willypuzzle Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willypuzzle commented Nov 21, 2025

Uh oh!

willypuzzle Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Nov 21, 2025

⚠️ JUnit XML file not found

Uh oh!

jdm commented Nov 22, 2025

Uh oh!

willypuzzle commented Nov 22, 2025

Uh oh!

jdm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

willypuzzle commented Nov 23, 2025

Uh oh!

jdm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

willypuzzle commented Dec 4, 2025

Uh oh!

willypuzzle commented Dec 4, 2025

Uh oh!

Uh oh!

jdm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

willypuzzle commented Dec 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

willypuzzle Jun 23, 2025 •

edited

Loading

willypuzzle Nov 21, 2025 •

edited

Loading