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.

Sign up for GitHub

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

Jump to bottom

[SkusSDK] - Rust-C++ FFI Improvements #23658

Merged
merged 1 commit into from
May 21, 2024
Merged

[SkusSDK] - Rust-C++ FFI Improvements #23658

merged 1 commit into from
May 21, 2024

Conversation

Brandon-T
Copy link
Contributor

@Brandon-T Brandon-T commented May 14, 2024 •
edited

Summary

  • Removed all Type signatures for callbacks.
  • Removed all CallbackState holders.
  • Propagate Rust Errors down the call stack so that users of SkusSDK can properly handle them.
  • Define a single class that is bound to a sequence and pass that to Rust. Rust is able to call the C++ member function, without having to do trampolines and C-style signatures and extern "C", etc. We're able to call C++ member functions directly.
  • Similar code will also work. IE: some_rust_func(base::BindOnce(&OnceCallback<void(T)>::Run, once_callback, args, ...) so long as once_callback is heap allocated.

Resolves brave/brave-browser#38327
This PR is a prerequisite for: #22944 which will be rebased onto this PR.

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@Brandon-T Brandon-T added this to the 1.68.x - Nightly milestone May 14, 2024
@Brandon-T Brandon-T requested a review from bridiver May 14, 2024 22:27
@Brandon-T Brandon-T self-assigned this May 14, 2024
@Brandon-T Brandon-T requested a review from a team as a code owner May 14, 2024 22:27
@github-actions GitHub Actions
Copy link
Contributor

[puLL-Merge] - brave/brave-core@23658

Description

This PR makes changes to the Rust-C++ bindings for the Skus SDK to improve error handling and simplify the callback mechanism between Rust and C++.

The main motivation appears to be refactoring the error handling to provide more detailed error messages from Rust to C++, and to streamline the various callback types into a single RustBoundPostTask type that can be used for all async Rust functions.

Changes

Changes

components/skus/browser/rs/cxx/BUILD.gn:

  • Adds a dependency on the skus/common:mojom target

components/skus/browser/rs/cxx/src/errors.rs:

  • Refactors the From trait implementations for SkusResult to include an error message string in addition to the result code
  • Adds a From implementation to convert from InternalError to SkusResultCode
  • Removes the result_to_string function

components/skus/browser/rs/cxx/src/httpclient.rs:

  • Updates HttpResponse creation to use the new SkusResult struct

components/skus/browser/rs/cxx/src/lib.rs:

  • Replaces the individual callback types with a single RustBoundPostTask type
  • Updates all the async Rust functions to use RustBoundPostTask instead of specific callback types
  • Removes the callback function pointer types

components/skus/browser/rs/cxx/src/shim.h:

  • Adds the SkusResult struct and SkusResultCode enum based on the Mojom definition
  • Defines the RustBoundPostTask class to replace the individual callback state classes
  • Removes the callback function pointer typedefs

components/skus/browser/rs/lib/src/errors.rs:

  • Improves the Display formatting for InternalError variants to include more error details

components/skus/browser/* C++ files:

  • Updates to use SkusResult struct instead of enum
  • Switches to using RustBoundPostTask instead of individual callback state objects
  • Removes the Rust callback shim functions

Summary

Overall, this is a sizable refactoring of the Skus SDK bindings to centralize error handling and simplify the callback mechanism between Rust and C++. The changes look reasonable and well-structured.

The main points to verify would be:

  1. The new SkusResult struct is populated correctly from Rust and marshalled properly to C++
  2. The switch to RustBoundPostTask doesn't break any existing functionality
  3. Rust errors are propagated and displayed correctly in C++ with the additional error message details

Assuming thorough testing of the SDK functionality, this PR should improve maintainability of the Skus bindings without introducing new issues. Nice work!

bridiver
bridiver reviewed May 14, 2024
View reviewed changes
components/skus/browser/rs/cxx/src/lib.rs Outdated
@@ -414,16 +393,14 @@ impl CppSDK {

fn submit_receipt(
self: &CppSDK,
callback: SubmitReceiptCallback,
callback_state: UniquePtr<ffi::SubmitReceiptCallbackState>,
callback_state: UniquePtr<ffi::RustBoundPostTask>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we keep the param name callback instead of callback_state?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

bridiver
bridiver reviewed May 14, 2024
View reviewed changes
components/skus/browser/skus_context_impl.cc Outdated
@@ -9,6 +9,7 @@
#include <utility>

#include "base/logging.h"
#include "base/task/bind_post_task.h"
#include "base/task/sequenced_task_runner.h"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this header still used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

@Brandon-T Brandon-T requested a review from bridiver May 15, 2024 02:54
@Brandon-T
Create a OnceCallback version for Rust Skus that's bound to a sequenc…
626b1e2 
…e task running.

Refactor Rust Skus to use the RustSequencedCallback directly.
Refactor SkusSDK to use the RustSequencedCallback.
Refactor Rust Skus to call C++ member functions and remove use of UniquePtr as we no longer have dangling pointers or UB.
@Brandon-T Brandon-T merged commit 6604d22 into master May 21, 2024
19 checks passed
@Brandon-T Brandon-T deleted the bugfix/skus-sdk-callback branch May 21, 2024 17:27
Brandon-T added a commit that referenced this pull request May 21, 2024
@Brandon-T
[SkusSDK] - Rust-C++ FFI Improvements (#23658)
aca2fb5 
- Create a OnceCallback version for Rust Skus that's bound to a sequence task running.
- Refactor Rust Skus to use the RustBoundTask directly.
- Refactor SkusSDK to use the RustBoundTask.

Signed-off-by: Brandon T <JustBrandonT@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bridiver bridiver bridiver approved these changes

Assignees

@Brandon-T Brandon-T

Labels
puLL-Merge unverified-commits
Projects
None yet
Milestone
1.68.x - Nightly
2 participants
@Brandon-T @bridiver