Prevents the app from crashing when the ReaderCommentService fails to start

antonis commented 1 month ago

Fixes #18666

Description

This PR prevents the app from crashing when the ReaderCommentService for comment snippet fails to start with a BackgroundServiceStartNotAllowedException/IllegalStateException and logs the caught exception.

Exception android.app.BackgroundServiceStartNotAllowedException:
    at android.app.ContextImpl.startServiceCommon (ContextImpl.java:1981)
    at android.app.ContextImpl.startService (ContextImpl.java:1927)
    at android.content.ContextWrapper.startService (ContextWrapper.java:834)
    at org.wordpress.android.ui.reader.services.comment.ReaderCommentService.startServiceForCommentSnippet (ReaderCommentService.java:83)
    at org.wordpress.android.ui.reader.services.comment.wrapper.ReaderCommentServiceStarterWrapper.startServiceForCommentSnippet (ReaderCommentServiceStarterWrapper.kt:11)

Note that a wider refactoring is needed to tackle the root of this issue (see https://github.com/wordpress-mobile/WordPress-Android/issues/18666#issuecomment-1690415225) but given that it is the 3rd crash in number of events/users for the last (24.5) version of the app, handling the crash sounds reasonable.

To Test:

I was not able to reproduce this crash thus I'd recommend a sanity of the functionality:

On the Reader tab
Tap on a post
Verify that the post details with the comments loads

Regression Notes

Potential unintended areas of impact
- Reader post comment
What I did to test those areas of impact (or what existing automated tests I relied on)
- Manual testing
What automated tests I added (or what prevented me from doing so)
- The specific code is hard to test without major refactoring

PR Submission Checklist:

[ ] I have completed the Regression Notes.
[ ] I have considered adding accessibility improvements for my changes.
[ ] I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing Checklist (strike-out the not-applying and unnecessary ones):

[ ] WordPress.com sites and self-hosted Jetpack sites.
[ ] Portrait and landscape orientations.
[ ] Light and dark modes.
[ ] Fonts: Larger, smaller and bold text.
[ ] High contrast.
[ ] Talkback.
[ ] Languages with large words or with letters/accents not frequently used in English.
[ ] Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
[ ] Large and small screen sizes. (Tablet and smaller phones)
[ ] Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)

sonarcloud[bot] commented 1 month ago

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

wpmobilebot commented 1 month ago

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr20636-f9674c5
	Commit	f9674c504af65b0de855c53626a63d5fabc78b57
	Direct Download	`jetpack-prototype-build-pr20636-f9674c5.apk`

Note: Google Login is not supported on these builds.

wpmobilebot commented 1 month ago

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr20636-f9674c5
	Commit	f9674c504af65b0de855c53626a63d5fabc78b57
	Direct Download	`wordpress-prototype-build-pr20636-f9674c5.apk`

Note: Google Login is not supported on these builds.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 40.46%. Comparing base (0384227) to head (f9674c5). Report is 6 commits behind head on release/24.7.

Files	Patch %	Lines
.../reader/services/comment/ReaderCommentService.java	0.00%	4 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## release/24.7 #20636 +/- ## ================================================ - Coverage 40.46% 40.46% -0.01% ================================================ Files 1484 1484 Lines 68413 68416 +3 Branches 11307 11307 ================================================ Hits 27683 27683 - Misses 38230 38233 +3 Partials 2500 2500 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dangermattic commented 4 weeks ago

	1 Warning
:warning:	This PR is assigned to the milestone 24.7 ❄️. The due date for this milestone has already passed. Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by :no_entry_sign: Danger

antonis commented 4 weeks ago

I've changed the target to 24.7 since this came up in the release rotation (ref pcdRpT-6oc-p2#comment-9821) and the code changes should not have side effects.

sentry-io[bot] commented 3 weeks ago

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ BackgroundServiceStartNotAllowedException: Not allowed to start service Intent { cmp=com.jetpack.android/org.wordpress.android.ui.reader.ser... org.wordpress.android.ui.reader.services.commen... View Issue
‼️ IllegalStateException: Not allowed to start service Intent { cmp=com.jetpack.android/org.wordpress.android.ui.reader.ser... org.wordpress.android.ui.reader.services.commen... View Issue

_{Did you find this useful? React with a 👍 or 👎}

wordpress-mobile / WordPress-Android