-
Notifications
You must be signed in to change notification settings - Fork 248
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
Add try catch to uncaught exception #553
base: master
Are you sure you want to change the base?
Conversation
if ([_currentAuthorizationFlow resumeExternalUserAgentFlowWithURL:url]) { | ||
_currentAuthorizationFlow = nil; | ||
return YES; | ||
@try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having trouble getting the timing right to reproduce this but based on the code changes, from the perspective of an app using the plugin from the Dart/Flutter side, the exception is silently caught and won't be informed that there was a problem. Is my understanding correct? If it is, can the PR make the appropriate changes that would raise an exception for apps to handle? The other thing is based on what you mentioned, I suspect macOS would also have a similar problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bug is somewhat rare, occurring about 1 to 2 times out of 20. I'd prefer to ignore this exception if possible, but the app crashes if it's not handled in objective-c. It works fine even if we don't raise the exception to Dart/Flutter, as long as it's managed in Objective-C. For macOS, I believe it would face the same issue if it uses the same login mechanism.
Edit: Do you have a suggestion for an exception type to raise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a simple throw in the catch functions and also added try catch to macOS as well.
Please let me know if you have any suggestion or question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: Do you have a suggestion for an exception type to raise?
The plugin will handle throwing the appropriate exception provided you make the calls on the native method channel side. There are existing examples do that this on iOS and macOS by calling this method. Calling this will raise the appropriate exception with the main part that does so being at this line that calls an API provided by Flutter. Since the user is hitting done then IMO this should trigger the plugin to raise a FlutterAppAuthUserCancelledException
as done here. Can you see how this could be done? I'm not sure what AppAuth SDK is throwing in the scenario you meaned as I haven't gotten to reproduce it but it may mean some custom logic. The logic on the iOS/macOS that would lead to the appropriate exception being raised can be see here
FYI I understand this is rare and how the PR in its current form avoids a crashed but since it's silently caught from the perspective of the Flutter side, it doesn't provide the app/developer the opportunity to catch the exception and leave it them to do decide what to do. If I'm not mistaken, the PR as it is would prevent an uncaught exception from going to something like Crashlytics too. If so and the PR in its current form was merged, when this rare scenario happens and a user reported to a dev about what happens, there's no exception etc to help debug. This is why I wanted to double check my understanding as having an exception thrown would be better
Edit: updated message with more details about exception to throw and references to existing logic to help guide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you mentioned, the exception is typically handled in the code you provided. However, this specific error originates from the code here. I've attached a video that should clarify things. In the video, I simply click the "done" button during the auto login process (no password entry required). There's a slight chance the app might either crash or freeze (in the video, it froze), and you can see the exception printed on the left side of the console.
crash-issue.mp4
You're right in your understanding, but I'm uncertain about how can I help debug when this happens. My code is intended to prevent the app from crashing only. Could you either demonstrate or guide me on the proper way to handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, I wasn't asking for helping to debug. I was asking for the code to be changed so that when the scenario happens, rather than silently swallowing the error, it results in an except that can by caught by applications. I understand this is rare but this would enable applications to know something happened and decide what to do. In terms of guidance, refer to the links in my previous post as the intent there to provide references to existing that shows how it's currently done to help guide. It has links to code that shows how the plugin raises exceptions starting from the native side and how that makes its way to the Flutter side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I see, thanks for your clarification.
As promised, I have created this pull request to address the bug where manually closing the login popup sheet could potentially trigger the following exception: