Added proper error handling on getResult of location settings

[tentenponce]

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:

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]

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]

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?