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.

Sign up for GitHub

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

Prevents the app from crashing when the foreground service fails to start #19987

Merged
merged 1 commit into from
Jan 22, 2024
Merged

Prevents the app from crashing when the foreground service fails to start #19987

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadNotifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,11 @@ private synchronized void startOrUpdateForegroundNotification(@Nullable PostImmu
updateNotificationBuilder(post);
if (sNotificationData.mNotificationId == 0) {
sNotificationData.mNotificationId = (new Random()).nextInt();
mService.startForeground(sNotificationData.mNotificationId, mNotificationBuilder.build());
try {
mService.startForeground(sNotificationData.mNotificationId, mNotificationBuilder.build());
} catch (RuntimeException exception) {
AppLog.e(T.POSTS, "startOrUpdateForegroundNotification failed; See issue #18714", exception);
}
Comment on lines +138 to +142
Copy link
Contributor

Choose a reason for hiding this comment

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

I won't be able to test this today, but wanted to ask if you were able to consistently reproduce the issue and test this workaround since I am not 100% sure what happens now when we start the services from the background.

Something else I noticed is that we never explicitly call startForegoundService for starting services that are promoted to the foreground, but I believe that's required (we simply call startService everywhere). Not sure if that could also be related to the crashes we see.

Anyway, my point is that I don't know if the app will simply crash at some other point if the service starts but fail to call startForeground. Maybe we might also need to stop the service on the catch clause here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your feedback @thomashorta 🙇

I won't be able to test this today, but wanted to ask if you were able to consistently reproduce the issue and test this workaround since I am not 100% sure what happens now when we start the services from the background.

I was able to reproduce the crash a couple of times and verified that it was user facing (crash popup). I wasn’t able to find specific consistent steps though (maybe due to the many exemptions) :(
Most related crash stack traces seem to go through the specific line.
Screenshot 2024-01-22 at 11 03 28 AM
I’ll have a 2nd look at this and iterate back if I manage to reproduce consistently.

Something else I noticed is that we never explicitly call startForegoundService for starting services that are promoted to the foreground, but I believe that's required (we simply call startService everywhere). Not sure if that could also be related to the crashes we see.

True. I think in all cases we use Context#startService(Intent). I'm not sure if explicitly calling startForegoundService would make a difference tbh.

Anyway, my point is that I don't know if the app will simply crash at some other point if the service starts but fail to call startForeground. Maybe we might also need to stop the service on the catch clause here.

That's indeed a valid concern. My reasoning behind this (temporary) workaround was to catch just the exception thrown from the startForeground assuming that running background service could continue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Most related crash stack traces seem to go through the specific line.
Screenshot 2024-01-22 at 11 03 28 AM I’ll have a 2nd look at this and iterate back if I manage to reproduce consistently.

Yeah, that's indeed the case, so I think that will get rid of the crash we see. My concern is that another crash might pop up anyway at other service-related place.

True. I think in all cases we use Context#startService(Intent). I'm not sure if explicitly calling startForegoundService would make a difference tbh.

I agree! I'm not entirely sure what startForegroundService does TBH, I think it might just set some flag in the system saying that we will call startForeground soon and if we don't it throws an Exception, but maybe just using startService works as well 🤷🏼‍♂️

That's indeed a valid concern. My reasoning behind this (temporary) workaround was to catch just the exception thrown from the startForeground assuming that running background service could continue.

I don't think the service will be able to run properly in the background since background services are very limited in regards to execution time. I guess that some other crash might pop up, but to be honest, I think this fix is a start towards a solution, and if another crash pops up we will have more details about what could be going on exactly.

That said, I will go ahead and approve this PR!

} else {
// service was already started, let's just modify the notification
doNotify(sNotificationData.mNotificationId, mNotificationBuilder.build(), null);
Expand Down
Loading