Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SR-6795] Swift 4 KVO swizzling is not thread-safe, causes crashes #3742

Closed
lilyball mannequin opened this issue Jan 19, 2018 · 11 comments
Closed

[SR-6795] Swift 4 KVO swizzling is not thread-safe, causes crashes #3742

lilyball mannequin opened this issue Jan 19, 2018 · 11 comments
Assignees

Comments

@lilyball
Copy link
Mannequin

lilyball mannequin commented Jan 19, 2018

Previous ID SR-6795
Radar rdar://36663633
Original Reporter @lilyball
Type Bug
Status Resolved
Resolution Done
Environment

Apple Swift version 4.0.3 (swiftlang-900.0.74.1 clang-900.0.39.2)
Target: x86_64-apple-macosx10.9

iOS 10/11

Additional Detail from JIRA
Votes 4
Component/s Foundation
Labels Bug
Assignee @lilyball
Priority Medium

md5: 53a1dd70f800671add744984e248da7d

is duplicated by:

  • SR-10920 Crash in NSObject.__old_unswizzled_keyPathsForValuesAffectingValue(forKey:)

Issue Description:

The method swizzling that Swift 4 KVO does to handle key paths is not thread-safe. Specifically, it's swizzling stuff in the wrong order, such that KVO on another thread at the right time will crash as it ends up invoking the method implementation that looks like

    @objc class func _old_unswizzled_keyPathsForValuesAffectingValue(forKey key: String?) -> Set<String> {
        fatalError("Should never be reached")
    }

We just released Twitch 5.8 with Swift 4 and we already have several hundred of these crashes, where the main thread is in the process of adding its first Swift 4 KVO observer and AVPlayerLayer on a background thread crashes trying to invoke its own KVO.

The crash backtrace looks like

0  libswiftCore.dylib             0x10239f95c specialized _assertionFailure(_:_:file:line:flags:) + 97848
1  libswiftFoundation.dylib       0x102761e58 specialized Set._bridgeToObjectiveC() + 14140
2  libswiftFoundation.dylib       0x1027efcc0 specialized static _KVOKeyPathBridgeMachinery.keyPathsForValuesAffectingValue(forKey:) + 3832
3  libswiftFoundation.dylib       0x102762538 @objc static _KVOKeyPathBridgeMachinery.keyPathsForValuesAffectingValue(forKey:) + 1004
4  Foundation                     0x182589a8c -[NSKeyValueUnnestedProperty _givenPropertiesBeingInitialized:getAffectingProperties:] + 200
5  Foundation                     0x182589794 -[NSKeyValueUnnestedProperty _initWithContainerClass:key:propertiesBeingInitialized:] + 152
6  Foundation                     0x182589678 NSKeyValuePropertyForIsaAndKeyPathInner + 284
7  Foundation                     0x182585c08 NSKeyValuePropertyForIsaAndKeyPath + 144
8  Foundation                     0x182586730 -[NSObject(NSKeyValueObserverRegistration) addObserver:forKeyPath:options:context:] + 92
9  AVFoundation                   0x1895016fc -[AVPlayer addObserver:forKeyPath:options:context:] + 160

If you read the Swift 4 KVO swizzling code, it first swizzles +[NSObject keyPathsForValuesAffectingValueForKey:] with the new implementation, and as soon as that one method is swizzled, calling it will crash. It's not until a later swizzle that the crash is repaired.

@JaviSoto
Copy link

Filed radar://36663633 as well

@belkadan
Copy link

cc @Catfish-Man

@lilyball
Copy link
Mannequin Author

lilyball mannequin commented Jul 17, 2018

I’m told there are updates on the radar. Can you please update this ticket instead?

@lilyball
Copy link
Mannequin Author

lilyball mannequin commented Jul 17, 2018

Also, I'm quite frankly shocked this bug is still open. It's really quite bad and makes Swift KVO completely unusable in any app where KVO could possibly be occurring on a background thread (which is any app that uses AVFoundation or NSOperation at the very least).

@JaviSoto
Copy link

I was told that the radar had an update where engineering was asking for a full crashlog. I provided that the day after (Back on February 5th) and there haven't been any updates since.

@swift-ci
Copy link
Contributor

Comment by Jamie Matthews (JIRA)

+1 this is happening in my app as well. Several hundred crashes per week. The scenario for us is the same - the main thread attempts to register a KVO observer at the same time that a background thread calls keyPathsForValuesAffectingValue.

Also interesting to note - I'm not using Swift 4, rather Swift 3.2 (Xcode 9.0.1)

@sjavora
Copy link

sjavora commented Jul 18, 2018

Used to happen to our app as well. We've largely abandoned Swift KVO in favor of the old Obj-C methods :-/

@lilyball
Copy link
Mannequin Author

lilyball mannequin commented Oct 27, 2018

I've submitted PR #20103 for this issue.

@swift-ci
Copy link
Contributor

Comment by Rob Phillips (JIRA)

FYI, Eridius (JIRA User)'s fix still isn't released as of iOS 12.2 / Xcode 10.2.1. We just started using Swift KVO (some on worker threads) and immediately hit thousands of crashes in production; not just in our code but in all code that runs any sort of `Operation`. This means Swift KVO is unusable in any sort of background ops throughout your entire application.

Even switching to ObjC (or using `PMKVObserver`) is not enough because other static libs that your app uses might be performing background operations you can't do anything about. All it takes is one observation on a background thread to corrupt all other libs within your app.

The only safe work around is to create a `main.swift` file, like suggested in apple/swift#20103 (comment)

E.g.

1. Remove the @UIApplicationMain decorator from your AppDelegate

2. Create a main.swift file:

import UIKit
import Foundation

private class CrashWorkaround: NSObject {
    @objc dynamic var test: String = ""    
    /// More info: https://github.com/apple/swift/pull/20103
    fileprivate static func workaroundSwiftKVOCrash() {
        assert(Thread.isMainThread)
        let obj = CrashWorkaround()
        let observer = obj.observe(\.test, options: [.new]) { (_, _) in
            fatalError("Shouldn't be called, value doesn't change")
        }
        observer.invalidate()
    }
}
/// This needs to execute before anything can register KVO in the background
/// so the best way to do this is before the application ever runs
CrashWorkaround.workaroundSwiftKVOCrash()
UIApplicationMain(CommandLine.argc, CommandLine.unsafeArgv, nil, NSStringFromClass(AppDelegate.self))

This will get called immediately after dylib loads but before UIApplicationMain does any work to setup the app delegate, runloop, etc.

@swift-ci
Copy link
Contributor

Comment by Dan Stenmark (JIRA)

I ran into this crasher today on Swift 5.

Fatal error: Should never be reached: file /BuildRoot/Library/Caches/com.apple.xbs/Sources/swiftlang_overlay_Foundation_Sim/swiftlang-1001.2.63.13/swift/stdlib/public/SDK/Foundation/NSObject.swift, line 46
Thread 1 Queue : TTConfigurationModelCoordinator (serial)
#&#8203;0  0x000000011849bba8 in specialized _assertionFailure(_:_:file:line:flags:) ()
#&#8203;1  0x0000000118295dd9 in _assertionFailure(_:_:file:line:flags:) ()
#&#8203;2  0x0000000118aa899a in @objc static NSObject.__old_unswizzled_keyPathsForValuesAffectingValue(forKey:) ()
#&#8203;3  0x0000000118aa9052 in static __KVOKeyPathBridgeMachinery.keyPathsForValuesAffectingValue(forKey:) ()
#&#8203;4  0x0000000118aa959d in @objc static __KVOKeyPathBridgeMachinery.keyPathsForValuesAffectingValue(forKey:) ()
#&#8203;5  0x000000010ff49fd0 in -[NSKeyValueUnnestedProperty _givenPropertiesBeingInitialized:getAffectingProperties:] ()
#&#8203;6  0x000000010ff49c17 in -[NSKeyValueUnnestedProperty _initWithContainerClass:key:propertiesBeingInitialized:] ()
#&#8203;7  0x000000010ff4a3c6 in NSKeyValuePropertyForIsaAndKeyPathInner ()
#&#8203;8  0x000000010ff4c364 in NSKeyValuePropertyForIsaAndKeyPath ()
#&#8203;9  0x000000010ff48e18 in _NSKeyValueCreateImplicitObservationInfo ()
#&#8203;10 0x000000010ff72bba in -[NSOperation init] ()
#&#8203;11 0x000000010ff204ab in +[NSFilesystemItemRemoveOperation filesystemItemRemoveOperationWithPath:] ()
#&#8203;12 0x000000010ff19d44 in -[NSFileManager removeItemAtPath:error:] ()
#&#8203;13 0x0000000113e53188 in TTConfigurationModelStorage.reset() at /Users/dstenmark/Work/ios/TTKit/Frameworks/Core/Library/Configuration/TTConfigurationModelStorage.swift:46
...

Thread 2 Queue : NSManagedObjectContext 0x60000036dfe0 (serial)
#&#8203;0  0x0000000112afb58d in flushCaches(objc_class*) ()
#&#8203;1  0x0000000112afd2cf in method_exchangeImplementations ()
#&#8203;2  0x0000000118aa8af4 in globalinit_33_6DA0945A07226B3278459E9368612FF4_func30 ()
#&#8203;3  0x0000000118f43d02 in _dispatch_client_callout ()
#&#8203;4  0x0000000118f454ce in _dispatch_once_callout ()
#&#8203;5  0x000000011855d579 in swift_once ()
#&#8203;6  0x0000000118aade57 in NSSortDescriptor.init<A, B>(keyPath:ascending:) ()
#&#8203;7  0x0000000113e0ee03 in closure #&#8203;1 in closure #&#8203;2 in TTInternalEventLogger.init(batchEventService:configuration:invalidStateLogger:) at /Users/dstenmark/Work/ios/TTKit/Frameworks/Core/Library/Events/FirstParty/TTInternalEventLogger.swift:172
...

@nolanw
Copy link

nolanw commented Aug 13, 2019

As best I can tell, this fix didn't make it into Swift 5.0 or 5.1? And since the stdlib and overlays are baked into iOS as of iOS 12.2, I think that means iOS 12.2 ..< 13.0 (maybe higher?) will always suffer from this crash?

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from apple/swift May 5, 2022
NachoSoto added a commit to RevenueCat/purchases-ios that referenced this issue Oct 28, 2022
This _might_ fix [CSDK-503].
The latest stacktrace provided gave me a clue:
> Fatal error: Should never be reached: file /BuildRoot/Library/Caches/com.apple.xbs/Sources/swiftlang_overlay_Foundation_Device/swiftlang-1001.2.63.13/swift/stdlib/public/SDK/Foundation/NSObject.swift, line 46
> 0xa3d10 @objc static NSObject.__old_unswizzled_keyPathsForValuesAffectingValue(forKey:) + 84
>53  libswiftFoundation.dylib       0xa436c static __KVOKeyPathBridgeMachinery.keyPathsForValuesAffectingValue(forKey:) + 272
>64  libswiftFoundation.dylib       0xa4864 @objc static __KVOKeyPathBridgeMachinery.keyPathsForValuesAffectingValue(forKey:) + 88

That crash is coming from [here](https://github.com/apple/swift-corelibs-foundation/blob/swift-DEVELOPMENT-SNAPSHOT-2022-10-24-a/Darwin/Foundation-swiftoverlay/NSObject.swift#L47).

KVO shipped broken on Swift at least throughout iOS 12.x (see apple/swift-corelibs-foundation#3742)

Thanks to @jckarter for [suggesting this workaround](https://twitter.com/jckarter/status/1586029206646898688?s=61&t=LJyPWVeXcT9TfIL2zMjFng).
NachoSoto added a commit to RevenueCat/purchases-ios that referenced this issue Oct 28, 2022
This _might_ fix [CSDK-503].

The latest stacktrace provided gave me a clue:
```
Fatal error: Should never be reached: file /BuildRoot/Library/Caches/com.apple.xbs/Sources/swiftlang_overlay_Foundation_Device/swiftlang-1001.2.63.13/swift/stdlib/public/SDK/Foundation/NSObject.swift, line 46
0xa3d10 @objc static NSObject.__old_unswizzled_keyPathsForValuesAffectingValue(forKey:) + 84
53  libswiftFoundation.dylib       0xa436c static __KVOKeyPathBridgeMachinery.keyPathsForValuesAffectingValue(forKey:) + 272
64  libswiftFoundation.dylib       0xa4864 @objc static __KVOKeyPathBridgeMachinery.keyPathsForValuesAffectingValue(forKey:) + 88
```

That crash is coming from
[here](https://github.com/apple/swift-corelibs-foundation/blob/swift-DEVELOPMENT-SNAPSHOT-2022-10-24-a/Darwin/Foundation-swiftoverlay/NSObject.swift#L47).
KVO shipped broken on Swift at least throughout iOS 12.x (see
apple/swift-corelibs-foundation#3742)

Thanks to @jckarter for [suggesting this
workaround](https://twitter.com/jckarter/status/1586029206646898688?s=61&t=LJyPWVeXcT9TfIL2zMjFng).

[CSDK-503]:
https://revenuecats.atlassian.net/browse/CSDK-503?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants