Generate Firefox Desktop front-end bindings with WebIDL #255

linabutler · 2020-08-27T04:02:48Z

This PR is a first cut at a UniFFI backend for Firefox! It's a work in progress, but can generate working bindings now, so I wanted to get it up for review. I'd love your comments! 💜

The backend works a little differently than the others. Firefox expects one header file per interface (and a separate one for the namespace that holds top-level functions), so UniFFI emits multiple .h and .cpp files. There's also complexity around handling different types. Some are reflected differently depending on whether they're arguments, type parameters, or local variables; some are returned directly and others are passed as out parameters; and the WebIDL implementation signatures have extra leading and trailing arguments. All this makes macros.cpp pretty gnarly 🙀

SharedHeaderTemplate.h contains the RustBuffer helpers and serialization logic. The ideas are the same as the Swift, Kotlin, and Python backends, but the implementation is a pile of C++ code. I'll add some more comments throughout it in the coming days, so that it's a bit easier to digest. This part can also be reused wholesale for our pure-C++ Nimbus interface, there's not much that's WebIDL specific here.

Getting this building

The setup for this is a little janky for now, but here's how to get this running! You'll need a full (not artifact) build of Firefox, since this touches lots of C++ and Rust code.

git checkout gecko to switch to the Gecko branch.
Make a new folder for the bindings: mkdir rondpoint-gecko
Generate the Gecko bindings for Rondpoint: cd uniffi_bindgen; and cargo run -- generate ../examples/rondpoint/src/rondpoint.idl -l gecko -o ../rondpoint-gecko
Symlink the generated WebIDL file into dom/chrome-webidl in m-c: ln -s /uniffi-rs/rondpoint-gecko/Rondpoint.webidl /gecko/dom/chrome-webidl/Rondpoint.webidl
Add Rondpoint.webidl to dom/chrome-webidl/moz.build, in the WEBIDL_FILES section.
Symlink the rondpoint-gecko folder from step (2) into an existing (or new!) directory in m-c that has a moz.build file. Any one will do; I picked toolkit/components/extensions/storage for no other reason than it's a pretty small directory and makes it easy to do directory-specific builds.
Symlink uniffi-rs into m-c.
Add uniffi-example-rondpoint as a dependency to toolkit/library/rust/shared/Cargo.toml, as a path dependency inside the symlink from step (7).
Add extern crate uniffi_rondpoint to toolkit/library/rust/shared/lib.rs.
Run mach vendor rust.
Add the generated sources and xpcshell test manifest to the moz.build in the folder from step (6), like this:

EXPORTS.mozilla.dom += [
    'rondpoint-gecko/Retourneur.h',
    'rondpoint-gecko/Rondpoint.h',
    'rondpoint-gecko/RondpointShared.h',
    'rondpoint-gecko/Stringifier.h',
]

UNIFIED_SOURCES += [
    'rondpoint-gecko/Retourneur.cpp',
    'rondpoint-gecko/Rondpoint.cpp',
    'rondpoint-gecko/Stringifier.cpp',
]

XPCSHELL_TESTS_MANIFESTS += [
    'uniffi-rs/examples/rondpoint/tests/gecko/xpcshell.ini'
]

And then run mach build, and you should be good to go! You can run mach xpcshell-test toolkit/components/extensions/storage/uniffi-rs/examples/rondpoint/tests/gecko/xpcshell.ini after the build is done! 😊

Hacks

I had to make two changes to the Cargo.tomls in UniFFI to get this building in Firefox, which we'll want to fix before merging:

Remove the top-level Cargo.toml, so that I could symlink uniffi-rs into my m-c tree and add uniffi-example-rondpoint as a path dependency. I'm not sure this is necessary; if we can avoid symlinking uniffi-rs into m-c, we can do without.
Remove the cargo_metadata dependency. When I tried building Firefox with the version of cargo_metadata from Crates.io, I got a panic in one of its derive macros. Looks lik that was fixed in bump serde dependency to 1.0.107 oli-obk/cargo_metadata#123 by bumping the Serde version, but that hasn't been published yet, so mach vendor rust doesn't know to pull the new version of Serde in. But changing cargo_metadata to a Git dependency confused Cargo: mach vendor rust didn't vendor any of UniFFI's dependencies, and then mach build complained that it couldn't resolve the versions properly. I'm not sure what's going on there, and didn't dig too much more into it.

I also had to change the crate-type to ["cdylib", "lib"], so that we could link the crate into gkrust. If it's just cdylib, extern crate won't find it.

uniffi_bindgen/src/bindings/gecko/templates/WebIDLTemplate.webidl

linabutler · 2020-08-27T05:06:49Z

uniffi_bindgen/src/bindings/gecko/gen_gecko.rs

+/// Returns a dummy empty value for the given type. This is used to
+/// implement the `cpp::bail` macro to return early if a WebIDL function or
+/// method throws an exception.
+pub fn default_return_value_cpp(type_: &Type) -> Option<String> {


It’s unfortunate that this has to exist. Basically, if you want to throw an error from inside a WebIDL method with a signature like int32_t Foo(ErrorResult& aRv), you can call aRv.Throw(...) in the method, but you still need to return some value—even though it’ll get ignored by the bindings code. Methods that return results via out params don’t need to do this; they can just do return; and not set the param.

uniffi_bindgen/src/bindings/gecko/gen_gecko.rs

uniffi_bindgen/src/bindings/gecko/templates/SharedHeaderTemplate.h

uniffi_bindgen/src/bindings/gecko/templates/WebIDLTemplate.webidl

uniffi_bindgen/src/bindings/gecko/templates/macros.cpp

eoger · 2020-08-27T12:48:28Z

examples/rondpoint/tests/gecko/test_rondpoint.js

@@ -0,0 +1,141 @@
+add_task(async function test_rondpoint() {


Can't believe this is real 🎉

What is add_task? I can see it used in this file, but I can't see it anywhere else.

I imagine the idea is to run these tests using mach xpcshell-test somehow - no idea what process lina has in mind for having mach find them though

Yep, this file is an xpcshell test!

no idea what process lina has in mind for having mach find them though

Haha, my workflow so far has been “symlink rondpoint into m-c and add it to moz.build with all the other stuff”, but that’s not going to fly for anything more than a demo. Still, if you want to take it for a spin locally, there are only 11 easy steps! 😆

It compiles in Gecko, but haven't tried running it or anything. There are probably memory safety bugs galore. But now it's time to relax, I'll fix these up on Friday.

* Remove XPIDL generation and wire up WebIDL. * Generate serializers for enums and dictionaries. * Generate proper C++ WebIDL implementations.

When building in Gecko, `cargo_metadata` panics in a macro. This is fixed in oli-obk/cargo_metadata#123 (and in Serde upstream), but pulling `uniffi` in as a path dependency in Gecko, and depending on `cargo_metadata` as a Git dependency, confuses Cargo. It doesn't update the vendored versions of Serde or other dependencies, then fails to build complaining it can't satisfy the newer versions that `uniffi` requests. In the interest of getting a demo working, I've just commented out the dependencies for now, and downgraded the other versions to match what's already vendored in m-c.

...But doesn't link, since I haven't figured out how to get `uniffi` vendored correctly. Progress, though, we're correctly generating WebIDL bindings to where they compile!

We can now generate (and compile!) the Rondpoint binding.

...And pass only the fields from the `ComponentInterface` that each template needs, not the full interface.

nnethercote

This looks good to me. A lot of boilerplate-y stuff, but that's unavoidable, and it's easy to read and well-commented. I have a few nits, mostly about improving comments.

uniffi_bindgen/src/bindings/gecko/templates/SharedHeaderTemplate.h

nnethercote · 2020-09-15T23:38:35Z

uniffi_bindgen/src/bindings/gecko/templates/SharedHeaderTemplate.h

+struct Serializable {
+  /// Returns the size of the serialized value, in bytes. This is used to
+  /// calculate the allocation size for the Rust byte buffer.
+  static CheckedInt<size_t> Size(const T& aValue) = delete;


Why is the return type CheckedInt?

I was using CheckedInt throughout to avoid accidental overflows when serializing, but with the RustBuffer changes on main now, Size isn't necessary anymore. I think we can remove CheckedInt from the other uses, too, since RustBuffers can only hold an int32_t worth of bytes.

uniffi_bindgen/src/bindings/gecko/templates/SharedHeaderTemplate.h

uniffi_bindgen/src/bindings/gecko/templates/WebIDLTemplate.webidl

This makes it hard to symlink UniFFI into the Firefox tree, but once this PR is merged, we shouldn't need that.

rfk

This looks really good.

I mean, I kinda glazed over a bit at the intricacies of the C++ code because I trust that more experienced folks are looked at it, but I'm confident I could use the guideposts here to dig in more to the details if needed!

I note that the generated bindings occupy a kind of global namespace, e.g. we get a class mozilla::dom::Optionnuer in a header file conditioned on #ifndef mozilla_dom_Optionneur. Are we able to add the uniffi package name as a level here, to give e.g. mozilla::dom::Rondpointer::Optionnuer and similar? I'm concerned about accidental name conflicts in future, but I'm also cautious about potential limitations of the way Firefox generates the bindings.

Similarly, if we had multiple uniffi libraries being build together in Firefox, would they run into trouble by both declaring RustBuffer structs etc? I think maybe it's OK because the RondpointShared.h file is only included by its .cpp files, not by any other header files. But I'm no expert, so kind of just looking for explicit re-assurance here :-)

A couple of other broad nits I noticed:

When running cargo test in this branch, I get quite a few warnings about unused symbols which it's probably worth cleaning up.
Running uniffi-bindgen via the cargo run -- method you describe in the PR works, but if I try to build and run the executable via ./target/debug/uniffi-bindgen using the same arguments, it produces an error 'Missing $CARGO_MANIFEST_DIR, cannot load config file: NotPresent'. We should expect that some consumers will use an external uniffi-bindgen rather than building and running it from within this repo.

examples/rondpoint/tests/gecko/test_rondpoint.js

uniffi_bindgen/src/bindings/swift/gen_swift.rs

examples/arithmetic/Cargo.toml

uniffi/src/testing.rs

uniffi_bindgen/src/bindings/gecko_js/gen_gecko_js.rs

uniffi_bindgen/src/bindings/gecko_js/templates/RustBufferHelper.h

uniffi_bindgen/src/bindings/gecko_js/templates/WebIDLTemplate.webidl

uniffi_bindgen/src/bindings/gecko_js/templates/macros.cpp

linabutler · 2020-09-28T21:50:33Z

I mean, I kinda glazed over a bit at the intricacies of the C++ code because I trust that more experienced folks are looked at it, but I'm confident I could use the guideposts here to dig in more to the details if needed!

Thank you so much for digging into this, Ryan, that's mainly what I wanted from your review! 🚀

I note that the generated bindings occupy a kind of global namespace, e.g. we get a class mozilla::dom::Optionnuer in a header file conditioned on #ifndef mozilla_dom_Optionneur. Are we able to add the uniffi package name as a level here, to give e.g. mozilla::dom::Rondpointer::Optionnuer and similar?

I don't think so 😞 I covered this in the Theory of Operation—which would be nice to convert into Markdown and check in to the repo, adding that to my list for this week ☑️ —but the binding declarations generated by Firefox's Codegen.py expect headers to be in mozilla/dom/InterfaceName.h, anda emit forward declarations that expect to find the interfaces and namespaces directly under mozilla::dom. (I think that also means type names will have to be globally unique, across UniFFI interfaces and all the Web APIs currently in Gecko).

Similarly, if we had multiple uniffi libraries being build together in Firefox, would they run into trouble by both declaring RustBuffer structs etc?

Yes, they would, because they're declared in the extern "C" block—thanks for noting this! I'll get that fixed up before landing, by prefixing RustBuffer, ForeignBytes, and RustError with the ffi_namespace(), like you do for the functions. It does mean we'll end up with duplication if we vendor multiple UniFFI components, but that's already the case today, and I suspect making cross-component helper deduping work is going to be more involved.

rfk · 2020-09-28T22:45:51Z

prefixing RustBuffer, ForeignBytes, and RustError with the ffi_namespace(), like you do for the functions.
It does mean we'll end up with duplication

I have perhaps too much faith in Sufficiently Smart Compilers, but hopefully these duplicated struct definitions would not result in duplicate code in the compiled artifacts, would they?

rfk · 2020-09-28T22:47:29Z

(I think that also means type names will have to be globally unique, across UniFFI interfaces and all the Web APIs currently in Gecko).

Hrm. I can see this being a problem (and worse, one that catches us by surprise). If you think there's scope for adding some sort of unique-ifying prefix to these names, it's probably worth putting the infra for that in place sooner rather than later.

linabutler · 2020-09-28T23:00:56Z

I have perhaps too much faith in Sufficiently Smart Compilers, but hopefully these duplicated struct definitions would not result in duplicate code in the compiled artifacts, would they?

Good question—my knowledge of how C++ compilers inline templates, functions, and types is nonexistent 😭 Just like you suggested profiling the size impact of vendoring Nimbus and the bindings, it would be worth measuring the impact of vendoring multiple UniFFI components.

The one thing I'm worried about that might make inlining difficult is that the serialization helpers call into component-specific allocation functions. In libxul or an App Services-style megazord, ffi_rondpoint_abc123_rustbuffer_alloc does the exact same thing as ffi_arithmetic_789xyz_rustbuffer_alloc—but they're exported as two different symbols, and we end up generating two copies of the helpers wrapping them—rondpoint_detail::{Reader, Writer, Serializable, ViaFfi} and arithmetic_detail::{Reader, Writer, Serializable, ViaFfi}, plus all the specializations. Is Clang smart enough to recognize that, and optimize some (or all) those calls out? I don't know 😞

linabutler · 2020-09-28T23:02:07Z

Hrm. I can see this being a problem (and worse, one that catches us by surprise). If you think there's scope for adding some sort of unique-ifying prefix to these names, it's probably worth putting the infra for that in place sooner rather than later.

Good idea. Let's do that now, so that this doesn't sneak up on us later.

Fix #301 cargo tests by updating and reenabling cargo_metadata

Finish the fix for #301

There are two pieces of information that we want to pass along to filters: * The `definition_prefix` config option, used to prefix the generated namespace, interface, enum, and dictionary names. * The `ffi_namespace`, used to generate the names for the `RustBuffer`, `RustError`, and `ForeignBytes` structs. Threading these arguments through Askama is tricky. For example, if the `type_ffi` filter needs to know the `ffi_namespace`, that infects any filters that call it (like `lift_cpp` and `lower_cpp`), all the way up the call chain. The same happens for the `definition_prefix`, which changes the names of the types declared in the WebIDL...so now all type declaration filters need to know about the prefix. Making this work in the generated code is difficult, so we take the least-worst possible option of passing around a `Context` argument (other, less-generic names welcome!) with all the names we need.

nnethercote

LGTM!

linabutler · 2020-09-30T07:31:34Z

Woohoo, thanks so much everyone! 😅 The latest push has the definition_prefix config option, prefixed names for the C structs (so we should be able to compile multiple UniFFIed components in Firefox), and a last-minute fix for nullable strings uncovered by the Rondpoint tests 👍🏻 (that example is paying for itself over and over!)

I'll go ahead and mash the "merge" button now and we can file follow-ups for anything we forgot!

linabutler mentioned this pull request Aug 27, 2020

Prototype bindings for Firefox Desktop front-end (JS) consumers #11

Closed

rfk reviewed Aug 27, 2020

View reviewed changes

uniffi_bindgen/src/bindings/gecko/templates/WebIDLTemplate.webidl Outdated Show resolved Hide resolved

linabutler commented Aug 27, 2020

View reviewed changes

eoger reviewed Aug 27, 2020

View reviewed changes

linabutler added 15 commits August 27, 2020 11:01

Initial work on Gecko bindings.

6fc2f8a

Implement remaining C++ type conversions.

71eb8a0

It compiles in Gecko, but haven't tried running it or anything. There are probably memory safety bugs galore. But now it's time to relax, I'll fix these up on Friday.

Nightly check-in. Getting closer to a working demo!

7de9d33

* Remove XPIDL generation and wire up WebIDL. * Generate serializers for enums and dictionaries. * Generate proper C++ WebIDL implementations.

More comments and TODOs.

9a37750

Add more templates for Gecko.

f7ce674

Stubs for remaining bindings; Rondpoint compiles on Desktop now!

1cf040d

...But doesn't link, since I haven't figured out how to get `uniffi` vendored correctly. Progress, though, we're correctly generating WebIDL bindings to where they compile!

Fix sequence serializer; implement record serializer.

d518345

🔥🗑

5d21dfb

Add remaining Desktop pieces.

3c2d813

We can now generate (and compile!) the Rondpoint binding.

It works!

1c4ed47

More dependency shenanigans.

d717469

must_use annotations; CheckedInt for Size.

ba37996

Clean up FFI macros, remove implicit [Throws] for methods.

e9bdbd3

...And pass only the fields from the `ComponentInterface` that each template needs, not the full interface.

Better docs about what we generate.

9d21a53

linabutler force-pushed the gecko branch from 7e8c414 to 9d21a53 Compare August 27, 2020 22:26

linabutler added 2 commits August 28, 2020 20:24

Refactor WebIDL argument and return type generation.

855f440

More docs for the new WebIDL types module.

e309d46

This was referenced Sep 10, 2020

Write an ADR for Desktop platform (C++) consumers #268

Closed

Prototype bindings for Firefox Desktop platform (C++) consumers #269

Closed

linabutler changed the title ~~Generate Firefox bindings~~ Generate Firefox Desktop front-end bindings with WebIDL Sep 10, 2020

linabutler mentioned this pull request Sep 11, 2020

Technical review for generated Desktop bindings (JS and C++) #270

Closed

nnethercote reviewed Sep 15, 2020

View reviewed changes

linabutler mentioned this pull request Sep 17, 2020

Generate Firefox Desktop platform bindings (C++) #285

Closed

linabutler added 2 commits September 22, 2020 01:01

Merge branch 'main' into gecko

ab872d1

Clean up and address remaining review feedback.

ed8c01f

linabutler marked this pull request as ready for review September 26, 2020 07:29

linabutler added 3 commits September 26, 2020 00:32

Restore Cargo.toml.

8ed57bf

This makes it hard to symlink UniFFI into the Firefox tree, but once this PR is merged, we shouldn't need that.

Replace remaining MOZ_MUST_USE with [[nodiscard]].

0912308

Comment cleanups, undefine primitive macro, use signed everywhere.

c9ecca7

linabutler requested review from rfk and nnethercote September 28, 2020 04:59

rfk approved these changes Sep 28, 2020

View reviewed changes

dmose and others added 9 commits September 28, 2020 17:23

Fixes #301 cargo tests by updating and reenabling cargo_metadata (#301)

f6c7113

Merge pull request #302 from dmose/cargo-test-gekco-fail

c04dd99

Fix #301 cargo tests by updating and reenabling cargo_metadata

Finish the fix for #301

cafcb95

Merge pull request #303 from dmose/cargo-test-fix-part-2

b66630c

Finish the fix for #301

Move the Rondpoint tests into gecko_js.

8d141fd

Fix remaining review comments.

d283aa0

Fix remaining RustError declaration.

2ca3547

Pass the context to all remaining filters.

3461092

nnethercote approved these changes Sep 30, 2020

View reviewed changes

linabutler added 2 commits September 30, 2020 00:21

Port remaining Rondpoint tests.

b90b465

Fix serialization of nullable strings.

80def1f

linabutler merged commit d938bfb into main Sep 30, 2020

linabutler deleted the gecko branch September 30, 2020 07:35

data-sync-user mentioned this pull request Sep 30, 2020

Update/merge Desktop/JS binding docs for tech review mozilla/experimenter#3619

Closed

data-sync-user mentioned this pull request Jun 23, 2021

Technical review for generated Desktop bindings (JS and C++) mozilla/application-services#4264

Closed

rfk mentioned this pull request Aug 2, 2021

Add support for Gecko C++ and JS consumers #991

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate Firefox Desktop front-end bindings with WebIDL #255

Generate Firefox Desktop front-end bindings with WebIDL #255

linabutler commented Aug 27, 2020

linabutler Aug 27, 2020

eoger Aug 27, 2020

nnethercote Sep 9, 2020

mhammond Sep 9, 2020 •

edited

Loading

linabutler Sep 9, 2020

nnethercote left a comment

nnethercote Sep 15, 2020

linabutler Sep 25, 2020

rfk left a comment

linabutler commented Sep 28, 2020

rfk commented Sep 28, 2020

rfk commented Sep 28, 2020

linabutler commented Sep 28, 2020

linabutler commented Sep 28, 2020

nnethercote left a comment

linabutler commented Sep 30, 2020

Generate Firefox Desktop front-end bindings with WebIDL #255

Generate Firefox Desktop front-end bindings with WebIDL #255

Conversation

linabutler commented Aug 27, 2020

Getting this building

Hacks

linabutler Aug 27, 2020

Choose a reason for hiding this comment

eoger Aug 27, 2020

Choose a reason for hiding this comment

nnethercote Sep 9, 2020

Choose a reason for hiding this comment

mhammond Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

linabutler Sep 9, 2020

Choose a reason for hiding this comment

nnethercote left a comment

Choose a reason for hiding this comment

nnethercote Sep 15, 2020

Choose a reason for hiding this comment

linabutler Sep 25, 2020

Choose a reason for hiding this comment

rfk left a comment

Choose a reason for hiding this comment

linabutler commented Sep 28, 2020

rfk commented Sep 28, 2020

rfk commented Sep 28, 2020

linabutler commented Sep 28, 2020

linabutler commented Sep 28, 2020

nnethercote left a comment

Choose a reason for hiding this comment

linabutler commented Sep 30, 2020

mhammond Sep 9, 2020 •

edited

Loading