mapzen / eraser-map

Privacy-focused mapping application for Android
GNU General Public License v3.0
74 stars 24 forks source link

Crash when exiting navigation while app is in background #824

Closed msmollin closed 7 years ago

msmollin commented 7 years ago
msmollin commented 7 years ago

Per @sarahlensing:

This is caused by IllegalStateException: Location update received after client was disconnected

There are actually a few things going on but the most important is proof of a race condition. When we exit routing mode via that notification the lifecycle events triggered are onNewIntent -> onResume -> onPause (when the app is in the bg). onResume enables the location layer in the sdk, and onPause disables it.

When it's enabled, a location is fired to be propagated to listeners, but by the time it reaches the service, onPause has already called code to shut the service down.

06-27 16:27:00.417  12194                  sarah  D  [lost] service delegate [reportLocation]
06-27 16:27:00.767  12140                  sarah  D  [em] onPause
06-27 16:27:00.768  12140                  sarah  D  [sdk] removeLocationUpdates
06-27 16:27:00.769  12194                  sarah  D  [lost] removing requests
06-27 16:27:00.769  12194                  sarah  D  [lost] disable
06-27 16:27:00.772  12140                  sarah  D  [lost] disconnect client
06-27 16:27:00.772  12140                  sarah  D  [lost] disconnect client
06-27 16:27:00.772  12140                  sarah  D  [lost] service disconnect
06-27 16:27:00.772  12140                  sarah  D  [lost] onDisconnect
06-27 16:27:00.772  12194                  sarah  D  [lost] init callback null
06-27 16:27:00.773  12194                  sarah  D  [lost] service delegate [init] null
06-27 16:27:00.777  12140                  sarah  D  [lost] run onLocationChanged

The first line shows the location is attempted to be reported from the service process. The last line shows the location is actually received after everything has been disconnected.

The path forward is:

  1. optimize EM to not start/stop location in this case
  2. Make EM unregister the routing location request it makes

Those two things will fix the crash.

Then, in LOST:

  1. test that Google play services throws an exception if clients haven't unregistered location updates.
  2. remove the code we have in LOST which throws an IllegalStateException if a location is received after the service is disconnected and just silently return
  3. possibly move the IllegalStateException into the service (if it exists in play services) so that this is thrown when the service is attempted to be shut down.

As a note about something I tried: https://developer.android.com/reference/android/os/Binder.html#flushPendingCommands() should be something we can use on the LOST side to help resolve these kinds of issues. However, I tried it on both binders- the one that sends from service to app, and then app to service, and on neither object did that function appear to do anything to flush the commands.

I'm going to spend a small amount of time trying to be 100% sure that I cant flush pending calls (maybe I need to use that method in conjunction with another!?). It really feels like it should be something I resolve using that api.