doublesymmetry / react-native-track-player

A fully fledged audio module created for music apps. Provides audio playback, external media controls, background mode and more!
https://rntp.dev/
Apache License 2.0
3.28k stars 1.01k forks source link

[Android] Crash - Not allowed to start service Intent #473

Closed ishigamii closed 3 years ago

ishigamii commented 5 years ago

Configuration

package.json

    "react": "16.3.1",
    "react-native": "0.55.4",
    "react-native-track-player": "^1.1.2",
    "react-native-swift": "^1.2.2",

react-native info

Environment:
  OS: macOS High Sierra 10.13.6
  Node: 8.11.2
  Yarn: 1.9.4
  npm: 5.6.0
  Watchman: 4.9.0
  Xcode: Xcode 10.1 Build version 10B61
  Android Studio: 3.2 AI-181.5540.7.32.5056338

Packages: (wanted => installed)
  react: 16.3.1 => 16.3.1
  react-native: 0.55.4 => 0.55.4

Problem

I am having more and more of the same crash on Android using the version 1.1.2, here is the stack :

Fatal Exception: java.lang.IllegalStateException: Not allowed to start service Intent { cmp=com.xxx/com.guichaguri.trackplayer.service.MusicService }: app is in background uid UidRecord{39a05c9 u0a454 CEM  bg:+1m5s685ms idle procs:1 seq(0,0,0)}
       at android.app.ContextImpl.startServiceCommon(ContextImpl.java:1538)
       at android.app.ContextImpl.startService(ContextImpl.java:1484)
       at android.content.ContextWrapper.startService(ContextWrapper.java:663)
       at android.content.ContextWrapper.startService(ContextWrapper.java:663)
       at com.guichaguri.trackplayer.module.MusicModule.waitForConnection(MusicModule.java:100)
       at com.guichaguri.trackplayer.module.MusicModule.getPosition(MusicModule.java:405)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.facebook.react.bridge.JavaMethodWrapper.invoke(JavaMethodWrapper.java:372)
       at com.facebook.react.bridge.JavaModuleWrapper.invoke(JavaModuleWrapper.java:160)
       at com.facebook.react.bridge.queue.NativeRunnable.run(NativeRunnable.java)
       at android.os.Handler.handleCallback(Handler.java:789)
       at android.os.Handler.dispatchMessage(Handler.java:98)
       at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:29)
       at android.os.Looper.loop(Looper.java:164)
       at com.facebook.react.bridge.queue.MessageQueueThreadImpl$3.run(MessageQueueThreadImpl.java:192)
       at java.lang.Thread.run(Thread.java:764)

service.js

module.exports = async function service() {
  TrackPlayer.addEventListener('remote-play', () => {
    MyPlayer.playTrack(() => {});
  });

  TrackPlayer.addEventListener('remote-pause', () => {
    MyPlayer.pauseTrack(() => {});
  });

  TrackPlayer.addEventListener('remote-seek', (time) => {
    TrackPlayer.seekTo(time.position);
  });

  TrackPlayer.addEventListener('remote-jump-backward', (time) => {
    TrackPlayer.getPosition().then((position) => {
      TrackPlayer.seekTo(position - time.interval);
    });
  });

  TrackPlayer.addEventListener('remote-jump-forward', (time) => {
    TrackPlayer.getPosition().then((position) => {
      TrackPlayer.seekTo(position + time.interval);
    });
  });

  TrackPlayer.addEventListener('remote-stop', () => {
    MyPlayer.stopTrack(() => {});
  });

  TrackPlayer.addEventListener('playback-queue-ended', (infos) => {
    console.log('ended', infos);
    if (infos && infos.track) {
      MyPlayer.stopTrack(() => {});
    }
  });
};

Question

Is this solved in the newest version 1.1.3? Cause I saw a bug correction that looks like this on the last version. ==> It's not solved in 1.1.3

Is there any workaround or maybe some configuration I am missing?

Thanks a lot

Guichaguri commented 5 years ago

This crash can be caused by multiple reasons, please, try 1.1.3 first so we can make sure it's not related to that.

ishigamii commented 5 years ago

@Guichaguri I actually don't reproduce myself but I can get crash from my users 👍 It seems to be what you explain in the other thread that seems to speak about it that's why I was wondering.

ishigamii commented 5 years ago

@Guichaguri Reproduced with latest version 1.1.3 with same exception happening on Android 8 and 9 (65% are from Android 8 and the rest for Android 9)

Fatal Exception: java.lang.IllegalStateException: Not allowed to start service Intent { cmp=com.xxx/com.guichaguri.trackplayer.service.MusicService }: app is in background uid UidRecord{d4e96d5 u0a217 CEM  bg:+1m3s136ms idle procs:1 seq(36,36,36)}
       at android.app.ContextImpl.startServiceCommon(ContextImpl.java:1538)
       at android.app.ContextImpl.startService(ContextImpl.java:1484)
       at android.content.ContextWrapper.startService(ContextWrapper.java:663)
       at android.content.ContextWrapper.startService(ContextWrapper.java:663)
       at com.guichaguri.trackplayer.module.MusicModule.waitForConnection(MusicModule.java:100)
       at com.guichaguri.trackplayer.module.MusicModule.getPosition(MusicModule.java:405)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.facebook.react.bridge.JavaMethodWrapper.invoke(JavaMethodWrapper.java:372)
       at com.facebook.react.bridge.JavaModuleWrapper.invoke(JavaModuleWrapper.java:160)
       at com.facebook.react.bridge.queue.NativeRunnable.run(NativeRunnable.java)
       at android.os.Handler.handleCallback(Handler.java:789)
       at android.os.Handler.dispatchMessage(Handler.java:98)
       at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:29)
       at android.os.Looper.loop(Looper.java:164)
       at com.facebook.react.bridge.queue.MessageQueueThreadImpl$3.run(MessageQueueThreadImpl.java:192)
       at java.lang.Thread.run(Thread.java:764)
AChevallier commented 5 years ago

Hello,

@Guichaguri I do get this error as well on the version 1.1.3.

bemnet4u commented 5 years ago

+1. I also see this but only on android 9.0

curiousdustin commented 5 years ago

I am also seeing this in my Crashlytics and Bugsnag reporting.

Android 8 and 9.

I have not seen it happen myself.

Guichaguri commented 5 years ago

Can I take a look into your playback service code?

ishigamii commented 5 years ago

@Guichaguri I just added it in the first post ;) you are talking about the service.js right?

curiousdustin commented 5 years ago

Mine is a bit more complicated, and doesn't directly call the TrackPlayer methods, but maybe it will be useful.

import Analytics from '../../utils/Analytics';
import { store } from '../../containers/Root';
import TrackPlayer from 'react-native-track-player';
import AudioPlayer, { SIZZLE_ID } from '../AudioPlayer';
import {
  audioPlayerStateChange,
  audioPlayerTrackChange,
  audioPlayerPlay,
  audioPlayerPause,
  audioPlayerJumpForward,
  audioPlayerJumpBack,
  audioPlayerStop,
  audioPlayerPlayNext,
  audioPlayerRemoveQueuedTrack,
  audioPlayerRemoveCurrentQueuedTrack,
  audioPlayerSeek,
  setAudioPlayerIntentToPause
} from '../../actions/audioPlayerActions';
import Device from '../../utils/Device';
import { getAudioPlayerState } from '../../utils/selectors';
import getCurrentlyPlayingTrackFactory from '../../utils/selectors/getCurrentlyPlayingTrackFactory';

let didAddEventListeners = false;
let didPauseTemporarily = false;

let getCurrentlyPlayingTrack = getCurrentlyPlayingTrackFactory();

async function logEventWithCurrentTrackInfo(eventName) {
  let params = {};
  try {
    const currentTrack = getCurrentlyPlayingTrack(store.getState());
    if (currentTrack) {
      params.showname = currentTrack.album;
      params.episodename = currentTrack.title;
    }
  } catch (e) {
    //
  }
  Analytics.logEvent(eventName, params);
}

module.exports = async function() {
  if (didAddEventListeners) {
    return;
  } else {
    didAddEventListeners = true;
  }

  TrackPlayer.addEventListener('remote-play', data => {
    store.dispatch(audioPlayerPlay());
    logEventWithCurrentTrackInfo('remote_play');
  });

  TrackPlayer.addEventListener('remote-pause', data => {
    store.dispatch(audioPlayerPause());
    logEventWithCurrentTrackInfo('remote_pause');
  });

  TrackPlayer.addEventListener('remote-stop', data => {
    store.dispatch(audioPlayerStop());
    logEventWithCurrentTrackInfo('remote_stop');
  });

  TrackPlayer.addEventListener('remote-jump-forward', data => {
    if (data) {
      store.dispatch(audioPlayerJumpForward(data.interval));
      logEventWithCurrentTrackInfo('remote_jumpforward');
    }
  });

  TrackPlayer.addEventListener('remote-jump-backward', data => {
    if (data) {
      store.dispatch(audioPlayerJumpBack(data.interval));
      logEventWithCurrentTrackInfo('remote_jumpback');
    }
  });

  TrackPlayer.addEventListener('remote-seek', data => {
    if (data) {
      store.dispatch(audioPlayerSeek(data.position));
      logEventWithCurrentTrackInfo('remote_seek');
    }
  });

  TrackPlayer.addEventListener('remote-next', data => {
    store.dispatch(audioPlayerJumpForward(AudioPlayer.options.jumpInterval));
    logEventWithCurrentTrackInfo('remote_jumpforward');
  });

  TrackPlayer.addEventListener('remote-previous', data => {
    store.dispatch(audioPlayerJumpBack(AudioPlayer.options.jumpInterval));
    logEventWithCurrentTrackInfo('remote_jumpback');
  });

  TrackPlayer.addEventListener('playback-state', data => {
    if (data) {
      const currentPlayerState = getAudioPlayerState(store.getState());

      let playerState = AudioPlayer.getStateForState(data.state);
      store.dispatch(audioPlayerStateChange(playerState));
      AudioPlayer.monitorProgress();
      if (playerState === AudioPlayer.STATE_PLAYING) {
        AudioPlayer.startMonitoring();
      } else {
        AudioPlayer.stopMonitoring();
      }

      if (playerState === AudioPlayer.STATE_PAUSED) {
        if (currentPlayerState === AudioPlayer.STATE_PLAYING) {
          store.dispatch(setAudioPlayerIntentToPause());
        }
      } else {
        didPauseTemporarily = false;
      }
    }
  });

  TrackPlayer.addEventListener('playback-track-changed', data => {
    if (data) {
      let { track, position, nextTrack } = data;
      if (track && track !== SIZZLE_ID) {
        AudioPlayer.storeTrackProgress(track, position);
      }
      if (nextTrack) {
        store.dispatch(audioPlayerTrackChange(track, position, nextTrack));
      }
    }
  });

  TrackPlayer.addEventListener('playback-queue-ended', data => {
    if (data) {
      let { track, position } = data;
      if (track) {
        if (track === SIZZLE_ID) {
          store.dispatch(audioPlayerStop());
        } else {
          AudioPlayer.storeTrackComplete(track);
          store.dispatch(audioPlayerRemoveQueuedTrack(track));
          store.dispatch(audioPlayerPlayNext(true));
        }
      }
    } else {
      store.dispatch(audioPlayerRemoveCurrentQueuedTrack()).then(() => {
        store.dispatch(audioPlayerPlayNext(true));
      });
    }
  });

  TrackPlayer.addEventListener('playback-error', data => {
    store.dispatch(audioPlayerRemoveCurrentQueuedTrack()).then(() => {
      store.dispatch(audioPlayerPlayNext(true));
    });
  });

  if (Device.isAndroid) {
    TrackPlayer.addEventListener('remote-duck', data => {
      let {
        paused: shouldPause,
        ducking: shouldDuck,
        permanent: permanentLoss
      } = data;

      if (shouldPause || shouldDuck) {
        store.dispatch(audioPlayerPause());
        didPauseTemporarily = !permanentLoss;
      } else if (didPauseTemporarily) {
        didPauseTemporarily = false;
        store.dispatch(audioPlayerPlay());
      }
    });

    TrackPlayer.addEventListener('remote-play-id', data => {});

    TrackPlayer.addEventListener('remote-play-search', data => {});

    TrackPlayer.addEventListener('remote-skip', data => {});

    TrackPlayer.addEventListener('remote-set-rating', data => {});
  }
};
ishigamii commented 5 years ago

@Guichaguri any more informations thanks to that or you need more ? 👍

Guichaguri commented 5 years ago

I think that's good, I'll take a better look in the weekend.

ishigamii commented 5 years ago

@Guichaguri tell us if we can help ;)

ishigamii commented 5 years ago

@curiousdustin did you find a workaround ?

curiousdustin commented 5 years ago

I haven't gotten around to investigating the cause yet. Like you, @ishigamii , I am getting these crash reports, but I have not experienced the issue in my own testing yet.

ishigamii commented 5 years ago

@curiousdustin yop sadly can't reproduce this yet too :/ have to try on android 9 cause i have a user telling me it stops instantly going background on Android 9

ishigamii commented 5 years ago

@Guichaguri any news on this ?

Guichaguri commented 5 years ago

Not only it's pretty hard to reproduce, but also I don't have the reports as you do. This is one of those issues where in the best case scenario we have to keep trying changing stuff until it's fixed.

ishigamii commented 5 years ago

yes it's pretty hard to reproduce to be honest just got it for the first time on debug phone After closing the app the player notification was still here and tapping on the notification got me this crash :

Fatal Exception: android.app.RemoteServiceException
Bad notification for startForeground: java.lang.RuntimeException: invalid channel for service notification: null
android.app.ActivityThread.main

Hope it can help 👍

Guichaguri commented 5 years ago

Which Android version were you testing on?

ishigamii commented 5 years ago

@Guichaguri here is an approximate list of crash android version we can resume that most of the crash happens with Android 8 and 9 like @curiousdustin said :

Capture d’écran 2019-05-01 à 16 42 23
minhtc commented 5 years ago

yes it's pretty hard to reproduce to be honest just got it for the first time on debug phone After closing the app the player notification was still here and tapping on the notification got me this crash :

Fatal Exception: android.app.RemoteServiceException
Bad notification for startForeground: java.lang.RuntimeException: invalid channel for service notification: null
android.app.ActivityThread.main

Hope it can help 👍

Try this: https://github.com/react-native-kit/react-native-track-player/pull/579/files

skleest commented 5 years ago

+1 very hard to reproduce, but seeing it on crash reports (bugsnag). might be crashing in the background? Android OS notified me that our app has been 'crashing frequently', although I haven't seen it crash personally.

EmaEvidence commented 5 years ago

I have a similar problem on crashlytics, I have not reproduced it while testing on devices though but my users experienced it. image

antontanderup commented 5 years ago

We're seeing this crash in crashlytics also. Just had this happen on my phone. Happened right after a long track finished (+10min), app in background, screen off. The crash is not visible to the user, but next track is not started either (playback-queue-ended is probably not sent).

curiousdustin commented 5 years ago

Has anyone tried @minhtc's solution yet? I'll be looking at this closer next week.

curiousdustin commented 5 years ago

@minhtc , to clarify, does your PR address the original error posted in this thread, or just the error that happens when tapping the notification after this crash?

curiousdustin commented 5 years ago

@Guichaguri , what is the downside to switching from startService() to startForegroundService() as recommended here: https://developer.android.com/about/versions/oreo/background.html#migration?

391

bemnet4u commented 5 years ago

@curiousdustin I published a new version of my app with @minhtc's fix and will see happens. Since I don't have local repro of this, i will have to wait for my customers to upgrade this, and repro this crash.

minhtc commented 5 years ago

@minhtc , to clarify, does your PR address the original error posted in this thread, or just the error that happens when tapping the notification after this crash?

My PR fix bug "Bad notification for startForeground: java.lang.RuntimeException: invalid channel for service notification: null" does not fix bug "Not allowed to start service Intent".

bemnet4u commented 5 years ago

Looks like @minhtc your fix is working. I don't see the invalid channel error anymore. I have 51% of my DAU using the code that has this fix and I don't see this crash. I however still see the not allowed to start service with this stack


Fatal Exception: java.lang.IllegalStateException: Not allowed to start service Intent { cmp=com.shegerapps.amharicradio/com.guichaguri.trackplayer.service.MusicService }: app is in background uid UidRecord{13e4d4a u0a217 TPSL idle procs:1 proclist:17174, seq(0,0,0)}
       at android.app.ContextImpl.startServiceCommon(ContextImpl.java:1666)
       at android.app.ContextImpl.startService(ContextImpl.java:1611)
       at android.content.ContextWrapper.startService(ContextWrapper.java:677)
       at android.content.ContextWrapper.startService(ContextWrapper.java:677)
       at com.guichaguri.trackplayer.module.MusicModule.waitForConnection(Unknown Source:29)
       at com.guichaguri.trackplayer.module.MusicModule.getState(Unknown Source:5)
       at java.lang.reflect.Method.invoke(Method.java)
       at com.facebook.react.bridge.JavaMethodWrapper.invoke(Unknown Source:148)
       at com.facebook.react.bridge.JavaModuleWrapper.invoke(Unknown Source:21)
       at com.facebook.react.bridge.queue.NativeRunnable.run(Unknown Source)
       at android.os.Handler.handleCallback(Handler.java:873)
       at android.os.Handler.dispatchMessage(Handler.java:99)
       at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(Unknown Source)
       at android.os.Looper.loop(Looper.java:214)
       at com.facebook.react.bridge.queue.MessageQueueThreadImpl$4.run(Unknown Source:37)
       at java.lang.Thread.run(Thread.java:764)```
puckey commented 5 years ago

Note sure if this is of help, but in this related issue in the google issue tracker they advise the following workaround:

There is a workaround to avoid application crash. 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.

puckey commented 5 years ago

For me, these crashes are occurring when TrackPlayer functions are called while the app is in the background and the MusicService service is no longer running.. For example, I track what is being listened to every 5 minutes for analytics purposes.

I am unable to reproduce the crash on my test devices (which are Google Pixel phones), but also see them happening (a lot) in the wild. Mainly on samsung devices.

As a temporary stopgap, I am going to see if using the following function to check if the service is running before calling any other TrackPlayer functions in the background fixes the crashes I am experiencing:

    @ReactMethod
    public void serviceIsRunning(final Promise promise) {
        ReactApplicationContext context = getReactApplicationContext();
        ActivityManager manager = (ActivityManager) context.getSystemService(Context.ACTIVITY_SERVICE);
        for (RunningServiceInfo serviceInfo : manager.getRunningServices(Integer.MAX_VALUE)) {
          if (serviceInfo.service.getClassName().equals(MusicService.class.getName())) {
              promise.resolve(true);
              return;
          }
        }
        promise.resolve(false);
    }
biomancer commented 5 years ago

@puckey I see that https://developer.android.com/reference/android/app/ActivityManager#getRunningServices(int) is deprecated, and maybe we can use simpler check for that binder is not null? There also seems to be a problem with that waitForConnection is designed to start service if it was not running and if we check for serviceIsRunning before all TrackPlayer functions that may be called in background, we change behaviour to never start service if it was not already running - would it be always fine while app is in background? Or are there some cases when service should be started?

puckey commented 5 years ago

@puckey I see that https://developer.android.com/reference/android/app/ActivityManager#getRunningServices(int) is deprecated, and maybe we can use simpler check for that binder is not null? There also seems to be a problem with that waitForConnection is designed to start service if it was not running and if we check for serviceIsRunning before all TrackPlayer functions that may be called in background, we change behaviour to never start service if it was not already running - would it be always fine while app is in background? Or are there some cases when service should be started?

Good point! Changed it to

    @ReactMethod
    public void isServiceConnected(final Promise promise) {
        promise.resolve(binder != null);
    }

I am adding this to all places where I am calling TrackPlayer functionality in the background. Haven't added it to things like TrackPlayer.addEventListener('remote-pause', () => TrackPlayer.pause());, because the fact that TrackPlayer can send the event means it is running.

aclement455 commented 5 years ago

As anyone tried this solution yet?

I have a widget which does relatively frequent updates when the device is awake and I was seeing thousands of crashes in just a few days.

The issue trigger

I even noticed the issue even on my Pixel 3 XL when I wouldn't have thought the device to have much load at all. And any and all code paths were covered with startForeground(). But then I realized that in many cases my service gets the job done really quickly. I believe the trigger for my app was that the service was finishing before the system actually got around to showing a notification.

The workaround/solution

I was able to get rid of all crashes. What I did was to remove the call to stopSelf(). (I was thinking about delaying the stop until I was pretty sure the notification was shown, but I don't want the user to see the notification if it isn't necessary.) When the service has been idle for a minute or the system destroys it normally without throwing any exceptions.

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
    stopForeground(true);
} else {
    stopSelf();
}

https://stackoverflow.com/questions/44425584/context-startforegroundservice-did-not-then-call-service-startforeground/53402038#53402038

biomancer commented 5 years ago

I see related changes in this pr https://github.com/react-native-kit/react-native-track-player/pull/466, @Guichaguri maybe it's easier to use startForegroundService() instead of startService() as curiousdustin suggested above, then calling startForeground and only then stopForeground if we do not need permanent notification? Are there some cons in that approach?

aclement455 commented 5 years ago

@Guichaguri FWIW, I got a reproducible scenario on my phone (Android 9, Galaxy Note 8). If I start to build the application and I locked my phone until the build is completely done and the application is started. I get the error mentioned above

Not allowed to start service Intent { cmp=com.xxx/com.guichaguri.trackplayer.service.MusicService }: app is in background uid UidRecord{...}

This happens all the time if my phone is locked while the application starts.

bemnet4u commented 5 years ago

@aclement-clevertech I was not able to repro this on emulator. Was this repro only possible on the emulator too or only physical device?

aclement455 commented 5 years ago

@bemnet4u I reproduced it on both physical and emulator device, both are running Android 9, I will try to provide a small project that manages to reproduce it.

bemnet4u commented 5 years ago

@aclement-clevertech I am able to repro this now with locked screen. I had to change my app to play audio on startup to cause this to happen. I changed this code in MusicService and it seams to solve the issue.

I also want to note that when the crash happened, it didn't actually affect the app no crash dialog was shown to the user.

// context.startService(intent); ContextCompat.startForegroundService(context, intent);

bemnet4u commented 5 years ago

Also I pushed all "fixes" i made here https://github.com/bemnet4u/react-native-track-player/commits/bemnet4u. None of them mine originally so not sending a pull request... posting here if anyone is interested https://github.com/bemnet4u/react-native-track-player/commits/bemnet4u

biomancer commented 5 years ago

@bemnet4u @curiousdustin I think I understood why just using startForegroundService won't work - using it requires us to call startForeground within 5 seconds, or app will crash with Context.startForegroundService() did not then call Service.startForeground() error.

Calling startForeground requires notification to be shown, and currently player is designed to call it only when playback begins, so if you do not start playing music within 5 seconds after service start, app will crash - that happens now if we just change startService to startForegroundService.

So using startForegroundService would require us to show some notification after service is started, and at that time we have no current track, etc, so it may be only some blank / generic notification, that will look ugly.

But I think we can call stopForeground(true) right after notification is created - actually I've tried that here https://github.com/perushevandkhmelev-agency/react-native-track-player/commit/b1d50dc20ccc1bf5ce662e7a1673f8c6c074ba6d - and it seems to work, but I have tested it only briefly. I'm also not sure that creating separate channel for this setup notification is ok there, but at a first glance it works, and probably is better than using playback notification channel as it may have lower importance or some other settings that fit better for this specific case. I've tried using code for creation blank notification from onStartForeground(), but it was failing with "invalid channel for service notification" on my android 9 device, so I'm not sure if that code is valid or if I was using it incorrectly somehow.

And last thing - actually calling stopForeground(true) may be optional there, as it's called anyway in updateNotification() as audio session is not active on start, but calling it right after notification is created should remove it a bit faster.

upd @bemnet4u looking through your commits and I see that we've actually implemented a very similar fix :)

upd2 I've also refactored a bit creating these blank notifications, inspired by your code https://github.com/perushevandkhmelev-agency/react-native-track-player/commit/13ed1186dd3d3dc6bbf6990704e59a2970277b74

biomancer commented 5 years ago

But anyway even if that helps not to crash, we need that isServiceConnected or similar method, as sometimes we would not want to start a service if it was not running already

puckey commented 5 years ago

@biomancer Have you tested this with users in the wild yet? If @Guichaguri thinks this looks like a good fix, I am happy to do a limited release to a few thousand of my users to see if it brings their ANR rate down.

biomancer commented 5 years ago

I've found one more issue related to this service stopping - after restarting you get corrupted playback notification without any actions. I have not seen that issue before as app was just crashing right after service was stopped 😅 And so after fixes that we are discussing here, that issue has showed up.

I've created a separate pr here https://github.com/react-native-kit/react-native-track-player/pull/619 and also applied it in my branch with other fixes, including https://github.com/react-native-kit/react-native-track-player/issues/607 that is also realted to service being stopped (but my branch is based on 1.13 as 1.14 seems to have many issues on ios and I did not have time to catch up with changes and test it).

@puckey I'll release build with these fixes to production too as it looks stable, will try to do it today or tomorrow.

biomancer commented 5 years ago

One thing that I did not get - what is the purpose of onStartForeground() method? I see that it's called when handling media button action, but I do not get why we need to stop service via stopSelf(); if was not in foreground. Could somebody please explain it?

Maybe we should call startForeground there unconditionally? Why is there a check for being in background and presence of react context?

The0racle commented 5 years ago

@biomancer did you see a decrease in crashes after releasing to production?

biomancer commented 5 years ago

@The0racle Yes, crash rate decreased significantly and I do not see this particular crash anymore, but some issues are still left - I get reports that for example users get crash when they start an app and then switch to another app without playing anything (crash is happening after some time passes, probably when background service is stopped, and sometimes when app is reopened after that). Maybe it's not related to this issue, but I'm worried about that media button handling and that it is probably needs to be updated too https://github.com/react-native-kit/react-native-track-player/issues/473#issuecomment-499966313

@dcvz @Guichaguri guys maybe you could explain briefly the idea behind that starting and stopping a service on media button intent in MusicService? When is that called (I guess on some media button events?) and why service needs to be stopped? I've tried sending media button keyevents but it looks that handling them does not touch that code, at least on android 9. If service is running, it's handled in ExoPlayback, and if it's not, then event is just ignored. Upd. I've got it - it's called when you press actions in notification. Will try to understand next if that should be updated somehow or is fine even with new way to start service.

Also I think that my fix may be not entirely correct in some way - I guess getPosition and other 'query' methods should actually never start background service, but should just resolve to null or something like that. Current implementation may result in bad behaviour if you call these methods from background without checking if service is running - it will just start the service when it's not running and there is no point to do that if nothing is playing, will just drain the battery probably.

biomancer commented 5 years ago

Ok, I've finally understood now that onStartCommand is called not only when service starts, but also when we communicate with running service via intent, such as actions from notification (android development is not my field of expertise as you can understand... 😬). But all that startForeground stuff should be called just once on service start, so we should move it from onStartCommand to onCreate that is called once on service creation. I've tried it here https://github.com/perushevandkhmelev-agency/react-native-track-player/commits/edge and it seems to work fine, will test it in production.

biomancer commented 5 years ago

I've also found reason for "Module AppRegistry is not a registered callable module (calling startHeadlessTask)" crash I've been seeing in reports for a long time - see https://github.com/perushevandkhmelev-agency/react-native-track-player/commit/ee0de65e5b8ec85cf1ec47a7db1f247967ebb799. START_STICKY caused cases when service is started by android automatically (it was possible to reproduce it on some devices by restarting an app multiple times, while other devices never did it, so it was not easy to catch), when there is no react context yet, no MusicModule initialized, etc - and when our code tries to register playback service, app crashes with that error.

I'm not sure that it's entirely ok to do that - maybe we should keep it sticky and stop if there is not react context present, but I currently see no reason to restart that service - it will not continue playback by itself anyway, so what's the point? It will be started when needed from the app as I understand it. Would be nice if someone could confirm that this is sane change, I'd create a PR then.