Prevents the app from crashing when the foreground service fails to start #19987
Conversation
…vice fails to start
antonis
changed the title
|
 |
App Name |  WordPress |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr19987-861cdb4 | |
| Commit | 861cdb4 | |
| Direct Download | wordpress-prototype-build-pr19987-861cdb4.apk |
|
 |
App Name |  Jetpack |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr19987-861cdb4 | |
| Commit | 861cdb4 | |
| Direct Download | jetpack-prototype-build-pr19987-861cdb4.apk |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## trunk #19987 +/- ##
==========================================
- Coverage 40.45% 40.45% -0.01%
==========================================
Files 1440 1440
Lines 66518 66521 +3
Branches 10959 10959
==========================================
Hits 26910 26910
- Misses 37121 37124 +3
Partials 2487 2487 ☔ View full report in Codecov by Sentry. |
Generated by 🚫 dangerJS |
| try { | ||
| mService.startForeground(sNotificationData.mNotificationId, mNotificationBuilder.build()); | ||
| } catch (RuntimeException exception) { | ||
| AppLog.e(T.POSTS, "startOrUpdateForegroundNotification failed; See issue #18714", exception); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
Most related crash stack traces seem to go through the specific line.
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 callingstartForegoundServicewould 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
startForegroundassuming 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!
There was a problem hiding this comment.
As mentioned here: #19987 (comment) I still have some concerns about this not getting completely rid of crashes in the background/foreground service flow, but this might be a start towards a solution, or at least a better understanding of the problem based on possible follow up issues/crashes in this flow.
That said, I will approve it. Thanks for the changes.
|
Thank you for reviewing and your feedback @thomashorta 🙇
This makes sense. I'll keep an eye on this during the beta period to catch any related crashes. |
Partially/Temporarily #18714
Description
This PR catches and logs the Runtime exception (ForegroundServiceStartNotAllowedException) thrown at the PostUploadNotifier when the service fails to start on Android 12+
Internal ref: pcdRpT-5hm-p2#comment-8537
Note: This is a temporary solution till we upgrade to WorkManager as part of the Android 14 Target SDK Update work (internal ref: pcdRpT-5fy-p2#comment-8380)
To Test:
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR Submission Checklist:
RELEASE-NOTES.txtif necessary.UI Changes Testing Checklist: