ClassCastException: android.os.BinderProxy cannot be cast to com.mapzen.android.lost.internal.FusedLocationProviderService$FusedLocationProviderBinder

zugaldia commented 7 years ago

Description

The FusedLocationProviderService crashes with a java.lang.ClassCastException when a custom process is set for an activity.

Steps to Reproduce

Include LOST in your project.
Create an activity that uses LOST, and set up a custom process for the activity adding android:process=":foo" in the manifest.
Launch the activity:

03-20 18:04:14.247 7383-7383/com.mapbox.mapboxandroiddemo:test D/AndroidRuntime: Shutting down VM
03-20 18:04:14.257 7383-7383/com.mapbox.mapboxandroiddemo:test E/AndroidRuntime: FATAL EXCEPTION: main
 Process: com.mapbox.mapboxandroiddemo:test, PID: 7383
 java.lang.ClassCastException: android.os.BinderProxy cannot be cast to com.mapzen.android.lost.internal.FusedLocationProviderService$FusedLocationProviderBinder
     at com.mapzen.android.lost.internal.FusedLocationProviderApiImpl.onServiceConnected(FusedLocationProviderApiImpl.java:45)
     at com.mapzen.android.lost.internal.FusedLocationServiceConnectionManager.onServiceConnected(FusedLocationServiceConnectionManager.java:79)
     at com.mapzen.android.lost.internal.FusedLocationProviderApiImpl.onServiceConnected(FusedLocationProviderApiImpl.java:65)
     at android.app.LoadedApk$ServiceDispatcher.doConnected(LoadedApk.java:1453)
     at android.app.LoadedApk$ServiceDispatcher$RunConnection.run(LoadedApk.java:1481)
     at android.os.Handler.handleCallback(Handler.java:751)
     at android.os.Handler.dispatchMessage(Handler.java:95)
     at android.os.Looper.loop(Looper.java:154)
     at android.app.ActivityThread.main(ActivityThread.java:6119)
     at java.lang.reflect.Method.invoke(Native Method)
     at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:886)
     at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:776)
03-20 18:04:14.270 1443-1608/system_process W/ActivityManager:   Force finishing activity com.mapbox.mapboxandroiddemo/com.mapbox.mapboxandroidweardemo.examples.SimpleMapViewActivity
03-20 18:04:14.276 1443-1608/system_process W/ActivityManager:   Force finishing activity com.mapbox.mapboxandroiddemo/com.mapbox.mapboxandroidweardemo.MainActivity

Lost & Android Version

Seen with LOST 2.1.2 on a Nexus 5x running Nougat but we have reports for other devices and versions.

Emeritus-DarranKelinske commented 7 years ago

When this happened to me I almost started crying. Please let me know if there is anything I can do to help debug / test.

ecgreb commented 7 years ago

Sorry for the lack of response here currently out on vacation.

Does this issue still occur in versions before 2.1.2? Does it still happen in 2.2.0?

Emeritus-DarranKelinske commented 7 years ago

My apologies, but I do not know. I hit this by using a downstream dependency named mapbox. I know it worked in mapbox 4.x, but the later versions of mapbox 5.x have shown this issue.

msmollin commented 7 years ago

I posed the same question downstream. In the mean time we'll see if we can replicate this internally although it may be a little slow since we're in the middle of a release cycle for our new iOS platforms and so most resources are focused on that.

Stay tuned!

zugaldia commented 7 years ago

I just tried upgrading the Mapbox Android SDK to LOST 2.2.0 but I'm still seeing the same crash.

Two more data points in case it helps troubleshoot the problem:

If the location permission request has not been accepted, there's no crash. The crash only happens after the permission has been granted.
The Mapbox Android SDK upgraded from LOST 1.x to 2.x when we moved from 4.x to 5.x (https://github.com/mapbox/mapbox-gl-native/issues/6537). Per https://github.com/mapzen/lost/issues/173#issuecomment-289816392 above, this issue seems to be new to LOST 2.x.

sarahsnow1 commented 7 years ago

This is a current limitation of LOST due to the way we bind to the underlying service. Currently, we use a Binder which isn't recommended for use across different processes. Migrating the underlying binding mechanism to support multiple processes will not be trivial.

In the meantime you could work around this (if LOST is only used in a single process) by specifying the process the LOST service should run in, in your manifest:

<service android:name="com.mapzen.android.lost.internal.FusedLocationProviderService"
        android:process=":foo"/>

As @msmollin said, we currently have our heads down for an iOS release but will give this attention after we get that out the door. Sorry for the delay and thanks for your help debugging.

Emeritus-DarranKelinske commented 7 years ago

Thank you so much for the workaround! <3

mariusboepple commented 7 years ago

This is one of our top crashes on Android with Mapbox 5.0.1 - using the workaround for now...

edit: Workaround is NOT working

Emeritus-DarranKelinske commented 7 years ago

Were you able to get the workaround to work? I was unsuccessful. If you do, can you please post a more complete code snippet. Thank you!

mariusboepple commented 7 years ago

Unfortunately it's crashing on every app start with the recommended workaround...

Emeritus-DarranKelinske commented 7 years ago

That's what happened to me too. I had to disable all location permissions which I think precludes me from using Google Play Services or another location provider.

mariusboepple commented 7 years ago

That's not an option for us. I think the only thing we can do is wait for a fix and ignore the crashes...

Emeritus-DarranKelinske commented 7 years ago

something like this needs to be added to test suites - we rely on mapbox heavily in our app as the map is the core component - we were also relying on the location services of mapbox in addition to google play services

mariusboepple commented 7 years ago

same here... Bugs like this are just disappointing and there are many of them in Mapbox... 👎

ecgreb commented 7 years ago

A couple things here:

This bug is on the Lost and not Mapbox side so this is not the right forum for feedback about the number of bugs in Mapbox.
Using custom processes in your app is not a standard use case and honestly not one we had considered up until now.

https://developer.android.com/guide/components/processes-and-threads.html

By default, all components of the same application run in the same process and most applications should not change this.

Out of curiosity what types of applications are you building that require a custom process?

That said we are currently evaluating potential solutions here. The current front-runner is using a Handler and Messenger to communicate with the fused location service.

https://developer.android.com/guide/components/bound-services.html#Messenger

We also need to consider the implications of Android O and its restrictions on background services and location limits which will certainly impact how we structure location updates in Lost moving forward.

https://developer.android.com/preview/features/background-location-limits.html

mariusboepple commented 7 years ago

We need a remote process which is not tied to the app's lifecycle to communicate with a Bluetooth LE device reliably in the background but you're right, that's not a standard use case.

zugaldia commented 7 years ago

This bug is on the Lost and not Mapbox side so this is not the right forum for feedback about the number of bugs in Mapbox.

Exactly, thank you for pointing that out @ecgreb and for your help making LOST awesomer. Please, if you have any issues using the Mapbox Android SDK please do open tickets on https://github.com/mapbox/mapbox-gl-native. We welcome feedback and PRs, feel free to tag me too.

mariusboepple commented 7 years ago

Ok, so it's a bug with Lost, thanks for pointing that out 😄 Sorry for being a bit grumpy...

dazza5000 commented 7 years ago

I have the same use case as marius. We have an app that communicates to IoT devices over ble and we need it running outside of the app process.

dazza5000 commented 7 years ago

Thank you to everyone who makes these projects possible <3

dazza5000 commented 7 years ago

Is there a plan to fix this? If so, what do you all think the ETA might be?

msmollin commented 7 years ago

@dazza5000 thanks for commenting, and we appreciate the support :) We're currently in the process of evaluating our options with an eye towards the future of Android as @ecgreb mentioned. No ETA is currently available as this is a core-level change and there are a few options to test out. As @ecgreb already mentioned, we do have a possibly path forward to try out, so we'll be testing some changes to ensure we don't impact the original implementation of running everything inside a single process.

As soon as we're in a position where we have a resolution plan, we will update this issue.

Emeritus-DarranKelinske commented 7 years ago

I don't know what ready means, but I like it. <3

ecgreb commented 7 years ago

After testing the Messenger approach to service IPC it appears this will not satisfy the requirements for Lost particularly in the case of FusedLocationProviderApi#getLastLocation() since this method returns a Location object but a Messenger does not support synchronous/blocking requests.

Therefore we are now looking into exposing the FusedLocationProviderService using AIDL which allows synchronous requests and would solve the current crash when using a custom process in your app.

AIDL would also potentially allow us to add cross-application support at some point in the future if Lost were installed on devices as its own APK a la play services.

ecgreb commented 7 years ago

Ok so I believe the path forward looks roughly like this:

[x] Remove deprecated provider enable/disabled callbacks from LocationListener interface (#181)
[x] Migrate LostClientManager outside of the fused location provider service since client objects are not Parcelable (#182)
[x] Add Parcelable interface to LocationRequest objects since they will need to be passed over IPC (#183)
[x] Migrate PendingResult return values from FusedLocationProviderService to FusedLocationProviderApiImpl (#184)
[ ] Generate AIDL interface for FusedLocationProviderService to enable communication from a custom process. (#185)
[ ] FusedLocationProviderService should communicate with the LostClientManager using a ~~Messenger~~ AIDL callback interface to report location updates and location availability. (#185)

ecgreb commented 7 years ago

The bulk of the work has now been completed to add multi-process support to Lost. Once #185 is merged support for requesting location updates from a custom process will be available in the latest snapshot.

These changes also declare the FusedLocationProviderService to run in its own custom process. This ensures all communication with the service is IPC compatible and allows us to verify that using all activities in the sample app.

It also sets us up to allow multiple applications to connect to a single instance of Lost in the future. It's possible there is a potential downside here in the way of performance. Are there any other drawbacks to running the fused location service in its own process to consider?

This is not a small set of changes and so some extra sets of 👀 and 👐 reviewing and testing these changes would certainly be appreciated!

zugaldia commented 7 years ago

@sarahlensing @ecgreb Congrats on closing the ticket! This was no small feat.

I see that this has been added to the 2.3 milestone but I can't find any SNAPSHOTs for that version on Sonatype. What would be the best artifact to try out to confirm we no longer see the issue?

ecgreb commented 7 years ago

Version number bumped. v2.3 SNAPSHOT is now available.

https://oss.sonatype.org/content/repositories/snapshots/com/mapzen/android/lost/2.3.0-SNAPSHOT/

Emeritus-DarranKelinske commented 7 years ago

nom nom nom

lostzen / lost