openid / AppAuth-Android

Android client SDK for communicating with OAuth 2.0 and OpenID Connect providers.
https://openid.github.io/AppAuth-Android
Apache License 2.0
2.82k stars 881 forks source link

Activity net.openid.appauthdemo.MainActivity has leaked ServiceConnection #91

Open dewafer opened 8 years ago

dewafer commented 8 years ago

I've encountered a leak problem. I was using a simulator without Chrome installed. On a Chrome installed simulator the leak won't happen.

This leak was occurred when I hit the back button and while the MainActivity turned into background.

And when I dig a little deeper into the problem I found that the mBrowserPackage in the BrowserHandler was resolved to com.android.browser and the return value of CustomTabsClient#bindCustomTabsService(...) is false. So the connection is not retained in the BrowserHandler and when BrowserHandler#unbind() is called, nothing is unbind.

I've tried to retained the connection even when CustomTabsClient#bindCustomTabsService(...) returns false. It seems solve the leak problem, but I'm not sure will there be any side effects.

07-06 10:12:45.442 3929-3929/net.openid.appauthdemo E/ActivityThread: Activity net.openid.appauthdemo.MainActivity has leaked ServiceConnection net.openid.appauth.BrowserHandler$1@2206f76 that was originally bound here
                                                                      android.app.ServiceConnectionLeaked: Activity net.openid.appauthdemo.MainActivity has leaked ServiceConnection net.openid.appauth.BrowserHandler$1@2206f76 that was originally bound here
                                                                          at android.app.LoadedApk$ServiceDispatcher.<init>(LoadedApk.java:1092)
                                                                          at android.app.LoadedApk.getServiceDispatcher(LoadedApk.java:986)
                                                                          at android.app.ContextImpl.bindServiceCommon(ContextImpl.java:1303)
                                                                          at android.app.ContextImpl.bindService(ContextImpl.java:1286)
                                                                          at android.content.ContextWrapper.bindService(ContextWrapper.java:604)
                                                                          at android.support.customtabs.CustomTabsClient.bindCustomTabsService(CustomTabsClient.java:60)
                                                                          at net.openid.appauth.BrowserHandler.bindCustomTabsService(BrowserHandler.java:86)
                                                                          at net.openid.appauth.BrowserHandler.<init>(BrowserHandler.java:61)
                                                                          at net.openid.appauth.AuthorizationService.<init>(AuthorizationService.java:101)
                                                                          at net.openid.appauthdemo.MainActivity.onCreate(MainActivity.java:63)
                                                                          at android.app.Activity.performCreate(Activity.java:6237)
                                                                          at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1107)
                                                                          at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2369)
                                                                          at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2476)
                                                                          at android.app.ActivityThread.-wrap11(ActivityThread.java)
                                                                          at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1344)
                                                                          at android.os.Handler.dispatchMessage(Handler.java:102)
                                                                          at android.os.Looper.loop(Looper.java:148)
                                                                          at android.app.ActivityThread.main(ActivityThread.java:5417)
                                                                          at java.lang.reflect.Method.invoke(Native Method)
                                                                          at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:726)
                                                                          at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:616)
hy9be commented 8 years ago

Same problem observed.

asinel commented 8 years ago

+1

kix2902 commented 7 years ago

I've the same exact problem with v0.3.0 and with v0.4.0 the trace changed a bit:

11-01 15:11:39.431 22219-22219/com.redinput.youtubestats E/ActivityThread: Activity com.redinput.youtubestats.ChannelsActivity has leaked ServiceConnection net.openid.appauth.CustomTabManager$1@7ba31ca that was originally bound here
                                                                           android.app.ServiceConnectionLeaked: Activity com.redinput.youtubestats.ChannelsActivity has leaked ServiceConnection net.openid.appauth.CustomTabManager$1@7ba31ca that was originally bound here
                                                                               at android.app.LoadedApk$ServiceDispatcher.<init>(LoadedApk.java:1136)
                                                                               at android.app.LoadedApk.getServiceDispatcher(LoadedApk.java:1030)
                                                                               at android.app.ContextImpl.bindServiceCommon(ContextImpl.java:1305)
                                                                               at android.app.ContextImpl.bindService(ContextImpl.java:1288)
                                                                               at android.content.ContextWrapper.bindService(ContextWrapper.java:604)
                                                                               at android.support.customtabs.CustomTabsClient.bindCustomTabsService(CustomTabsClient.java:64)
                                                                               at net.openid.appauth.CustomTabManager.bind(CustomTabManager.java:85)
                                                                               at net.openid.appauth.AuthorizationService.<init>(AuthorizationService.java:107)
                                                                               at net.openid.appauth.AuthorizationService.<init>(AuthorizationService.java:85)
                                                                               at net.openid.appauth.AuthorizationService.<init>(AuthorizationService.java:74)
                                                                               at com.redinput.youtubestats.ChannelsActivity.selectAccount(ChannelsActivity.java:96)
                                                                               at com.redinput.youtubestats.ChannelsActivity_ViewBinding$1.doClick(ChannelsActivity_ViewBinding.java:37)
                                                                               at butterknife.internal.DebouncingOnClickListener.onClick(DebouncingOnClickListener.java:22)
                                                                               at android.view.View.performClick(View.java:5201)
                                                                               at android.view.View$PerformClick.run(View.java:21209)
                                                                               at android.os.Handler.handleCallback(Handler.java:739)
                                                                               at android.os.Handler.dispatchMessage(Handler.java:95)
                                                                               at android.os.Looper.loop(Looper.java:148)
                                                                               at android.app.ActivityThread.main(ActivityThread.java:5525)
                                                                               at java.lang.reflect.Method.invoke(Native Method)
                                                                               at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:730)
                                                                               at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:620)
blablanumerodeux commented 7 years ago

I've resolved the problem by using only ONE AuthorizationService for the app. Do not use anonymous AuthorizationService. Hope it'll help =)

madhuteja commented 7 years ago

@blablanumerodeux can you please elaborate what you have done to use a single AuthorizationService instance. I'm having the same issue in my app. Thanks

rahulsingh0089 commented 7 years ago

When click on system back button from one activity to go another activity then getting this issue. Please resolve as soon as possible.....

04-13 09:33:39.388 25408-25408/com.googleappauth.rahukuma.googlesigninbrowser E/ActivityThread: Activity com.googleappauth.rahukuma.googlesigninbrowser.Main2Activity has leaked ServiceConnection net.openid.appauth.BrowserHandler$1@40d3e4d0 that was originally bound here android.app.ServiceConnectionLeaked: Activity com.googleappauth.rahukuma.googlesigninbrowser.Main2Activity has leaked ServiceConnection net.openid.appauth.BrowserHandler$1@40d3e4d0 that was originally bound here at android.app.LoadedApk$ServiceDispatcher.(LoadedApk.java:969) at android.app.LoadedApk.getServiceDispatcher(LoadedApk.java:863) at android.app.ContextImpl.bindService(ContextImpl.java:1418) at android.app.ContextImpl.bindService(ContextImpl.java:1407) at android.content.ContextWrapper.bindService(ContextWrapper.java:473) at android.support.customtabs.CustomTabsClient.bindCustomTabsService(CustomTabsClient.java:60) at net.openid.appauth.BrowserHandler.bindCustomTabsService(BrowserHandler.java:86) at net.openid.appauth.BrowserHandler.(BrowserHandler.java:61) at net.openid.appauth.AuthorizationService.(AuthorizationService.java:98) at com.googleappauth.rahukuma.googlesigninbrowser.Main2Activity.handleAuthorizationResponse(Main2Activity.java:63) at com.googleappauth.rahukuma.googlesigninbrowser.Main2Activity.checkIntent(Main2Activity.java:44) at com.googleappauth.rahukuma.googlesigninbrowser.Main2Activity.onStart(Main2Activity.java:35) at android.app.Instrumentation.callActivityOnStart(Instrumentation.java:1164) at android.app.Activity.performStart(Activity.java:5114) at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2153) at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2230) at android.app.ActivityThread.access$600(ActivityThread.java:141) at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1234) at android.os.Handler.dispatchMessage(Handler.java:99) at android.os.Looper.loop(Looper.java:137) at android.app.ActivityThread.main(ActivityThread.java:5041) at java.lang.reflect.Method.invokeNative(Native Method) at java.lang.reflect.Method.invoke(Method.java:511) at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:793) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:560) at dalvik.system.NativeStart.main(Native Method)

I have seen all issue related to this but not able to solve. Please help..

iainmcgin commented 7 years ago

Should be fixed by #211.

mateuszwlodarczyk265 commented 7 years ago

Is this issue really solved? I'm still getting the same error... @rahulsingh0089 @madhuteja @blablanumerodeux @kix2902 @asinel @hy9be @dewafer you dont expierience this anymore?

iainmcgin commented 7 years ago

Are you using 0.6.0 and still seeing the issue? If so, I'll re-open.

mateuszwlodarczyk265 commented 7 years ago

@iainmcgin We are using fork of your latest main branch with some changes related with the Fragment response type.

ghost commented 7 years ago

I am using 0.6.0 and this issue is reproducible.

H3c4t0nch1r3 commented 6 years ago

i am reproducing this issue while using 0.7.0

awahnteh commented 6 years ago

This issue still exists :(, and I am using: compile 'net.openid:appauth:0.7.0' here is my stack trace:

Activity activities.MainActivity has leaked ServiceConnection net.openid.appauth.browser.CustomTabManager$1@3b8f611 that was originally bound here android.app.ServiceConnectionLeaked: Activity activities.MainActivity has leaked ServiceConnection net.openid.appauth.browser.CustomTabManager$1@3b8f611 that was originally bound here at android.app.LoadedApk$ServiceDispatcher.(LoadedApk.java:1399) at android.app.LoadedApk.getServiceDispatcher(LoadedApk.java:1294) at android.app.ContextImpl.bindServiceCommon(ContextImpl.java:1503) at android.app.ContextImpl.bindService(ContextImpl.java:1475) at android.content.ContextWrapper.bindService(ContextWrapper.java:688) at android.support.customtabs.CustomTabsClient.bindCustomTabsService(CustomTabsClient.java:70) at net.openid.appauth.browser.CustomTabManager.bind(CustomTabManager.java:95) at net.openid.appauth.AuthorizationService.(AuthorizationService.java:113) at net.openid.appauth.AuthorizationService.(AuthorizationService.java:91) at net.openid.appauth.AuthorizationService.(AuthorizationService.java:80)

Copy33 commented 6 years ago

Getting the same issue. @iainmcgin

finchatticus commented 6 years ago

reproduce issue with 0.7.0

graves501 commented 6 years ago

i'm facing the same problems with 0.7.0... this issue isn't solved at all.

PieterIng commented 6 years ago

disposing the AuthorizationService helped for me... (edit: I found out when looking at the source of AuthorizationService, maybe add it to the documentation)

awahnteh commented 6 years ago

@Quxan , can you provide some source code of how you disposed of the AuthorizationService?

PieterIng commented 6 years ago

@awahnteh in your case in the MainActivity you probably have something like: authService = new AuthorizationService(this);

you can for example dispose it in onDestroy authService.dispose()

the doc describes the following:

/**

  • Disposes state that will not normally be handled by garbage collection. This should be
  • called when the authorization service is no longer required, including when any owning
  • activity is paused or destroyed (i.e. in {@link android.app.Activity#onStop()}). */
awahnteh commented 6 years ago

Hi @Quxan , I actually did that, but that actually didn't solve the problem for me. In my case it is a bit further complicated by the fact that my instantiation of authService doesn't happen in an activity where I can cleanly dispose of it in onDestroy. It happens deep in the hierarchy of another component, that is instantiated by another component in my hierarchy.

Yes, I imagine climbing down the hierarchy and building some sort of cascade dispose method could be a solution, but not only is that far more complicated than I would prefer in my use case -- it didn't actually solve the problem for me.

What is worse is that I would have loved to know before hand (#documentation!) that AuthorizationService is something that needs to be explicitly disposed upon termination (or onDestroy) of the activity that contains it; that would have changed my component architecture from the get-go than having to go through a lengthy re-architecture exercise.

Ok setting aside my rant for a moment and focusing on some productive sharing ;) for a moment; what I did notice that seemed to work for me is: to not instantiating the AuthorizationService using my activity (in @Quxan's example above that reference is this) as the context. Instead, what I am doing, is I am using my ApplicationContext ( getApplicationContext()) as the context for my AuthorizationService. This ApplicationContext has a far longer lifespan (which is the entire time the application is open), than for example an Activity, which might be destroyed by the operating system when I switch activities). Because of this nature of how the android os works, it is less likely to leak the ServiceConnection cause it is always attached to the ApplicationContext.

Now the real question for Mr. McGinniss (@iainmcgin), does latching my AuthorizationService to my ApplicationContext instead of an Activity have negative repercussions to my app?

awahnteh commented 6 years ago

Also, a supplemental side bar question for @iainmcgin, in your architecture would changing the relationship between the context and the AuthorizationService from a Strong Reference to a Weak Reference solve this problem?

This is a snippet from the existing source code of AuthorizationService.java's constructor (pay attention to line 117, in the snippet below it will be line 5):

1:  AuthorizationService(@NonNull Context context,
2:                          @NonNull AppAuthConfiguration clientConfiguration,
3:                          @Nullable BrowserDescriptor browser,
4:                          @NonNull CustomTabManager customTabManager) {
5: //--->  mContext = checkNotNull(context); <--- This line here (Line #107 in AuthorizationService.java source code)
6:         mClientConfiguration = clientConfiguration;
7:         mCustomTabManager = customTabManager;
8:         mBrowser = browser;
9: 
10:         if (browser != null && browser.useCustomTab) {
11:             mCustomTabManager.bind(browser.packageName);
12:         }
13:     }

if you detect that the weakReference is broken, mContext == null, you can then at that point cleanly dispose of the Authorization service (or gracefully replace it with an ApplicationContext). or better yet, completely remove the need for passing in a context of any type during the instantiation of the AuthService and simply internally use the Application's context.

I have perused the internals of the AuhtorizationService class and so far nothing has struck me in a way that convinces me that an actual activity context is needed (I could be wrong though as all I did was a quick perusal).

Would love to hear your thoughts/feedback on this.

Cheers & Happy new year!

AlexanderDavydov commented 6 years ago

The issue is still reproducible on the last release.

In my case i do service.dispose(); And i don't see the leakage message in logs anymore.

private AuthorizationService service;

private void authorize() {
        if (mService != null) {
            mService.dispose();
            mService = null;
        }
        service = new AuthorizationService(this, token);
        Intent intent = service.getAuthorizationRequestIntent(getAuthRequest());
        startActivityForResult(intent, REQUEST_AUTH);
}

@Override
protected void onActivityResult(int requestCode, int resultCode, Intent data) {

            // ...

            AuthorizationResponse resp = AuthorizationResponse.fromIntent(data);
            AuthorizationException ex = AuthorizationException.fromIntent(data);
            if (resp != null) {

                // ...

                service.performTokenRequest(
                        resp.createTokenExchangeRequest(parametersMap),
                        new AuthorizationService.TokenResponseCallback() {
                            @Override
                            public void onTokenRequestCompleted(TokenResponse resp, AuthorizationException ex) {
                                if (resp != null) {
                                    // handle success                        
                                } else {
                                   // handle error
                                }
                                service.dispose();
                                service = null;
                            }
                        });

                // ...
}  
arpitjindal97 commented 6 years ago

Should be marked as "Critical Bug"

iainmcgin commented 6 years ago

Ok, coming back to this: it is clear that the issue for most people is that they do not dispose of their AuthorizationService instances at the appropriate time. Though, I do also understand @awahnteh's point that the need to do this needs to be more prominently documented (it is at least documented in the Javadoc for that type, but it's possibly worthy of putting into the main overview docs).

@arpitjindal97 critical in what sense? Is this crashing your app?

I think the real "fix" for this problem for most apps would be if we made it easier to treat an AuthorizationService as a lifecycle-aware component, so that the effects of the dispose() call could be automated. A WeakReference might also be sufficient as a short term fix, I'll think upon that some more.

mtangoo commented 6 years ago

it is clear that the issue for most people is that they do not dispose of their AuthorizationService instances at the appropriate time.

@iainmcgin, when is the appropriate time to dispose it? I face the same issue and in two devices it crashes the app and in one device it works. So that will help me so much.

In those two devices, it crashes immediately after launching chrome tabs. If I blacklist chrome tabs and use normal chrome browser, it doesn't crash though. Only error I get is that one (hence my assumption that it is the cause).

I just wanted to add information/ask in case its relevant

iainmcgin commented 6 years ago

I usually call dispose during onDestroy or onStop, depending on when the instance was created (i.e. in onCreate, onStart or at some later point asynchronously). Usually, onDestroy is your last opportunity to dispose of the instance, otherwise you will see the leak warnings.

mtangoo commented 6 years ago

Thank you

SWRHARD commented 5 years ago

i cant decide this problem too, when i'm login first time and then press "back pressed btn", i have this message "has leaked serviceConnection...", i tried to call "authorizationService.dispose()" in onDestroy, but nothing is changed(

SWRHARD commented 5 years ago

still not working, 0.7.1

still not fixed, will be fixed this? it's important problem, cant understand why this is not fixed yet

juanmendez commented 4 years ago

It worked for me like this using 0.71 private var authService: AuthorizationService? = null

override fun onStop() {
// this needs to be done before returning, not in onActivityResult as described above
// remember the activity ends and then RedirectUriRegisterActivity takes over
        super.onStop()
        authService?.dispose()
        authService = null // prevents next: java.lang.IllegalStateException: Service has been disposed and rendered inoperable
}
<activity
            android:name="net.openid.appauth.RedirectUriReceiverActivity"
            tools:node="replace">
            <intent-filter>
                <action android:name="android.intent.action.VIEW"/>
                <category android:name="android.intent.category.DEFAULT"/>
                <category android:name="android.intent.category.BROWSABLE"/>
                <data android:scheme="yourScheme"/>
            </intent-filter>
        </activity>

Hope this helps anyone.

Allan-Nava commented 4 years ago

It worked for me like this using 0.71 private var authService: AuthorizationService? = null

// after getting server configuration authState = AuthState(serviceConfig)

override fun onStop() {
// this needs to be done before returning, not in onActivityResult as described above
// remember the activity ends and then RedirectUriRegisterActivity takes over
        super.onStop()
        authService?.dispose()
        authService = null // prevents next: java.lang.IllegalStateException: Service has been disposed and rendered inoperable
}
<activity
            android:name="net.openid.appauth.RedirectUriReceiverActivity"
            tools:node="replace">
            <intent-filter>
                <action android:name="android.intent.action.VIEW"/>
                <category android:name="android.intent.category.DEFAULT"/>
                <category android:name="android.intent.category.BROWSABLE"/>
                <data android:scheme="yourScheme"/>
            </intent-filter>
        </activity>

Hope this helps anyone.

Is fixed with 0.7.1?

Allan-Nava commented 4 years ago

Is possible to implement with the MVP? I'm trying to implement the login with idp_hint

juanmendez commented 4 years ago

you may need to instantiate authService again to go further down. I am not so sure if you mean minimum viable product, or mode-view-presenter. I imagine you can use setLoginHint("jdoe@user.example.com") as it appears in the read me document.

Allan-Nava commented 4 years ago

you may need to instantiate authService again to go further down. I am not so sure if you mean minimum viable product, or mode-view-presenter. I imagine you can use setLoginHint("jdoe@user.example.com") as it appears in the read me document.

How can I implement the appAuth with model view presenter?

cutiko commented 11 months ago

We start to get this problem recently with the latest Chrome update. It happens only in OS 11 or lower together with Chrome version 117 or above.

It started happening with the Chrome updated but the update doesn't seem to be the issue:

One of our engineers found that setting the SESSION to null solves the issue for the "old devices":

//version validation
val intent = customTabsIntent.intent
val sessionKey = "android.support.customtabs.extra.SESSION"
val value: String? = null
intent.putExtra(sessionKey, value)

So this doesn't seem strictly related to dispose because the issue happenn when the custom tab is launched.