Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove DerefMut impl for RootedGuard #572

Merged
merged 3 commits into from
Mar 28, 2025
Merged

Conversation

gmorenz
Copy link
Contributor

@gmorenz gmorenz commented Mar 26, 2025

  • We no longer construct MutableHandle via deref_mut. Doing so meant that the raw pointer in MutableHandle was only valid for as long as the mutable reference created in deref_mut would be. I.e. until the next time GC runs. This is an extension of RootedGuard<T> construction related unsoundness #564, and doesn't require a change in public API.
  • It removes the deref_mut impl entirely, as it's an unsafe footgun that isn't just marked as safe, but is silently invoked.
  • Creates a helper take function for RootedGuard<Option<T>>, since that was the primary use of DerefMut in servo, and is a safe thing to do.
  • It adds allow_unrooted_interior to MutableHandle because the accompanying servo PR requires that crown understand MutableHandle may point to things that must be rooted over GC passes.

Accompanying servo PR: servo/servo#36158

Addresses a small part of #569.

Signed-off-by: Greg Morenz <greg-morenz@droid.cafe>
@gmorenz
Copy link
Contributor Author

gmorenz commented Mar 26, 2025

Servo's CI is complaining because MutableHandle<T> is unrooted, and crown doesn't know about that type. So I've added another commit to this PR that adds crown::unrooted_must_root_lint::allow_unrooted_interior to MutableHandle. I believe that's correct because the purpose of this Handle is to point at something that is rooted... and thus the GC will update everything it needs to. That said I don't deeply understand what crown is checking.

@gmorenz gmorenz force-pushed the rooted_guard_deref_mut branch from a54f814 to 21a954b Compare March 26, 2025 21:03
gmorenz added 2 commits March 27, 2025 15:45
Signed-off-by: Greg Morenz <greg-morenz@droid.cafe>
Signed-off-by: Greg Morenz <greg-morenz@droid.cafe>
@gmorenz gmorenz force-pushed the rooted_guard_deref_mut branch from 21a954b to da7d6e1 Compare March 27, 2025 19:45
Copy link
Member

@sagudev sagudev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but with this GC stuff it's always good to have another pair of eyes.

@sagudev sagudev requested a review from jdm March 27, 2025 20:34
@jdm jdm added this pull request to the merge queue Mar 28, 2025
Merged via the queue into servo:main with commit 36f1e77 Mar 28, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants