Implement bindings for calling XPCOM from Rust

RESOLVED FIXED in Firefox 60

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
27 days ago

People

(Reporter: Nika, Assigned: Nika)

Tracking

(Blocks 1 bug)

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(8 attachments, 31 obsolete attachments)

2.92 KB, patch
Details | Diff | Splinter Review
11.97 KB, patch
Details | Diff | Splinter Review
11.56 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
16.54 KB, patch
Details | Diff | Splinter Review
4.76 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
3.78 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
49.07 KB, patch
Details | Diff | Splinter Review
36.88 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
The goal with these bindings is to make it easier to port new pieces of Gecko into rust, by providing the tools for interacting with XPCOM interfaces from rust. 

This patch only implements the calling side of the bindings, and does not provide the tools for implementing these interfaces in rust code. That will come in an upcoming patch based on this one.
(Assignee)

Updated

3 years ago
Depends on: 1293364
(Assignee)

Comment 1

3 years ago
Attachment #8779012 - Flags: review?(nfroyd)
(Assignee)

Comment 4

3 years ago
Attachment #8779015 - Flags: review?(nfroyd)
(Assignee)

Comment 5

3 years ago
Attachment #8779016 - Flags: review?(nfroyd)
(Assignee)

Comment 6

3 years ago
Comment on attachment 8779013 [details] [diff] [review]
Part 2: Add rust bindings to RefPtr and nsA*String

Manish, I'm pretty sure that the unsafe stuff in this is OK (even the super sketchy &'static) because of the module acting as a safety boundary, but I'd appreciate it if you could look it over and make sure I'm not invoking UB anywhere.

I am fairly confident that RefPtr isn't completely safe because of mem::forget() and the fact that mRefCnt in C++ doesn't saturate in the same way that it does in Rust, but it's safe-enough that I feel comfortable making it safe here.
Attachment #8779013 - Flags: feedback?(manishearth)
(Assignee)

Comment 7

3 years ago
Comment on attachment 8779014 [details] [diff] [review]
Part 3: Generate rust bindings for calling xpcom interfaces from xpidl files

I'd also appreciate it if you could look over the unsafe stuff I'm doing in this patch and make sure it is OK as well.
Attachment #8779014 - Flags: feedback?(manishearth)
What is the end goal here?

* We have XPCOM things we want to implement in Rust and expose to JS;
* We have things we want to write in Rust that need interop with XPCOM things;
* We want to show off our hacking skills;
* Something else;
* Some combination of the above.

And when I say "things" in the first two, I mean concrete plans that are implementable in the next couple months.

Just trying to get a feel for what this buys us.
Priority: -- → P3
(Assignee)

Comment 9

3 years ago
(In reply to Nathan Froyd [:froydnj] from comment #8)
> What is the end goal here?
> 
> * We have XPCOM things we want to implement in Rust and expose to JS;
> * We have things we want to write in Rust that need interop with XPCOM
> things;
> * We want to show off our hacking skills;
> * Something else;
> * Some combination of the above.
> 
> And when I say "things" in the first two, I mean concrete plans that are
> implementable in the next couple months.
> 
> Just trying to get a feel for what this buys us.

I do not believe we currently have any concrete plans within the next couple of months for code which we want to implement in rust which will need to interop with XPCOM things. I mostly implemented this in my spare time as a side project, because it seemed like it would be fun to implement. I originally wasn't going to bother polishing it to the point that I would submit a patch, but people seemed to be interested in it, so I figured I might as well.

I would be fine with this code sitting around for a while until we get a more concrete use case.
Comment on attachment 8779012 [details] [diff] [review]
Part 1: Vendor rust's libc crate in tree

Review of attachment 8779012 [details] [diff] [review]:
-----------------------------------------------------------------

Given that there's now an established way of using cargo crates / cargo vendor in tree, perhaps we should make this a dep of libgkrust (or whatever the libxul rust crate was)? Fine if done in a subsequent patch.
Attachment #8779012 - Flags: feedback+
> Given that there's now an established way of using cargo crates / cargo
> vendor in tree, perhaps we should make this a dep of libgkrust (or whatever
> the libxul rust crate was)? Fine if done in a subsequent patch.

That is exactly what you did, never mind. I didn't know libgkrust was in toolkit/ :)
Comment on attachment 8779014 [details] [diff] [review]
Part 3: Generate rust bindings for calling xpcom interfaces from xpidl files

Review of attachment 8779014 [details] [diff] [review]:
-----------------------------------------------------------------

Haven't really looked at the codegen stuff.

I'll probably run this locally and have a look at the generated files to see if it's safe.

::: xpcom/idl-parser/xpidl/rust.py
@@ +18,5 @@
> +
> +if printdoccomments:
> +    def printComments(fd, clist, indent):
> +        for c in clist:
> +            fd.write("%s%s\n" % (indent, c))

Won't this need a // ?

@@ +30,5 @@
> +
> +
> +# Attribute VTable Methods
> +def attributeNativeName(a, getter):
> +    binaryname = a.binaryname is not None and a.binaryname or a.name

nit: don't mix and and or without parens

@@ +41,5 @@
> +
> +
> +def attributeVTableParamlist(iface, a, getter):
> +    l = ["this: *const " + iface.name]
> +    l += ["%s: %s" % (attributeVTableParamName(a), 

nit: trailing space

::: xpcom/xpcom-rs/src/base.rs
@@ +5,5 @@
> +
> +pub const NS_OK: nsresult = 0;
> +pub const NS_ERROR_FAILURE: nsresult = 0x80004005;
> +pub const NS_NOINTERFACE: nsresult = 0x80004002;
> +// XXX: There are a lot of errors - do we want to export all of them?

bindgen is your friend

If we plan to use bindgen here, leave a comment to that effect and manual-import for now, ultimately bindgenning everything (which will be hidden away in a generated file so it doesn't matter how large it gets)

::: xpcom/xpcom-rs/src/nonidl.rs
@@ +20,5 @@
> +        }
> +
> +        unsafe impl $crate::RefCounted for $name {
> +            unsafe fn addref(&self) {
> +                self.AddRef();

Where do these get defined?
Comment on attachment 8779013 [details] [diff] [review]
Part 2: Add rust bindings to RefPtr and nsA*String

Review of attachment 8779013 [details] [diff] [review]:
-----------------------------------------------------------------

Safety-wise lgtm, aside from a phantomdata issue and some small quibblings.

::: xpcom/string/nsSubstring.cpp
@@ +351,5 @@
> +
> +// Expose the ability to release nsStringBuffers to rust code, for xpcom
> +// ns*String bindings.
> +void
> +xpcrs_release_ns_string_buffer(void* p)

Stylo usually names these functions in CamelCase. Not sure if we want to be consistent here, probably doesn't matter much.

::: xpcom/xpcom-rs/Cargo.toml
@@ +1,4 @@
> +[package]
> +name = "xpcom"
> +version = "0.1.0"
> +authors = ["nobody@mozilla.org"]

Finally, that slacker gets some work done.

::: xpcom/xpcom-rs/src/lib.rs
@@ +1,1 @@
> +#![allow(bad_style)]

Would prefer this to specifically whitelist the lints that will come up here (probably just the snake case one?)

::: xpcom/xpcom-rs/src/refptr.rs
@@ +12,5 @@
> +
> +/// A smart pointer holding a RefCounted object. The object itself manages its
> +/// own memory. RefPtr will invoke the addref and release methods at the
> +/// appropriate times to facilitate the bookkeeping.
> +pub struct RefPtr<T: RefCounted + 'static> {

I'd feel safer if all the structs here were generated via bindgen. If you tried this, was there a reason why it didn't work?

If not, the current patch is ok too (bindgen is tricky to work with). If the stylo stuff gets merged in we might be able to move the gecko_bindings crate to a more central location in m-c and share the bindgen process.

@@ +18,5 @@
> +    // instead of an *const T or Shared<T>, because Shared and NonZero are
> +    // unstable, and we need to build on stable rust.
> +    // I believe that this is "safe enough", as this module is private and
> +    // no other module can read this reference.
> +    _ptr: &'static T,

Please include a PhantomData<T> somewhere.

Not having the right phantom type can introduce dropck unsoundness. Shared/Unique encapsulate this.

@@ +27,5 @@
> +    pub fn new(p: &T) -> RefPtr<T> {
> +        unsafe {
> +            p.addref();
> +            RefPtr {
> +                _ptr: mem::transmute(p),

I am not fond of the transmute, but I guess it's inevitable because of the `&'static`. Though `&* (p as *const _)` would also work. But that's just as bad.

@@ +71,5 @@
> +}
> +
> +impl <T: RefCounted + 'static> Clone for RefPtr<T> {
> +    fn clone(&self) -> RefPtr<T> {
> +        unsafe {

nit: Reuse RefPtr::new here?

::: xpcom/xpcom-rs/src/string.rs
@@ +35,5 @@
> +impl Deref for nsACString {
> +    type Target = [u8];
> +    fn deref(&self) -> &[u8] {
> +        let data = if self.data.is_null() {
> +            0x1 as *const u8 // XXX: arbitrary non-null value

nit: debug assert that length is 0 here

@@ +56,5 @@
> +            flags: ns_str_flags::F_NONE,
> +        })
> +    }
> +
> +    pub fn from_slice(s: &[u8]) -> nsCString {

nit: doc comment that this allocates and creates a copy

(probably could use std::convert::From here, not necessary)

@@ +58,5 @@
> +    }
> +
> +    pub fn from_slice(s: &[u8]) -> nsCString {
> +        unsafe {
> +            let data = libc::malloc(s.len()) as *mut u8;

This relies on the allocators being shared. IIRC that is the case, but I'm not 100% sure on the details. Nathan would know.

@@ +68,5 @@
> +            })
> +        }
> +    }
> +
> +    pub unsafe fn dependent_from_slice(s: &[u8]) -> nsCString {

Not too happy about dependent strings not having a lifetime. It's always nice when unsafety is cleanly scoped, and in this case the deref of the nsCString is an unsafe operation. The `unsafe` on this function means that the user must ensure that they will satisfy all safety requirements in the future, which is not as nice as being asked to satisfy all safety requirements in the immediate vicinity of the code and nowhere else.

Perhaps we could have a DependentNSCString that can be safely obtained and has a lifetime, but can be unsafely cast to an nsCString? Or is that overkill? It has the same problem, but is more explicit.
Attachment #8779013 - Flags: feedback?(manishearth) → feedback+
Ah, I see, nsISupports implements AddRef and Release. Neat.
For folks who want to go through the generated bindings without building this branch, I uploaded them:

http://people.mozilla.org/~mgoregaokar/xpcrs/ if you want to browse online, and http://people.mozilla.org/~mgoregaokar/xpcrs.tar.gz is the whole thing compressed.
Comment on attachment 8779014 [details] [diff] [review]
Part 3: Generate rust bindings for calling xpcom interfaces from xpidl files

Review of attachment 8779014 [details] [diff] [review]:
-----------------------------------------------------------------

I investigated a bit, seems mostly okay safety-wise. Needs a more thorough check but I'd like to wait until we know if/when this will merge :)
Attachment #8779014 - Flags: feedback?(manishearth) → feedback+
Comment on attachment 8779012 [details] [diff] [review]
Part 1: Vendor rust's libc crate in tree

Review of attachment 8779012 [details] [diff] [review]:
-----------------------------------------------------------------

r+ to Manish's comments as well.
Attachment #8779012 - Flags: review?(nfroyd) → review+
Comment on attachment 8779013 [details] [diff] [review]
Part 2: Add rust bindings to RefPtr and nsA*String

Review of attachment 8779013 [details] [diff] [review]:
-----------------------------------------------------------------

I'm largely relying on Manish's comments; they all seem appropriate.  Please address them.

::: xpcom/string/nsSubstring.cpp
@@ +351,5 @@
> +
> +// Expose the ability to release nsStringBuffers to rust code, for xpcom
> +// ns*String bindings.
> +void
> +xpcrs_release_ns_string_buffer(void* p)

As Manish mentions, CamelCase names, please.

::: xpcom/xpcom-rs/src/string.rs
@@ +58,5 @@
> +    }
> +
> +    pub fn from_slice(s: &[u8]) -> nsCString {
> +        unsafe {
> +            let data = libc::malloc(s.len()) as *mut u8;

Is libc::malloc going through __rust_allocate, or is it actually calling out to malloc?  Either way, we should be covered.
Attachment #8779013 - Flags: review?(nfroyd) → review+
Comment on attachment 8779015 [details] [diff] [review]
Part 4: Add rust bindings for xpcom globals

Review of attachment 8779015 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/components/nsComponentManager.cpp
@@ +2073,5 @@
> +extern "C" {
> +
> +// Expose the nsIComponentManager to rust through an extern "C" API
> +nsIComponentManager*
> +xpcrs_get_ns_component_manager()

As before, CamelCase names.
Attachment #8779015 - Flags: review?(nfroyd) → review+
Comment on attachment 8779016 [details] [diff] [review]
Part 5: Add a test for calling xpcom from rust

Review of attachment 8779016 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/tests/gtest/rust.rs
@@ +9,5 @@
> +
> +#[no_mangle]
> +pub unsafe extern fn gtest_xpcom_rust() -> bool {
> +    let io_svc: RefPtr<nsIIOService2> = xpcom::get_service(&NS_IOSERVICE_CID).unwrap();
> +    let uri = io_svc.newURI(b"https://google.com", ptr::null(), None).unwrap().unwrap();

What?  google.com in test files?  At least use mozilla.com! ;)
Attachment #8779016 - Flags: review?(nfroyd) → review+
Comment on attachment 8779014 [details] [diff] [review]
Part 3: Generate rust bindings for calling xpcom interfaces from xpidl files

Review of attachment 8779014 [details] [diff] [review]:
-----------------------------------------------------------------

I'd really like to see the XXX comments addressed before we land.  I think this is reasonable; I have some small Python comments below, and I'd like to see an example of the code this generates.

The build system changes are relatively benign, and I think I have enough authority to r+ them.

::: xpcom/idl-parser/xpidl/xpidl.py
@@ +30,5 @@
>          'returns the member signature as IDL'
>  """
>  
> +def rust_sanitize(s):
> +    keywords = [

This should be a set, not a list.

@@ +48,5 @@
> +        return s + "_"
> +    return s
> +
> +def rust_blacklisted_forward(s):
> +    blacklisted = [

Same here.

@@ +155,5 @@
>  
> +    def rustTypeInfo(self, calltype, varname='_'):
> +        if calltype == 'in':
> +            return {
> +                'vtable': self.rustname,

This is really something more like 'vtableArgType', right?  Let's be explicitly long about names here.

@@ +156,5 @@
> +    def rustTypeInfo(self, calltype, varname='_'):
> +        if calltype == 'in':
> +            return {
> +                'vtable': self.rustname,
> +                'param_ty': self.rustname,

'paramType', and so on throughout.

@@ +161,5 @@
> +                'arg': varname,
> +            }
> +        return {
> +            'vtable': "*mut %s" % self.rustname,
> +            'ret_ty': self.rustname,

'returnType', and so on throughout.

@@ +462,5 @@
> +                'param_ty': "Option<&%s>" % self.name,
> +                'arg': "%s.map_or(::std::ptr::null(), |x| x as *const _)" % varname,
> +            }
> +        # XXX: If we return a valid pointer and a non-NS_OK value, we leak
> +        # FIXME

Would be good to not introduce leaking code.

::: xpcom/xpcom-rs/build.rs
@@ +21,5 @@
> +
> +    xpcrs_path.push("mod.rs");
> +    let mut librs = File::create(xpcrs_path).unwrap();
> +
> +    // Write out the mod declarations

Are we writing these files out every time something this crate depends on changes?  Sigh...
Attachment #8779014 - Flags: review?(nfroyd) → feedback+
(Assignee)

Comment 22

3 years ago
(In reply to Nathan Froyd [:froydnj] from comment #18)
> ::: xpcom/xpcom-rs/src/string.rs
> @@ +58,5 @@
> > +    }
> > +
> > +    pub fn from_slice(s: &[u8]) -> nsCString {
> > +        unsafe {
> > +            let data = libc::malloc(s.len()) as *mut u8;
> 
> Is libc::malloc going through __rust_allocate, or is it actually calling out
> to malloc?  Either way, we should be covered.

This goes directly to malloc - should we go though __rust_allocate instead?

(In reply to Nathan Froyd [:froydnj] from comment #21)
> I'd really like to see the XXX comments addressed before we land.  I think this is reasonable; I have some small Python > comments below, and I'd like to see an example of the code this generates.

Manish has uploaded the generated code to his people.mozilla account in comment 15. If you want to see some of the generated code you can look at that. It isn't exactly pretty (I would say it, in general, looks worse than the generated C++ code for those header files), but it seems to be functional.

> @@ +462,5 @@
> > +                'param_ty': "Option<&%s>" % self.name,
> > +                'arg': "%s.map_or(::std::ptr::null(), |x| x as *const _)" % varname,
> > +            }
> > +        # XXX: If we return a valid pointer and a non-NS_OK value, we leak
> > +        # FIXME
> 
> Would be good to not introduce leaking code.

Oops - that comment was actually from an earlier version of the code, I fixed it with the addition of the AlreadyAddrefed helper struct in refptr.rs.

> ::: xpcom/xpcom-rs/build.rs
> @@ +21,5 @@
> > +
> > +    xpcrs_path.push("mod.rs");
> > +    let mut librs = File::create(xpcrs_path).unwrap();
> > +
> > +    // Write out the mod declarations
> 
> Are we writing these files out every time something this crate depends on
> changes?  Sigh...

Now that you point this out, I'm not sure that we have the correct behavior here. The .rs files for each of the xpidl interfaces are written out at the same time as the xpcom headers, and through the same code path. The difference is that all of the xpcom rust bindings need to be imported into a single module, such that they share their namespaces. For some reason, I decided to do that inside of a build.rs script, rather than inside of the python code.

I should probably change this to generate mod.rs inside of the python code along with the other .rs files, and remove the build.rs script.
I will take a look at the generated code.

(In reply to Michael Layzell [:mystor] from comment #22)
> (In reply to Nathan Froyd [:froydnj] from comment #18)
> > ::: xpcom/xpcom-rs/src/string.rs
> > @@ +58,5 @@
> > > +    }
> > > +
> > > +    pub fn from_slice(s: &[u8]) -> nsCString {
> > > +        unsafe {
> > > +            let data = libc::malloc(s.len()) as *mut u8;
> > 
> > Is libc::malloc going through __rust_allocate, or is it actually calling out
> > to malloc?  Either way, we should be covered.
> 
> This goes directly to malloc - should we go though __rust_allocate instead?

malloc is OK.

> > @@ +462,5 @@
> > > +                'param_ty': "Option<&%s>" % self.name,
> > > +                'arg': "%s.map_or(::std::ptr::null(), |x| x as *const _)" % varname,
> > > +            }
> > > +        # XXX: If we return a valid pointer and a non-NS_OK value, we leak
> > > +        # FIXME
> > 
> > Would be good to not introduce leaking code.
> 
> Oops - that comment was actually from an earlier version of the code, I
> fixed it with the addition of the AlreadyAddrefed helper struct in refptr.rs.

Please make sure XXX comments are up to date, then. :)

> I should probably change this to generate mod.rs inside of the python code
> along with the other .rs files, and remove the build.rs script.

I think that's a good idea.
(Assignee)

Updated

3 years ago
Depends on: 1294553
(Assignee)

Comment 24

3 years ago
(In reply to Manish Goregaokar [:manishearth] from comment #12)
> ::: xpcom/idl-parser/xpidl/rust.py
> @@ +18,5 @@
> > +
> > +if printdoccomments:
> > +    def printComments(fd, clist, indent):
> > +        for c in clist:
> > +            fd.write("%s%s\n" % (indent, c))
> 
> Won't this need a // ?

I copied this from the C++ generator, and as C++ and rust have the same comment styles, I think it should be fine.

> ::: xpcom/xpcom-rs/src/base.rs
> @@ +5,5 @@
> > +
> > +pub const NS_OK: nsresult = 0;
> > +pub const NS_ERROR_FAILURE: nsresult = 0x80004005;
> > +pub const NS_NOINTERFACE: nsresult = 0x80004002;
> > +// XXX: There are a lot of errors - do we want to export all of them?
> 
> bindgen is your friend
> 
> If we plan to use bindgen here, leave a comment to that effect and
> manual-import for now, ultimately bindgenning everything (which will be
> hidden away in a generated file so it doesn't matter how large it gets)

From what I can understand, we don't support bindgen in tree yet. It would be awesome to make some parts of this code less repetitive from C++ with bindgen (such as contract IDs), but for now I have to manually import them. I'll add a comment to that effect.

> ::: xpcom/xpcom-rs/src/nonidl.rs
> @@ +20,5 @@
> > +        }
> > +
> > +        unsafe impl $crate::RefCounted for $name {
> > +            unsafe fn addref(&self) {
> > +                self.AddRef();
> 
> Where do these get defined?

nsISupports has an AddRef and Release method, and $name has a deref impl to &nsISupports.

(In reply to Manish Goregaokar [:manishearth] from comment #13)

> ::: xpcom/xpcom-rs/Cargo.toml
> @@ +1,4 @@
> > +[package]
> > +name = "xpcom"
> > +version = "0.1.0"
> > +authors = ["nobody@mozilla.org"]
> 
> Finally, that slacker gets some work done.

It doesn't really make sense to put anyone else's name there. This is the author of libgkrust I believe as well.

> ::: xpcom/xpcom-rs/src/refptr.rs
> @@ +12,5 @@
> > +
> > +/// A smart pointer holding a RefCounted object. The object itself manages its
> > +/// own memory. RefPtr will invoke the addref and release methods at the
> > +/// appropriate times to facilitate the bookkeeping.
> > +pub struct RefPtr<T: RefCounted + 'static> {
> 
> I'd feel safer if all the structs here were generated via bindgen. If you
> tried this, was there a reason why it didn't work?
> 
> If not, the current patch is ok too (bindgen is tricky to work with). If the
> stylo stuff gets merged in we might be able to move the gecko_bindings crate
> to a more central location in m-c and share the bindgen process.

I suppose that RefPtr<T> might not be the best name for this type. This type is not actually the same as C++s' RefPtr type. It doesn't share the same layout or anything like that. Instead, RefPtr<T> is a rust-only type which acts like Arc<T> providing reference counting for the contained type, except that instead of it controlling the refcount directly, it goes through a trait which has the addref and release methods (I call this trait RefCounted). 

XPCOM never actually passes nsCOMPtr or RefPtr around, it only passes around raw pointers. This is a wrapper type which I use to make these raw pointers safe to use on the rust side, and has nothing to do with FFI directly. This type could theoretically just as easily be used to make safe rust-only invasive refcounted objects.

> @@ +27,5 @@
> > +    pub fn new(p: &T) -> RefPtr<T> {
> > +        unsafe {
> > +            p.addref();
> > +            RefPtr {
> > +                _ptr: mem::transmute(p),
> 
> I am not fond of the transmute, but I guess it's inevitable because of the
> `&'static`. Though `&* (p as *const _)` would also work. But that's just as
> bad.

I agree it's pretty awful, but I'm not sure that &*(p as *const T) is much better. I'll switch though.

> @@ +68,5 @@
> > +            })
> > +        }
> > +    }
> > +
> > +    pub unsafe fn dependent_from_slice(s: &[u8]) -> nsCString {
> 
> Not too happy about dependent strings not having a lifetime. It's always
> nice when unsafety is cleanly scoped, and in this case the deref of the
> nsCString is an unsafe operation. The `unsafe` on this function means that
> the user must ensure that they will satisfy all safety requirements in the
> future, which is not as nice as being asked to satisfy all safety
> requirements in the immediate vicinity of the code and nowhere else.
> 
> Perhaps we could have a DependentNSCString that can be safely obtained and
> has a lifetime, but can be unsafely cast to an nsCString? Or is that
> overkill? It has the same problem, but is more explicit.

I'm reworking the string bindings - I'll f? you on an updated version
I think interacting with XPCOM strings from Rust would definitely be useful (i.e. bug 1294742).

I'm more skeptical about exposing arbitrary XPIDL interfaces and nsISupports stuff. There is so much magic that XPConnect, the Cycle Collector, and other things do in this space, and it seems best to have a very clear and auditable API surface rather than exposing everything and relying on the consumer to audit.
Let's leave this bug open strictly for XPIDL bindings, and split out the XPCOM string stuff into a separate bug.  The XPCOM string stuff will talk a lot less thought to review and be more immediately useful when it does land.  Sound plausible?
Flags: needinfo?(michael)
(Assignee)

Comment 27

3 years ago
(In reply to Nathan Froyd [:froydnj] from comment #26)
> Let's leave this bug open strictly for XPIDL bindings, and split out the
> XPCOM string stuff into a separate bug.  The XPCOM string stuff will talk a
> lot less thought to review and be more immediately useful when it does land.
> Sound plausible?

That is actually my current plan. I was thinking of splitting the string bindings into a separate crate like `nsstring`, and just using that inside of this patch. 

The new rewrite is a bit more in depth than the current one because I believe that stylo wants to be interacting with `nsString` struct members as well as `nsAString` pointer arguments (which is all that the original bindings supported). I'll try to have safe, and easy to use bindings. They are going to probably be a bit more gross than the originals though.

The big problem is that we can't implement Drop on any of the functions we expose to the FFI, because of drop flags. This means that we will need two nsString types - one to be used in #[repr(C)] structs which leaks memory on drop, and may be unsafe - and one which is safe and has a Drop method for use in general code. I'm hoping to make moving between these types as easy as possible.

bholley, I'd love it if you could confirm my suspicion about you wanting to work with ns[C]Strings in structs, so I can decide how much time I should put into making that interface nice right now, instead of just working more on it down the line.
Flags: needinfo?(michael)
(Assignee)

Updated

3 years ago
Flags: needinfo?(bobbyholley)
The most important thing by far is to have an dead-simple and safe mechanism to create and manipulate manipulate UTF-16 strings from Servo code. This is important because we want to reduce the temptation for people to just use str and String to build strings and then convert to UTF-16 at the binding layer when they need to pass to Gecko, since that's wasted work.

My dream would be if we could just compile geckolib with String/str as UTF16. That's obviously not possible because of the ways that the internal storage are observable, but getting as close as possible to that kind of transparency would be great. In particular, it would be great if people writing new non-stylo-specific code in the style crate (like selector serialization implementations) could get UTF16 strings as automatically as possible (a trait or conditional compilation could help).

The second-most-important goal is to have an easy-to-use abstraction around nsAString (and also nsACString) so that we can avoid converting to/from byte buffers at the FFI boundaries.

The third-most-important goal is applying the aforementioned nsA{,C}String abstractions to struct fields. That would be useful, but I would prefer not to sacrifice ergonomics for goals (1) and (2) for its sake, and could live with FFI manipulation of nsStrings-in-structs in the worst case. My hope is that we can be clever and get all three without too much in the way of tradeoffs.
Flags: needinfo?(bobbyholley)
(Assignee)

Updated

3 years ago
Depends on: 1295762
(Assignee)

Comment 29

2 years ago
In my spare time last weekend I resurrected the patches on this bug, and added the ability to implement interfaces in rust.

This was mostly because of nikomatsakis's blog post http://smallcultfollowing.com/babysteps/blog/2017/05/02/gnome-class-integrating-rust-and-the-gnome-object-system/ which reminded me of this work I had done, and made me want to work on it again.

That being said, I've also been seeing occasional emails etc. where people are interested in implementing more parts of Gecko in rust, and this might be a useful tool for doing that. For example, #[derive(xpcom)] might be usable to implement nsIStandardURL for RustURL entirely from within rust, which could be nice (although I believe after 57 we're going to want to move away from XPCOM for urls).

I'm flagging you for feedback on this patchset, but I don't think it's entirely in a final state. For example, the #[derive(xpcom)] work doesn't adapt the FFI types to rust types yet, meaning that implementing interfaces in rust is a bit unsafe right now.

I'll also post a link to a github branch which has this patchset applied, and an example generated dist/xprs directory.

MozReview-Commit-ID: 6OIoyxBe3ov
Attachment #8866555 - Flags: feedback?(nfroyd)
Attachment #8866555 - Flags: feedback?(manishearth)
(Assignee)

Updated

2 years ago
Attachment #8779015 - Attachment is obsolete: true
Attachment #8779016 - Attachment is obsolete: true
Attachment #8779012 - Attachment is obsolete: true
Attachment #8779013 - Attachment is obsolete: true
Attachment #8779014 - Attachment is obsolete: true
(Assignee)

Comment 30

2 years ago
MozReview-Commit-ID: KgTJs8Ua9zE
(Assignee)

Comment 31

2 years ago
MozReview-Commit-ID: 4PtsNkFzGyQ
(Assignee)

Comment 32

2 years ago
MozReview-Commit-ID: F2149d2sEnf
(Assignee)

Comment 36

2 years ago
MozReview-Commit-ID: 5qqF7Mkfymr
(Assignee)

Comment 37

2 years ago
MozReview-Commit-ID: HVXdISSySlb
Comment on attachment 8866555 [details] [diff] [review]
Rust implementation of RefPtr for XPCOM-style internal refcount structs

Review of attachment 8866555 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/rust/xpcom/src/refptr.rs
@@ +17,5 @@
> +/// own memory. RefPtr will invoke the addref and release methods at the
> +/// appropriate times to facilitate the bookkeeping.
> +pub struct RefPtr<T: RefCounted + 'static> {
> +    // We're going to cheat and store the internal reference as an &'static T
> +    // instead of an *const T or Shared<T>, because Shared and NonZero are

Make sure your Send/Sync !impls exist; that's one more thing Shared<T> gets you IIRC.

Since RefPtr *can* be threadsafe refcounted, you may want to make Sendness depend on some property of the inner type (probably Sync + HasThreadSafeRefCount)
Attachment #8866555 - Flags: feedback?(manishearth) → feedback+
(Assignee)

Comment 39

2 years ago
This is what the generated output directory looks like right now: https://github.com/mystor/dist-xprs-example
The tree with all patches applied: https://github.com/mystor/mozilla-central/tree/xpcom_rust

I'm also hoping to add in a follow-up patch the ability to mark a #[derive(xpcom)] type with #[xpcyclecollected] to generate a cycle collection implementation, so you can cycle collect through rust types.

Perhaps I will also look into doing that with non-xpcom rust types as cycle collecting through rust types might be nice.
(Assignee)

Comment 40

2 years ago
(In reply to Manish Goregaokar [:manishearth] from comment #38)
> Comment on attachment 8866555 [details] [diff] [review]
> Rust implementation of RefPtr for XPCOM-style internal refcount structs
> 
> Review of attachment 8866555 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/rust/xpcom/src/refptr.rs
> @@ +17,5 @@
> > +/// own memory. RefPtr will invoke the addref and release methods at the
> > +/// appropriate times to facilitate the bookkeeping.
> > +pub struct RefPtr<T: RefCounted + 'static> {
> > +    // We're going to cheat and store the internal reference as an &'static T
> > +    // instead of an *const T or Shared<T>, because Shared and NonZero are
> 
> Make sure your Send/Sync !impls exist; that's one more thing Shared<T> gets
> you IIRC.
> 
> Since RefPtr *can* be threadsafe refcounted, you may want to make Sendness
> depend on some property of the inner type (probably Sync +
> HasThreadSafeRefCount)

The type `T` will implement neither Send nor Sync if it is an XPCOM interface (due to the second last patch in the patch set).

I think this should make RefPtr<T> neither send nor sync as well.
Comment on attachment 8866555 [details] [diff] [review]
Rust implementation of RefPtr for XPCOM-style internal refcount structs

Review of attachment 8866555 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable to me.
Attachment #8866555 - Flags: feedback?(nfroyd) → feedback+
MozReview-Commit-ID: 9EB18vTItt9
(Assignee)

Updated

a year ago
Attachment #8866555 - Attachment is obsolete: true
Attachment #8866556 - Attachment is obsolete: true
Attachment #8866557 - Attachment is obsolete: true
Attachment #8866558 - Attachment is obsolete: true
Attachment #8866559 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8866560 - Attachment is obsolete: true
Attachment #8866561 - Attachment is obsolete: true
Attachment #8866562 - Attachment is obsolete: true
Attachment #8866563 - Attachment is obsolete: true
MozReview-Commit-ID: F9y58RKVSxX
MozReview-Commit-ID: Gd5Mz3zydLA
MozReview-Commit-ID: EnIprKJ68f0
MozReview-Commit-ID: FULbC5mUE3m
MozReview-Commit-ID: INMrPmIamPx
MozReview-Commit-ID: Ienjk9ySwiW
(Assignee)

Updated

a year ago
See Also: → 1428139
I've updated the patches after our conversation in Austin, so I'm going to put the code up for review now. Let me know what sorts of things you're going to want me to change.

As some additional resources to make it easier to review the code:

The current state of the tree with these patches applied is on github here: 
https://github.com/mystor/mozilla-central/tree/xpcrs

I ran rustdoc on the xpcom crate, and have hosted the results of that here:
https://mystor.github.io/xpcrs-rustdoc/xpcom/

I've tried my best to keep good documentation for this crate, which you can read there, as well as see the source text for generated files.

===

There are some changes which I'm going to want to make, but which I figured should be done after the initial pass has landed in tree:

* Adding support for Cycle Collection through rust-implemented xpcom interfaces
* Cleaning up IDL files to remove the need for the forwardBlacklist in xpcom.py
* I'm sure other things which I've forgotten to write here
Please see comment 49 for details

MozReview-Commit-ID: 9EB18vTItt9
Attachment #8939997 - Flags: review?(nfroyd)
(Assignee)

Updated

a year ago
Attachment #8932250 - Attachment is obsolete: true
Attachment #8932251 - Attachment is obsolete: true
Attachment #8932253 - Attachment is obsolete: true
Attachment #8932254 - Attachment is obsolete: true
Attachment #8932255 - Attachment is obsolete: true
Attachment #8932256 - Attachment is obsolete: true
Attachment #8932257 - Attachment is obsolete: true
MozReview-Commit-ID: H5nxsk4cg2E
Attachment #8939998 - Flags: review?(nfroyd)
MozReview-Commit-ID: IPcuPuXDZho
Attachment #8939999 - Flags: review?(nfroyd)
MozReview-Commit-ID: K37KyHkKsSl
Attachment #8940000 - Flags: review?(nfroyd)
MozReview-Commit-ID: 90Yv3H40jZW
Attachment #8940001 - Flags: review?(nfroyd)
MozReview-Commit-ID: 5oy4wVaaVeP
Attachment #8940002 - Flags: review?(nfroyd)
MozReview-Commit-ID: D4kvNFgmIpH
Attachment #8940003 - Flags: review?(nfroyd)
MozReview-Commit-ID: 1b6tfHtyDWf
Attachment #8940004 - Flags: review?(nfroyd)
(In reply to Nika Layzell [:mystor] from comment #49)
> * Adding support for Cycle Collection through rust-implemented xpcom
> interfaces

This sounds utterly terrifying, and I'm not sure we want to even imagine a world where this is possible.
Comment on attachment 8939997 [details] [diff] [review]
Part 1: Refactor idl files to work better with rust bindings

Review of attachment 8939997 [details] [diff] [review]:
-----------------------------------------------------------------

I assume this is so everybody has a consistent view of what nsViewID is, otherwise you either:

a) get multiple incompatible (newtyped?) nsViewIDs on the Rust side; or
b) rustc complains that you're defining nsViewID multiple times?

A slightly extended explanation here and in the commit message would be super nice.
Attachment #8939997 - Flags: review?(nfroyd) → review+
Comment on attachment 8939998 [details] [diff] [review]
Part 2: Add skeleton crates for xpcom bindings

Review of attachment 8939998 [details] [diff] [review]:
-----------------------------------------------------------------

Um.  Sure?  Why is this a separate commit instead of other patches just adding the necessary infrastructure as they see fit?  It's a little hard to judge if the dependencies pre-declared actually make sense; one has to sort of take it on faith that subsequent patches will use them appropriately...
Attachment #8939998 - Flags: review?(nfroyd) → review+
Comment on attachment 8939999 [details] [diff] [review]
Part 3: Add rust type information to the idl-parser for xpidl.py

Review of attachment 8939999 [details] [diff] [review]:
-----------------------------------------------------------------

Cancelling review because of rustBlacklistedForward, but interested in your thoughts on other things below.

::: xpcom/idl-parser/xpidl/xpidl.py
@@ +248,5 @@
>  
> +class RustNoncompat(Exception):
> +    """Thie exception is raised when a particular type or function cannot be safely exposed to rust code"""
> +    def __init__(self, reason):
> +        self.reason = reason

`reason` suggests to me an arbitrary human-readable string--and some uses of RustNoncompat act in this way--while the usage of RustNoncompat to produce doccomments in part 4 assumes `reason` is more like a simple name.  I don't think this is a great state of affairs.  Maybe the doccomment stuff should just say:

"Unable to generate binding because `...`"

and reason should always be an arbitrary human-readable string?

@@ +419,5 @@
>          return "%s %s" % (self.name,
>                            calltype != 'in' and '* *' or '*')
>  
> +    def rustType(self, calltype):
> +        if rustBlacklistedForward(self.name):

What is this supposed to be?  It doesn't appear to be defined in this patch.

@@ +451,5 @@
> +        'char': "u8",
> +        'char16_t': "u16",
> +        'nsID': "nsID",
> +        'nsIID': "nsIID",
> +        'nsCID': "nsCID",

We have mappings for ns*ID in Rust already?
Attachment #8939999 - Flags: review?(nfroyd)
Comment on attachment 8940000 [details] [diff] [review]
Part 4: Generate runtime bindings for calling xpcom methods from rust

Review of attachment 8940000 [details] [diff] [review]:
-----------------------------------------------------------------

Nothing to complain about code-wise, just a bunch of documentation nits to straighten out.

::: xpcom/idl-parser/xpidl/rust.py
@@ +14,5 @@
> +# notxpcom methods return their results directly by value. The x86 windows
> +# stdcall ABI returns aggregates by value differently for methods than
> +# functions, and rust only exposes the function ABI, so that's the one we're
> +# using. The correct ABI can be emulated for notxpcom methods returning
> +# aggregates by passing an &mut ReturnType parameter as the second parameter.

Is this a hazard in the "don't call notxpcom methods" sense?  Or something else?  It's not clear to me what this paragraph is warning against.  Should we just not generate bindings to those methods?

@@ +22,5 @@
> +# nostdcall methods on x86 windows will use the thiscall ABI, which is not
> +# stable in rust right now, so we cannot generate bindings to them.
> +
> +# In general, passing C++ objects by value over the C ABI is not a good idea,
> +# and when possible we should avoid doing so.

...meaning we should avoid generating bindings for such methods, or people just have to be aware to "not do that"?

@@ +301,5 @@
> +                fd.write("/* unable to generate %s typedef, as %s is not supported yet */\n\n" %
> +                         (p.name, reason))
> +
> +
> +uuid_decoder = re.compile(r"""(?P<m0>[a-f0-9]{8})-

Can we move this a little closer to the point of use?

@@ +323,5 @@
> +"""
> +
> +
> +deref_tmpl = """\
> +impl ::std::ops::Deref for %(name)s {

These templates are mostly self-explanatory, but WDYT about adding some comments about how they fit in to the larger picture or what they're meant to enable?  Or is commenting on what they're meant to enable too basic?

Or even *having* a comment describing the big picture of what a Rust binding for a small IDL looks like, and how people might implement the interface in Rust?

@@ +333,5 @@
> +        }
> +    }
> +}
> +
> +// Enable coercing to base classes

This comment seems pretty basic (though helpful!). ;)

@@ +397,5 @@
> +"""
> +
> +
> +wrapper_tmpl = """\
> +impl %(name)s {

For instance, why do we call this `wrapper_tmpl`?  How does that fit into the bigger picture?

@@ +445,5 @@
> +            entries += "/* %s */\n" % member.toIDL()
> +            entries += "%s,\n" % attrAsVTableEntry(iface, member, True)
> +            if not member.readonly:
> +                entries += "%s,\n" % attrAsVTableEntry(iface, member, False)
> +            entries += "\n"

For all of this and some of the below, can we see if structuring them as format templates works better?  Something a little higher level than pasting together individual lines would be nice.  Maybe even just stuffing each line in `entries` -- which would become a list -- and then ',\n'.join(entries), too?  (That's a little tricky with comments, though...)

::: xpcom/rust/xpcom/src/interfaces/nonidl.rs
@@ +43,5 @@
> +}
> +
> +nonidl!(nsIDocument,
> +        nsID(0xce1f7627, 0x7109, 0x4977,
> +             [0xba, 0x77, 0x49, 0x0f, 0xfd, 0xe0, 0x7a, 0xaa]));

Should we have comments in Rust and C++ about keeping these IIDs in sync?
Attachment #8940000 - Flags: review?(nfroyd)
Comment on attachment 8940002 [details] [diff] [review]
Part 6: Expose the cached XPCOM service getters to rust code

Review of attachment 8940002 [details] [diff] [review]:
-----------------------------------------------------------------

No complaints here.

::: xpcom/build/Services.py
@@ +144,5 @@
> +    output.write(CPP_INCLUDES)
> +
> +    for svc in services:
> +        output.write("""
> +static {1}* g{0} = nullptr;

I tend to think the %(NAME)s format is a little more readable and robust; can we use that throughout instead?

@@ +196,5 @@
> +
> +        output.write("""
> +/// Fetches a cached reference to the `{0}`.
> +/// This function will return `None` during XPCOM shutdown.
> +pub fn get_{0}() -> Option<RefPtr<::interfaces::{1}>> {{

Why can we not return a null RefPtr here?  It looks like we're never returning None?
Attachment #8940002 - Flags: review?(nfroyd) → review+
Comment on attachment 8940001 [details] [diff] [review]
Part 5: Allow implementing xpcom interfaces from rust code with a custom derive

Review of attachment 8940001 [details] [diff] [review]:
-----------------------------------------------------------------

This patch would benefit from--at the very least--some big-picture documentation about how to use the constructs introduced therein that I'm going to cancel the review for now.

::: xpcom/rust/xpcom/xpcom_macros/src/lib.rs
@@ +497,5 @@
> +            }
> +        }
> +
> +        /// This trait is implemented on the interface types which this
> +        /// `#[derive(xpcom)]` type can be safely ane cheaply coerced to using

Nit: "...and be safely and cheaply..."?

@@ +502,5 @@
> +        /// the `coerce` method.
> +        ///
> +        /// The trait and its method should usually not be used directly, but
> +        /// rather acts as a trait bound and implementation for the `coerce`
> +        /// method's.

Nit: no apostrophe here?
Attachment #8940001 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #63)
> @@ +196,5 @@
> > +
> > +        output.write("""
> > +/// Fetches a cached reference to the `{0}`.
> > +/// This function will return `None` during XPCOM shutdown.
> > +pub fn get_{0}() -> Option<RefPtr<::interfaces::{1}>> {{
> 
> Why can we not return a null RefPtr here?  It looks like we're never
> returning None?

Oh, the RefPtr constructor turns things into Option<> for us?  Is that how this works?
Comment on attachment 8940003 [details] [diff] [review]
Part 7: Expose the nsComponentManager static interfaces to rust code

Review of attachment 8940003 [details] [diff] [review]:
-----------------------------------------------------------------

Should the documentation make reference to the C++ constructs that the Rust code is replicating here?  A few comments below.

::: xpcom/rust/xpcom/src/statics.rs
@@ +1,1 @@
> +use std::ffi::CStr;

Mmmm, do we need licenses in our Rust code?  I guess we think the Cargo.toml declaration is sufficient?  Have we asked lawyers (i.e. Gerv) about this?

@@ +55,5 @@
> +
> +/// Helper for calling `nsIComponentManager::CreateInstance` on the global
> +/// `nsIComponentRegistrar`.
> +#[inline]
> +pub fn create_instance<T: XpCom>(cid: &nsCID) -> Option<RefPtr<T>> {

I think this is the more unusual form and...

@@ +74,5 @@
> +
> +/// Helper for calling `nsIComponentManager::CreateInstanceByContractID` on the
> +/// global `nsIComponentRegistrar`.
> +#[inline]
> +pub fn create_instance_by_contract_id<T: XpCom>(id: &CStr) -> Option<RefPtr<T>> {

...this would be the more common form.  So `create_instance_by_contract_id` should really have the shorter name, i.e. create_instance.  Actually, I think we should just dispense with the nsCID form unless somebody *really* needs it.

@@ +93,5 @@
> +
> +/// Helper for calling `nsIServiceManager::GetService` on the global
> +/// `nsIServiceManager`.
> +#[inline]
> +pub fn get_service<T: XpCom>(cid: &nsCID) -> Option<RefPtr<T>> {

The same argument from above applies to this pair of functions as well.
Attachment #8940003 - Flags: review?(nfroyd)
Comment on attachment 8940004 [details] [diff] [review]
Part 8: Add some tests for using xpcom interfaces from rust code

Review of attachment 8940004 [details] [diff] [review]:
-----------------------------------------------------------------

One comment, which may or may not be easy to resolve.

::: xpcom/rust/gtest/xpcom/Test.cpp
@@ +6,5 @@
> +
> +TEST(RustXpcom, CallIURIFromRust)
> +{
> +  // XXX: Improve
> +  EXPECT_TRUE(Rust_CallIURIFromRust());

Let's, um, not commit this without improving it?  Or remove the comment?  Or maybe return a reference to the IO service that we can assert is identical to the IO service the C++ side can access?  I'm unsure of what you wanted to improve here.

@@ +21,5 @@
> +
> +  EXPECT_TRUE(runnable);
> +  EXPECT_FALSE(itWorked);
> +  runnable->Run();
> +  EXPECT_TRUE(itWorked);

Incredibly terrifying.
Attachment #8940004 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #60)
> Um.  Sure?  Why is this a separate commit instead of other patches just
> adding the necessary infrastructure as they see fit?  It's a little hard to
> judge if the dependencies pre-declared actually make sense; one has to sort
> of take it on faith that subsequent patches will use them appropriately...

As you might've expected, these patches have... evolved quite a bit as I was writing them. When trying to turn them into a reviewable patch series, I found it easier to just build the structure up front and then fill it in, rather than try to let you review each stage of the patches as it evolved. The order of the patches you're getting isn't exactly the order the features were implemented in.

(In reply to Nathan Froyd [:froydnj] from comment #61)
> Cancelling review because of rustBlacklistedForward, but interested in your
> thoughts on other things below.

Oops - I think I add that in the next patch - I probably just screwed up while I was rebasing.

> `reason` suggests to me an arbitrary human-readable string--and some uses of
> RustNoncompat act in this way--while the usage of RustNoncompat to produce
> doccomments in part 4 assumes `reason` is more like a simple name.  I don't
> think this is a great state of affairs.  Maybe the doccomment stuff should
> just say:
> 
> "Unable to generate binding because `...`"
> 
> and reason should always be an arbitrary human-readable string?

Sure, I'll do that.

> @@ +451,5 @@
> > +        'char': "u8",
> > +        'char16_t': "u16",
> > +        'nsID': "nsID",
> > +        'nsIID': "nsIID",
> > +        'nsCID': "nsCID",
> 
> We have mappings for ns*ID in Rust already?

They're added in the next patch. This patch used to be folded into the same patch as the next one, but I felt like it was distinct enough to pull it apart - perhaps that was a mistake.

(In reply to Nathan Froyd [:froydnj] from comment #62)
> Is this a hazard in the "don't call notxpcom methods" sense?  Or something
> else?  It's not clear to me what this paragraph is warning against.  Should
> we just not generate bindings to those methods?

Yeah, these comments are me documenting the things I've learned about how ABIs work on windows, and how that interacts with the ABIs which we have avaliable in rust. We don't generate any methods which violate these constraints AFAIK, but I wanted to have this written down so if someone else came in and wanted to expose more methods to rust, they'd know what some of the pitfalls were. I'll try to clarify the comments.

> Can we move this a little closer to the point of use?

Ok

> These templates are mostly self-explanatory, but WDYT about adding some
> comments about how they fit in to the larger picture or what they're meant
> to enable?  Or is commenting on what they're meant to enable too basic?
> 
> Or even *having* a comment describing the big picture of what a Rust binding
> for a small IDL looks like, and how people might implement the interface in
> Rust?

Sure, I'll add some comments to the templates explaining how the implementation works & giving a big-picture overview. I generally avoided adding too many comments into the templates because the c++ headers don't have that many comments, and I didn't want to diverge too much.

> For all of this and some of the below, can we see if structuring them as
> format templates works better?  Something a little higher level than pasting
> together individual lines would be nice.  Maybe even just stuffing each line
> in `entries` -- which would become a list -- and then ',\n'.join(entries),
> too?  (That's a little tricky with comments, though...)

Sure, I can probably make these a bit nicer to work with. I'll read over it & see what I can do.

> Should we have comments in Rust and C++ about keeping these IIDs in sync?

I'll add those.

(In reply to Nathan Froyd [:froydnj] from comment #63)
> I tend to think the %(NAME)s format is a little more readable and robust;
> can we use that throughout instead?

Can do - I originally tried using the format API as this is new code, and IIRC the format API is newer, but I don't have any personal preference.

> Why can we not return a null RefPtr here?  It looks like we're never
> returning None?

To fit with other rust pointer types, RefPtr<T> is never null. Instead, the type Option<RefPtr<T>> is used to describe possibly null RefPtrs. As the getter for the service can technically fail, I have to return an Option<RefPtr<T>> here.

The RefPtr::from_raw_dont_addref() function takes a raw pointer as an argument, which can be null, so it returns an Option<T>.

(In reply to Nathan Froyd [:froydnj] from comment #64)
> This patch would benefit from--at the very least--some big-picture
> documentation about how to use the constructs introduced therein that I'm
> going to cancel the review for now.

Ok - I'll add a bunch of big picture documentation. For the most part, this patch doesn't add very many APIs, but rather adds one complex macro: `#[derive(xpcom)]` which does pretty much all of the work for you. It's unfortunately not super well documented right now, so I'll fix that & add some examples to the docs.

The internals of the macro aren't super easy to follow - but I can also add some documentation explaining how it's trying to work. Unfortunately procedural macros in rust are a bit gross sometimes :-/.

(In reply to Nathan Froyd [:froydnj] from comment #66)
> Should the documentation make reference to the C++ constructs that the Rust
> code is replicating here?  A few comments below.

Sure, I can add a note saying that `create_instance` is like C++'s `do_CreateInstance` and `get_service` is like C++'s `do_GetService`? Are you thinking of some other type of comments?

> Mmmm, do we need licenses in our Rust code?  I guess we think the Cargo.toml
> declaration is sufficient?  Have we asked lawyers (i.e. Gerv) about this?

I'll go through and add licenses to the files. It's probably a good idea to do that.

> I think this is the more unusual form and...
> 
> ...this would be the more common form.  So `create_instance_by_contract_id`
> should really have the shorter name, i.e. create_instance.  Actually, I
> think we should just dispense with the nsCID form unless somebody *really*
> needs it.

Sure, I can just remove the nsCID based options. The reasoning behind the naming here is that I gave the functions the same names which they have in C++. *shrug*.

If nsCID based calls are really uncommon, people can just call the function directly if they need it. Might as well encourage the correct API by giving it a short name and not implementing the other one.

(In reply to Nathan Froyd [:froydnj] from comment #67)
> Let's, um, not commit this without improving it?  Or remove the comment?  Or
> maybe return a reference to the IO service that we can assert is identical
> to the IO service the C++ side can access?  I'm unsure of what you wanted to
> improve here.

I'm not sure either - TBH I haven't opened this file in ages... This comment has probably been around since my first patches in oct 2016...

I think I'll return a reference to some structs to C++ so that I can assert they match? Sounds reasonable to me.

> > +  EXPECT_TRUE(itWorked);
> 
> Incredibly terrifying.

*cackles madly*
This change is needed in order to avoid generating multiple `type nsViewID`
declarations in the generated rust code, as that causes build failures.

MozReview-Commit-ID: 9EB18vTItt9
(Assignee)

Updated

a year ago
Attachment #8939997 - Attachment is obsolete: true
Attachment #8939998 - Attachment is obsolete: true
Attachment #8939999 - Attachment is obsolete: true
Attachment #8940000 - Attachment is obsolete: true
Attachment #8940001 - Attachment is obsolete: true
Attachment #8940002 - Attachment is obsolete: true
Attachment #8940003 - Attachment is obsolete: true
Attachment #8940004 - Attachment is obsolete: true
MozReview-Commit-ID: H5nxsk4cg2E
MozReview-Commit-ID: IPcuPuXDZho
Attachment #8941501 - Flags: review?(nfroyd)
MozReview-Commit-ID: K37KyHkKsSl
Attachment #8941502 - Flags: review?(nfroyd)
MozReview-Commit-ID: 90Yv3H40jZW
Attachment #8941503 - Flags: review?(nfroyd)
MozReview-Commit-ID: D4kvNFgmIpH
Attachment #8941505 - Flags: review?(nfroyd)
MozReview-Commit-ID: 1b6tfHtyDWf
Attachment #8941506 - Flags: review?(nfroyd)
The doc URL should be updated to the new version of the code. https://mystor.github.io/xpcrs-rustdoc/xpcom/
(Assignee)

Updated

a year ago
Blocks: 1429816
Attachment #8941501 - Flags: review?(nfroyd) → review+
Comment on attachment 8941502 [details] [diff] [review]
Part 4: Generate runtime bindings for calling xpcom methods from rust

Review of attachment 8941502 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/idl-parser/xpidl/rust.py
@@ +274,5 @@
> +
> +
> +def idl_basename(f):
> +    """returns the base name of a file with the last extension stripped"""
> +    return os.path.basename(f).rpartition('.')[0]

I think this is just os.path.splitext(...basename(f))[0]?

::: xpcom/rust/xpcom/src/interfaces/mod.rs
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +//! This module contains the xpcom interfaces exposed to rust code.

These comments are super helpful, thank you!
Attachment #8941502 - Flags: review?(nfroyd) → review+
Comment on attachment 8941503 [details] [diff] [review]
Part 5: Allow implementing xpcom interfaces from rust code with a custom derive

Review of attachment 8941503 [details] [diff] [review]:
-----------------------------------------------------------------

I have some scattered comments on the documentation, but this is a definite improvement, thank you for writing docs.

::: xpcom/rust/xpcom/xpcom_macros/src/lib.rs
@@ +9,5 @@
> +//!
> +//! ```ignore
> +//! // This derive should be placed on the initialization struct in order to
> +//! // trigger the procedural macro.
> +//! #[derive(xpcom)]

These attributes are all being stuck on `struct InitImplRunnable`, correct?  It's not super-clear to me whether you're doing documentation for the custom attributes, an example, or both.  Maybe you should have:

```
#[derive(xpcom),xpimplements(nsIRunnable),refcnt = "atomic"]
...example struct and impl here...

...explain what gets generated...
```

and then have a little blurb on each of the attributes and how they work?

@@ +15,5 @@
> +//! // The xpimplements attribute should be passed the names of the IDL
> +//! // interfaces which you want to implement. These can be separated by commas
> +//! // if you want to implement multiple interfaces.
> +//! //
> +//! // If any of the methods in the interface listed are not rust-compat,

What is "rust-compat", exactly?  I mean, clearly it is "rust-compatible", but I don't know what that means.  Is it defined somewhere else in the documentation?

@@ +30,5 @@
> +//! //    ~= NS_DECL_CYCLE_COLLECTING_ISUPPORTS in C++
> +//! //    (NOTE: This is not implemented yet)
> +//! #[refcnt = "atomic"]
> +//!
> +//! // The struct with the attribute on it's name must start with `Init`.

Nit: "its name"

@@ +31,5 @@
> +//! //    (NOTE: This is not implemented yet)
> +//! #[refcnt = "atomic"]
> +//!
> +//! // The struct with the attribute on it's name must start with `Init`.
> +//! // The custom derive will define the actual underlying struct.

Nit: maybe this should be "generate the actual underlying struct"?  And talk about how it will be named?

@@ +42,5 @@
> +//! }
> +//!
> +//! // Methods should be implemented directly on the generated struct. All
> +//! // methods other than `AddRef`, `Release`, and `QueryInterface` must be
> +//! // implemented manually.

It seems confusing to talk about `impl` on the generated struct before explaining the generated struct.  Maybe have the example, explain what all gets generated from it, and then you can talk about `impl`'ing things?

@@ +88,5 @@
> +//! impl ImplRunnableCoerce for nsIRunnable { .. }
> +//! impl ImplRunnableCoerce for nsISupports { .. }
> +//! ```
> +//!
> +//! And finally, the `RefCounted` trait will be implemented.

Is there a way to link this to the `xpcom` documentation?  Or would that magically happen if we were generating documentation from a crate that included them both, like gkrust?
Attachment #8941503 - Flags: review?(nfroyd)
Attachment #8941505 - Flags: review?(nfroyd) → review+
Attachment #8941506 - Flags: review?(nfroyd) → review+
(Assignee)

Updated

a year ago
Attachment #8941502 - Attachment is obsolete: true
Attachment #8941503 - Attachment is obsolete: true
Comment on attachment 8942347 [details] [diff] [review]
Part 5: Allow implementing xpcom interfaces from rust code with a custom derive

Review of attachment 8942347 [details] [diff] [review]:
-----------------------------------------------------------------

This version looks good!  I have a few comments, but they are mostly of a "WDYT" nature, rather than the "this should definitely be changed" nature.

::: xpcom/rust/xpcom/xpcom_macros/src/lib.rs
@@ +51,5 @@
> +//! //  * "nonatomic" == non atomic reference count
> +//! //    ~= NS_DECL_ISUPPORTS in C++
> +//! //  * "cyclecollected" == cycle collected reference count
> +//! //    ~= NS_DECL_CYCLE_COLLECTING_ISUPPORTS in C++
> +//! //    (NOTE: This is not implemented yet)

...and, sanity prevailing, never will be. =D

I almost think that we should just leave cyclecollected out of the implementation and documentation entirely, perhaps with a note that cycle collected classes are not supported at this time.  WDYT?

@@ +67,5 @@
> +//!     i: i32,
> +//! }
> +//! ```
> +//!
> +//! This will generate an underlying `ImplRunnable` struct, which will implement

Personal hangup time: I had a professor who insisted on never using a bare "this" in prose; always be specific about what you're referring to.  I thought it was quirky advice at the time, but I've come to think prose does improve by using it.  Maybe say "The above example" or "The above code"?

@@ +81,5 @@
> +//! // Allocates and initializes a new instance of this type. The values will
> +//! // be moved from the `Init` struct which is passed in.
> +//! fn allocate(init: InitImplRunnable) -> RefPtr<Self>;
> +//!
> +//! // Helper for performing the `query_interface` operation to case to a

Nit: "to cast", I think?

@@ +195,5 @@
> +}
> +
> +/// The type of the reference count to use for the struct.
> +#[derive(Debug, Eq, PartialEq, Copy, Clone)]
> +enum RefcntType {

WDYT about calling this `RefcntKind` instead, with attendant changes all throughout (e.g. get_refcnt_kind instead of get_refcnt_type)?  I think it's reasonable to call it `RefcntType`, but I also try to reserve "type" to describe actual programming language-level types, which doesn't really apply here.

@@ +203,5 @@
> +}
> +
> +impl RefcntType {
> +    /// Get the type of the Refcnt.
> +    fn as_ty(&self) -> Result<Ty, Box<Error>> {

To be clear, if we did rename the above, I think it's appropriate to keep the "type" nomenclature here.

@@ +420,5 @@
> +    };
> +
> +    // Include each of the method definitions for this interface.
> +    let mut vtable_init = Vec::new();
> +    for method in methods {

This is a style suggestion only, and you are not obliged to change any code: You use a lot of loops to push onto mutable vecs.  Consider building iterators with map() and then calling collect() instead.  I think we should try to consider pushing an iterator-based approach as much as possible in our Rust code.

@@ +449,5 @@
> +        #[inline]
> +        unsafe fn recover_self<'a, T>(this: *const T, _: &'a ()) -> &'a #name {
> +            // Calculate the offset of the field in our struct.
> +            // XXX: Should we use the fact that our type is #[repr(C)] to avoid
> +            // this?

My sense here is "no", but I am open to hearing alternative opinions.
Attachment #8942347 - Flags: review?(nfroyd) → review+

Comment 83

a year ago
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8db6468a787
Part 1: Refactor idl files to work better with rust bindings, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/374a2c304e65
Part 2: Add skeleton crates for xpcom bindings, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/95d9c9dd9909
Part 3: Add rust type information to the idl-parser for xpidl.py, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/da3b2cacc7b3
Part 4: Generate runtime bindings for calling xpcom methods from rust, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/7727478c8217
Part 5: Allow implementing xpcom interfaces from rust code with a custom derive, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc16d00a1e4d
Part 6: Expose the cached XPCOM service getters to rust code, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/8365c868dd1d
Part 7: Expose the nsComponentManager static interfaces to rust code, r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/129011c68478
Part 8: Add some tests for using xpcom interfaces from rust code, r=froydnj
Great to see this land!

(In reply to Nika Layzell [:mystor] from comment #0)
> This patch only implements the calling side of the bindings, and does not
> provide the tools for implementing these interfaces in rust code. That will
> come in an upcoming patch based on this one.

Is the a bug on file for this part? I'm not seeing one.
That comment is misleading. Impementing Xpcom interfaces is supported now.
Depends on: 1433211

Comment 87

a year ago
Out of curiosity. Is there an easy example or any other sort of documentation on how to implement XPCOM interfaces in Rust?

(In reply to MarkBauer from comment #87)

Out of curiosity. Is there an easy example or any other sort of
documentation on how to implement XPCOM interfaces in Rust?

Lina recently posted about this: https://groups.google.com/d/msg/mozilla.dev.platform/u8scZop3FkM/5Kra3EHQAwAJ

You need to log in before you can comment on or make changes to this bug.