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

Fix service manager crash #5

Merged
merged 2 commits into from
May 2, 2024
Merged

Conversation

carriehall
Copy link

Potentially fixes crash: https://console.firebase.google.com/u/0/project/td-project-01/crashlytics/app/android:com.todoist/issues/5c94bfaef8b88c29631d145f?time=last-ninety-days&sessionEventKey=661EFB9F0356000131FD3657D1CE98D6_1937057838887752195

Not reproducible, but I suspect the flow is something like this:

  • App not running in background
  • User receives notification about a new comment
  • User taps notification to deep link into item details > comment list
  • App crashes

According to this issue, the activity might not be "fully awake" so they'll be paused immediately and then resumed.

Applications can get the process state in Activity.onResume() by calling ActivityManager.getRunningAppProcesses() and avoid starting Service if the importance level is lower than ActivityManager.RunningAppProcessInfo.IMPORTANCE_FOREGROUND. If the device hasn’t fully awake, activities would be paused immediately and eventually be resumed again after its fully awake.

I've added logs to debug this and I can see that when you go through this flow, the service is started, paused, and started again, although in my testing the app does reach the correct foreground state (importance = 100) so this PR is still a guess about a fix.

@carriehall carriehall requested review from a team and rastislavvasko and removed request for a team April 17, 2024 11:00
Copy link
Member

@rastislavvasko rastislavvasko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay! I'm unsure if we should just silently ignore this, because it could mean that the audio won't play but we won't know about it. I.e. should we at least log an event?

ActivityManager activityManager = (ActivityManager) mActivity.getSystemService(Context.ACTIVITY_SERVICE);
List<ActivityManager.RunningAppProcessInfo> runningAppProcesses = activityManager.getRunningAppProcesses();
if (runningAppProcesses != null) {
int importance = runningAppProcesses.get(0).importance;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we find our process or are we certain it's always the first one?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be the first one as that's the foreground one. I've added that in the failure callback though so we can log it, just in case there's something else amiss.

@carriehall
Copy link
Author

Sorry for the delay! I'm unsure if we should just silently ignore this, because it could mean that the audio won't play but we won't know about it. I.e. should we at least log an event?

Unfortunately we don't have Datadog or Logger in AudioPlayerRecorder. I've update the callbacks so that we can log from the AttachmentDelegate. If we log the fails and successful starts we can verify if the issue was the same (i.e. we get a start/stop/start) and I'll remove the logs again if so.

Copy link
Member

@rastislavvasko rastislavvasko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Let's try it out!

@carriehall carriehall force-pushed the carrie/fix-service-manager-crash branch from b984ac1 to a717977 Compare May 2, 2024 14:13
@carriehall carriehall merged commit 3d51954 into main May 2, 2024
1 check passed
@rastislavvasko rastislavvasko deleted the carrie/fix-service-manager-crash branch May 2, 2024 21:50
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

Successfully merging this pull request may close these issues.

2 participants