StoreKit2 support for Apple devices#1846
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for logging Apple StoreKit 2 transactions on iOS by adding a new LogAppleTransaction API, including a Swift bridge for native interaction and updated CMake configurations to support Swift compilation. Several issues were identified in the CMake configuration, including a regression for Windows builds, incorrect platform detection logic for tvOS, non-portable header inclusion, and reliance on Xcode-specific variables that break other generators like Ninja.
Integration test with FLAKINESS (succeeded after retry)Requested by @AustinBenoit on commit c5edd5e
Add flaky tests to go/fpl-cpp-flake-tracker |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the LogAppleTransaction API to Firebase Analytics, enabling the logging of StoreKit 2 transactions on iOS and tvOS. The implementation includes a Swift-based verification layer using AppleTransactionWorker and bridges it to the C++ SDK. Additionally, the PR incorporates critical workarounds for Xcode 15+ compatibility on older iOS versions (15 and 16), specifically addressing potential crashes related to Swift Concurrency and __libcpp_verbose_abort. Build system updates across multiple modules ensure proper Swift/C++ interop and framework linking. Review feedback suggests addressing a potential memory leak in the iOS asynchronous completion handling and improving the portability of the CMake configuration by removing hardcoded framework paths.
| return Future<void>(api, future_handle.get()); | ||
| } | ||
|
|
||
| SafeFutureHandle<void>* handle_ptr = new SafeFutureHandle<void>(future_handle); |
There was a problem hiding this comment.
The SafeFutureHandle<void> is allocated on the heap using new, but there is a risk of a memory leak if the verifyWithTransactionId completion block is never executed. While the current Swift implementation appears to always call the completion handler, it is safer to use a mechanism that ensures cleanup even if the asynchronous task fails to complete or if the app state changes unexpectedly.
| set(analytics_framework_dir "${pods_dir}/FirebaseAnalytics/Frameworks/FirebaseAnalytics.xcframework/${analytics_slice}") | ||
| set(measurement_framework_dir "${pods_dir}/GoogleAppMeasurement/Frameworks/GoogleAppMeasurement.xcframework/${analytics_slice}") |
There was a problem hiding this comment.
The paths for FirebaseAnalytics.xcframework and GoogleAppMeasurement.xcframework are hardcoded based on a specific CocoaPods directory structure. This may cause build failures if the environment uses a different version of the Firebase Apple SDK or a different dependency management setup. Consider using CMake variables or a more flexible discovery mechanism to locate these frameworks.
… ios 15+ which should bundle the concurrency headers
| ${analytics_generated_headers_dir}/event_names.h | ||
| ${analytics_generated_headers_dir}/parameter_names.h | ||
| ${analytics_generated_headers_dir}/user_property_names.h) | ||
| if (FIREBASE_XCODE_TARGET_FORMAT STREQUAL "frameworks") |
There was a problem hiding this comment.
Does this need the check for FIREBASE_XCODE_TARGET_FORMAT? I feel like it would need the analytics headers for all platforms.
Description
StoreKit2 support for Apple devices. Make the LogAppleTransaction a no-op for Android and iOS
Testing
Added in a new integration test and steps to manually test the transactions.
Performed the manual testing as describe:
Type of Change
Place an
xthe applicable box:Notes
Release Notessection ofrelease_build_files/readme.md.