PierfrancescoSoffritti / android-youtube-player

YouTube Player library for Android and Chromecast, stable and customizable.
https://pierfrancescosoffritti.github.io/android-youtube-player/
MIT License
3.44k stars 765 forks source link

Leaking issue in LegacyYouTubePlayerView #1058

Open hoangthan opened 1 year ago

hoangthan commented 1 year ago

Bug Report

Leaking issue in LegacyYouTubePlayerView

Description of the Bug

In init block of LegacyYouTubePlayerView. You're adding the listener to NetworkBroadcastReceiver

networkObserver.listeners.add(object : NetworkObserver.Listener {
      override fun onNetworkAvailable() {
        if (!isYouTubePlayerReady) {
          initialize()
        }
        else {
          playbackResumer.resume(webViewYouTubePlayer.youtubePlayer)
        }
      }

      override fun onNetworkUnavailable() { }
    })

It will fine if the view is bound with a lifecycle like fragment/activity. But in case the LegacyYouTubePlayerView is put on custom view which has no default lifecycle --> The listener will be leaked.

Recommendation

Moving the block adding the network state listener from init to view.doOnAttach { //Add network listener } And remove it when view is going on detached by view.onViewDetached { //Remove network listener }

This will make all the user consistence and no need to care about the lifecycle of view.

Youtube Player Library Version: 10.0.5 or 0.23 (for chromecast)

PierfrancescoSoffritti commented 1 year ago

Have you trying calling YouTubePlayerView #release? That is the intended usage when the view is not attached to a lifecycle. See doc.

hoangthan commented 1 year ago

@PierfrancescoSoffritti Yes I know we should the release function to release the resource once the view is going on destroyed. Since you're currently implementing LifeCycle to listen to the state of lifeCycleOwner to release resources once it going onDestroyed (if has any). So why do you make it run automatically to make the developer can avoid the leaking issue if forgets to bind it with the view lifecycleOwner ? And the last one, if we put the YoutubePlayerView in a custom view, we still need call the release function when view is being on detached. Why don't you put it onDetached of YoutubePlayerView itself ?

PierfrancescoSoffritti commented 1 year ago

Ok, that seems reasonable. Feel free to send a pull request if you have time :)

hoangthan commented 1 year ago

Hi @PierfrancescoSoffritti I've submitted a PR: https://github.com/PierfrancescoSoffritti/android-youtube-player/pull/1059