Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SkusSDK] - Improve Rust-C++ FFI #38327

Closed
Brandon-T opened this issue May 14, 2024 · 0 comments · Fixed by brave/brave-core#23658
Closed

[SkusSDK] - Improve Rust-C++ FFI #38327

Brandon-T opened this issue May 14, 2024 · 0 comments · Fixed by brave/brave-core#23658
Assignees
Labels
OS/Android Fixes related to Android browser functionality OS/Desktop OS/iOS Fixes related to iOS browser functionality QA/Yes release-notes/exclude

Comments

@Brandon-T
Copy link

Brandon-T commented May 14, 2024

Description:

  • Current Rust and C++ communication is rough. It requires generating callbacks on the C++ side and passing them to Rust and hoping that the caller passes the correct arguments. Otherwise it will crash.
  • Current Rust and C++ communication requires creating a callback-state holder that holds the callback, and a bunch of callback signatures:
type RefreshOrderCallbackState;
type RefreshOrderCallback = crate::RefreshOrderCallback;
type FetchOrderCredentialsCallbackState;
type FetchOrderCredentialsCallback = crate::FetchOrderCredentialsCallback;
type PrepareCredentialsPresentationCallbackState;
type PrepareCredentialsPresentationCallback = crate::PrepareCredentialsPresentationCallback;
type CredentialSummaryCallbackState;
type CredentialSummaryCallback = crate::CredentialSummaryCallback;
type SubmitReceiptCallbackState;
type SubmitReceiptCallback = crate::SubmitReceiptCallback;
type CreateOrderFromReceiptCallbackState;
type CreateOrderFromReceiptCallback = crate::CreateOrderFromReceiptCallback;

where create::XXXX is also defined in C++ via a shim.h file.

  • Current Rust and C++ communication also requires Unsafe code and "extern C" style functions where the first argument is a pointer to an instance allocated on the heap, and the second argument is the function to be called, and it's called via a trampoline:
#[allow(improper_ctypes_definitions)]
#[repr(transparent)]
pub struct RefreshOrderCallback(
    pub  extern "C" fn(
        callback_state: *mut ffi::RefreshOrderCallbackState,
        result: ffi::SkusResult,
        order: &str,
    ),
);

unsafe impl ExternType for RefreshOrderCallback {
    type Id = type_id!("skus::RefreshOrderCallback");
    type Kind = cxx::kind::Trivial;
}

This is done for every single C++ function that Skus Rust code needs to call. It's tedious, and error prone, and can easily lead to Undefined Behaviour if proper care is not taken to ensure the right signatures and arguments are passed on the heap.

@Brandon-T Brandon-T added OS/Android Fixes related to Android browser functionality OS/Desktop OS/iOS Fixes related to iOS browser functionality labels May 14, 2024
@Brandon-T Brandon-T added this to the 1.68.x - Nightly milestone May 14, 2024
@Brandon-T Brandon-T self-assigned this May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OS/Android Fixes related to Android browser functionality OS/Desktop OS/iOS Fixes related to iOS browser functionality QA/Yes release-notes/exclude
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant