17

I get random crashes (which I can't reproduce on devices I own) in my app with exception:

Cannot remove an observer Foundation.NSKeyValueObservation 0xaddress for the key path "readyForDisplay" from AVPlayerLayer 0xaddress because it is not registered as an observer.

This happens when I deallocate a UIView which contains AVPlayerLayer.

My init:

private var playerLayer : AVPlayerLayer { return self.layer as! AVPlayerLayer }

init(withURL url : URL) {
    ...
    self.asset = AVURLAsset(url: url)
    self.playerItem = AVPlayerItem(asset: self.asset)
    self.avPlayer = AVPlayer(playerItem: self.playerItem)
    super.init(frame: .zero)
    ...
    let avPlayerLayerIsReadyForDisplayObs = self.playerLayer.observe(\AVPlayerLayer.isReadyForDisplay, options: [.new]) { [weak self] (plLayer, change) in ... }
    self.kvoPlayerObservers = [..., avPlayerLayerIsReadyForDisplayObs, ...]
    ...
    }

My deinit where exception is thrown:

deinit {
    self.kvoPlayerObservers.forEach { $0.invalidate() }
    ...
    NotificationCenter.default.removeObserver(self)
}

According to Crashlytics it happens on iOS 11.4.1 on different iPhones.

The code leading to deinit is pretty simple:

// Some UIViewController context.
self.viewWithAVLayer?.removeFromSuperview()
self.viewWithAVLayer = nil

I would appreciate any thoughts on why this happens.

I have seen this bug but it doesn't seem to be the cause for me.

EDIT 1:

Additional info for posterity. On iOS 10 if I don't invalidate I get reproducible crash on deinit. On iOS 11 it works without invalidation (not checked yet if crash disappears if I don't invalidate and let observers to be deinited with my class).

EDIT 2:

Additional info for posterity: I have also found this Swift bug which might be related - SR-6795.

iur
  • 2,056
  • 2
  • 13
  • 30
  • Register and unregister in the main thread. And remove the observation from the array once you're done (Array.removeAll should work). – Pranav Kasetti Sep 30 '18 at 12:55
  • @PranavKasetti I register only in `init(withURL:` and unregister only in `deinit`. I assume that it happens on main thread since it is a UIView subclass and I don't touch it in other threads. This also means that if I created view not thru `init(withURL:` there would be no observers to invalidate. – iur Oct 01 '18 at 14:43

2 Answers2

13

After

self.kvoPlayerObservers.forEach { $0.invalidate() }

Add

self.kvoPlayerObservers.removeAll()

Also I don’t like this line:

self.kvoPlayerObservers = [..., avPlayerLayerIsReadyForDisplayObs, ...]

kvoPlayerObservers should be a Set and you should insert observers one by one as you receive them.

matt
  • 515,959
  • 87
  • 875
  • 1,141
  • Thanks for checking this question. I'll try this solution. I have thought that since it happens in `deinit` I don't have to explicitly remove them. Also what difference does it make to add them immediately or a couple of lines later? I agree that Set might be a better container for them than Array but I don't see why it would crash with Array in this particular case. – iur Oct 01 '18 at 14:40
  • I don’t know if it will solve anything, so if not, we can investigate further. – matt Oct 01 '18 at 14:46
  • Unfortunately that is the worst part of it, I can only check if it works by releasing the new app version, since I can't reproduce it on devices I have. Will keep you posted though. – iur Oct 01 '18 at 14:53
  • Have you checked that `deinit` is actually being called? Also, is there a possibility of a threading issue? – matt Oct 01 '18 at 15:45
  • Yes, `deinit` is definitely called, as I have added in my edit. I will double check threading issues, but there should be none since I tried to be very careful with dispatching all UI work to main thread. – iur Oct 01 '18 at 16:36
  • All UI work, maybe, but what about observer management and whatever causes this view controller's deinit to be called? Whenever users get a bug but you don't, you should worry about threads... – matt Oct 01 '18 at 16:52
  • I don't understand how you can meaningfully set a bounty when you have no way of knowing whether an answer is helpful or not (because you can't reproduce the issue to begin with). – matt Oct 06 '18 at 18:19
  • Well, I think after a couple of weeks in production without crashes related to that place I can confirm that your answer was helpful. I will add my own answer with detailed description of what I did if someone is interested. – iur Oct 18 '18 at 11:37
0

I have accepted matt's answer but I want to provide more info on how I actually tackled my problem.

My deinit that doesn't crash looks like this:

if let exception = tryBlock({ // tryBlock is Obj-C exception catcher.
        self.kvoPlayerObservers.forEach { $0.invalidate() };
        self.kvoPlayerObservers.removeAll()
}) {
    remoteLoggingSolution.write(exception.description)
}
... // do other unrelated stuff

Basically I try to catch Obj-C exception if it occurs and try to log it remotely.

I have this code in production for past 2 weeks and since then I didn't receive neither crashes nor exception logs so I assume matt's proposal to add kvoPlayerObservers.removeAll() was correct (at least for my particular case).

iur
  • 2,056
  • 2
  • 13
  • 30