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

Added proper error handling on getResult of location settings #1557

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tentenponce
Copy link

@tentenponce tentenponce commented Aug 17, 2024

Resolves #1366 by adding try catch in getResult of checkLocationSettings to prevent crash when encountering RuntimeExecutionException when one of these errors happened:

  • API failed to connect while resuming due to an unknown error.
  • The connection to Google Play services was lost due to service disconnection.
  • The connection to Google Play services was lost due to service disconnection. Last reason for disconnect: DeadObjectException thrown while running ApiCallRunner.
  • The connection to Google Play services was lost due to service disconnection. Last reason for disconnect: Timing out service connection.

This also includes the changes from #1367 which is a nice addition for a failure listener as well.

Replication steps:

Given the error above which caused by connection issues, this doesn't have any direct replication steps. However, since we have logs in our Firebase Crashlytics, I tried simulating the error the check the behavior (by forcing throwing an error) and the app is actually crashing.

To forcefully replicate the error, simply throw an exception (in the response.getResult):

LocationSettingsResponse lsr = response.getResult();
if (true) {
  throw new RuntimeExecutionException(new Exception("test"));
}

Here's the sample screenshot of one of our crash logs:
image

Sample handling on Flutter side:

Since the app will not crash anymore and is properly returning an error, we can now handle it on the Flutter side. One example is to simply wrap it in a try catch, and do what the app should do when isLocationServiceEnabled fails (retry, show proper error dialog, etc):

try {
  serviceEnabled = await geolocatorAndroid.isLocationServiceEnabled();
  ...logic here on location service enabled result
} catch (e) {
  if (e is PlatformException && e.code == 'LOCATION_SERVICES_FAILED') {
    ...appropriate error handling here
  }
}

Pre-launch Checklist

  • I made sure the project builds.
  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is does not need version changes.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I rebased onto main.
  • I added new tests to check the change I am making, or this PR does not need tests.
  • I made sure all existing and new tests are passing.
  • I ran dart format . and committed any changes.
  • I ran flutter analyze and fixed any errors.

@tentenponce
Copy link
Author

tentenponce commented Aug 17, 2024

hi @TimHoogstrate requesting to review this PR, related to your PR as well with added handling: #1367

Asking for help also in the checks, since it fails on main.dart file even though I didn't updated any flutter related files. This should be fix or to be updated on a separate PR.

@tentenponce
Copy link
Author

tentenponce commented Aug 27, 2024

Just to add to the finding, after we applied the try-catch fix, we encounter an issue caused by:

image

Leading to the code where in we are still processing the result even if its unsuccessful already:

if (!response.isSuccessful()) {
  listener.onLocationServiceError(ErrorCodes.locationServicesDisabled);
  // it should stop here because response is unsuccessful, adding return should do the trick
}

So my hypothesis is that the crash is actually because the result is still being processed even if the response is unsuccessful. I will update this comment once we release the fix and monitor the crash logs if it will be actually fixed.

Update:

We have around 70k users, and there's 4k events for the crash in the old build (without the fix):
image

After the fix, there was no even a single error being encountered, hence the fix worked properly.

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

Successfully merging this pull request may close these issues.

[Bug]: FusedLocationClient.isLocationServiceEnabled crashes
1 participant