Added proper error handling on getResult of location settings

[tentenponce]

Resolves https://github.com/Baseflow/flutter-geolocator/issues/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 https://github.com/Baseflow/flutter-geolocator/pull/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:

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

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

[tentenponce]

hi @TimHoogstrate requesting to review this PR, related to your PR as well with added handling: https://github.com/Baseflow/flutter-geolocator/pull/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]

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

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):

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

[greg-tekai-com]

Just my two cents. It looks pretty good to me although I'm curious if a failure can hit both the failure listener as well as the completion one? I haven't had a chance to try the branch out yet, but your results sound promising.

FYI, we found the same issue where it was returning a result more than once. Definitely one of the big issues.

[tentenponce]

I'm curious if a failure can hit both the failure listener as well as the completion one?

Hi @greg-tekai-com, I tried these scenarios:

• forcing an exception in the addOnCompleteListener outside the try catch and check if it will fall on failure listener
• forcing an exception in the addOnCompleteListener inside the try catch and check if it will fall on failure listener
• turning off GPS

All of these cases passed and I haven't encountered any errors, crashes or anything related to calling both complete and failure listener.

[martin-labanic]

Hey there, any idea when this pull might be merged in?

[willp]

I believe I'm seeing the same crash, 100% reproducible, each time I call isLocationServiceEnabled() running an android which has no GPS receiver hardware, the "onn 4k" media streaming stick, mass produced for Walmart distribution in the US. I haven't tried the PR patch yet, I'm not sure how to override the platform plugin in the pubspec, sorry.

My stack trace crashes at line 172 in FusedLocationProvider.java, on the same line, with an ApiException, "Not implemented on this platform." The #1366 PR wraps that line in an exception handler, so I'm sure that will make a difference, probably fixing it entirely. (Sorry I'm not adept enough with pubspec plugin overrides, or I'd test. It seems the endorsement config keeps pointing to the original repo... Sorry, that's a me problem, not a you problem.)

Here's the relevant lines of the stack trace:

02-24 20:46:47.271 10777 10777 E AndroidRuntime: FATAL EXCEPTION: main
02-24 20:46:47.271 10777 10777 E AndroidRuntime: Process: com.example.redacted, PID: 10777
02-24 20:46:47.271 10777 10777 E AndroidRuntime: com.google.android.gms.tasks.RuntimeExecutionException: com.google.android.gms.common.api.ApiException: 10: Not implemented on this platform.
02-24 20:46:47.271 10777 10777 E AndroidRuntime: 	at com.google.android.gms.tasks.zzw.getResult(com.google.android.gms:play-services-tasks@@18.1.0:3)
02-24 20:46:47.271 10777 10777 E AndroidRuntime: 	at com.baseflow.geolocator.location.FusedLocationClient.lambda$isLocationServiceEnabled$0(FusedLocationClient.java:172)
02-24 20:46:47.271 10777 10777 E AndroidRuntime: 	at com.baseflow.geolocator.location.FusedLocationClient$$ExternalSyntheticLambda4.onComplete(Unknown Source:2)
02-24 20:46:47.271 10777 10777 E AndroidRuntime: 	at com.google.android.gms.tasks.zzi.run(com.google.android.gms:play-services-tasks@@18.1.0:1)
[ ... zap ... ]

Thanks for the library and all your work! -w

[emakar]

@tentenponce Hi! could you please resolve conflicts?

[tentenponce]

@tentenponce Hi! could you please resolve conflicts?

@emakar resolved, I removed the CHANGELOG as it conflicts on their releases, I will let the authors to manage the CHANGELOG on this one.

[BartholomeNiji]

Hello, when this pull request will be merged ?

[Ellynnn]

Hi, any update on this?

[unexus]

I would suggest replacing RuntimeExecutionException with a normal Exception to avoid a crash during catch.

[tentenponce]

I would suggest replacing RuntimeExecutionException with a normal Exception to avoid a crash during catch.

@unexus I specifically add the RuntimeExecutionException to isolate the handling for this specific issue only. If we make it Exception, then it will swallow all errors and we don't have a way to identify if there's any other issue happening, and some errors may require specific handling. Better to throw them for visibility and address them specifically.

[tentenponce]

closing this now, resolved with similar idea in this PR: https://github.com/Baseflow/flutter-geolocator/pull/1730