Skip to content

Conversation

@AlmostMatt
Copy link
Contributor

@AlmostMatt AlmostMatt commented Jun 24, 2023

Description

On some platforms there was previously an issue with the code to convert TimeoutMilliseconds from uint32_t to java long, causing it to become a negative value and then getting rejected by Android validation.
Example: firebase/firebase-unity-sdk#764

I was able to reproduce the original issue with the Unity Auth testapp, which calls VerifyPhoneNumber with a PhoneAuthOptions.
The visible error was: "VerifyPhoneNumber: builder faild to create PhoneAuhtOptions"
Looking into the android logcat, the internal error was: "We only support 0-120 seconds for sms-auto-retrieval timeout"

This change should fix this by having an explicit cast from uint32_t to jlong before passing the timeout into the java long constructor. This should make the code work regardless of whether jlong is a 32 bit or 64 bit number.


Testing

I verified the fix by building the unity sdk with this change included and running the Unity auth testapp. VerifyPhoneNumber ran successfully on my device.


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.

@AlmostMatt AlmostMatt changed the title Fix PhoneAuthOptions Timeout Fix PhoneAuthOptions Timeout overflow on some platforms Jun 24, 2023
@AlmostMatt AlmostMatt requested a review from jonsimantov June 24, 2023 00:18
@AlmostMatt AlmostMatt added the tests-requested: quick Trigger a quick set of integration tests. label Jun 24, 2023
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jun 24, 2023
@github-actions
Copy link

github-actions bot commented Jun 24, 2023

❌  Integration test FAILED

Requested by @AlmostMatt on commit cd6c183
Last updated: Mon Jun 26 17:38 PDT 2023
View integration test log & download artifacts

Failures Configs
firestore [BUILD] [ERROR] [Android] [1/3 os: ubuntu]
[TEST] [ERROR] [Android] [1/3 os: ubuntu] [All 2 android_device]

Add flaky tests to go/fpl-cpp-flake-tracker

## Release Notes
### Upcoming Release
- Changes
- Auth (Android): Fixed an issue where VerifyPhoneNumber's internal
Copy link
Contributor

Choose a reason for hiding this comment

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

How would this actually occur to a developer using our SDK? Would it always fail or only if they specified a timeout? Probably best to describe this from the developer's perspective.

Copy link
Contributor Author

@AlmostMatt AlmostMatt Jun 26, 2023

Choose a reason for hiding this comment

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

The issue was reported by a developer in firebase/firebase-unity-sdk#764.
I tried to phrase the readme description similar to the error message that was shown to the user.

I think it would always fail. VerifyPhoneNumber takes a PhoneAuthOptions which always has a timeout milliseconds field. Regardless of what they set (0 or 60 seconds), it would still overflow to some invalid value and the user gets an error.

That said, when I tested our C++ integration tests VerifyPhoneNumber succeeded even without this fix. I think this must be something like a different compiler that uses 32 bits for a long, and therefore no overflow occured. So the issue might be specific to Unity users.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok. So maybe add "with certain compiler settings" to the end of the explanation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@AlmostMatt AlmostMatt merged commit cd6c183 into main Jun 26, 2023
@github-actions github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. and removed tests: succeeded This PR's integration tests succeeded. labels Jun 26, 2023
@firebase-workflow-trigger firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jun 27, 2023
@firebase firebase locked and limited conversation to collaborators Jul 27, 2023
@AlmostMatt AlmostMatt deleted the amatt-auth-options-timeout branch August 1, 2023 19:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

tests: failed This PR's integration tests failed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants