script: Use a WeakRef for references to WebGLRenderingContext
#40725
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e1a2c4a to
4ec2c4a
Compare
|
🔨 Triggering try run (#19477498670) for Linux (WPT) |
|
Test results for linux-wpt from try job (#19477498670): Flaky unexpected result (53)
Stable unexpected results that are known to be intermittent (22)
|
|
✨ Try run (#19477498670) succeeded. |
mukilan
approved these changes
Nov 19, 2025
| if !std::ptr::eq(program.context(), &*self.base) { | ||
| self.base.webgl_error(InvalidOperation); | ||
| if let Err(error) = self.base.validate_ownership(program) { | ||
| self.base.webgl_error(error); |
Member
There was a problem hiding this comment.
Is it correct that this sets the last_error to a ContextLost error in the case that the program's owning context is lost? Presumably that will only happen if the self context is not the owner, as the current method means the self context is still alive, so shouldn't it still return an INVALID_OPERATION error?
Member
Author
There was a problem hiding this comment.
Good point. I have modified validate_ownership to return Err(InvalidOperation) in this case.
| } | ||
|
|
||
| let context = self.upcast::<WebGLObject>().context(); | ||
| let Some(context) = self.upcast::<WebGLObject>().context() else { |
Member
There was a problem hiding this comment.
Seem like in many places we can remove the ::<WebGLObject> annotation as WebGLObject is the only ancestor interface and the type checker is able to infer that.
Member
Author
There was a problem hiding this comment.
I've removed all type annotations on these type of upcasts.
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 changes 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. Signed-off-by: Martin Robinson <mrobinson@igalia.com>
4ec2c4a to
48ed31c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Many WebGL objects refer to a
WebGLRenderingContextand rely on it formessaging to the
WebGLThread. This poses a problem, because WebGLobjects often need to send a message to the
WebGLThreadduring theirDropimplementation. If theDropis triggered as part of garbagecollection, references to the
WebGLRenderingContextmight 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
WeakRefinstead of a
Dom<>. TheWeakRefis only used if it can be rooted,otherwise a
ContextLosterror is given. In cases where only messagingis needed, a cloned
WebGLMsgSenderis used to perform messagesregardless 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
WebGLMsgSenderand theWeakRefcould be stored in the droppableportion 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.