Dealloc in background - updateTracksForPlayer: not call on main thread

[pyby]

Issue type

Incorrect behavior

Description of the problem

Some logs in our CI during tests reported an assertion: [SRGMediaPlayerController updateTracksForPlayer:] not call on main thread.

On our CI, we saw in logs that on Letterbox-Apple CI, it's due to a MAKVONotificationCenter deallocation from a background thread. It's an assertion from updateTracksForPlayer: method on SRGMediaController.

Environment information

• Library version: 7.0.2 and above
• iOS / tvOS version: 15.5
• Device: Any

Reproducibility

Update: Always reproducible

Was Difficult to reproduce

eps to reproduce

Dealloc a controller in a background thread.

[pyby]

Thanks to @defagos , we can reproduce the issue easy with Letterbox Demo and the "feeds" view.

We confirmed that an SRGRequest can be cancel in background and MAKVONotificationCenter sends deallocation in a background thread.

[pyby]

SRGMediaPlayer contrat is to publish notifications on the main thread. Playback state, subtitles change, audio selection are usually matched for the UI.

Isolate [SRGMediaPlayerController updateTracksForPlayer:] and [SRGMediaPlayerController setPlaybackState:withUserInfo:] calls on main thread was a first step.

But in [SRGMediaPlayerController stopWithUserInfo:releasePlayer:], the player is also set to nil, and it has to be done on main thread for SRGMediaPlayerView. I got warnings (playerPlayer access in background), thanks to Xcode diagnosing Memory, Thread.

So we should call [SRGMediaPlayerController stopWithUserInfo:releasePlayer:] always on main thread.

On feature/player-dealloc branch, proposition can be discussed.

[defagos]

I don't know whether the trick you propose is safe or not, without further investigations I really cannot say. What I am quite certain of, though, is that we should avoid this kind of trick at deallocation time for the sake of eliminating an assertion, except if you are totally confident this is a safe.

If not then I would rather remove the assertion and do better in the future. We know we don't have any major issue with the current code in production (the assertion is stripped out from release builds after all), but I don't know what might happen if we release this kind of fix without being sure it will not be a problem when deployed to hundreds of thousands devices.

[defagos]

When running the UTs a deadlock might sometimes occur. I suspect this is related to the use of dispatch_sync and is likely a sign we should probably avoid this trick.

[defagos]

Assertions were simply removed in the end.