Skip to content

Add try catch to uncaught exception #553

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+21 −10
Open

Add try catch to uncaught exception #553

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
be1d0c0
add try catch to uncaught exception
john-slow Oct 11, 2024
5cd7ec7
Clang Format
Oct 11, 2024
e741f47
add try catch to macOS on resumeExternalUserAgentFlowWithURL
john-slow Oct 17, 2024
104627d
Clang Format
Oct 17, 2024
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
14 changes: 9 additions & 5 deletions flutter_appauth/ios/Classes/FlutterAppauthPlugin.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,12 +483,16 @@ - (BOOL)application:(UIApplication *)application
openURL:(NSURL *)url
options:
(NSDictionary<UIApplicationOpenURLOptionsKey, id> *)options {
if ([_currentAuthorizationFlow resumeExternalUserAgentFlowWithURL:url]) {
_currentAuthorizationFlow = nil;
return YES;
@try {
Copy link
Owner

@MaikuB MaikuB Oct 13, 2024

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

Copy link
Contributor Author

@john-slow john-slow Oct 15, 2024 •
edited
Loading

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?

Copy link
Contributor Author

@john-slow john-slow Oct 17, 2024

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.

Copy link
Owner

@MaikuB MaikuB Oct 19, 2024 •
edited
Loading

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

Copy link
Contributor Author

@john-slow john-slow Oct 23, 2024

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?

Copy link
Owner

@MaikuB MaikuB Dec 10, 2024

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

Copy link
Contributor Author

@john-slow john-slow Dec 12, 2024

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.

if ([_currentAuthorizationFlow resumeExternalUserAgentFlowWithURL:url]) {
_currentAuthorizationFlow = nil;
return YES;
}
return NO;
} @catch (NSException *exception) {
NSLog(@"Exception caught: %@, Reason: %@", exception.name,
exception.reason);
}

return NO;
}

- (BOOL)application:(UIApplication *)application
Expand Down
Loading