fix(android): avoid service crashes #1974

puckey · 2023-03-23T21:15:07Z

Based on @kyunkakata’s work at #1666 (comment)

@kyunkakata

Based on @kyunkakata’s work at doublesymmetry#1666 (comment)

android/src/main/java/com/doublesymmetry/trackplayer/service/MusicService.kt

as per doublesymmetry#1974 (comment)

…reground.

android/src/main/java/com/doublesymmetry/trackplayer/module/MusicModule.kt

puckey · 2023-03-24T11:42:41Z

@kyunkakata did you figure out a way to consistently cause these crashes to happen? I would like to test to make sure everything is fixed now..

kyunkakata · 2023-03-24T12:56:51Z

The crash "Context.startForegroundService() did not then call Service.startForeground() within 5s" is easy to reproduce. Just calling setupPlayer without invoking add function. An ANR or a crash should happen.
The second one - "#1666" is diffcult to reproduce. Base on our crash logs, our users that received this type of crash is when the app was in the background and received a push notification, then the crash happens. We tried to reproduce but got no luck. So that we need to check is app in the foreground to start MusicService and is that a MusicService notification or not to ensure that the crash will not happen.

since it is never mutated..

puckey · 2023-03-24T20:07:39Z

The crash "Context.startForegroundService() did not then call Service.startForeground() within 5s" is easy to reproduce. Just calling setupPlayer without invoking add function. An ANR or a crash should happen.

Just to make sure: did the fake_audio_channel notification code in your diff fix this issue? Would it stop the anr from happening when you don't call add() quickly enough?

puckey · 2023-03-25T14:26:58Z

The crash "Context.startForegroundService() did not then call Service.startForeground() within 5s" is easy to reproduce. Just calling setupPlayer without invoking add function. An ANR or a crash should happen.

Just to make sure: did the fake_audio_channel notification code in your diff fix this issue? Would it stop the anr from happening when you don't call add() quickly enough?

So I added back the ANR fix, since it is necessary when you don't want to start the exoplayer notification within 5 seconds of starting MusicService. I also reject the setupPlayer promise with error code android_cannot_setup_player_in_background and have adjusted the example app implementation to retry if it failed in this way.

@kyunkakata any feedback is welcome!

android/src/main/java/com/doublesymmetry/trackplayer/service/MusicService.kt

example/src/App.tsx

dcvz

Clever workaround! There are some typescript issues but this is looking good to go.

Thanks @puckey and @kyunkakata

example/src/App.tsx

puckey · 2023-03-27T09:55:41Z

Fixed the ts issues, so should be good to go

- reject promise when calling setupPlayer in the background - in demo app retry setupPlayer while it fails due to being in the background - Add back and extract ANR fix - Fix R import - Remove unnecessary foreground check in NotificationState.POSTED - use DefaultLifecycleObserver over deprecated LifecycleObserver

MortadhaFadhlaoui · 2023-03-28T20:51:43Z

Nice Work! Thank you everyone for the fix, but we still experience the issue, due to this line

 is NotificationState.POSTED -> {
                            startForeground(it.notificationId, it.notification)
                    }

maybe this fix should be included

MortadhaFadhlaoui · 2023-03-29T21:25:44Z

android/src/main/java/com/doublesymmetry/trackplayer/service/MusicService.kt

-                        if (AppForegroundTracker.foregrounded && it.notificationId == MUSIC_NOTIFICATION_ID) {
-                            startForeground(it.notificationId, it.notification)
-                        }
+                        startForeground(it.notificationId, it.notification)


This line causing a crash as I mentioned here, @puckey wdyt about this quick fix

if (Build.VERSION.SDK_INT < Build.VERSION_CODES.S) { startForeground(it.notificationId, it.notification) }

@MortadhaFadhlaoui Could you open a new pr for this with some explanation on your changes? Thanks!

martinmidtsund · 2023-03-31T19:25:04Z

I'm not entirely sure the service should be foregrounded immediately after the notification is posted. Could this, together with START_STICKY which restarts the service if killed be the reason for the mAllowForeground error? In the Pocket Casts repo they only use setForeground when they start playing sound. https://github.com/Automattic/pocket-casts-android/blob/ee8da0c095560ef64a82d3a31464491b8d713104/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackService.kt#L250

@kyunkakata

* fix(android): avoid service crashes Based on @kyunkakata’s work at doublesymmetry#1666 (comment) * fix(android): reject setupPlayer if backgrounded * chore(android): Remove superfluous fix as per doublesymmetry#1974 (comment) * fix(android): When backgrounded, call setupPlayer when it comes to foreground. * chore(android): remove more remnants of superfluous fix * chore(android): remove unused imports * chore(android): inline MusicModule#eventHandler since it is never mutated.. * chore(android): simplify readableArrayToTrackList * chore(android): remove unnecessary try catch from setupPlayer * chore(android): refactor MusicModule#getConstants() * fix(android): add back anr fix and reject promise when in bg - reject promise when calling setupPlayer in the background - in demo app retry setupPlayer while it fails due to being in the background - Add back and extract ANR fix - Fix R import - Remove unnecessary foreground check in NotificationState.POSTED - use DefaultLifecycleObserver over deprecated LifecycleObserver * chore(docs): background error thrown in setupPlayer

@kyunkakata

* fix(android): avoid service crashes Based on @kyunkakata’s work at doublesymmetry#1666 (comment) * fix(android): reject setupPlayer if backgrounded * chore(android): Remove superfluous fix as per doublesymmetry#1974 (comment) * fix(android): When backgrounded, call setupPlayer when it comes to foreground. * chore(android): remove more remnants of superfluous fix * chore(android): remove unused imports * chore(android): inline MusicModule#eventHandler since it is never mutated.. * chore(android): simplify readableArrayToTrackList * chore(android): remove unnecessary try catch from setupPlayer * chore(android): refactor MusicModule#getConstants() * fix(android): add back anr fix and reject promise when in bg - reject promise when calling setupPlayer in the background - in demo app retry setupPlayer while it fails due to being in the background - Add back and extract ANR fix - Fix R import - Remove unnecessary foreground check in NotificationState.POSTED - use DefaultLifecycleObserver over deprecated LifecycleObserver * chore(docs): background error thrown in setupPlayer

jspizziri · 2023-04-06T23:54:51Z

@martinmidtsund care to submit a PR or test your theory on a fork?

martinmidtsund · 2023-04-11T09:11:57Z

@jspizziri Yes, I'll be testing this and can submit a PR. Can't set a timeline though, don't know when I will get the time.

puckey · 2023-04-12T12:38:06Z

I'm not entirely sure the service should be foregrounded immediately after the notification is posted. Could this, together with START_STICKY which restarts the service if killed be the reason for the mAllowForeground error? In the Pocket Casts repo they only use setForeground when they start playing sound. https://github.com/Automattic/pocket-casts-android/blob/ee8da0c095560ef64a82d3a31464491b8d713104/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackService.kt#L250

I wonder though – isn't there a big chance the app will be in the background already by the time the sound starts playing? i.e. the users changes the track and moves to another app directly

martinmidtsund · 2023-04-12T12:48:41Z

I wonder though – isn't there a big chance the app will be in the background already by the time the sound starts playing? i.e. the users changes the track and moves to another app directly

@puckey Yes, but don't you think in those cases the mAllowForeground is true, not false? It should be allowed to start a foreground service from the foreground, and from a notification, if I'm not mistaken. This has to be tested of course.

puckey · 2023-04-12T12:52:59Z

@martinmidtsund shall we regroup on this with a fresh issue containing stack traces as experienced using the latest rc release? Would you mind opening one?

martinmidtsund · 2023-04-12T13:49:59Z

@puckey Sorry, we are not using v4 yet, so we don't have any stack traces as of now. I could create a new issue with the stack traces we do have, but they are from v3.2.0. Not sure if that helps, but I would guess it's still the same problem in the new RCs?

@kyunkakata

* fix(android): avoid service crashes Based on @kyunkakata’s work at doublesymmetry/react-native-track-player#1666 (comment) * fix(android): reject setupPlayer if backgrounded * chore(android): Remove superfluous fix as per doublesymmetry/react-native-track-player#1974 (comment) * fix(android): When backgrounded, call setupPlayer when it comes to foreground. * chore(android): remove more remnants of superfluous fix * chore(android): remove unused imports * chore(android): inline MusicModule#eventHandler since it is never mutated.. * chore(android): simplify readableArrayToTrackList * chore(android): remove unnecessary try catch from setupPlayer * chore(android): refactor MusicModule#getConstants() * fix(android): add back anr fix and reject promise when in bg - reject promise when calling setupPlayer in the background - in demo app retry setupPlayer while it fails due to being in the background - Add back and extract ANR fix - Fix R import - Remove unnecessary foreground check in NotificationState.POSTED - use DefaultLifecycleObserver over deprecated LifecycleObserver * chore(docs): background error thrown in setupPlayer

puckey mentioned this pull request Mar 23, 2023

Service.startForeground() not allowed due to mAllowStartForeground false #1666

Closed

fix(android): avoid service crashes

8eae14d

Based on @kyunkakata’s work at doublesymmetry#1666 (comment)

puckey force-pushed the fix/service-crashes branch from ec55c89 to 8eae14d Compare March 23, 2023 21:18

martinmidtsund reviewed Mar 24, 2023

View reviewed changes

android/src/main/java/com/doublesymmetry/trackplayer/service/MusicService.kt Outdated Show resolved Hide resolved

puckey added 2 commits March 24, 2023 10:35

fix(android): reject setupPlayer if backgrounded

e0b7513

chore(android): Remove superfluous fix

bec8d24

as per doublesymmetry#1974 (comment)

puckey force-pushed the fix/service-crashes branch from 07095e3 to 209fb15 Compare March 24, 2023 10:36

fix(android): When backgrounded, call setupPlayer when it comes to fo…

ef8f162

…reground.

puckey force-pushed the fix/service-crashes branch from 209fb15 to ef8f162 Compare March 24, 2023 11:13

puckey marked this pull request as ready for review March 24, 2023 11:18

puckey commented Mar 24, 2023

View reviewed changes

android/src/main/java/com/doublesymmetry/trackplayer/module/MusicModule.kt Outdated Show resolved Hide resolved

chore(android): remove more remnants of superfluous fix

6f51889

puckey force-pushed the fix/service-crashes branch from 8054f2a to 6f51889 Compare March 24, 2023 11:40

chore(android): remove unused imports

6899f94

puckey added 3 commits March 24, 2023 15:18

chore(android): inline MusicModule#eventHandler

6a404a9

since it is never mutated..

chore(android): simplify readableArrayToTrackList

42c2ecb

chore(android): remove unnecessary try catch from setupPlayer

ea096b1

puckey marked this pull request as draft March 24, 2023 20:08

chore(android): refactor MusicModule#getConstants()

a932fb9

puckey marked this pull request as ready for review March 25, 2023 14:30

puckey requested a review from dcvz as a code owner March 25, 2023 14:30

jspizziri reviewed Mar 25, 2023

View reviewed changes

android/src/main/java/com/doublesymmetry/trackplayer/service/MusicService.kt Outdated Show resolved Hide resolved

jspizziri reviewed Mar 25, 2023

View reviewed changes

example/src/App.tsx Show resolved Hide resolved

dcvz reviewed Mar 27, 2023

View reviewed changes

example/src/App.tsx Outdated Show resolved Hide resolved

puckey force-pushed the fix/service-crashes branch from 2ac70da to b6a4b9e Compare March 27, 2023 09:53

puckey force-pushed the fix/service-crashes branch from b6a4b9e to 2efefc6 Compare March 27, 2023 09:56

chore(docs): background error thrown in setupPlayer

b68c620

dcvz merged commit b70fbd7 into doublesymmetry:main Mar 27, 2023

MortadhaFadhlaoui reviewed Mar 29, 2023

View reviewed changes

mitchdowney mentioned this pull request Apr 25, 2023

[4.0.0-rc04] Crash: Unable to start service com.doublesymmetry.trackplayer.service.MusicService@78647c9 with null #1986

Closed

lovegaoshi mentioned this pull request Apr 1, 2024

Unable to start service MusicService with null: android.app.ForegroundServiceStartNotAllowedException: Service.startForeground() not allowed due to mAllowStartForeground false #2244

Open

lovegaoshi mentioned this pull request May 24, 2024

Fatal Exception: android.app.RemoteServiceException$ForegroundServiceDidNotStartInTimeException TheWidlarzGroup/react-native-video#3834

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(android): avoid service crashes #1974

fix(android): avoid service crashes #1974

puckey commented Mar 23, 2023

puckey commented Mar 24, 2023

kyunkakata commented Mar 24, 2023

puckey commented Mar 24, 2023

puckey commented Mar 25, 2023

dcvz left a comment

puckey commented Mar 27, 2023

MortadhaFadhlaoui commented Mar 28, 2023 •

edited

Loading

MortadhaFadhlaoui Mar 29, 2023

puckey Apr 12, 2023

martinmidtsund commented Mar 31, 2023

jspizziri commented Apr 6, 2023

martinmidtsund commented Apr 11, 2023

puckey commented Apr 12, 2023 •

edited

Loading

martinmidtsund commented Apr 12, 2023

puckey commented Apr 12, 2023

martinmidtsund commented Apr 12, 2023

fix(android): avoid service crashes #1974

fix(android): avoid service crashes #1974

Conversation

puckey commented Mar 23, 2023

puckey commented Mar 24, 2023

kyunkakata commented Mar 24, 2023

puckey commented Mar 24, 2023

puckey commented Mar 25, 2023

dcvz left a comment

Choose a reason for hiding this comment

puckey commented Mar 27, 2023

MortadhaFadhlaoui commented Mar 28, 2023 • edited Loading

MortadhaFadhlaoui Mar 29, 2023

Choose a reason for hiding this comment

puckey Apr 12, 2023

Choose a reason for hiding this comment

martinmidtsund commented Mar 31, 2023

jspizziri commented Apr 6, 2023

martinmidtsund commented Apr 11, 2023

puckey commented Apr 12, 2023 • edited Loading

martinmidtsund commented Apr 12, 2023

puckey commented Apr 12, 2023

martinmidtsund commented Apr 12, 2023

MortadhaFadhlaoui commented Mar 28, 2023 •

edited

Loading

puckey commented Apr 12, 2023 •

edited

Loading