Closed Bug 1309848 Opened 8 years ago Closed 8 years ago

stylo: Add RefPtr bindings, use for `quotes`

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: manishearth, Assigned: manishearth)

References

Details

Attachments

(1 file, 2 obsolete files)

Bug 1309848 - Add bindings for constructing nsStyleQuoteValues;
58 bytes, text/x-review-board-request
emilio
: review+
Details 
      No description provided.
I am not yet convinced we *need* these. The GeckoArcFoo stuff was working fine before (aside from one method which should have been marked as unsafe). Most unsafe code dealing with RefPtr<T> will still need unsafe, since Gecko RefPtr<T>s can't be trusted. We could make the fields of structs::RefPtr<T> private, though. I'm not sure if that's enough to make it possible to use these without unsafe -- we also need to be able to construct Servo RefPtr<T>s from raw pointers passed over FFI, which accounts for most of the unsafe blocks needed for this.

However, a trait-based solution is still better than a macro-based one, and this doesn't make the unsafe situation worse, so we can keep it even if we don't really need it I guess. And this will probably be something we'll want once Rust stable loses inline drop flags since we'll be able to embed Servo RefPtr<T> directly (the same goes for the situation with nsStringRepr and nsString)
Comment on attachment 8800652 [details]
Bug 1309848 - Add safer bindings for RefPtr;

https://reviewboard.mozilla.org/r/85524/#review84144

Clearing the review flag, I'd like to review the next iteration on this.

::: servo/components/style/gecko_bindings/sugar/refptr.rs:23
(Diff revision 1)
> +pub unsafe trait ThreadSafeRefCounted: RefCounted {}
> +
> +#[derive(Debug)]
> +pub struct RefPtr<T: RefCounted> {
> +    ptr: *mut T,
> +    _marker: PhantomData<T>,

This PhantomData doesn't seem to be needed?

::: servo/components/style/gecko_bindings/sugar/refptr.rs:35
(Diff revision 1)
> +/// as a RefPtr<T> (with refcount 1)
> +///
> +/// This is useful when you wish to create a refptr
> +/// and mutate it temporarily, while it is still
> +/// uniquely owned.
> +pub struct UniqueRefPtr<T: RefCounted>(RefPtr<T>);

What guarantees can we provide so this isn't misused? It seems extremely easy to misuse.

::: servo/components/style/gecko_bindings/sugar/refptr.rs:41
(Diff revision 1)
> +
> +impl<T: RefCounted> RefPtr<T> {
> +    /// Create a new RefPtr from an already addrefed
> +    /// pointer obtained from FFI. Pointer
> +    /// must be valid, non-null and have been addrefed
> +    pub unsafe fn new(ptr: *mut T) -> Self {

Assert it's not null?

::: servo/components/style/gecko_bindings/sugar/refptr.rs:50
(Diff revision 1)
> +        }
> +    }
> +    /// Create a new RefPtr from a pointer obtained
> +    /// from FFI. Pointer must be valid and non null.
> +    /// This method calls addref() internally
> +    pub unsafe fn new_addref(ptr: *mut T) -> Self {

I think we usually want `new_addref` to be the default?

I mean, a leak is way better than a UAF in every possible sense.

Couldn't we make `new` addref by default, then another method with a more scary name to do the opposite?
Attachment #8800652 - Flags: review?(ecoal95)
Comment on attachment 8800653 [details]
Bug 1309848 - Add bindings for constructing nsStyleQuoteValues;

https://reviewboard.mozilla.org/r/85526/#review84154
Attachment #8800653 - Flags: review?(ecoal95) → review+
Comment on attachment 8800654 [details]
Bug 1309848 - stylo: implement |quotes| property;

https://reviewboard.mozilla.org/r/85528/#review84156

Since it depends on the first patch and that can make this change, I'm clearing the review flag, but looks good appart from that.

::: servo/components/style/properties/gecko.mako.rs:1467
(Diff revision 1)
> +        use gecko_bindings::bindings::Gecko_NewStyleQuoteValues;
> +        use gecko_bindings::sugar::refptr::UniqueRefPtr;
> +        use nsstring::nsCString;
> +
> +        let mut refptr = unsafe {
> +            UniqueRefPtr::new(Gecko_NewStyleQuoteValues(other.0.len() as u32))

I'd rather use the raw pointer here instead of creating an abstraction like `UniqueRefPtr`, though I'd like to see your argument in the previous patch.
Attachment #8800654 - Flags: review?(ecoal95)
Comment on attachment 8800652 [details]
Bug 1309848 - Add safer bindings for RefPtr;

https://reviewboard.mozilla.org/r/85524/#review84144

> This PhantomData doesn't seem to be needed?

It's for drop semantics

> What guarantees can we provide so this isn't misused? It seems extremely easy to misuse.

You can't misuse it. It doesn't implement clone. All you can do is drop it or turn it into a refptr.

> I think we usually want `new_addref` to be the default?
> 
> I mean, a leak is way better than a UAF in every possible sense.
> 
> Couldn't we make `new` addref by default, then another method with a more scary name to do the opposite?

Basically, the defaults depend on the use case. When we get refptrs from Gecko in `Servo_*` function call parameters, we get them as un-addrefed pointers. When we get them from the return values of `Gecko_*`, it's already addrefed.

But yeah, swapping the defaults makes sense.
Comment on attachment 8800652 [details]
Bug 1309848 - Add safer bindings for RefPtr;

https://reviewboard.mozilla.org/r/85524/#review84144

> You can't misuse it. It doesn't implement clone. All you can do is drop it or turn it into a refptr.

I meant you can mutate it if you create one even though the inner pointer is not unique. How can't you misuse it that way?
> I meant you can mutate it if you create one even though the inner pointer is not unique. How can't you misuse it that way?

That's why the creation method is unsafe and the type has `Unique` in its name. That should be clear enough imo.
Comment on attachment 8800652 [details]
Bug 1309848 - Add safer bindings for RefPtr;

https://reviewboard.mozilla.org/r/85524/#review84144

> Basically, the defaults depend on the use case. When we get refptrs from Gecko in `Servo_*` function call parameters, we get them as un-addrefed pointers. When we get them from the return values of `Gecko_*`, it's already addrefed.
> 
> But yeah, swapping the defaults makes sense.

Regarding using a raw pointer instead: I had thought about it (I don't like having an extra type for this purpose). I'd prefer if raw pointers were consumed *immediately* after the FFI boundary and turned into safe types, as much as possible. It can be misused, but not more than raw pointers, or `RefPtr::new_from_addrefed`. I've renamed its constructor to also be `new_from_addrefed` which should make it clearer.
Comment on attachment 8800652 [details]
Bug 1309848 - Add safer bindings for RefPtr;

https://reviewboard.mozilla.org/r/85524/#review84212

Generally looks good. I wish that stylo was far enough along that we could share the rust binding infrastructure with gecko.

::: servo/components/style/gecko_bindings/sugar/refptr.rs:14
(Diff revision 2)
> +use std::ops::{Deref, DerefMut};
> +
> +/// Trait for all objects that have Addref() and Release
> +/// methods and can be placed inside RefPtr<T>
> +pub trait RefCounted {
> +    fn addref(&self);

mRefCnt in gecko is usually a `uint32_t` equivalent IIRC. If this method was in the rust standard library, `addref()` would not be safe because of that. 

I think it's fine to mark it as safe here though. These bindings already have some "technically unsafe" code, and this is probably far from the worst offender.

::: servo/components/style/gecko_bindings/sugar/refptr.rs:18
(Diff revision 2)
> +pub trait RefCounted {
> +    fn addref(&self);
> +    unsafe fn release(&self);
> +}
> +
> +pub unsafe trait ThreadSafeRefCounted: RefCounted {}

I feel like ThreadSafeRefCounted implies a Sync bound. I'm not sure if that's worth writing here.

::: servo/components/style/gecko_bindings/sugar/refptr.rs:22
(Diff revision 2)
> +
> +pub unsafe trait ThreadSafeRefCounted: RefCounted {}
> +
> +#[derive(Debug)]
> +pub struct RefPtr<T: RefCounted> {
> +    ptr: *mut T,

I feel like you want a `*const T` here. It's the type which is used in `Shared<T>` and IIRC it has better variance for this situation.

::: servo/components/style/gecko_bindings/sugar/refptr.rs:37
(Diff revision 2)
> +/// This is useful when you wish to create a refptr
> +/// and mutate it temporarily, while it is still
> +/// uniquely owned.
> +pub struct UniqueRefPtr<T: RefCounted>(RefPtr<T>);
> +
> +impl<T: RefCounted> RefPtr<T> {

I feel like there should be a way to safely construct an `RefPtr<T>` from an `&T`, such as `RefPtr::new()` or `impl<T: RefCounted> From<&T> for RefPtr<T>`.

I think that this might cause problems with `UniqueRefPtr`, as it would then be possible to addref the value inside of the `UniqueRefPtr` by creating a RefPtr from the value inside, which is unfortunate. In gecko we do do that sort of conversion (from borrowed to owned) all of the time, however, so I worry that you will be wanting it in stylo.

While writing this comment I talked myself out of saying you should implement this. I think that a safe UniqueRefPtr is probably more valuable. Perhaps you should add a comment explaining that this conversion does not exist in order to keep UniqueRefPtr safe.

::: servo/components/style/gecko_bindings/sugar/refptr.rs:41
(Diff revision 2)
> +
> +impl<T: RefCounted> RefPtr<T> {
> +    /// Create a new RefPtr from an already addrefed
> +    /// pointer obtained from FFI. Pointer
> +    /// must be valid, non-null and have been addrefed
> +    pub unsafe fn new_from_addrefed(ptr: *mut T) -> Self {

The `new_` prefix seems unnecessary. Perhaps `from_already_addrefed` to parallel with `already_AddRefed` in gecko, or just `from_addrefed`?

::: servo/components/style/gecko_bindings/sugar/refptr.rs:59
(Diff revision 2)
> +            ptr: ptr,
> +            _marker: PhantomData,
> +        };
> +        ret.addref();
> +        ret
> +    }

nit: add spaces between function declarations in this impl block

::: servo/components/style/gecko_bindings/sugar/refptr.rs:78
(Diff revision 2)
> +    // Addref the inner data
> +    //
> +    // Leaky on its own

nit: Make this a doc comment

::: servo/components/style/gecko_bindings/sugar/refptr.rs:152
(Diff revision 2)
> +    }
> +
> +    /// Replace a structs::RefPtr<T> with a different one, appropriately addref/releasing
> +    ///
> +    /// Must be valid, but can be null
> +    pub unsafe fn set(&mut self, other: &Self) {

I think you should also have a set() which takes other by value, and doesn't addref on assignment, only releasing the old value. Perhaps `set_move()`?

::: servo/components/style/gecko_bindings/sugar/refptr.rs:153
(Diff revision 2)
> +        if !self.mRawPtr.is_null() {
> +            (*self.mRawPtr).release();
> +        }
> +        if !other.mRawPtr.is_null() {
> +            *self = other.to_safe().forget();
> +        }

This is wrong. In the case where other.mRawPtr.is_null(), and !self.mRawPtr.is_null() you end up with a non-null mRawPtr which points to potentially invalid memory!

This should be:
```
if !self.mRawPtr.is_null() {
    (*self.mRawPtr).release();
}
self.mRawPtr = ptr::null_mut();
if !other.mRawPtr.is_null() {
    *self = other.to_safe().forget();
}
```

::: servo/components/style/gecko_bindings/sugar/refptr.rs:191
(Diff revision 2)
> +// Companion of NS_DECL_THREADSAFE_FFI_REFCOUNTING
> +//
> +// Gets you a free RefCounted impl
> +macro_rules! impl_threadsafe_refcount {
> +    ($t:ty, $addref:ident, $release:ident) => (
> +        impl RefCounted for $t {
> +            fn addref(&self) {
> +                unsafe { ::gecko_bindings::bindings::$addref(self as *const _ as *mut _) }
> +            }
> +            unsafe fn release(&self) {
> +                ::gecko_bindings::bindings::$release(self as *const _ as *mut _)
> +            }
> +        }
> +        unsafe impl ThreadSafeRefCounted for $t {}
> +    );
> +}
> +
> +impl_threadsafe_refcount!(::gecko_bindings::bindings::ThreadSafePrincipalHolder,
> +                          Gecko_AddRefPrincipalArbitraryThread,
> +                          Gecko_ReleasePrincipalArbitraryThread);
> +impl_threadsafe_refcount!(::gecko_bindings::bindings::ThreadSafeURIHolder,
> +                          Gecko_AddRefURIArbitraryThread,
> +                          Gecko_ReleaseURIArbitraryThread);
> +impl_threadsafe_refcount!(::gecko_bindings::structs::nsStyleQuoteValues,
> +                          Gecko_AddRefQuoteValuesArbitraryThread,
> +                          Gecko_ReleaseQuoteValuesArbitraryThread);

I feel like this should be a `macro_rules! impl_refcount` and then threadsafe ones can have an extra `unsafe impl ThreadSafeRefCounted for $t` afterwards. Having a macro specifically for threadsafe refcounts when it only adds a single extra line feels silly.
Attachment #8800652 - Flags: review?(michael) → review-
(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> This PhantomData doesn't seem to be needed?

The PhantomData is required IIRC to get the variance right for drop safety. It's used in `Shared<T>` which would be the pointer type I would recommend using here if it wasn't unstable.
Comment on attachment 8800652 [details]
Bug 1309848 - Add safer bindings for RefPtr;

https://reviewboard.mozilla.org/r/85524/#review84144

> I meant you can mutate it if you create one even though the inner pointer is not unique. How can't you misuse it that way?

That's why the function is unsafe, the type has "Unique" in its name, and the function has "addrefed" in its name.

The alternative is to use raw pointers, which have exactly the same problem but are worse.
Comment on attachment 8800652 [details]
Bug 1309848 - Add safer bindings for RefPtr;

https://reviewboard.mozilla.org/r/85524/#review84212

> I feel like ThreadSafeRefCounted implies a Sync bound. I'm not sure if that's worth writing here.

These are all FFI types with raw pointers, so no `Sync` bound. I think the trait itself should be unsafe.

> I feel like you want a `*const T` here. It's the type which is used in `Shared<T>` and IIRC it has better variance for this situation.

I put a `*mut T` because we don't know how the refcount is maintained (usually a field, could be something else) and it's best to pretend it's mutable. Variance doesn't matter since T won't have lifetimes in it, and `*mut T` is stricter anyway so that works out. This way there are fewer casts, too.

> I think you should also have a set() which takes other by value, and doesn't addref on assignment, only releasing the old value. Perhaps `set_move()`?

We won't need it. `structs::RefPtr` only exists inside style structs, which means that you cannot consume by-move.

> I feel like this should be a `macro_rules! impl_refcount` and then threadsafe ones can have an extra `unsafe impl ThreadSafeRefCounted for $t` afterwards. Having a macro specifically for threadsafe refcounts when it only adds a single extra line feels silly.

This is stylo, everything is (or should be) threadsafe anyway (that's why there's no NS_DECL_FFI_REFCOUNTING on the C++ side). I kept the threadsafe trait just in case we need to use it for something non-threadsafe, but for the property glue we shouldn't need it at all.
Comment on attachment 8800652 [details]
Bug 1309848 - Add safer bindings for RefPtr;

https://reviewboard.mozilla.org/r/85524/#review84242
Attachment #8800652 - Flags: review?(michael) → review+
Don't know how to re-request cleared review, so ni?ing.

This is the next iteration :) Haven't removed unique yet because the alternative is raw pointers, or two-step construction (FFI to construct, then FFI to box it). We could box it on the rust side if we need to, but then we need to introduce conversions between `Box<T>` and `RefPtr<T>`.
Flags: needinfo?(ecoal95)
Comment on attachment 8800652 [details]
Bug 1309848 - Add safer bindings for RefPtr;

https://reviewboard.mozilla.org/r/85524/#review84270

I guess I'm fine with `UniqueRefPtr`, though I hope there were more stuff we could assert there.

::: servo/components/style/gecko_bindings/sugar/refptr.rs:162
(Diff revision 4)
> +    /// Both `self` and `other` must be valid, but can be null
> +    pub unsafe fn set(&mut self, other: &Self) {
> +        if !self.mRawPtr.is_null() {
> +            (*self.mRawPtr).release();
> +        }
> +        self.mRawPtr = ptr::null_mut();

nit: You can move `self.mRawPtr = ptr::null_mut()` inside the first conditional.

::: servo/components/style/gecko_bindings/sugar/refptr.rs:171
(Diff revision 4)
> +    }
> +
> +    /// Replace a `structs::RefPtr<T>` with a `RefPtr<T>`,
> +    /// consuming the `RefPtr<T>`, and releasing the old
> +    /// value in `self` if necessary.
> +    /// 

nit: trailing whitespace.
Attachment #8800652 - Flags: review?(ecoal95) → review+
Comment on attachment 8800654 [details]
Bug 1309848 - stylo: implement |quotes| property;

https://reviewboard.mozilla.org/r/85528/#review84274
Attachment #8800654 - Flags: review?(ecoal95) → review+
Flags: needinfo?(ecoal95)
Attachment #8800653 - Flags: review?(bobbyholley)
Comment on attachment 8800653 [details]
Bug 1309848 - Add bindings for constructing nsStyleQuoteValues;

https://reviewboard.mozilla.org/r/85526/#review84278
Attachment #8800653 - Flags: review?(bobbyholley) → review+
Attachment #8800652 - Attachment is obsolete: true
Attachment #8800654 - Attachment is obsolete: true
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/163d5cd6befa
Add bindings for constructing nsStyleQuoteValues; r=bholley,emilio
https://hg.mozilla.org/mozilla-central/rev/163d5cd6befa
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: