KVO crash related with NSKeyValueObservation

Development Core Libraries
, ,
1

Some days ago I have got crash like this
Environment: iPhone 5s with last iOS 12, Xcode 12. On simulator is likely ok.

This happened when an observed object was deallocated, but NSKeyValueObservation.Helper still alive. In this case, func invalidate() has no any effect.
By following documentation, we must call func removeObserver(_:_:) before a observed object is deallocated, but we don't.

We could be make helper property is weak and wait when Helper object is released to detect deallocation of an observed object. But when an associated object is releasing, an observed object already is nil.
I tried to set unowned(unsafe) reference for an observed object and it really works without crashes. But I don't know how it safely or not.
By making a strong reference on an observed object, we would violate API convention.

 public class NSKeyValueObservation : NSObject {
     private class Helper : NSObject {
-        @nonobjc weak let object : NSObject?
+        @nonobjc unowned(unsafe) let object : NSObject
         ...
+        @nonobjc var isValid: Bool = true

         @nonobjc func invalidate() {
-            guard let object = self.object else { return }
+            guard isValid else { return }
+            isValid = false
             object.removeObserver(self, forKeyPath: path, context: nil)
             objc_setAssociatedObject(object, associationKey(), nil, .OBJC_ASSOCIATION_ASSIGN)
-            self.object = nil
         }

         ...
         
+        deinit {
+            invalidate()
+        }
     }
     
-    @nonobjc private let helper: Helper
+    @nonobjc private weak var helper: Helper?

     @objc public func invalidate() {
-        helper.invalidate()
+        helper?.invalidate()
     }
     
     deinit {
         invalidate()
     }
 }

Also I reviewed SR-6066, and I suppose to replace the runtime casting with compile time version:

-// Used for type-erase Optional type
-private protocol _OptionalForKVO {
-    static func _castForKVO(_ value: Any) -> Any?
-}
-
-extension Optional: _OptionalForKVO {
-    static func _castForKVO(_ value: Any) -> Any? {
-        return value as? Wrapped
-    }
-}

 extension _KeyValueCodingAndObserving {
     public func observe<Value>(
             _ keyPath: KeyPath<Self, Value>,
             options: NSKeyValueObservingOptions = [],
             changeHandler: @escaping (Self, NSKeyValueObservedChange<Value>) -> Void)
         -> NSKeyValueObservation {
         return NSKeyValueObservation(object: self as! NSObject, keyPath: keyPath, options: options) { (obj, change) in
-            
-            let converter = { (changeValue: Any?) -> Value? in
-                if let optionalType = Value.self as? _OptionalForKVO.Type {
-                    // Special logic for keyPath having a optional target value. When the keyPath referencing a nil value, the newValue/oldValue should be in the form .some(nil) instead of .none
-                    // Solve https://bugs.swift.org/browse/SR-6066
-                    
-                    // NSNull is used by KVO to signal that the keyPath value is nil.
-                    // If Value == Optional<T>.self, We will get nil instead of .some(nil) when casting Optional(<null>) directly.
-                    // To fix this behavior, we will eliminate NSNull first, then cast the transformed value.
-                    
-                    if let unwrapped = changeValue {
-                        // We use _castForKVO to cast first.
-                        // If Value != Optional<NSNull>.self, the NSNull value will be eliminated.
-                        let nullEliminatedValue = optionalType._castForKVO(unwrapped) as Any
-                        let transformedOptional: Any? = nullEliminatedValue
-                        return transformedOptional as? Value
-                    }
-                }
-                return changeValue as? Value
-            }
-            
-            let notification = NSKeyValueObservedChange(kind: change.kind,
-                                                        newValue: converter(change.newValue),
-                                                        oldValue: converter(change.oldValue),
-                                                        indexes: change.indexes,
-                                                        isPrior: change.isPrior)
+            let notification = NSKeyValueObservedChange(
+                kind: change.kind,
+                newValue: change.newValue as? Value,
+                oldValue: change.oldValue as? Value,
+                indexes: change.indexes,
+                isPrior: change.isPrior
+            )
+            changeHandler(obj as! Self, notification)
+        }
+    }
+
+    public func observe<T>(
+            _ keyPath: KeyPath<Self, Optional<T>>,
+            options: NSKeyValueObservingOptions = [],
+            changeHandler: @escaping (Self, NSKeyValueObservedChange<Optional<T>>) -> Void)
+        -> NSKeyValueObservation {
+        return NSKeyValueObservation(object: self as! NSObject, keyPath: keyPath, options: options) { (obj, change) in
+            let notification = NSKeyValueObservedChange(
+                kind: change.kind,
+                newValue: change.newValue.map { $0 as? T },
+                oldValue: change.oldValue.map { $0 as? T },
+                indexes: change.indexes,
+                isPrior: change.isPrior
+            )
             changeHandler(obj as! Self, notification)
         }
     }

Will it affect on ABI stability?
Current version also works incorrect, because in the documentation the NSKeyValueObservingOptions.initial event has next description:

The change dictionary in the notification will always contain an NSKeyValueChangeNewKey entry if NSKeyValueObservingOptionNew is also specified but will never contain an NSKeyValueChangeOldKey entry

But in tests we have:
// CHECK-51-NEXT: oldValue = Optional(nil), newValue = Optional(nil)
instead of:
// CHECK-51-NEXT: oldValue = nil, newValue = Optional(nil)
This change would resolve it.

I decided to test all of this on stdlib build.

What I did:

  1. Built the swift project.
  2. Made my changes.
  3. Ran tests with build-script and they passed.

What I can't:

  • Xcode build was stopped with test failing. Ninja is ok.
  • Incremental build through Xcode: when ReportCrash process takes more than 12GB memory, all fails.
  • Use my build of stdlib in a real project with Xcode. I set up SWIFT_EXEC with my swiftc location, Xcode logs show correct location, but runtime behavior stays old. Also, the compiler understands new definitions, but in runtime crash with SIGABRT.
  • Switch scheme release/5.3 through utils/update-checkout, because it crashes in update_checkout.py -> def check_parallel_results -> r.repo_path with undefined property error.