Completely rewrite CredentialsApiHelper.java and hopefully fix memory leak #566

SUPERCILEX · 2017-02-01T02:29:13Z

@samtstern This should fix the memory leak because we aren't storing the activity anywhere.

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2017-02-01T02:31:42Z

auth/src/main/java/com/firebase/ui/auth/util/GoogleApiHelper.java

+
+    public static int getSafeAutoManageId() {
+        IDS.add(null);
+        return IDS.size() - 1;


This is sooooo much better than my previous solution. 😄

SUPERCILEX · 2017-02-01T02:32:08Z

auth/src/main/java/com/firebase/ui/auth/util/OldCredentialsApiHelper.java

+ * API.
+ */
+@Deprecated
+public class OldCredentialsApiHelper {


You don't need to look here, I just copied this from the old CredentialsApiHelper.

Let's pick a better name for this, maybe ActivityCredentialsApiHelper or something. "Old" as a prefix does not help to show what was wrong with it.

Also I'd argue that we should keep this class named CredentialsApiHelper and the new class is the one that should have a new name since it reduces churn.

SUPERCILEX · 2017-02-01T02:34:18Z

auth/src/main/java/com/firebase/ui/auth/AuthUI.java

     */
+    @Deprecated
    public Task<Void> signOut(@NonNull Activity activity) {


This makes me so happy: because our new signOut method uses FragmentActivity which extends Activity, people will be automatically upgraded. How's that for customer service! 😄

samtstern · 2017-02-01T18:18:39Z

auth/src/main/java/com/firebase/ui/auth/util/GoogleApiHelper.java

+import java.util.List;
+
+public class GoogleApiHelper {
+    private static final List<Void> IDS = new ArrayList<>();


I think this would do better as an AtomicInteger and then your static function would just be wrapping incrementAndGet()

@samtstern Never knew that existed, sounds awesome!

samtstern · 2017-02-01T18:23:34Z

auth/src/main/java/com/firebase/ui/auth/AuthUI.java

     */
+    @Deprecated
    public Task<Void> delete(@NonNull Activity activity) {


As far as I can tell the difference between delete(FragmentActivity) and delete(Activity) is just that the former uses CredentialApiHelper and the latter uses OldCredentialApiHelper.

Could this all be simplified by having both of the CredentialApiHelper classes implement a common interface? I think it would look like this:

interface CredentialApiHelper { Task<Status> delete(Credential credential); Task<Status> disableAuthSignIn(); }

And then these two delete() functions would just be a matter of constructing the proper Helper and then calling to a private Task<Void> delete(CredentialApiHelper helper)

@samtstern To address this comment and this one, I didn't add an interface or a good name because my intention for us to throw this away in FBUI v2.0. Do you disagree with that? If so, then we should definitely clean things up and maybe undeprecate thoses classes. What do you think?

2.0 could be a while away, let's try and keep things clean in the meantime.

@samtstern SGTM, I'll leave things deprecated so we know to remove that stuff if we go that route, but I keep things clean in the meantime.

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2017-02-01T19:17:10Z

@samtstern I'm happy with it now, PTAL.

samtstern · 2017-02-01T19:31:07Z

auth/src/main/java/com/firebase/ui/auth/AuthUI.java

+        CredentialApiHelper credentialHelper = CredentialsApiHelper.getInstance(gacHelper);
+
+        // Get all SmartLock credentials associated with the user
+        List<Credential> credentials = SmartLockBase.credentialsFromFirebaseUser(firebaseUser);


From here down to the end of the function seems duplicated between the two delete() methods. Can we abstract into a common private method?

Yup, forgot to refactor that.

Signed-off-by: Alex Saveau <[email protected]>

samtstern · 2017-02-01T20:28:58Z

auth/src/main/java/com/firebase/ui/auth/util/CredentialsApiHelper.java

@@ -31,7 +31,8 @@
 * A {@link com.google.android.gms.tasks.Task Task} based wrapper for the Smart Lock for Passwords
 * API.
 */
-public class CredentialsApiHelper {
+@Deprecated
+public class CredentialsApiHelper implements CredentialApiHelper {


One last (annoying) comment: it's pretty confusing to have these two names only differ by an 's' in the middle. Let's change the name of the interface? Maybe CredentialTaskApi or something like that.

@samtstern SGTM, I just can't wait for us to throw this stuff away anyway.

Signed-off-by: Alex Saveau <[email protected]>

samtstern · 2017-02-01T23:51:53Z

auth/src/main/java/com/firebase/ui/auth/util/GoogleSignInHelper.java

+
+    public Task<Status> signOut() {
+        final TaskCompletionSource<Status> statusTask = new TaskCompletionSource<>();
+        getConnectedApiTask().addOnSuccessListener(new OnSuccessListener<Bundle>() {


Missed this, but shouldn't we also add an onFailureListener to getConnectedApiTask() that propagates the failure to statusTask through setException?

samtstern · 2017-02-01T23:54:01Z

auth/src/main/java/com/firebase/ui/auth/util/signincontainer/SignInDelegate.java

-        credentialsApiHelper.delete(mCredential)
-                .addOnCompleteListener(new OnCompleteListener<Status>() {
+        AuthUI.getInstance()
+                .delete(getActivity())


This now also deletes the underlying Firebase Auth account in addition to deleting the credentials. This has two new properties:

Will fail if there is no currently signed in user

Will fail if we don't have recent authentication (last 5 minutes)

Did you intend to change this behavior?

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2017-02-02T00:11:34Z

@samtstern Sorry about those mistakes, please let me take another look at this PR to make sure I didn't mess up again. And maybe you could take another look too? (I've been distracted by my other PRs 😄)

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2017-02-02T00:28:50Z

@samtstern Ok, I think it's all good now.

samtstern · 2017-02-02T00:47:07Z

auth/src/main/java/com/firebase/ui/auth/util/GoogleSignInHelper.java

@@ -26,7 +25,7 @@ public static GoogleSignInHelper getInstance(FragmentActivity activity) {

    public Task<Status> signOut() {
        final TaskCompletionSource<Status> statusTask = new TaskCompletionSource<>();
-        getConnectedApiTask().addOnSuccessListener(new OnSuccessListener<Bundle>() {
+        getConnectedApiTask().addOnSuccessListener(new ExceptionForwarder<Bundle>(statusTask) {


I don't think this will work, it's still setting the ExceptionForwarder using addOnSuccessListener so even though it implements onFailure that will never be called.

You may want to make this an onCompleteListener instance instead so you'll get notified of success or failure.

@samtstern Today's not my day with tasks! 😵 PTAL at 15fbed3, I'm pretty sure that will work.

samtstern · 2017-02-02T00:48:22Z

auth/src/main/java/com/firebase/ui/auth/util/GoogleSignInHelper.java

+
+    public Task<Status> signOut() {
+        final TaskCompletionSource<Status> statusTask = new TaskCompletionSource<>();
+        getConnectedApiTask().addOnSuccessListener(new ExceptionForwarder<Bundle>(statusTask) {


I don't think this will work, it's still setting the ExceptionForwarder using addOnSuccessListener so even though it implements onFailure that will never be called.

You may want to make this an onCompleteListener instance instead so you'll get notified of success or failure.

Signed-off-by: Alex Saveau <[email protected]>

samtstern · 2017-02-02T01:23:28Z

Woohoo looks good! I will merge once tests pass.

SUPERCILEX · 2017-02-02T01:35:49Z

@samtstern OMG finally! Thanks for sticking with me on this one! 😄

Completely rewrite CredentialsApiHelper.java

ba0091d

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX commented Feb 1, 2017

View reviewed changes

samtstern reviewed Feb 1, 2017

View reviewed changes

SUPERCILEX added 3 commits February 1, 2017 10:31

Use AtomicInteger to get safe ID

43bcaed

Signed-off-by: Alex Saveau <[email protected]>

Attempt to simplify class organization

ec84f7d

Signed-off-by: Alex Saveau <[email protected]>

Whoops

3ba8b69

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX changed the title ~~Completely rewrite CredentialsApiHelper.java and hopefully fix memory leak~~ [DON'T MERGE] Completely rewrite CredentialsApiHelper.java and hopefully fix memory leak Feb 1, 2017

Simplify stuff even further

499e719

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX changed the title ~~[DON'T MERGE] Completely rewrite CredentialsApiHelper.java and hopefully fix memory leak~~ Completely rewrite CredentialsApiHelper.java and hopefully fix memory leak Feb 1, 2017

samtstern reviewed Feb 1, 2017

View reviewed changes

Refactor delete method

a57bf05

Signed-off-by: Alex Saveau <[email protected]>

samtstern reviewed Feb 1, 2017

View reviewed changes

Rename CredentialTaskApi.java

26a3aea

Signed-off-by: Alex Saveau <[email protected]>

samtstern reviewed Feb 1, 2017

View reviewed changes

SUPERCILEX added 2 commits February 1, 2017 16:04

Forward exceptions

0e3400d

Signed-off-by: Alex Saveau <[email protected]>

Fix refactor mistake

782800c

Signed-off-by: Alex Saveau <[email protected]>

Fix modifier order

530e753

Signed-off-by: Alex Saveau <[email protected]>

samtstern reviewed Feb 2, 2017

View reviewed changes

Oh boy, this should work

15fbed3

Signed-off-by: Alex Saveau <[email protected]>

samtstern merged commit 85d7b70 into firebase:version-1.2.0-dev Feb 2, 2017

samtstern added this to the 1.2.0 milestone Feb 2, 2017

SUPERCILEX deleted the asav/#561 branch February 2, 2017 04:19

Completely rewrite CredentialsApiHelper.java and hopefully fix memory leak #566

Completely rewrite CredentialsApiHelper.java and hopefully fix memory leak #566

Uh oh!

Conversation

SUPERCILEX commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX commented Feb 1, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX commented Feb 2, 2017

Uh oh!

SUPERCILEX commented Feb 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samtstern commented Feb 2, 2017

Uh oh!

SUPERCILEX commented Feb 2, 2017

Uh oh!

Uh oh!

SUPERCILEX commented Feb 1, 2017 •

edited

Loading