Skip to content
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.

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

NPE in AuthenticationActivity #137

Closed
billgockeler opened this issue Jan 2, 2018 · 14 comments
Closed

NPE in AuthenticationActivity #137

billgockeler opened this issue Jan 2, 2018 · 14 comments
Labels
bug This points to a verified bug in the code

Comments

@billgockeler
Copy link

We're experiencing the following crash. It seems that a null check is needed here:
Fatal Exception: java.lang.RuntimeException: Unable to resume activity {tv.fubo.mobile/com.auth0.android.provider.AuthenticationActivity}: java.lang.NullPointerException: Attempt to invoke virtual method 'android.os.Parcelable android.os.Bundle.getParcelable(java.lang.String)' on a null object reference
at android.app.ActivityThread.performResumeActivity(ActivityThread.java:3521)
at android.app.ActivityThread.handleResumeActivity(ActivityThread.java:3552)
at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2872)
at android.app.ActivityThread.access$900(ActivityThread.java:181)
at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1476)
at android.os.Handler.dispatchMessage(Handler.java:102)
at android.os.Looper.loop(Looper.java:145)
at android.app.ActivityThread.main(ActivityThread.java:6134)
at java.lang.reflect.Method.invoke(Method.java)
at java.lang.reflect.Method.invoke(Method.java:372)
at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1399)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1194)
Caused by java.lang.NullPointerException: Attempt to invoke virtual method 'android.os.Parcelable android.os.Bundle.getParcelable(java.lang.String)' on a null object reference
at com.auth0.android.provider.AuthenticationActivity.launchAuthenticationIntent(AuthenticationActivity.java:98)
at com.auth0.android.provider.AuthenticationActivity.onResume(AuthenticationActivity.java:76)
at android.app.Instrumentation.callActivityOnResume(Instrumentation.java:1255)
at android.app.Activity.performResume(Activity.java:6495)
at android.app.ActivityThread.performResumeActivity(ActivityThread.java:3510)
at android.app.ActivityThread.handleResumeActivity(ActivityThread.java:3552)
at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2872)
at android.app.ActivityThread.access$900(ActivityThread.java:181)
at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1476)
at android.os.Handler.dispatchMessage(Handler.java:102)
at android.os.Looper.loop(Looper.java:145)
at android.app.ActivityThread.main(ActivityThread.java:6134)
at java.lang.reflect.Method.invoke(Method.java)
at java.lang.reflect.Method.invoke(Method.java:372)
at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1399)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1194)

@lbalmaceda
Copy link
Contributor

Adding a null check there is nonsense as the method will do nothing.. :/ The activity is meant to be launched automatically by the WebAuthProvider class. Unless you're doing something in behind, this NPE should not be thrown. Can you please share any steps to reproduce it?

@billgockeler
Copy link
Author

billgockeler commented Jan 2, 2018

You are correct, the NPE should not be thrown, and yet it is - by your own code, not ours. As you said, the AuthenticationActivity is launched by OAuth internally, not by us. (actually, it's your OAuthManager that's starting the AuthenticationActivity, not WebAuthProvider as you stated).

We have not yet figured out the steps to reproduce the crash. There must be some condition in restoring the activity state that's leading to the NPE.

Regardless of how it's happening, your library should handle the error - not crash. The fact that your launchAuthenticationIntent() will do "nothing" if the bundle is null is another problem. It's called lack of error handling. It would be preferable if you could properly handle the null bundle and recover gracefully, otherwise a simple NP check would at least stop it from crashing.

BTW, the crash is happening across all devices and Android versions 4-7

Lastly, telling customers that their efforts to identify and fix bugs in your software is 'nonsense' is pretty unprofessional.

@lbalmaceda
Copy link
Contributor

Ok let's review how this is supposed to work, step by step:

  1. Using the WebAuthProvider you configure and start the authentication. This calls the internal class OAuthManager that will launch the AuthenticationActivity by passing among other things, the authorize Url. This value is generated by us and it's never null.
  2. Because the activity was never launched before, the intentLaunched flag takes the false value and the authentication with the browser is requested. This is the step where I say that adding a null check will do nothing, as we need that authorize Url value to begin and it's supposed to be present at that time. Always. The intentLaunched flag turns true before opening the browser.
  3. If the OS requests the activity state to be persisted, the flag value is stored for a later check when coming back like in step (2) or (4).
  4. Once the callback URL is hit, the RedirectActivity activity catches the intent and launches the AuthenticationActivity that, because it's already running, will receive a call in the OnNewIntent method. The intent is set to the activity and the next method to be called is either OnCreate if the activity was recreated, where the flag value is read from the saved state or OnResume where the data is obtained from the intent and returned up to the WebAuthProvider.

The state as I see, is correctly managed as the only valuable info is the intentLaunched flag and that's already being handled 👌 . Adding null checks everywhere in the code probably won't hurt but you need to analyze how a value may end up being null before that and see if that's a valid scenario or not.

Finally, note that all the activities are public and anyone could have launched the activity either via an app that read extracts packages and activities from app's manifests or via adb. If this is the case you can ignore the report. In any other case that you find this continues to happen, please provide as much details as possible so we can solve it. A sample project showing how to reproduce it helps a lot. 👍

Thanks

@billgockeler
Copy link
Author

billgockeler commented Jan 11, 2018

We've been trying to identify and reproduce this error so that we can provide you with more info and/or a sample app to repro - no luck so far. During that process we've been able to isolate it down to your 1.9.0 release. This crash was not present before that release. Perhaps that might provide a clue as to what changes you made in that release that would cause it.

@MattPhilpot
Copy link

I'd also like to comment on this. We've been experiencing this same issue since we started using auth0 within our app. We love the features of the service and library, but this is particularly frustrating as it has lead to several bad reviews.

Unfortunately, this specific crash is the #1 crash for our app(s), causing more crashes than everything else combined.

As far as statistics, I can say that we see this error most often on, from most to least:
API 23 (33%)
API 24 (30.8%)
API 25 (13.2%)
It doesn't happen on Oreo much, but that's probably just because there aren't many devices on it. It happens prior to 23 as well, but not nearly as much as 23 itself. Again, this could just be confirmation bias due to most devices today having 23 installed, but it's worth making these listed above the primary target of investigation

It has occurred for us both in 1.9.0 and your latest release. We haven't tried rolling back to a previous version yet.

The most common devices are (by a long shot, these two accounting for >50%):
Samsung Galaxy S7 Edge running API 24 (70%), the rest API 25 (30%)
Asus ZenFone 2 (ZE551ML) (Z00A) running API 24 (71%), the rest API 25 (29%)

It also tends to happen a lot of x86 devices, no matter the platform (but API 24 is most common)

As a stopgap to prevent this crash, we are considering modifying the source code to just finish the activity and go back to the previous screen with a toast or broadcasting a message reporting the failure, therefore allowing us to handle the error in a safer manner than crashing.

We'd really prefer not to make this change on our side, but hopefully this information helps in your investigation of the issue.

@lbalmaceda
Copy link
Contributor

@billgockeler @MattPhilpot There is an error case which was not being contemplated, when the browser tab is kept open after a successful authentication and the user goes back to the browser, refreshes the page and tries to log in again. This time it fails because there was no expected authentication. I've added the test case and prepared a patch for this. I'll try to make a release asap.

@lbalmaceda lbalmaceda added the bug This points to a verified bug in the code label Feb 1, 2018
@lbalmaceda lbalmaceda added this to the v1-Next milestone Feb 1, 2018
@lbalmaceda lbalmaceda removed this from the v1-Next milestone Feb 1, 2018
@lbalmaceda
Copy link
Contributor

Please try out version 1.12.1 and leave some feedback.

@MattPhilpot
Copy link

Thanks @lbalmaceda for the quick turn around. We will include this fix in our next release and will let you know!

@MattPhilpot
Copy link

Hey @lbalmaceda, just wanted to let you know that the fix is looking good so far. Haven't had any crash reports.

Thanks again for the quick turnaround.

@lbalmaceda
Copy link
Contributor

@MattPhilpot thanks for the feedback!

@vanshg
Copy link

vanshg commented Feb 20, 2018

We're getting this crash as well, but we're using Lock. Is it safe to manually include the new auth0 dependency and override the auth0 library version Lock is using (1.11.0)?

@lbalmaceda
Copy link
Contributor

@vanshg there shouldn't be any breaking changes since 1.11.0 at least, but I'll try to make a release of lock.android tomorrow. 👍

@vanshg
Copy link

vanshg commented Feb 20, 2018

Great, thank you!

@lbalmaceda
Copy link
Contributor

@vanshg I've released 2.8.1. Should be available in the next hours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This points to a verified bug in the code
Projects
None yet
Development

No branches or pull requests

4 participants