Refactor FirebaseUser to handle C++ objects better #829
Conversation
| /// preset error code: kAuthErrorUnimplemented. | ||
| [System.Obsolete("Please use `Task<AuthResult> ReauthenticateWithProviderAsync(FederatedAuthProvider)` instead", false)] | ||
| public async Task<SignInResult> ReauthenticateWithProviderAsync_DEPRECATED(FederatedAuthProvider provider) { | ||
| FirebaseUserInternal userInternal = GetValidFirebaseUserInternal(); |
There was a problem hiding this comment.
Why trying to get a reference to FirebaseUserInternal here but never use it?
There was a problem hiding this comment.
It is using it, unless I'm misunderstanding what you mean.
auth/src/swig/auth.i
Outdated
| @@ -1193,21 +1118,7 @@ static CppInstanceManager<Auth> g_auth_instances; | |||
|
|
|||
| %typemap(csclassmodifiers) firebase::auth::Auth "public sealed class"; | |||
| %typemap(csclassmodifiers) firebase::auth::User "public sealed class"; | |||
There was a problem hiding this comment.
I believe firebase::auth::User should be internal sealed class now since we do not want to expose FirebaseUserInternal
There was a problem hiding this comment.
Ah, it was getting changed to internal by the post process thing that changes things with "Internal" in the name, but it is definitely confusing, so will change it here as well.
| public FirebaseUser User { | ||
| // Both FirebaseAuth.CurrentUser and AuthResult.User returns null if the | ||
| // user is invalid. | ||
| get { return (UserInternal != null && UserInternal.IsValid()) ? UserInternal : null; } | ||
| // Return the Auth's User, since there can currently only be one user per Auth. | ||
| get { | ||
| return authProxy != null ? authProxy.CurrentUser : null; | ||
| } | ||
| } |
There was a problem hiding this comment.
I am expecting SignInResult.User would have exactly the same issue. I think we should properly fix it until we actually remove this API.
There was a problem hiding this comment.
Agreed. SignInResult already does this logic, of using the authProxy and returning the User based on it instead.
Description
The C++ object for FirebaseUser isn't reliable enough in memory for the C# object to hold onto a reference to it (for example, if the AuthResult is deleted, the User will be as well). Some users were seeing crashes being caused by this.
Fix it by fetching the C++ object when needed, which guarantees that it will be a valid C++ object.
Testing
https://github.com/firebase/firebase-unity-sdk/actions/runs/5863182504
Type of Change
Place an
xthe applicable box: