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

[native code] foreground service is killed and never restarted #1042

Open
shankari opened this issue Jan 22, 2024 · 5 comments
Open

[native code] foreground service is killed and never restarted #1042

shankari opened this issue Jan 22, 2024 · 5 comments

Comments

@shankari
Copy link
Contributor

I have noticed this multiple times on my phone recently. The foreground service is killed, so we don't get fine-grained locations, and is never restarted. I assume that the background services are also killed, so we don't even get batched callbacks.

Note that, since Android 12+, apps cannot start foreground services while the app is running in the background
https://developer.android.com/develop/background-work/services/foreground-services
UNLESS "The user turns off battery optimizations for your app."

That's why we now have users turn off battery optimizations for the app during installation. However, on my phone:

  • the foreground service was not started over multiple days
  • the foreground service was not started even when I launched the app activity, multiple times

I finally had to turn tracking off and on for it to actually start.

We need to look at the code that checks and restarts the foreground service
https://github.com/e-mission/e-mission-data-collection/blob/103b787db3347254488083714d178624ec76092a/src/android/location/TripDiaryStateMachineReceiver.java#L210
and:

  1. make sure that it runs when the app is launched (e.g. onStart and onResume)
  2. see if the function is launched in the background (add additional logs if needed)
  3. check to see if the foreground service is actually created when the function is launched in the background

(it might help to manually kill the foreground service during testing - see how I did this earlier
#838 (comment))

I have filed an internal issue with the logs.

@louisg1337
Copy link

Status Update
Just wanted to post a status update as to what my findings/theories are so far. I've spent a good amount of time playing around with the emulator testing the different scenarios above and this is what I found.

1. make sure that it runs when the app is launched (e.g. onStart and onResume)
At least on the emulator that I am running, each time I killed the foreground service and restarted the app, the foreground service always seemed to come back on after about 30 seconds.

2. see if the function is launched in the background (add additional logs if needed)
This one was a bit tricky for me. Looking at the logs, it seems that performPeriodicActivity, which is what triggers checkForegroundNotification, did occasionally get called. However, during 1/20 - 1/21 (the days with no foreground activity) this function never appeared in the logs.

3. check to see if the foreground service is actually created when the function is launched in the background
It appears that we are able to launch a foreground service in the background. To test this I killed the foreground service via adb (referenced the 838 comment), and then I sent a custom broadcast intent to TripDiaryStateMachineReceiver to call performPeriodicActivity. I did this entirely while the app was in the background with no foreground notification, and the foreground activity came back online.

Thoughts so far
I initially was inclined to say the performPeriodicActivity was broken as this is what should be getting called in the background to ensure the foreground service is alive. Before fully blaming this though, I definitely need a bit more help in understanding how this works in terms of how frequently this should get called, how exactly it gets called, etc.

What is stumping me, however, is why reopening the app did not trigger the foreground service to restart. Based on the logs, it still seems like it is tracking and/or detecting movement as LocationChangeIntentService and ActivityRecognitionChangeIntentService still show up with location points (during our 1/20 and 1/21 period). Maybe it is possible that we are stuck spinning our wheels in some state which prevents the foreground service to run. Throughout most of 1/20 we get this log...

"BuiltinUserCache : Considering transition 1.705768675052E9: {""currState"":""local.state.waiting_for_trip_start"",""transition"":""local.transition.exited_geofence"",""ts"":1.705768675051E9}"

I remember seeing in the code for checkForegroundNotification how changing the state matters when restarting the foreground service...

// It is not enough to move to the foreground; you also need to reinitialize tracking
// https://github.com/e-mission/e-mission-docs/issues/580#issuecomment-700747931
ctxt.sendBroadcast(new ExplicitIntent(ctxt, R.string.transition_initialize));

I'm not sure if this matters or not, but I figured I'd bring it to attention regardless just in case we aforementioned state could be blocking us from launching the foreground service.

Potential Solution
I definitely need to dig into this deeper, but as of right now a potential solution I have is to run performPeriodicActivity or checkForegroundNotification each time the app loads. I think this would fix a lot of our issues in regards to why our foreground service stops. It seems that relying on the server to call performPeriodicActivity or for the foreground service to resume on app load is unreliable. In that regard, calling either of the functions above when the app loads may be a more viable option that we could consider. It is also an user friendly option, as typically if an app doesn't work, I think most people would try to restart it. If we implemented this solution, a quick app restart could fix the foreground service issue. I definitely feel as if there is definitely a better solution out there, but those are just my initial thoughts as of right now. I would love your feedback @shankari in terms of what you think about all of this and potential next steps.

@louisg1337
Copy link

I was investigating into this issue a bit more today, and I ran into something weird. I said above, in response to the first question, that...

1. make sure that it runs when the app is launched (e.g. onStart and onResume)
At least on the emulator that I am running, each time I killed the foreground service and restarted the app, the foreground service always seemed to come back on after about 30 seconds.

Today, for some reason, the foreground service doesn't seem to restart whenever I reload the app. I'm not really quite sure why it doesn't restart, but nonetheless I think this reinforces the idea of the solution I mentioned above, to manually check the foreground notification each time we load the app. I put together a draft pr here where we can see the necessary change that would need to be made. If the code decides to work and the foreground service loads properly, checkForegroundNotification should hopefully detect it has started and back off (pending further testing). On the flip side, if the normal flow doesn't work, it should be able to get the foreground service working again.

I decided to put it in checkLocationSettings as this function gets called whenever a logged in user loads, and as long as the location permissions work we should be all good to start a foreground service. There is a potential issue which is that if a user 1) turns off a (non location) permission and 2) starts the app, a foreground service will still start despite them not being able to fully use the app until they return that permission on. I feel like there is definitely a better place to put this code, but I'm not sure where. I think if there is some centralized function that determines whether or not all permissions are allowed that would probably be the best, I am just not sure if we have something like that.

Regardless, I did some quick tests with this code and it seems to consistently restart the foreground service whenever I kill it. As mentioned above I still need to do more testing and think about this a bit more but I wanted to get it out here. Going forward I also want to play around with the airplane mode issue and see if this solution could potentially fix it as well.

@shankari
Copy link
Contributor Author

@louisg1337 couple of updates from my current trip:

  • this happened again to me this weekend. Uploading screenshots and log to internal PR
  • the airplane mode is a red herring (I took a plane trip today and made sure to double check). The foreground service is not killed when the app is in airplane mode BUT when we leave airplane mode, it does not appear to restart the FSM, at least on my OnePlus. I have created a new internal issue and added logs.

@louisg1337
Copy link

Status Update

I was able to write the code for the changes listed above, and it can be found in this draft PR.

Code

I essentially implemented what was stated above, I put the checkForegroundNotification within the DataCollectionPlugin.onResume(). I decided against putting it in both onStart and onResume, however, because I was worried about the function getting redundantly called twice. Performance wise I feel like this is bad and also having two calls to this function back to back could potentially mess with the FSM a bit, hence why I decided on putting it in only one.

I decided to put it in onResume because I think we are guaranteed to always hit onResume, but not onStart. Based on the diagram, whenever we hit onStart, we subsequently hit onResume, meaning they are almost equivalent for our purposes. However, if we get stuck in a situation such as Activity Running --> onPause --> onResume, we can only hit onResume, and not onStart. For that reason, if we had to choose only one place to put checkForegroundNotification in, I feel like onResume is the better option.

ActivityLifecycleinAndroid-601x660

I could totally be wrong here with my assumptions, and it may not even make a difference if we have 1 or 2 calls, so please let me know about this.

Testing

I tested two different scenarios which were...

  1. Fresh install of app (no opcode, no permissions) and see if the foreground service starts
    Unfortunately the foreground service does start and the state of the FSM is local.state.start. I'm not sure if this is bad practice, starting a foreground service regardless of whether the user has signed in/given permissions. If it is, I can write a bit more code that does permission checks and/or consent checks to ensure that we only call checkForegroundNotification when we know for sure the user is going to use the app.

  2. Test killing foreground service via adb when the app is in the background, and then load the app
    Here we are simulating an user's foreground service randomly getting killed when they aren't using the app. When they open the app back up the new code launches a new foreground service so we are all good with that.

Please let me know @shankari what you think about both issues I talked about above, being the onResume vs onStart debate and the foreground service starting for a brand new user.

@shankari
Copy link
Contributor Author

shankari commented Feb 4, 2024

@louisg1337

  1. for onResume versus onStart, I agree with you that onResume is fine
  2. for the case of the brand new user, I think it would be better if we could wait to start the foreground service until the user has consented so that we don't freak the user out that we are collecting data before consent. There are existing examples on how to do this https://github.com/e-mission/e-mission-data-collection/blob/103b787db3347254488083714d178624ec76092a/src/android/location/TripDiaryStateMachineReceiver.java#L85

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

No branches or pull requests

2 participants