Skip to content

Fb mem leak + remove BuildConfig.DEBUG #442

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.

Sign up for GitHub

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 9 commits into from
Dec 7, 2016
Merged

Fb mem leak + remove BuildConfig.DEBUG #442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f4a1790
Update gradle, update dependencies, fix Facebook theme, fix Facebook CCT
SUPERCILEX Dec 6, 2016
d901d4c
Remove unnecessary setApplicationId
SUPERCILEX Dec 6, 2016
93e78ef
Remove unnecessary theme
SUPERCILEX Dec 6, 2016
fe4739c
Fix activity leak
SUPERCILEX Dec 6, 2016
f84f06b
Cleanup
SUPERCILEX Dec 6, 2016
38da2d2
Cleanup
SUPERCILEX Dec 6, 2016
f0707fa
Merge remote-tracking branch 'firebase/master' into fb-mem-leak
SUPERCILEX Dec 6, 2016
203a831
Merge remote-tracking branch 'firebase/version-1.1.0-dev' into fb-mem…
SUPERCILEX Dec 6, 2016
c04dc2f
Cleanup
SUPERCILEX Dec 7, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 52 additions & 35 deletions auth/src/main/java/com/firebase/ui/auth/provider/FacebookProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import com.facebook.login.LoginManager;
import com.facebook.login.LoginResult;
import com.firebase.ui.auth.AuthUI;
import com.firebase.ui.auth.BuildConfig;
import com.firebase.ui.auth.IdpResponse;
import com.firebase.ui.auth.R;
import com.google.firebase.auth.AuthCredential;
Expand All @@ -45,13 +44,15 @@

public class FacebookProvider implements IdpProvider, FacebookCallback<LoginResult> {
private static final String TAG = "FacebookProvider";
protected static final String ERROR = "err";
protected static final String ERROR_MSG = "err_msg";
private static final String EMAIL = "email";
private static final String PUBLIC_PROFILE = "public_profile";
private static final CallbackManager CALLBACK_MANAGER = CallbackManager.Factory.create();
private static final String ERROR = "err";
private static final String ERROR_MSG = "err_msg";

private static CallbackManager sCallbackManager;

private final List<String> mScopes;
// DO NOT USE DIRECTLY: see onSuccess(String, LoginResult) and onFailure(Bundle) below
private IdpCallback mCallbackObject;

public FacebookProvider(Context appContext, AuthUI.IdpConfig idpConfig, @StyleRes int theme) {
Expand Down Expand Up @@ -86,8 +87,9 @@ public String getProviderId() {

@Override
public void startLogin(Activity activity) {
sCallbackManager = CallbackManager.Factory.create();
LoginManager loginManager = LoginManager.getInstance();
loginManager.registerCallback(CALLBACK_MANAGER, this);
loginManager.registerCallback(sCallbackManager, this);

List<String> permissionsList = new ArrayList<>(mScopes);

Expand All @@ -111,18 +113,11 @@ public void setAuthenticationCallback(IdpCallback callback) {

@Override
public void onActivityResult(int requestCode, int resultCode, Intent data) {
CALLBACK_MANAGER.onActivityResult(requestCode, resultCode, data);
sCallbackManager.onActivityResult(requestCode, resultCode, data);
}

@Override
public void onSuccess(final LoginResult loginResult) {
if (BuildConfig.DEBUG) {
Log.d(TAG, "Login to facebook successful with Application Id: "
+ loginResult.getAccessToken().getApplicationId()
+ " with Token: "
+ loginResult.getAccessToken().getToken());
}

GraphRequest request = GraphRequest.newMeRequest(
loginResult.getAccessToken(),
new GraphRequest.GraphJSONObjectCallback() {
Expand All @@ -131,19 +126,19 @@ public void onCompleted(JSONObject object, GraphResponse response) {
FacebookRequestError requestError = response.getError();
if (requestError != null) {
Log.e(TAG, "Received Facebook error: " + requestError.getErrorMessage());
mCallbackObject.onFailure(new Bundle());
onFailure(new Bundle());
return;
}
if (object == null) {
Log.w(TAG, "Received null response from Facebook GraphRequest");
mCallbackObject.onFailure(new Bundle());
onFailure(new Bundle());
} else {
try {
String email = object.getString("email");
mCallbackObject.onSuccess(createIDPResponse(loginResult, email));
onSuccess(email, loginResult);
} catch (JSONException e) {
Log.e(TAG, "JSON Exception reading from Facebook GraphRequest", e);
mCallbackObject.onFailure(new Bundle());
onFailure(new Bundle());
}
}
}
Expand All @@ -155,27 +150,11 @@ public void onCompleted(JSONObject object, GraphResponse response) {
request.executeAsync();
}

private IdpResponse createIDPResponse(LoginResult loginResult, String email) {
return new IdpResponse(
FacebookAuthProvider.PROVIDER_ID,
email,
loginResult.getAccessToken().getToken());
}

public static AuthCredential createAuthCredential(IdpResponse response) {
if (!response.getProviderType().equals(FacebookAuthProvider.PROVIDER_ID)) {
return null;
}
return FacebookAuthProvider
.getCredential(response.getIdpToken());
}

@Override
public void onCancel() {
Bundle extra = new Bundle();
extra.putString(ERROR, "cancelled");
mCallbackObject.onFailure(extra);

onFailure(extra);
}

@Override
Expand All @@ -184,6 +163,44 @@ public void onError(FacebookException error) {
Bundle extra = new Bundle();
extra.putString(ERROR, "error");
extra.putString(ERROR_MSG, error.getMessage());
mCallbackObject.onFailure(extra);
onFailure(extra);
}

private IdpResponse createIdpResponse(String email, LoginResult loginResult) {
return new IdpResponse(
FacebookAuthProvider.PROVIDER_ID,
email,
loginResult.getAccessToken().getToken());
}

private void onSuccess(String email, LoginResult loginResult) {
gcCallbackManager();
mCallbackObject.onSuccess(createIdpResponse(email, loginResult));
}

private void onFailure(Bundle bundle) {
gcCallbackManager();
mCallbackObject.onFailure(bundle);
}

private void gcCallbackManager() {
// sCallbackManager must be static to prevent it from being destroyed if the activity
// containing FacebookProvider dies.
// In startLogin(Activity), LoginManager#registerCallback(CallbackManager, FacebookCallback)
// stores the FacebookCallback parameter--in this case a FacebookProvider instance--into
// a HashMap in the CallbackManager instance, sCallbackManager.
// Because FacebookProvider which contains an instance of an activity, mCallbackObject,
// is contained in sCallbackManager, that activity will not be garbage collected.
// Thus, we have leaked an Activity.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we want to keep this explanation? It's a bit lengthy and could probably be replaced with // Fixes activity leaks.

Here is the Facebook code in question:

CallbackManager.Factory.create()

public static CallbackManager create() {
    return new CallbackManagerImpl();
}
loginManager.registerCallback(sCallbackManager, ***this***);

->

/**
 * Registers a login callback to the given callback manager.
 * @param callbackManager The callback manager that will encapsulate the callback.
 * @param callback The login callback that will be called on login completion.
 */
public void registerCallback(
final CallbackManager callbackManager,
final FacebookCallback<LoginResult> callback) {
    if (!(callbackManager instanceof CallbackManagerImpl)) {
        throw new FacebookException("Unexpected CallbackManager, " +
                                            "please use the provided Factory.");
    }
    ((CallbackManagerImpl) callbackManager).registerCallback( // sCallbackManager is storing callback into itself
            CallbackManagerImpl.RequestCodeOffset.Login.toRequestCode(),
            new CallbackManagerImpl.Callback() {
                @Override
                public boolean onActivityResult(int resultCode, Intent data) {
                    return LoginManager.this.onActivityResult(
                            resultCode,
                            data,
                            ***callback***);
                }
            }
    );
}

->

private Map<Integer, CallbackManagerImpl.Callback> callbacks = new HashMap<>();

public void registerCallback(int requestCode, Callback callback) {
    Validate.notNull(callback, "callback");
    callbacks.put(requestCode, ***callback***); // Callback is now in callbacks HashMap
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the super-detailed investigation. I am fine with the long comment, it makes people more scared to undo your fix.

sCallbackManager = null;
}


public static AuthCredential createAuthCredential(IdpResponse response) {
if (!response.getProviderType().equals(FacebookAuthProvider.PROVIDER_ID)) {
return null;
}
return FacebookAuthProvider
.getCredential(response.getIdpToken());
}
}
4 changes: 2 additions & 2 deletions auth/src/main/java/com/firebase/ui/auth/provider/GoogleProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public void disconnect() {
}
}

private IdpResponse createIDPResponse(GoogleSignInAccount account) {
private IdpResponse createIdpResponse(GoogleSignInAccount account) {
return new IdpResponse(
GoogleAuthProvider.PROVIDER_ID, account.getEmail(), account.getIdToken());
}
Expand All @@ -122,7 +122,7 @@ public void onActivityResult(int requestCode, int resultCode, Intent data) {
GoogleSignInResult result = Auth.GoogleSignInApi.getSignInResultFromIntent(data);
if (result != null) {
if (result.isSuccess()) {
mIDPCallback.onSuccess(createIDPResponse(result.getSignInAccount()));
mIDPCallback.onSuccess(createIdpResponse(result.getSignInAccount()));
} else {
onError(result);
}
Expand Down
20 changes: 10 additions & 10 deletions auth/src/main/java/com/firebase/ui/auth/provider/TwitterProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public void startLogin(Activity activity) {

@Override
public void success(Result<TwitterSession> result) {
mCallbackObject.onSuccess(createIDPResponse(result.data));
mCallbackObject.onSuccess(createIdpResponse(result.data));
}

@Override
Expand All @@ -70,6 +70,15 @@ public void failure(TwitterException exception) {
mCallbackObject.onFailure(new Bundle());
}

private IdpResponse createIdpResponse(TwitterSession twitterSession) {
return new IdpResponse(
TwitterAuthProvider.PROVIDER_ID,
null,
twitterSession.getAuthToken().token,
twitterSession.getAuthToken().secret);
}


public static AuthCredential createAuthCredential(IdpResponse response) {
if (!response.getProviderType().equalsIgnoreCase(TwitterAuthProvider.PROVIDER_ID)){
return null;
Expand All @@ -78,13 +87,4 @@ public static AuthCredential createAuthCredential(IdpResponse response) {
response.getIdpToken(),
response.getIdpSecret());
}


private IdpResponse createIDPResponse(TwitterSession twitterSession) {
return new IdpResponse(
TwitterAuthProvider.PROVIDER_ID,
null,
twitterSession.getAuthToken().token,
twitterSession.getAuthToken().secret);
}
}
7 changes: 2 additions & 5 deletions auth/src/main/java/com/firebase/ui/auth/ui/idp/AuthMethodPickerActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@

import com.firebase.ui.auth.AuthUI;
import com.firebase.ui.auth.AuthUI.IdpConfig;
import com.firebase.ui.auth.BuildConfig;
import com.firebase.ui.auth.IdpResponse;
import com.firebase.ui.auth.R;
import com.firebase.ui.auth.ResultCodes;
Expand Down Expand Up @@ -102,10 +101,8 @@ private void populateIdpList(List<IdpConfig> providers) {
findViewById(R.id.email_provider).setVisibility(View.VISIBLE);
break;
default:
if (BuildConfig.DEBUG) {
Log.d(TAG, "Encountered unknown IDPProvider parcel with type: "
+ idpConfig.getProviderId());
}
Log.e(TAG, "Encountered unknown IDPProvider parcel with type: "
+ idpConfig.getProviderId());
}
}

Expand Down
5 changes: 1 addition & 4 deletions auth/src/main/java/com/firebase/ui/auth/util/FirebaseAuthWrapperImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import android.support.v4.app.FragmentActivity;
import android.util.Log;

import com.firebase.ui.auth.BuildConfig;
import com.google.android.gms.auth.api.Auth;
import com.google.android.gms.auth.api.credentials.CredentialPickerConfig;
import com.google.android.gms.auth.api.credentials.HintRequest;
Expand Down Expand Up @@ -207,9 +206,7 @@ private <T> T await(@NonNull Task<T> curTask) {
Tasks.await(curTask, mTimeoutMs, TimeUnit.MILLISECONDS);
return curTask.getResult();
} catch (ExecutionException | InterruptedException | TimeoutException e) {
if (BuildConfig.DEBUG) {
Log.w(TAG, "API request dispatch failed", e);
}
Log.w(TAG, "API request dispatch failed", e);
return null;
}
}
Expand Down