Skip to content

Updated Analytics Windows to allow multiple DLL hashes, and add usage of Options struct. #1744

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

Merged
merged 22 commits into from
Jun 25, 2025

Conversation

jonsimantov
Copy link
Contributor

@jonsimantov jonsimantov commented Jun 25, 2025

Description

Provide details of the change, and generalize the change in the PR title above.

This PR updates the generate_windows_stubs.py script to keep track of known DLL hashes in known_hashes.txt and allow any of those when loading the Analytics DLL. This allows us to test multiple versions (e.g. compiled in debug mode vs. release mode) of the DLL.

This PR also adds support for the GoogleAnalytics_Options struct, which is required to initialize Analytics for Windows. This struct is populated from the Firebase App's AppOptions, except for the flag to enable data collection on first run, which defaults to true and can be disabled with a call to firebase::analytics::SetAnalyticsCollectionEnabled(false) prior to Initialize() (on iOS/Android this is set via the app's Info.plist and AndroidManifest.xml, neither of which exist on Windows).


Testing

Describe how you've tested these changes. Link any manually triggered Integration tests or CPP binary SDK Packaging Github Action workflows, if applicable.

Tested manually on Windows. Stub mode tested via integration test run.


Type of Change

Place an x the applicable box:

  • Bug fix. Add the issue # below if applicable.
  • New feature. A non-breaking change which adds functionality.
  • Other, such as a build process or documentation change.

Notes

  • Bug fixes and feature changes require an update to the Release Notes section of release_build_files/readme.md.
  • Read the contribution guidelines CONTRIBUTING.md.
  • Changes to the public API require an internal API review. If you'd like to help us make Firebase APIs better, please propose your change in a feature request so that we can discuss it together.

jonsimantov and others added 15 commits June 16, 2025 10:57
Add Crypt32.lib to the linked libraries for the firebase_analytics
target on Windows. This resolves a linker error LNK2019 for
_CryptBinaryToStringA@20, which was occurring in builds that
included analytics_windows.obj (e.g., firebase_analytics_test).

While CryptBinaryToStringA is not directly called in
analytics_windows.cc, the linker indicates that analytics_windows.obj
requires this symbol, possibly due to an indirect dependency or
other build system interactions. Linking Crypt32.lib provides the
necessary symbol.
I've added Crypt32.lib to the dependencies of the firebase_analytics_test
target in `analytics/tests/CMakeLists.txt` when building on Windows.

This is to resolve a persistent LNK2019 error for
_CryptBinaryToStringA@20, which occurs during the linking phase of
firebase_analytics_test.obj. This change directly links the
required system library at the test executable level.
This change updates the desktop analytics initialization to use the
newly required Options struct for the Windows C API.

- In `analytics/src/analytics_desktop.cc`:
  - `firebase::analytics::Initialize(const App& app)` now retrieves
    `app_id` and `package_name` from `app.options()`.
  - It calls `GoogleAnalytics_Options_Create()` to create the options struct.
  - Populates `app_id`, `package_name`, and sets a default for
    `analytics_collection_enabled_at_first_launch`.
  - Calls `GoogleAnalytics_Initialize()` with the populated options struct.
  - String lifetimes for `app_id` and `package_name` are handled by
    creating local `std::string` copies before passing their `c_str()`
    to the C API.
* Initialize Analytics C SDK with AppOptions on desktop

This change updates the desktop analytics initialization to use the
newly required Options struct for the Windows C API.

- In `analytics/src/analytics_desktop.cc`:
  - `firebase::analytics::Initialize(const App& app)` now retrieves
    `app_id` and `package_name` from `app.options()`.
  - It calls `GoogleAnalytics_Options_Create()` to create the options struct.
  - Populates `app_id`, `package_name`, and sets a default for
    `analytics_collection_enabled_at_first_launch`.
  - Calls `GoogleAnalytics_Initialize()` with the populated options struct.
  - String lifetimes for `app_id` and `package_name` are handled by
    creating local `std::string` copies before passing their `c_str()`
    to the C API.

* Format code.

* Fix build issues.

---------

Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
@jonsimantov jonsimantov changed the base branch from main to analytics-dll-secure June 25, 2025 19:48
@jonsimantov jonsimantov changed the title Updated Analytics stub DLL with options added Updated Analytics Windows to allow multiple DLL hashes, and add usage of Options struct. Jun 25, 2025
@jonsimantov jonsimantov requested a review from a-maurice June 25, 2025 19:59
@jonsimantov jonsimantov marked this pull request as ready for review June 25, 2025 19:59
@jonsimantov jonsimantov added the skip-release-notes Skip release notes check label Jun 25, 2025
f.write("// clang-format off\n")
f.write(f'\n// Number of Google Analytics functions expected to be loaded from the DLL.')
f.write(f'\nconst int FirebaseAnalytics_DynamicFunctionCount = {len(function_details_for_loader)};\n\n');
# f.write("#if defined(_WIN32)\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove

@@ -104,6 +104,9 @@ set_property(TARGET firebase_analytics PROPERTY FOLDER "Firebase Cpp")
# Set up the dependency on Firebase App.
target_link_libraries(firebase_analytics
PUBLIC firebase_app)
if(WIN32)
target_link_libraries(firebase_analytics PUBLIC Crypt32.lib)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be PUBLIC? Looking over other cases, we usually try to make these private, though there might be a reason this one needs to be public

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's try it and see what happens!

@jonsimantov jonsimantov merged commit 57f303d into analytics-dll-secure Jun 25, 2025
15 checks passed
@github-actions github-actions bot added the tests: in-progress This PR's integration tests are in progress. label Jun 25, 2025
Copy link

github-actions bot commented Jun 25, 2025

✅  Integration test succeeded!

Requested by @jonsimantov on commit 57f303d
Last updated: Wed Jun 25 16:39 PDT 2025
View integration test log & download artifacts

@github-actions github-actions bot added the tests: succeeded This PR's integration tests succeeded. label Jun 25, 2025
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-release-notes Skip release notes check tests: succeeded This PR's integration tests succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants