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

Illegal callback invocation from native module during authorize flow #240

Closed
2 of 9 tasks
sraka1 opened this issue Feb 7, 2019 · 7 comments
Closed
2 of 9 tasks

Comments

@sraka1
Copy link

sraka1 commented Feb 7, 2019

Issue description

App crashes with the following fatal exception:

Fatal Exception: java.lang.RuntimeExceptionFailure delivering result ResultInfo{who=null, request=0, result=0, data=Intent { launchParam=MultiScreenLaunchParams { mDisplayId=0 mBaseDisplayId=0 mFlags=0 }(has extras) }} to activity {app.name/app.name.MainActivity}: java.lang.RuntimeException: Illegal callback invocation from native module. This callback type only permits a single invocation from native code.

Looking at the stack trace:

Caused by java.lang.RuntimeExceptionIllegal callback invocation from native module. This callback type only permits a single invocation from native code. Raw Text
--
  | com.facebook.react.bridge.CallbackImpl.invoke (CallbackImpl.java:30)
  | com.facebook.react.bridge.PromiseImpl.reject (PromiseImpl.java:70)
  | com.rnappauth.RNAppAuthModule.onActivityResult (RNAppAuthModule.java:213)
  | com.facebook.react.bridge.ReactContext.onActivityResult (ReactContext.java:256)
  | com.facebook.react.ReactInstanceManager.onActivityResult (ReactInstanceManager.java:601)
  | com.facebook.react.ReactActivityDelegate.onActivityResult (ReactActivityDelegate.java:149)
  | com.facebook.react.ReactActivity.onActivityResult (ReactActivity.java:77)
  | android.app.Activity.dispatchActivityResult (Activity.java:7289)
  | com.android.internal.os.ZygoteInit.main (ZygoteInit.java:1386)

Looking at RNAppAuthModule.java:213, we see this is now line 208 since #238 was merged in. Basically promise.reject("Failed to authenticate", exception.errorDescription);.

I would love to give more info, but I've been unable to replicate this myself, I've only seen it in Fabric/Crashlytics. I've seen it with Android OS versions 7, 8 and 5 (in that order), so I don't think OS version is that relevant here.

Proposed solutions/ideas

Im not that familiar with Android, so I'm not sure if this is related or not, but in RNAppAuthModule.java#L212 you are referencing the authorizePromise with this (so as an instance property) and then calling authorizePromise.resolve in case the authorize procedure goes through. However earlier and later, if you reject the promise, you call a promise variable without this. I assume this is auto-magically resolved, but it's still inconsistent?

In any case, per Promise spec, multiple resolve/rejects should be allowed, however there is an open issue with RN, so we must mitigate this/create a workaround for now. Maybe storing a bool like RN does itself?

The actual root problem seems to lie though in onActivityResult being called multiple times for whatever reason (presumably with only 1 invokation of the authorization flow).

The former might be related to #130 / #237 (same issue).


Environment

  • Library version: 4.0.0
  • Your Identity Provider: Keycloak
  • Platform that you're experiencing the issue on:
    • iOS
    • Android
    • iOS but have not tested behavior on Android
    • Android but have not tested behavior on iOS
    • Both
  • Are you using Expo, e.g. ExpoKit?
    • No
    • Yes, I've not ejected
    • Yes, but I have ejected to ExpoKit
    • Yes, but I have ejected to vanilla React Native
@kadikraman kadikraman added this to the Next Release 🚀 milestone Feb 7, 2019
@kadikraman
Copy link
Contributor

Thanks for all the details, it's very helpful in debugging! 🙏

I hope not to have to go down the boolean flag route - each resolve/reject should be terminating execution, so there may be a race condition or a missing return somewhere.

I know it's not easy to tell from Fabric/Crashlytics, but do you think this error is occurring when calling authorize or refresh?

@sraka1
Copy link
Author

sraka1 commented Feb 7, 2019

I’m fairly certain it happens when calling authorize. Could this even happen during refresh? I was under the impression this code only gets executed on authorize...

@KeiShadow
Copy link

KeiShadow commented Feb 7, 2019

I think the refresh doesn't make anything with this problem. My problem was update chrome to new version 72 then authorize stop working. In my app user click on login button and he is redirected to ids server where type username and password, after click login he will be redirected to a page with redirect to app button, but this part is missing when i updated chrome to version 72. He click on login and the custom tabs crash and return parameters with null values then the app crash too.

If you using custom tabs you need to set redirect uri into page where is button to redirect back to app then it will work 69 version of chrome.

The Google restrict the feature autoredirect into app without user interact and the solution up is workaround but only on 69 version -

@sraka1
Copy link
Author

sraka1 commented Feb 12, 2019

Btw, I just managed to reproduce this locally. We trigger authorize with a button. However, we weren't locking the button after it has been pressed. But since the browser takes a bit to startup (especially on slower Android phones - let's say 300-500ms) the user pressed the "login" button again. This triggers another browser flow. Exiting this flow then crashes the app...

@kadikraman
Copy link
Contributor

kadikraman commented Feb 18, 2019

@sraka1 - that makes sense! So in order to fix this, you'd have to disable the button when the request is already in flight. I could add the logic into our example app.

I'm not sure if it's worth actually building this into the library or not.

@sraka1
Copy link
Author

sraka1 commented Feb 22, 2019

Glad I could help. #237 is still causing us a lot of trouble though (dataIntent must not be null). And yet I’ve still been unable to replicate it on a test device. Any progress on that front perhaps? ☺️

@kadikraman
Copy link
Contributor

I'll have a look now. I don't own an Android phone so I'll have to see if I can reproduce it on the emulator.

Closing this issue since I think it's resolved by your explanation here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants