Reactivecocoa: Crash during runloop's autorelease pool drain on macOS 10.15 Catalina

[iby]

Experiencing a naughty crash with buttons and all inheriting classes inside NSStackView with enabled Detaches Hidden Views when the window gets closed and deallocated in the latest Catalina build:

2019-10-06 15:04:27.548688+0100 App[4474:58829] [general] Caught exception during runloop's autorelease pool drain of client objects NSRangeException: Cannot remove an observer <NSStackView 0x10461aca0> for the key path "hidden" from <NSButton 0x10461c7f0> because it is not registered as an observer. userInfo: (null)
2019-10-06 15:04:27.550252+0100 App[4474:58829] [General] An uncaught exception was raised
2019-10-06 15:04:27.550313+0100 App[4474:58829] [General] Cannot remove an observer <NSStackView 0x10461aca0> for the key path "hidden" from <NSButton 0x10461c7f0> because it is not registered as an observer.
    0   CoreFoundation                      0x00007fff36166d63 __exceptionPreprocess + 250
    1   libobjc.A.dylib                     0x00007fff6bf56bd4 objc_exception_throw + 48
    2   Foundation                          0x00007fff387a7395 -[NSObject(NSKeyValueObserverRegistration) _removeObserver:forProperty:] + 578
    3   Foundation                          0x00007fff387a7106 -[NSObject(NSKeyValueObserverRegistration) removeObserver:forKeyPath:] + 74
    4   Foundation                          0x00007fff387bf963 -[NSObject(NSKeyValueObserverRegistration) removeObserver:forKeyPath:context:] + 190
    5   AppKit                              0x00007fff334e0cc9 -[NSStackView dealloc] + 233
    6   CoreFoundation                      0x00007fff360c5176 -[__NSArrayI dealloc] + 73
    7   libobjc.A.dylib                     0x00007fff6bf6653a _ZN19AutoreleasePoolPage12releaseUntilEPP11objc_object + 134
    8   libobjc.A.dylib                     0x00007fff6bf4cc70 objc_autoreleasePoolPop + 175
    9   CoreFoundation                      0x00007fff3608767e _CFAutoreleasePoolPop + 22
    10  CoreFoundation                      0x00007fff360b59fc __CFRunLoopRun + 2404
    11  CoreFoundation                      0x00007fff360b4e13 CFRunLoopRunSpecific + 499
    12  HIToolbox                           0x00007fff34c41b2d RunCurrentEventLoopInMode + 292
    13  HIToolbox                           0x00007fff34c4186d ReceiveNextEventCommon + 600
    14  HIToolbox                           0x00007fff34c415f7 _BlockUntilNextEventMatchingListInModeWithFilter + 64
    15  AppKit                              0x00007fff332eaac4 _DPSNextEvent + 990
    16  AppKit                              0x00007fff332e9834 -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 1352
    17  AppKit                              0x00007fff332e3fd4 -[NSApplication run] + 658
    18  AppKit                              0x00007fff332d5e7d NSApplicationMain + 777

As far as I can tell this has been happening since Catalina Beta was released and still occurs on the latest GM seed, which is worrying… Builds produced with the latest stable Xcode and Xcode 11.2 Beta suffer from the same problem. Everything pre-Catalina works 100% fine.

All of the above narrows down to ReactiveCocoa bindings. Removing the following observeValues line fixes the issue:

override func viewDidLoad() {
     self.launchOnStartupCheckbox.reactive.boolValues.observeValues({ Swift.print($0) })
}

It seems like there's something wrong with ObjC swizzling that ReactiveCocoa does behind the scenes? It also looks there're some observer-related issues, is this something related or already known? Otherwise happy to try isolating the issue into a sample project, but there's really not much details besides that: NSStackView with enabled Detaches Hidden Views + NSButton + reactive observer on button's IBOutlet = 💥 on window deallocation!

[iby]

With Catalina out in the wild could you comment on the above?

[ohbargain]

I hit this problem. @ianbytchek were you able to work around this problem?

[iby]

@ohbargain I disabled Detaches Hidden Views on all stack views where it wasn't needed, apparently that's a good thing anyway, i.e., less observers = less resources. In my case this wasn't needed in over 95% cases. For the rest I reverted to @IBActions in favour of reactive observers for the simplicity sake. But you can also wrap these controls into another view and hide it instead, so the NSStackView would observe the wrapper and the ReactiveCocoa would swizzle the control itself without causing a conflict between the two.

[andersio]

This is quite an interesting issue, because RAC would not have swizzled things that affect KVO observer registration. 🤔

[iby]

Doesn't seem to be happening any longer on latest Catalina 10.15.4. I encountered a few other notorious AppKit crashes on 10.15, so this probably was one of these internal oversights. Feel free to close.

[iby]

Sorry, false negative. I must have tested a piece without any bindings. Still occurs on 10.15.4.

This is quite an interesting issue, because RAC would not have swizzled things that affect KVO observer registration. 🤔

@andersio It is interesting, indeed. Since ReactiveCocoa does some swizzling and things start to work with literally commenting a single line where it hooks into NSObject, I'd say this a good indication there's a problem with the way it does it. There's probably a very subtle change in Catalina, that affects the behaviour, it might be the super invocation order or something less trivial. I can only guess.

[iby]

@andersio FYI, you seem to have already worked on the related #3433 issue a few years back and fixed it in #3435. Does that ring the bell? I tried poking the codebase but I feel completely out of context with the swizzling part… :(

[iby]

Another interesting fact. If I understand correctly ReactiveCocoa uses different swizzling for different things. So, depending on that and on the observation order it shows different results. There are cases when turning off hidden view detaching doesn't even work.

extension Reactive where Base: NSView {
    /// Returns a signal for the given key path using native AppKit block observation.
    internal func _signal<U>(`for` keyPath: KeyPath<Base, U>) -> Signal<Bool, Never> {
        Signal({ rxObserver, lifetime in
            let nsObserver = self.base.observe(keyPath, changeHandler: { r, _ in rxObserver.send(value: r.isHidden) })
            lifetime.observeEnded({ let _ = nsObserver }) // Retain observer for the lifetime of signal.
            lifetime += self.lifetime.observeEnded(rxObserver.sendCompleted) // Complete signal when the object released.
        })
    }
}

autoreleasepool {
    let w = NSWindow()
    let s = NSStackView()
    let b = NSButton()
    s.addArrangedSubview(b)
    w.contentView?.addSubview(s)

    // Single observer: 👍
    // s.detachesHiddenViews = true
    // b.reactive.signal(for: \.isHidden).observeValues({ print("isHidden:", $0) })

    // Single observer: 💣
    // s.detachesHiddenViews = true
    // b.reactive.boolValues.observeValues({ print("boolValue:", $0) })

    // Single observer: 👍
    // s.detachesHiddenViews = false
    // b.reactive.boolValues.observeValues({ print("boolValue:", $0) })

    // Multiple observers: 👍
    // s.detachesHiddenViews = false
    // b.reactive.boolValues.observeValues({ print("boolValue:", $0) })
    // b.reactive.signal(for: \.isHidden).observeValues({ print("isHidden:", $0) })

    // 🔥🔥🔥 Same as above, different order: 💣
    s.detachesHiddenViews = false
    b.reactive.signal(for: \.isHidden).observeValues({ print("isHidden:", $0) })
    b.reactive.boolValues.observeValues({ print("boolValue:", $0) })

    // Using custom observer: 👍
    // s.detachesHiddenViews = false
    // b.reactive._signal(for: \.isHidden).observeValues({ print("isHidden:", $0) })
    // b.reactive.boolValues.observeValues({ print("boolValue:", $0) })

    b.performClick(nil)
    b.performClick(nil)
    b.isHidden = true
    b.isHidden = false
}

[iby]

😅 Using custom _signal signal extension leads to a whole different crash.

#0  0x00007fff689ff8fe in object_isClass ()
#1  0x00007fff322b8348 in KVO_IS_RETAINING_ALL_OBSERVERS_OF_THIS_OBJECT_IF_IT_CRASHES_AN_OBSERVER_WAS_OVERRELEASED_OR_SMASHED ()
#2  0x00007fff32456301 in NSKeyValueWillChangeWithPerThreadPendingNotifications ()
#3  0x00007fff2cf02f35 in -[NSView _viewDidChangeAppearance:] ()
#4  0x00007fff2cf0312f in -[NSView _viewDidChangeAppearance:] ()
#5  0x00007fff2ceeaf8c in -[NSView initWithCoder:] ()
#6  0x00007fff2cf3f9f7 in -[NSStackView initWithCoder:] ()
#7  0x00007fff322a2ae0 in _decodeObjectBinary ()
#8  0x00007fff322a1f40 in _decodeObject ()
#9  0x00007fff322a1dd4 in -[NSKeyedUnarchiver decodeObjectForKey:] ()
#10 0x00007fff2ceeb32b in -[NSResponder initWithCoder:] ()
#11 0x00007fff2cee9b47 in -[NSView initWithCoder:] ()
#12 0x00007fff2cf410a6 in -[NSControl initWithCoder:] ()
#13 0x00007fff2cf40fe6 in -[NSButton initWithCoder:] ()
#14 0x00000001002e20fe in BooleanSetting.init(coder:)
…

This is a random crash and doesn't necessarily happen in the same place, occurs when loading a view from a storyboard and creating fresh binding for the 3rd-4th time. I'm guessing it has to do with retaining nsObserver and releasing it at around the same time when the base object gets deallocated.

Outside-the-box thinking led me to an "elegant" hack of creating signals in the order that doesn't lead to crash, even when I don't need them, i.e., place let _ = self.reactive.boolValues before any key path signals.

[andersio]

Could you share at which point you are creating the bindings, and what specifically have you used (RAC KVO only, or RAC method interception)? It feels like there is a possibility of NSSecureCoding checks having kicked in, when the instance was swizzled before the NSSecureCoding process is completed.

On paper though, crashing at this symbol likely means we have undeteched observers that weren't covered by 10.13+ KVO auto-removal. 🤔

[iby]

Of course! The original crash with extra cases is fully covered in the comment above, it can be copy-pasted in the ReactiveCocoa AppKit playground. I didn't find a way to show full debug info inside the playground, but you can observe the same issue in NSbuttonSpec, which currently fails as mentioned by @simba909. These bindings are made inside viewDidLoad() in NSViewController and inside awakeFromNib in NSView.

As for the KVO_IS_RETAINING_ALL_…_OBSERVER_WAS_OVERRELEASED_OR_SMASHED crash, it's not very clear how much of it is caused by ReactiveCocoa. It doesn't occur every single time and the stack trace isn't always the same, sometimes containing ReactiveCocoa/Swift frames, sometimes not. However:

Signal({ rxObserver, lifetime in
    let nsObserver = self.base.observe(keyPath, changeHandler: { r, _ in rxObserver.send(value: r.isHidden) })
    // 👉 If inside here I retain the self (+base), the crash stops happening, but this leads to
    // 👉 a retain cycle unless the disposable is explicitly destroyed.
    lifetime.observeEnded({ let _ = (nsObserver, self) }) // Retain observer for the lifetime of signal.
    lifetime += self.lifetime.observeEnded(rxObserver.sendCompleted) // Complete signal when the object released.
})

If I understand correctly, the …OBSERVER_WAS_OVERRELEASED_OR_SMASHED is caused by unbalanced observers, i.e., removing an observer that was never added. Though, NSKeyValueObservation is supposed to handle everything automatically behind the scenes it also uses swizzling, which might be conflicting with what ReactiveCocoa does. I would also make these bindings inside viewDidLoad() in NSViewController and inside awakeFromNib in NSView. If I can be useful with hunting this down, please do tell!

[andersio]

@ianbytchek This looks like isa-swizzling RAC did is intefering with some assumptions made by NSStackView or KVO internals in Catalina. Have to look deeper into how this can be resolved.

[andersio]

It seems the issue is around that we isa-swizzle the class after the internal NSKeyValueProperty instance was created, which had cached the isa pointer to the original NSButton class. But then at removal time, it was perhaps attempting to match the observer using the new isa pointer to our swizzled class.

:|

They have likely tightened how KVO detects/tolerates isa swizzling.

[iby]

Interesting! Does it mean that removing the view from the NSStackView, adding reactive observers, and adding it to view might solve the issue? Or, alternatively, swizzling the view at the init stage to achieve the same?

[andersio]

Make sure RAC swizzling has happened, e.g. by calling _ = button.reactive.states, before adding any NSButton to a NSStackView would likely workaround the issue reliably.

[andersio]

Making sure all RAC bindings (that swizzle) having been setup before adding the NSViews to a NSStackView would workaround the issue reliably.

[andersio]

Alright, I managed to figure out the likely cause. RAC deliberately avoided this situation by not isa-swizzling if KVO has isa-swizzled already (so we don't subclass the KVO NSKVONotifying_ runtime subclass).

That's said Catalina appears to have broken it because object_getClass is now conceiving NSKVONotifying_ sometimes, when it had been reflecting the isa honestly before 10.15 Catalina AFAIK. So we stopped detecting KVO influence.

---

Heh, it is getting more weird. NSStackView adds an KVO observer to the NSButton during addArrangedSubview, but object_getClass reports no isa-swizzling. Neither does addObserver. But if I add a KVO observer via the Swift KeyPath API manually, it reports the NSKVONotifying_ subclass ~as expected~ _sometimes_. 🤷‍♂️

Also, multiple samples of AppKit classes exhibit such behavior, while Swift NSObject subclasses are seemingly unaffected. 🤦

[andersio]

Since it affects only macOS Catalina for now, the fix I have in mind would be the RAC internal ActionProxy swizzling the NSControl directly, instead of isa-swizzling. That hopefully would solve the issue around using with NSStackView, and should not change the behavior of APIs depending on ActionProxy.

Having that said, this might reemerge with method wiretapping with trigger(for:) and signal(for:), unfortunately. Changing them not to do isa-swizzling would require a RAC major release with a precaution notice, because we can no longer guarantee that the method invocation callout happens after the actuall call (including the recursive super calls), hence potentially leading to unintended behavioral change. :(

[iby]

Thanks for digging into this @andersio. I use trigger(for:) and signal(for:) a lot and concerned whether this solution is swapping one issue for another?

Having that said, this might reemerge with method wiretapping with trigger(for:) and signal(for:), unfortunately. Changing them not to do isa-swizzling would require a RAC major release with a precaution notice, because we can no longer guarantee that the method invocation callout happens after the actuall call (including the recursive super calls), hence potentially leading to unintended behavioral change. :(

If that happens, would it be because of some other code potentially swizzling the "already directly swizzled NSControl", i.e., it's my responsibility that nothing messes with it, or can there be other situations that lead to problems? In other words, what are the real life consequences of this?

[andersio]

@ianbytchek The issue stands on its own in an environment having only RAC. So I am fairly confident it is not a user-end error.

We probably also need field reports/tests on how RAC is doing with iOS 14 and macOS 11 as min deployment, since both contain some new Objective-C runtime changes.

[iby]

Thanks for the update @andersio! Not sure if I follow, though. Are you saying this is more of an implementation problem/challenge, and not something for the end user to be concerned about?

We probably also need field reports/tests on how RAC is doing with iOS 14 and macOS 11 as min deployment, since both contain some new Objective-C runtime changes.

True. If I come across anything else I'll certainly let know.