1275913 - stylo: Represent already_AddRefed safely from Servo

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

This patch allows representing already_AddRefed values from servo using the type system.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: 0001-stylo-Introduce-a-safe-way-to-represent-already_AddR.patch

Comment 2

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8203e580d93a

Comment 3

[Emilio Cobos Álvarez (:emilio)]

BTW, Bobby, sorry for the huge delay for the patch, but I was falling into an unrelated assertion in stylo, and I wanted to make sure this wasn't the cause.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Hg patch

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Bah, yay for windows, who can't detect it's just a simple pointer:

> c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\obj-firefox\dist\include\mozilla/ServoBindings.h(187): error C2526: 'Servo_GetComputedValues': C linkage function cannot return C++ class 'ServoFFIAddRefed<ServoComputedValues>'
> c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\obj-firefox\dist\include\mozilla/ServoBindings.h(192): error C2526: 'Servo_GetComputedValuesForAnonymousBox': C linkage function cannot return C++ class 'ServoFFIAddRefed<ServoComputedValues>'
> c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\obj-firefox\dist\include\mozilla/ServoBindings.h(199): error C2526: 'Servo_GetComputedValuesForPseudoElement': C linkage function cannot return C++ class 'ServoFFIAddRefed<ServoComputedValues>'
> c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\obj-firefox\dist\include\mozilla/ServoBindings.h(202): error C2526: 'Servo_InheritComputedValues': C linkage function cannot return C++ class 'ServoFFIAddRefed<ServoComputedValues>'

Comment 6

[Bobby Holley (:bholley)]

Given that the struct method didn't work, what do you think about an approach like the one I sketched out in https://pastebin.mozilla.org/8878741 ?

Note that we could also use this to represent UniquePtr.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (busy) from comment #6)
> Given that the struct method didn't work, what do you think about an
> approach like the one I sketched out in https://pastebin.mozilla.org/8878741
> ?
> 
> Note that we could also use this to represent UniquePtr.

Yeah, I think that might work, this week is being kind of hectic, and I'm trying to get animations landed in Servo, but could be a nice improvement.

I'm wondering how something like this would look like in the rust side, but if it's fine enough I can try to do it after this week :)

Comment 8

[Bobby Holley (:bholley)]

Continuing the conversation from bug 1291885:

(In reply to Manish Goregaokar [:manishearth] from comment #11)
> > It's might be kind of up Manish's alley given the other FFI safety stuff
> > he's been working on. Manish, any chance you might have the cycles to fix
> > our already_AddRefed story at some point?
> 
> At some point -- probably, but not too soon. I can prioritize it if it needs
> to be though.

I think its priority is on par with the nsStyleCoord safety stuff - we can live without it, but the additional safety abstractions will save us time in the long run.

> I'm also not yet clear on how we're using it in Servo at the moment, so I'd
> have to investigate that.

We're not really. My pastebin from comment 6 died, unfortunately, but the basic idea would be to have some kind of handshake pointer type that indicates "I am a strong reference". This doesn't help us with the "strong references must be consumed by an owning type" part of already_AddRefed (since that requires a destructor, which is hard to pass over FFI), but it does help us avoid leaks like the ones in https://hg.mozilla.org/mozilla-central/rev/14c9cd79e79e

Ideally we'd use this bidirectionally, and have something for both Arc/RefPtr and Box/UniquePtr.

> Leaving this needinfo open till I have a closer look.

Comment 9

[Manish Goregaokar [:manishearth]]

So AIUI the problem in bug 1291885 is that we're returning addreffed values and not using already_AddRefed, correct?

Some observations (tell me if I'm wrong on any of these):

We're treating RefPtr<T> as if it's Rc<T> (or Arc), but it's actually not. It's more like `Rc<Option<T>>`. already_AddRefed is also `Rc<Option<T>>`, but with a leak-protected destructor that asserts on Some(T). The C++ RAII pattern of nontrivial copy/move constructors doesn't translate cleanly to Rust, sadly. Rust's stronger move semantics means that it allows for equivalent types in pure Rust, but translating between them is tricky.

I'm not sure if we ever expect a cleared-out RefPtr from the Gecko side.

We also only care about already_AddRefed at the boundary. We should be dealing with RefPtr or Arc everywhere else.

Finally, looking at ArcHelpers I noticed that we're assuming that the layout of Arc is the same as the layout of Gecko threadsafe refcounted boxes. This assumption isn't correct, Rust structs have an undefined layout. Eddy claims that in the current implementation this should be fine, but it's something that could change any moment now (in the nightly -- till 1.12 it should be fine).


-------------------

A rough proposal for safety follows:

We add bindings to already_AddRefed or a servo-specific copy of it. The struct is marked private. On the Servo side, dereferencing already_AddRefed is unsafe (perhaps even private). It has two methods: One which constructs a RefPtr from it, and one which constructs it from a RefPtr. Both methods assert on a nullptr. The RefPtr construction method consumes the RefPtr by move, so we don't need to implicitly addref it.

RefPtr itself is the set of servo-side bindings to RefPtr or a copy of its definition. This struct is dereferenceable (but with unsafe private fields as usual). I'm not sure how we should handle this Option-like behavior -- how often does this occur in practice? We can make it 100% safe by having a `.try_deref()` method that returns an `Option<&T>` or whatever.

Both already_AddRefed and RefPtr are nocopy. already_AddRefed will only exist at the FFI boundary, where it is immediately converted to/from RefPtr. Arc/Rc stop existing in servo-side stylo code that is shared with Gecko. We pass RefPtr around as if it's a regular-joe Arc, because really it's the same thing. We need to somehow ensure that we never return RefPtr to Gecko directly, unless behind a different pointer. Easy to do with a lint, but not on stable.

I'm assuming that Servo is not only receiving RefPtr from calls into gecko that return already_AddRefed, but also as children of other structs. I think that manipulating these via the RefPtr abstraction on the Servo side is still safe, though again I'm not sure what we should do about nullptr.

I'd love for RefPtr<T> to be bounded on a trait. We could generate trait impls for things containing NS_DECL_INLINE_THREADSAFE_REFCOUNTING via bindgen (probably?), though that's some very gecko-specific code making its way into bindgen. Alternatively, we can manually impl this on the servo side with a macro that simultaneously creates a compile-time test that ensures that it has some fields (simplest way of doing this is a `#[test]` block with an unused function inside it, though we could use compiletest-rs if we want).

Another concern about RefPtr is mutability. Mutable aliasing in Rust is undefined behavior (not just that it can cause unsafety like it does in C++ -- the optimizer assumes it doesn't happen in absence of UnsafeCell so benign-looking mutable aliasing can cause problems). We can make RefPtr immutable but I'm not sure if that will be enough for us. Quick glance at Servo's current usage says that it should be (so far). If we need mutability, we can use something like RefCell, but there's no place to store the additional borrow flag. We can use a Cell-like model but that involves a deep copy on each get/set, which could be bad depending on the contents.

This is another one of those incompatibilities between C++ and Rust that's hard to get right when making them talk to each other. Clearly we should rewrite Gecko in---oh wait never mind.


Does this sound about right?

Comment 10

[Manish Goregaokar [:manishearth]]

I suspected I was wrong above, so I had a chat with mystor. I was :)

 - The issue isn't that we're returning RefPtr instead of already_AddRefed, the issue is that we're returning raw pointers instead of already_AddRefed, and the constructor of RefPtr for raw pointers does an addref. Returning a RefPtr is safe because that goes through the move constructor.

 - We don't cast between Arc and RefPtr; instead, Arc has its own AddRef and Release impls. This is slightly better, but ArcHelpers is still super unsafe :)

----

The problem with returning RefPtr or already_AddRefed is that both types are templated and can't be put in `extern "C"` on msvc. Apparently this is *supposed* to work but doesn't.

We have multiple solutions:

The first solution is to use a macro. We wrap the Servo_GetFoo functions with macros that implicitly dont_addRef() values. This can be done by having the macro mangle the old function name and define a new function with the old name, or just having it shadow the function.

The second solution is to always use outparams for RefPtr return values. We remove most methods on ArcHelpers and give it two abilities -- the ability to take a null refptr outparam and replace it with a given value, and the ability to convert a raw pointer to a bindgen-RefPtr. I'm not a fan of outparams, but this seems nicer -- it forces you to use it safely (on the Rust side).

Furthermore, ArcHelpers is bounded by an unsafe trait with an associated type param for the Servo-side type. This trait must be manually implemented for types and is required for use with RefPtr.

RefPtr assumes that the insides may be null (apparently null refptrs are a common occurrence), and has a method that provides Option<&T>. RefPtr itself cannot be cloned; Servo cannot trigger addrefs. If we have need for more complicated manipulation of RefPtrs, we can create a SafeRefPtr type that can be constructed from a non-null RefPtr. It is bounded on the same unsafe trait (the trait provides addref and decref methods). I don't think we need this yet, though. If we do, mystor has a similar abstraction in bug 1293362.

(To make RefPtr cloneable we can also bound it on the same trait. The issue is that this requires bindgen hackery, but it's of the kind that we can merge upstream so we can do this if necessary)

Comment 11

[Bobby Holley (:bholley)]

I also would rather avoid outparams (though I'm open to considering them if nothing else solves our problem). But I think there's a simpler approach.

The basic problem we have right now is that we don't have any way to indicate (in a way that is understandable to both C++ and bindgen-generated Rust) the ownership semantics of a pointer type (borrowed, strong, or owning reference). This is primarily an issue with return values, though occasionally an issue with arguments (arguments are generally assumed to be borrowed).

My proposal from comment 6 (which pastebin unhelpfully deleted) was more or less to define an associated opaque pointer type for every reference-counted type whose sole purpose was to convey the presence of a strong ref across the FFI boundary. ArcHelpers would know how to convert to/from these types, and we'd define a similar helper on the C++ side to do the same. We would convert at the boundary and never use the types for anything else. We could use templates to do this pretty easily, I think.

The main downside of using a raw pointer type to indicate a strong reference is that we don't have an automatic way to ensure that the type is consumed exactly once (0 times is a leak, > 1 times is a security bug). However, for FFI functions that are designed to return an object (like Servo_GetComputedValues) are pretty hard to screw up in this respect.

YDYT?

Comment 12

[Nika Layzell [:nika] (ni? for response)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> I also would rather avoid outparams (though I'm open to considering them if
> nothing else solves our problem). But I think there's a simpler approach.
> 
> The basic problem we have right now is that we don't have any way to
> indicate (in a way that is understandable to both C++ and bindgen-generated
> Rust) the ownership semantics of a pointer type (borrowed, strong, or owning
> reference). This is primarily an issue with return values, though
> occasionally an issue with arguments (arguments are generally assumed to be
> borrowed).
> 
> My proposal from comment 6 (which pastebin unhelpfully deleted) was more or
> less to define an associated opaque pointer type for every reference-counted
> type whose sole purpose was to convey the presence of a strong ref across
> the FFI boundary. ArcHelpers would know how to convert to/from these types,
> and we'd define a similar helper on the C++ side to do the same. We would
> convert at the boundary and never use the types for anything else. We could
> use templates to do this pretty easily, I think.
> 
> The main downside of using a raw pointer type to indicate a strong reference
> is that we don't have an automatic way to ensure that the type is consumed
> exactly once (0 times is a leak, > 1 times is a security bug). However, for
> FFI functions that are designed to return an object (like
> Servo_GetComputedValues) are pretty hard to screw up in this respect.
> 
> YDYT?

The big problem with this idea is that we cannot put templates in the FFI function signatures. MSVC doesn't allow C++ templates to be written directly in the return type of an extern "C" function. (other compilers do). If we wanted to have a specialized type which marked a strong reference, then we would need a different concrete non-template struct type for each type of pointer we would want to send across the language boundary.

An out parameter has the advantage of not requiring one of these template types in the function signature, which means that MSVC won't get all grumpy and error out. It also has the same guarantee that the value isn't over-used (assuming that the caller uses getter_AddRefs). It is ugly though.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Michael Layzell [:mystor] from comment #12)
> The big problem with this idea is that we cannot put templates in the FFI
> function signatures. MSVC doesn't allow C++ templates to be written directly
> in the return type of an extern "C" function. (other compilers do).

Do we actually need the functions to be extern "C"? Now that we have C++ support in bindgen, what would happen if we removed that?

> If we
> wanted to have a specialized type which marked a strong reference, then we
> would need a different concrete non-template struct type for each type of
> pointer we would want to send across the language boundary.

Seems like we could do this with a macro if it came down to it.

> An out parameter has the advantage of not requiring one of these template
> types in the function signature

I'd think it would, unless we want to abandon the type safety and just mandate a convention that outparam means strong ref.

>, which means that MSVC won't get all grumpy
> and error out. It also has the same guarantee that the value isn't over-used
> (assuming that the caller uses getter_AddRefs). It is ugly though.

Yeah, it feels pretty XPCOM-y, which is kind of the opposite direction from where we'd like to go with this...

Comment 14

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #13)
> (In reply to Michael Layzell [:mystor] from comment #12)
> > The big problem with this idea is that we cannot put templates in the FFI
> > function signatures. MSVC doesn't allow C++ templates to be written directly
> > in the return type of an extern "C" function. (other compilers do).
> 
> Do we actually need the functions to be extern "C"? Now that we have C++
> support in bindgen, what would happen if we removed that?

The you would be using another calling convention that rust doesn't support, so bad things would happen.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #14)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #13)
> > (In reply to Michael Layzell [:mystor] from comment #12)
> > > The big problem with this idea is that we cannot put templates in the FFI
> > > function signatures. MSVC doesn't allow C++ templates to be written directly
> > > in the return type of an extern "C" function. (other compilers do).
> > 
> > Do we actually need the functions to be extern "C"? Now that we have C++
> > support in bindgen, what would happen if we removed that?
> 
> The you would be using another calling convention that rust doesn't support,
> so bad things would happen.

How do we deal with this for SM?

Comment 16

[Bobby Holley (:bholley)]

But anyway - some macro like DEFINE_STRONG_REF_TYPE and DEFINE_OWNING_REF_TYPE should also work fine.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #15)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #14)
> > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #13)
> > > (In reply to Michael Layzell [:mystor] from comment #12)
> > > > The big problem with this idea is that we cannot put templates in the FFI
> > > > function signatures. MSVC doesn't allow C++ templates to be written directly
> > > > in the return type of an extern "C" function. (other compilers do).
> > > 
> > > Do we actually need the functions to be extern "C"? Now that we have C++
> > > support in bindgen, what would happen if we removed that?
> > 
> > The you would be using another calling convention that rust doesn't support,
> > so bad things would happen.
> 
> How do we deal with this for SM?

We use the JS and JS friend API in general, which use the C calling convention if I'm not wrong. Maybe marking the functions as exported symbols will make the template thing work.

Comment 18

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #17)
> We use the JS and JS friend API in general, which use the C calling
> convention if I'm not wrong. Maybe marking the functions as exported symbols
> will make the template thing work.

It seems like it ought to be solvable somehow, given that we use templated types as function arguments in rust-mozjs:

https://github.com/servo/rust-mozjs/blob/dc5652899658f739c4f2fc16d433309e21213231/src/jsapi_windows_msvc14_64.rs#L6327

Comment 19

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #18)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #17)
> > We use the JS and JS friend API in general, which use the C calling
> > convention if I'm not wrong. Maybe marking the functions as exported symbols
> > will make the template thing work.
> 
> It seems like it ought to be solvable somehow, given that we use templated
> types as function arguments in rust-mozjs:
> 
> https://github.com/servo/rust-mozjs/blob/
> dc5652899658f739c4f2fc16d433309e21213231/src/jsapi_windows_msvc14_64.rs#L6327

Yeah, but a few things noted:

 * MozJS doesn't return template parameters (which could make a difference?).
 * MozJS doesn't call into rust using template parameters.
 * MozJS has a jsglue.cpp file with some functions that presumably we can't use directly from rust? (https://github.com/servo/rust-mozjs/blob/dc5652899658f739c4f2fc16d433309e21213231/src/jsglue.cpp)

I think it's worth checking with jdm, Ms2ger or nox to see if any of them remember why are things that way instead of spending a lot of time discovering it the hard way.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Oh, btw, just for the record, this evening on IRC we realized of the difference between Stylo and SM bindings. Basically, trying to just use |extern| instead of |extern "C"| *should* work, so far. Differences:

 * Using the default CC instead of C's, not a problem in platforms we care about it seems.
 * Symbol mangling: This *can* be a problem, at least until we run bindgen on builds, given we'll need to have different files per platform, so adding a new function could be a pain.

Comment 21

[Manish Goregaokar [:manishearth]]

Vlad worked on the Servo MSVC stuff, and might know more about the issues with templated parameters in bindings and other ways to get around them.

Comment 22

[Manish Goregaokar [:manishearth]]

nvm, emilio just said that this info was obtained with vlad's help, so he's already in the loop :)

Comment 23

[Manish Goregaokar [:manishearth]]

> But anyway - some macro like DEFINE_STRONG_REF_TYPE and DEFINE_OWNING_REF_TYPE should also work fine.

So yeah, this was another solution which I brought up whilst discussing with mystor. You can use macros in C to fake templates -- GCC did this for years before they switched to C++. Alternatively, we can mangle the name on both sides using macros with the macros providing the filler necessary to have strongly-typed (non-extern C) functions on either side. Note that concat_idents! in the return type position in Rust is unstable so we can't use it the regular way. I think that you can make this work by nesting macros, but I'm not sure. Of course, we can have people name these types explicitly on the Rust side.

My issue with most of the macro solutions is that they're not the default; it's easy to forget. On the other hand, we can make the outparam typesafe by making it possible to assign to an outparam only via a RustRefPtr.forget_into(outparam) function on the Rust side.

The DEFINE_STRONG_REF_TYPE stuff is safe, too, however, since it creates a new type which Rust doesn't know how to safely cast to. We can have a macro on the rust side that creates a safe way of casting to the mangled type.

I guess you're proposing that DEFINE_STRONG_REF_TYPE will define a newtype around RefPtr<T>? Or would a typedef still be safe to use in the C ABI on MSVC?

Comment 24

[Bobby Holley (:bholley)]

(In reply to Manish Goregaokar [:manishearth] from comment #23)
> > But anyway - some macro like DEFINE_STRONG_REF_TYPE and DEFINE_OWNING_REF_TYPE should also work fine.
> 
> So yeah, this was another solution which I brought up whilst discussing with
> mystor. You can use macros in C to fake templates -- GCC did this for years
> before they switched to C++. Alternatively, we can mangle the name on both
> sides using macros with the macros providing the filler necessary to have
> strongly-typed (non-extern C) functions on either side. Note that
> concat_idents! in the return type position in Rust is unstable so we can't
> use it the regular way. I think that you can make this work by nesting
> macros, but I'm not sure. Of course, we can have people name these types
> explicitly on the Rust side.

I'm happy with something imperfect for now, especially if we think we can do comment 20 once we're generating bindings at build time.

> My issue with most of the macro solutions is that they're not the default;
> it's easy to forget. On the other hand, we can make the outparam typesafe by
> making it possible to assign to an outparam only via a
> RustRefPtr.forget_into(outparam) function on the Rust side.

You have to remember to use an outparam too, right? We're going to have to think about these when creating APIs either way - the point is just to prevent the _consumers_ of the API from screwing it up.

> 
> The DEFINE_STRONG_REF_TYPE stuff is safe, too, however, since it creates a
> new type which Rust doesn't know how to safely cast to. We can have a macro
> on the rust side that creates a safe way of casting to the mangled type.
> 
> I guess you're proposing that DEFINE_STRONG_REF_TYPE will define a newtype
> around RefPtr<T>? Or would a typedef still be safe to use in the C ABI on
> MSVC?

I was imagining it would just be an opaque newtype for T* - i.e. we'd pass simple opaque pointers across the boundary, and the helpers on each side would know how to convert them into something usable while doing the safe thing.

Comment 25

[Manish Goregaokar [:manishearth]]

> You have to remember to use an outparam too, right? 

This would be coupled with a paring down of the ArcHelpers API so that the only way it *can* be used is if you have an outparam :)

> the point is just to prevent the _consumers_ of the API from screwing it up.

Right. However, this code is only going to get larger.

One issue with bindings is that it's almost all code that's within `unsafe`. `unsafe` code becomes the norm in this area, and we should try to leverage the other tools rust provides (namely, type safety and privacy) to make this unsafe code hard to get wrong.

But yes, first priority is consumers, who shouldn't have to worry about this. Me fretting about future-Manish writing wrong unsafe code is a problem for another time :)

(It's not like being super paranoid about hardening the type safety of unsafe code is something often done in Rust anyway, so we probably can survive without it)

> I was imagining it would just be an opaque newtype for T*

Oh, right. Yeah, that's fine by me. I'll start implementing it tomorrow.

I assume this would mean that Gecko would still need to explicitly call some function on the value returned by Servo_Foo() before setting it to an AddRef? (But this is okay because there's no way to get a refptr without doing this). I would like it to be implicit and magical, though I'm not sure if that's possible without adding extra constructors to RefPtr for each opaque type.

(It is possible with the function-mangling macro, but that isn't safe-by-default because you can forget to use it)

Comment 26

[Nika Layzell [:nika] (ni? for response)]

Is this what you're thinking bholley? I _think_ that it should be safe and not _too_ awkward to use...

#define DEFINE_STRONG_REF_TYPE(name, T)         \
  struct name {                                 \
    T* ptr;                                     \
    already_AddRefed<T> GetRefPtr() {           \
      RefPtr<T> result = dont_AddRef(ptr);      \
      ptr = nullptr;                            \
      return result;                            \
    }                                           \
  }

DEFINE_STRONG_REF_TYPE(ServoTypeStrong, ServoType);

ServoTypeStrong Servo_GetSomething();


// RUST

macro_rules! define_strong_ref_type {
  ($name:ident, $t:ty) => {
    #[repr(C)]
    struct $name {
      ptr: *const $t,
    }

    impl From<RefPtr<$t>> for $name {
      fn from(ptr: RefPtr<T>) -> Self{
        let r = $name { ptr: ptr.mRawPtr };
        ptr.mRawPtr = ::std::ptr::null();
        r
      }
    }
  }
}

define_strong_ref_type!(ServoTypeString, ServoType);
#[no_mangle]
unsafe extern "C" fn Servo_GetSomething() -> ServoTypeStrong {
  SomePointer().into()
}

Comment 27

[Manish Goregaokar [:manishearth]]

That's similar to what I'm thinking at any rate, thanks for the code :)

I'll also need to see the variety of ways we're using ArcHelpers can be made safe with this (preferably getting rid of ArcHelpers entirely, or paring down its API).

Comment 28

[Bobby Holley (:bholley)]

(In reply to Michael Layzell [:mystor] from comment #26)
> Is this what you're thinking bholley? I _think_ that it should be safe and
> not _too_ awkward to use...
> 
> #define DEFINE_STRONG_REF_TYPE(name, T)         \
>   struct name {                                 \
>     T* ptr;                                     \
>     already_AddRefed<T> GetRefPtr() {           \
>       RefPtr<T> result = dont_AddRef(ptr);      \
>       ptr = nullptr;                            \
>       return result;                            \
>     }                                           \
>   }
> 
> DEFINE_STRONG_REF_TYPE(ServoTypeStrong, ServoType);
> 
> ServoTypeStrong Servo_GetSomething();

Sorta, though I was imagining a static-y struct with just a typedef instead of an actual member, and a static conversion routine - i.e. the types we actually pass over FFI would be pointer type rather than struct type, which might help with things on balance. It may not matter though, so if there are advantages to this approach that's fine too.

And as mention elsewhere, we'll want something similar for Owned/UniquePtr.

Comment 29

[Manish Goregaokar [:manishearth]]

So I started work on this. Return values can be handled, but things like ComputedValues* parameters are ambiguous -- is it passing ownership, or just a borrow?

On further investigation, it seems like when a refptr-ish thing appears in the parameter list, it it *always* borrowed (via Helpers::with);

I think I'll add a ServoTypeBorrowed that is a thin wrapper around a RefPtr (or raw ptr), but on the servo side has a lifetime and derefs to an &Arc<T> (and asserts that it isn't null).

We'll need to replace the type on the servo side with one that has a phantom lifetime so that it can't escape the scope.

ArcHelpers can be deleted and replaced with macro-based From and Deref impls.

Comment 30

[Manish Goregaokar [:manishearth]]

First part at https://github.com/servo/servo/pull/12826

Comment 31

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1275913 - Use already_addrefed properly when dealing with arcs sent from servo to gecko;

Review commit: https://reviewboard.mozilla.org/r/71236/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71236/

Comment 32

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1275913 - Add Borrowed types for sharing arcs with Rust;

Review commit: https://reviewboard.mozilla.org/r/71238/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/71238/

Comment 33

[Manish Goregaokar [:manishearth]]

Companion Servo bug at https://github.com/servo/servo/pull/12826

Comment 34

[Manish Goregaokar [:manishearth]]

Comment on attachment 8780535 [details]
Bug 1275913 - Use already_addrefed properly when dealing with arcs sent from servo to gecko;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71236/diff/1-2/

Comment 35

[Manish Goregaokar [:manishearth]]

Comment on attachment 8780536 [details]
Bug 1275913 - Add Borrowed types for sharing arcs with Rust;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71238/diff/1-2/

Comment 36

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8780536 [details]
Bug 1275913 - Add Borrowed types for sharing arcs with Rust;

https://reviewboard.mozilla.org/r/71238/#review68746

::: layout/style/ServoBindings.h:29
(Diff revision 2)
>      };                                                    \
>      already_AddRefed<T> GetRefPtr();                      \
>    }
>  
> +#define DEFINE_BORROWED_REF_TYPE(name, T)                 \
> +  /** <div rustbindgen private></div> */                  \

Won't this not work for the same reason it didn't work in the strong case?

::: layout/style/ServoBindings.h:35
(Diff revision 2)
> +  struct name {                                           \
> +    T* mPtr;                                              \
> +    MOZ_IMPLICIT                                          \
> +    name(T* x): mPtr(x) {};                               \
> +    MOZ_IMPLICIT                                          \
> +    name(const RefPtr<T>& aPtr) {mPtr = aPtr.get();};     \

name(const RefPtr<T>& aPtr) : mPtr(aPtr.get()) {}

is probably a better way to write this.

::: layout/style/ServoBindings.h:36
(Diff revision 2)
> +    T* mPtr;                                              \
> +    MOZ_IMPLICIT                                          \
> +    name(T* x): mPtr(x) {};                               \
> +    MOZ_IMPLICIT                                          \
> +    name(const RefPtr<T>& aPtr) {mPtr = aPtr.get();};     \
> +    operator T*() const & { return mPtr; }                \

Please put the return on its own line

Comment 37

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8780535 [details]
Bug 1275913 - Use already_addrefed properly when dealing with arcs sent from servo to gecko;

https://reviewboard.mozilla.org/r/71236/#review68744

Just some drive by comments:

::: layout/style/ServoBindings.h:20
(Diff revision 2)
>  #include "nsStyleCoord.h"
>  #include "nsStyleStruct.h"
>  #include "stdint.h"
>  
> +#define DEFINE_STRONG_REF_TYPE(name, T)                   \
> +  struct name {                                           \

I recommend marking this struct MOZ_MUST_USE_TYPE such that our C++ static analysis complains if you don't use a value of this type (as that is probably an error).

::: layout/style/ServoBindings.h:23
(Diff revision 2)
>  
> +#define DEFINE_STRONG_REF_TYPE(name, T)                   \
> +  struct name {                                           \
> +    T* mPtr;                                              \
> +    ~name() {                                             \
> +      GetRefPtr();                                        \

already_AddRefed just MOZ_ASSERTs that its mPtr is null in its destructor. Not sure if there's a good reason _not_ to run GetRefPtr() here though.

::: layout/style/ServoBindings.h:57
(Diff revision 2)
> +/** <div rustbindgen private></div> */
> +DEFINE_STRONG_REF_TYPE(ServoComputedValuesStrong, ServoComputedValues);

It's really unfortunate that rust bindgen uses comments to denote private fields rather than attributes. 

I would have expected (as it is based on clang) for it to use something like 

    __attribute__((annotate("rustbindgen_private"))) int mFoo;

Where we could put the __attribute__ into a macro and only expose it during rustbindgen runs.

We also usually use DECL_... in gecko (e.g. NS_DECL_ISUPPORTS and NS_IMPL_ISUPPORTS)

::: layout/style/ServoBindings.cpp:29
(Diff revision 2)
>  
>  #include "mozilla/EventStates.h"
>  #include "mozilla/ServoElementSnapshot.h"
>  #include "mozilla/dom/Element.h"
>  
> +#define DEFINE_STRONG_REF_TYPE_METHODS(name, T) \

IMPL_... is usually the pattern we use in Gecko

::: layout/style/ServoBindings.cpp:34
(Diff revision 2)
> +#define DEFINE_STRONG_REF_TYPE_METHODS(name, T) \
> +  already_AddRefed<T> name::GetRefPtr() {       \
> +    RefPtr<T> result = dont_AddRef(mPtr);       \
> +    mPtr = nullptr;                             \
> +    return result.forget();                     \
> +  }; 

Trailing whitespace

Comment 38

[Manish Goregaokar [:manishearth]]

Comment on attachment 8780535 [details]
Bug 1275913 - Use already_addrefed properly when dealing with arcs sent from servo to gecko;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71236/diff/2-3/

Comment 39

[Manish Goregaokar [:manishearth]]

Comment on attachment 8780536 [details]
Bug 1275913 - Add Borrowed types for sharing arcs with Rust;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71238/diff/2-3/

Comment 40

[Bobby Holley (:bholley)]

Comment on attachment 8780535 [details]
Bug 1275913 - Use already_addrefed properly when dealing with arcs sent from servo to gecko;

https://reviewboard.mozilla.org/r/71236/#review68762

::: layout/style/ServoBindings.h:23
(Diff revision 3)
> +    ~name() {                                             \
> +      GetRefPtr();                                        \
> +    };                                                    \

I don't understand - the whole reason we can't pass an already_AddRefed as the return value is that it has a destructor. If that's not true, then we should just pass already_AddRefed directly?

::: layout/style/ServoBindings.h:26
(Diff revision 3)
> +  struct name {                                           \
> +    T* mPtr;                                              \
> +    ~name() {                                             \
> +      GetRefPtr();                                        \
> +    };                                                    \
> +    already_AddRefed<T> GetRefPtr();                      \

I think the name is misleading here. Let's call it Consume()?

Alternatively, could we just define an operator already_AddRefed<T> and use implicit conversion?

::: layout/style/ServoBindings.h:58
(Diff revision 3)
> +/** <div rustbindgen private></div> */
> +DEFINE_STRONG_REF_TYPE(ServoComputedValuesStrong, ServoComputedValues);

What does this annotation do?

::: layout/style/ServoBindings.h:60
(Diff revision 3)
> +/** <div rustbindgen private></div> */
> +DEFINE_STRONG_REF_TYPE(RawServoStyleSheetStrong, RawServoStyleSheet);

Nit: Maybe we should define these immediately after forward-declaring the types?

i.e.

struct ServoComputedValues;
DEFINE_STRING_REF_TYPE(ServoComputedValuesStrong, ServoComputedValues);
struct RawServoStyleSheet;
DEFINE_STRING_REF_TYPE(RawServoStyleSheetStrong, RawServoStyleSheet);

Comment 41

[Manish Goregaokar [:manishearth]]

Comment on attachment 8780535 [details]
Bug 1275913 - Use already_addrefed properly when dealing with arcs sent from servo to gecko;

https://reviewboard.mozilla.org/r/71236/#review68762

> I don't understand - the whole reason we can't pass an already_AddRefed as the return value is that it has a destructor. If that's not true, then we should just pass already_AddRefed directly?

Oh, I thought it was because it was templated :)

This destructor isn't necessary though. I'll remove it.

But yes, if we can work around the C linkage issue we should use already_AddRefed.

> I think the name is misleading here. Let's call it Consume()?
> 
> Alternatively, could we just define an operator already_AddRefed<T> and use implicit conversion?

Both work. Wasn't sure what the codebase's opinion on implicit operators was, but if it's fine I'll use that.

> What does this annotation do?

Makes the fields private, because it is totally safe in rust to set a raw pointer to a random number, and if the fields are public it would be possible to do this.

> Nit: Maybe we should define these immediately after forward-declaring the types?
> 
> i.e.
> 
> struct ServoComputedValues;
> DEFINE_STRING_REF_TYPE(ServoComputedValuesStrong, ServoComputedValues);
> struct RawServoStyleSheet;
> DEFINE_STRING_REF_TYPE(RawServoStyleSheetStrong, RawServoStyleSheet);

Will do.

Comment 42

[Bobby Holley (:bholley)]

Comment on attachment 8780536 [details]
Bug 1275913 - Add Borrowed types for sharing arcs with Rust;

https://reviewboard.mozilla.org/r/71238/#review68764

r=me with comments addressed.

Still need the Owned/UniquePtr variant as well btw, but presumably that's a separate patch.

::: layout/style/ServoBindings.h:29
(Diff revision 3)
>        GetRefPtr();                                        \
>      };                                                    \
>      already_AddRefed<T> GetRefPtr();                      \
>    }
>  
> +#define DEFINE_BORROWED_REF_TYPE(name, T)                 \

As mystor mentioned, the normal convention is DECL/IMPL.

::: layout/style/ServoBindings.h:34
(Diff revision 3)
> +    MOZ_IMPLICIT                                          \
> +    name(const RefPtr<T>& aPtr) : mPtr(aPtr.get()) {};    \

Given that RefPtr already implicitly converts to T*, I'm not sure this is necessary.

::: layout/style/ServoBindings.h:36
(Diff revision 3)
> +    T* mPtr;                                              \
> +    MOZ_IMPLICIT                                          \
> +    name(T* x): mPtr(x) {};                               \
> +    MOZ_IMPLICIT                                          \
> +    name(const RefPtr<T>& aPtr) : mPtr(aPtr.get()) {};    \
> +    operator T*() const & {                               \

what does the & here do? I've never seen that before.

::: layout/style/ServoBindings.cpp:977
(Diff revision 3)
>    MOZ_CRASH("stylo: shouldn't be calling Servo_RestyleSubtree in a "
>              "non-MOZ_STYLO build");
>  }
>  
>  #define STYLE_STRUCT(name_, checkdata_cb_)                                     \
>  const nsStyle##name_*                                                          \

I wonder if we should annotate the return type as borrowed as well? Not super important because this case is rare. but it doesn't follow the usual convention, and would be nice to have that explicit.

Comment 43

[Manish Goregaokar [:manishearth]]

Comment on attachment 8780536 [details]
Bug 1275913 - Add Borrowed types for sharing arcs with Rust;

https://reviewboard.mozilla.org/r/71238/#review68764

> Given that RefPtr already implicitly converts to T*, I'm not sure this is necessary.

Apparently the double implicit conversion (auto-deref + auto-constructor) doesn't happen

> what does the & here do? I've never seen that before.

copied this from the refptr definition, it takes `this` by ref

> I wonder if we should annotate the return type as borrowed as well? Not super important because this case is rare. but it doesn't follow the usual convention, and would be nice to have that explicit.

This would require some mako magic on the other side to get working. Not sure if it's worth it right now, might do later.

Comment 44

[Nika Layzell [:nika] (ni? for response)]

(In reply to Manish Goregaokar [:manishearth] from comment #43)
> > what does the & here do? I've never seen that before.
> 
> copied this from the refptr definition, it takes `this` by ref

To clarify, & is a ref qualifier which controls how overload resolution selects multiple overloads of a given method based on the this parameter. Effectively, this qualifier states that this method should bind as though the this parameter was written as type T&. There is also a && qualifier which makes it bind as type T&& (an r-value). 

We used not to support this on all of our supported compilers, but it's supported in MSVC 2015, so we can use it now.

I'm not sure it's important in this situation. I would probably remove it unless we decided it was important.

Comment 45

[Manish Goregaokar [:manishearth]]

Comment on attachment 8780535 [details]
Bug 1275913 - Use already_addrefed properly when dealing with arcs sent from servo to gecko;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71236/diff/3-4/

Comment 46

[Manish Goregaokar [:manishearth]]

Comment on attachment 8780536 [details]
Bug 1275913 - Add Borrowed types for sharing arcs with Rust;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71238/diff/3-4/

Comment 47

[Manish Goregaokar [:manishearth]]

Comment on attachment 8780536 [details]
Bug 1275913 - Add Borrowed types for sharing arcs with Rust;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71238/diff/4-5/

Comment 48

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8780536 [details]
Bug 1275913 - Add Borrowed types for sharing arcs with Rust;

https://reviewboard.mozilla.org/r/71238/#review68778

::: layout/style/ServoBindings.cpp:31
(Diff revisions 3 - 4)
>      RefPtr<T> result = dont_AddRef(mPtr);       \
>      mPtr = nullptr;                             \
>      return result.forget();                     \

You should be able to actually write this as:

    already_AddRefed<T> result(mPtr);
    mPtr = nullptr;
    return result;
    
I think, but this implementation is just fine too.

Comment 49

[Bobby Holley (:bholley)]

Comment on attachment 8780535 [details]
Bug 1275913 - Use already_addrefed properly when dealing with arcs sent from servo to gecko;

https://reviewboard.mozilla.org/r/71236/#review68776

::: layout/style/ServoBindings.h:19
(Diff revision 4)
>  #include "nsProxyRelease.h"
>  #include "nsStyleCoord.h"
>  #include "nsStyleStruct.h"
>  #include "stdint.h"
>  
> +#define DEFINE_STRONG_REF_TYPE(name, T)                   \

call this DECL_STRING_REF_TYPE

::: layout/style/ServoBindings.h:21
(Diff revision 4)
> +  struct name {                                           \
> +    T* mPtr;                                              \
> +    already_AddRefed<T> Consume();                        \

Might it be possible to delete the copy ctor and operator= without triggering the calling convention weirdness? If so that would be great to do, otherwise don't worry about it.

Comment 50

[Bobby Holley (:bholley)]

Comment on attachment 8780535 [details]
Bug 1275913 - Use already_addrefed properly when dealing with arcs sent from servo to gecko;

https://reviewboard.mozilla.org/r/71236/#review68780

Comment 51

[Bobby Holley (:bholley)]

And still needs the UniquePtr pieces.

Comment 52

[Bobby Holley (:bholley)]

(In reply to Michael Layzell [:mystor] from comment #48)
> ::: layout/style/ServoBindings.cpp:31
> (Diff revisions 3 - 4)
> >      RefPtr<T> result = dont_AddRef(mPtr);       \
> >      mPtr = nullptr;                             \
> >      return result.forget();                     \
> 
> You should be able to actually write this as:
> 
>     already_AddRefed<T> result(mPtr);
>     mPtr = nullptr;
>     return result;
>     
> I think, but this implementation is just fine too.

I think Manish's implementation is more idiomatic actually.

Comment 53

[Manish Goregaokar [:manishearth]]

Comment on attachment 8780536 [details]
Bug 1275913 - Add Borrowed types for sharing arcs with Rust;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71238/diff/5-6/

Comment 54

[Manish Goregaokar [:manishearth]]

Comment on attachment 8780535 [details]
Bug 1275913 - Use already_addrefed properly when dealing with arcs sent from servo to gecko;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71236/diff/4-5/

Comment 55

[Manish Goregaokar [:manishearth]]

Comment on attachment 8780536 [details]
Bug 1275913 - Add Borrowed types for sharing arcs with Rust;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71238/diff/6-7/

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/324554b04f19
Use already_addrefed properly when dealing with arcs sent from servo to gecko; r=bholley
https://hg.mozilla.org/integration/autoland/rev/4420244e8fba
Add Borrowed types for sharing arcs with Rust; r=bholley

Comment 57

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/8430fc70c108 for static analysis bustage, https://treeherder.mozilla.org/logviewer.html#?job_id=1977980&repo=autoland

Comment 58

[Xidorn Quan [:xidorn] UTC+10]

I wonder why couldn't we just add bindings of already_AddRefed or even RefPtr directly in Rust. The storage structure of those classes are as simple as the one you are creating here. I don't think it makes much sense to create another one, especially given that we are on the way to reduce usage of already_AddRefed in C++ code.

Comment 59

[Manish Goregaokar [:manishearth]]

That's what Emilio's patch did, but MSVC doesn't like certain kinds of templated types in extern C. So we do this small bit of type gymnastics  at the boundary.

Comment 60

[Xidorn Quan [:xidorn] UTC+10]

Oh, okay, extern C... That makes sense, then.

Comment 61

[Manish Goregaokar [:manishearth]]

Comment on attachment 8780535 [details]
Bug 1275913 - Use already_addrefed properly when dealing with arcs sent from servo to gecko;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71236/diff/5-6/

Comment 62

[Manish Goregaokar [:manishearth]]

Comment on attachment 8780536 [details]
Bug 1275913 - Add Borrowed types for sharing arcs with Rust;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/71238/diff/7-8/

Comment 63

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b66852d401a
Use already_addrefed properly when dealing with arcs sent from servo to gecko; r=bholley
https://hg.mozilla.org/integration/autoland/rev/c0cb35822062
Add Borrowed types for sharing arcs with Rust; r=bholley

Comment 64

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0b66852d401a
https://hg.mozilla.org/mozilla-central/rev/c0cb35822062