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

Documentation for Heap::handle is unclear #536

Open
jdm opened this issue Dec 14, 2024 · 2 comments
Open

Documentation for Heap::handle is unclear #536

jdm opened this issue Dec 14, 2024 · 2 comments

Comments

@jdm
Copy link
Member

jdm commented Dec 14, 2024

mozjs/mozjs-sys/src/jsgc.rs

Lines 333 to 348 in d6c3bd9

/// Retrieves a Handle to the underlying value.
///
/// # Safety
///
/// This is only safe to do on a rooted object (which Heap is not, it needs
/// to be additionally rooted), like RootedGuard, so use this only if you
/// know what you're doing.
///
/// # Notes
///
/// Since Heap values need to be informed when a change to underlying
/// value is made (e.g. via `get()`), this does not allow to create
/// MutableHandle objects, which can bypass this and lead to crashes.
pub unsafe fn handle(&self) -> JS::Handle<T> {
JS::Handle::from_marked_location(self.ptr.get() as *const _)
}

I no longer remember when "if you know what you're doing applies", but we have a lot of usages in Servo which makes me nervous.

@sagudev
Copy link
Member

sagudev commented Dec 14, 2024

IIUC this is only safe when done on RootedGuard<Heap<T>> (or similar, something that ensures that heap is rooted), so we could just do

impl RootedGuard<Heap<T>> {
pub fn handle(&self) -> JS::Handle<T> { 
         JS::Handle::from_marked_location(self.ptr.get() as *const _) 
     } 
}

@Redfire75369
Copy link
Contributor

From my understanding, it's also safe if you have any Heap<T>, as long as it's traced, eg via a custom trace op or a global tracer.

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

No branches or pull requests

3 participants