[SDK] Fix multiple issues with Swift KVO

[lilyball]

This PR fixes a number of issues with Swift KVO. It fixes a bad thread safety issue, a problem where NSKeyValueObservation was incorrectly replacing NSObject.observeValue(forKeyPath:of:change:context:), a potential race with NSKeyValueObservation handling a KVO notice during deinit, an issue with NSSortDescriptor.keyPath returning the wrong value, and a problem where the NSKeyValueObservingCustomization methods could be called with the wrong KeyPath if KeyPath-related work is happening concurrently on another thread.

Resolves SR-6795, SR-9074, SR-9075, SR-9076, and SR-9077.

[millenomi]

@swift-ci please smoke test macos

[millenomi]

One test is failing:

/Users/buildnode/jenkins/workspace/swift-PR-osx-smoke-test/branch-master/swift/test/stdlib/KVOKeyPaths.swift:107:16: error: CHECK-NEXT: expected string not found in input

[lilyball]

Fun fact: That test is actually broken. As in, the test was confirming that the previous behavior of this code was broken. The implementation of automaticallyNotifiesObservers(for:) was never actually being invoked with the \Target.objcValue2 or Target.objcValue3 key paths, because those key paths were never explicitly used prior and therefore never put into the global table. The code itself even has comments saying which lines are intended to trigger KVO notifications and which aren't. But then the CHECK lines went ahead and checked for the notifications that weren't supposed to be sent.

In other words, there were 3 lines that looked like

// CHECK-NEXT: swiftValue 13, objcValue four

but only 1 was supposed to be sent.

That said, from the test failure it sounds like that line is now printed zero times instead of the expected 1 time, so I guess it's still broken (and the implication being that keyPathsAffectingValue(for:) isn't being called for \Target.objcValue), but I haven't actually confirmed this yet.

[lilyball]

I've updated the PR to fix that test failure. The test itself is still broken, of course, but the current behavior now matches the previous one (as far as that test is concerned).

[JunyuKuang]

Hi Lily, thank you for your contribution. Do you know any workaround for the thread safety of KVO swizzling crash? My app is randomly crash at launch with Fatal error: Should never be reached and it's really disturbing. I'm considering change all of my Swift KVO related implementation to Objective-C.

[lilyball]

@JunyuKuang There are only two workarounds for now:

1. Stop using Swift KVO. This is the most foolproof solution. I actually wrote and maintain an open-source library that does block-based KVO fast and thread-safe if you're interested, called PMKVObserver. Or you could just use the existing old KVO registration method from Obj-C (you can still call it from Swift of course)
2. Ensure that you register a Swift KVO observer at some point prior to the use of KVO on a background thread. Pretty much the only way to actually guarantee this is to use an explicit main.swift and install the KVO prior to calling NSApplicationMain(_:_:)/UIApplicationMain(_:_:_:_:). You can let the KVO uninstall itself immediately after installation, all that matters is that you have installed a KVO observer at least once prior to the use of KVO on a background thread.

[lilyball]

I've added what tests I can reasonably write. I can't test the NSKeyValueObservingCustomization threading issues because of the global locks in the KVO implementation that prevent me from literally running the 2 observe calls simultaneously (and not literally running them simultaneously means I can't distinguish a threading issue from the other issue I am testing already, which is that we restore a prior keypath mapping after an observation).

This PR is ready to move forward.

[lilyball]

@millenomi Is there anything left for me to do on this PR?

[millenomi]

@swift-ci please test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 39714de352d4cb3e5a81181170f214b0c3a712f4

[millenomi]

I'm still in the throes of apple/swift-corelibs-foundation#1755 and won't have time to look at other stuff until it lands T_T

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 4f0cd71cc276ca019dd35bee105df54f280512fe

[millenomi]

This is blocked by https://bugs.swift.org/browse/SR-9228.

[zetasq]

Should we replace the initialize-once pattern below:

static var swizzler: ()? = {
    // Some operations
    return nil
}()

with

static let swizzler: Void = {
    // Some operations
}()

I think we don't need to return an optional void value here, and let should be used since it won't be mutated.

[lilyball]

Good point. It was written the way it is because it was adapted from a previous static var swizzler : NSKeyValueObservation? but you're right in that it should be perfectly fine to be a let and have the type ().

[lilyball]

@millenomi SR-9228 was merged 21 days ago. Is this still blocked?

[millenomi]

It isn't; I still was v.v'

[millenomi]

@swift-ci please test

[millenomi]

Ah, dang. @kballard — this looks fine to me, but it needs conflict resolution.

[millenomi]

Do we want to have a private context for added safety?

[lilyball]

The Helper isn't exposed to anyone else and this is the only observation set up for it. The only way I can think of to get a second observation is if someone swizzles e.g. -[NSObject init] and has it set up an observation on every single object, which would be pretty nuts. If you think it's worth defending against that sort of thing we could, but I'm not particularly worried (any such swizzle would break a significant amount of KVO code).

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - de3dbe69e1ecaae6e77160222d7f5251dfc732e4

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - de3dbe69e1ecaae6e77160222d7f5251dfc732e4

[lilyball]

I can fix the conflicts, but the reported build failure is compilation failure in completely unrelated code.

[millenomi]

Yeah; please ignore them — they probably just checked out your PR over whatever commit it was pushed on, erasing unrelated progress. (I didn't see the conflict marker until I asked the bot to start.)

Fixing the conflict should address that too.

[lilyball]

Rebasing my PR locally actually didn't produce any conflicts at all. I'm wondering if GitHub is reporting a conflict just because the number of commits between the branch and master (over 2000) is so high. The local rebase took a noticeable amount of time to do 3-way merges, so maybe GitHub doesn't want to do the work and just bails.

Anyway I'm just running a local build to sanity-check before updating the branch with the rebase.

[lilyball]

In fact, the "conflicts" are just that the public/SDK folder was renamed to public/Darwin. There was apparently no other changes to either file on master ¯\_(ツ)_/¯

[lilyball]

Given that, I just updated the branch now instead of waiting for my local build.

[shajrawi]

there's a radar open for that - rdar://problem/47708852

[millenomi]

Looking at it.

[lilyball]

It sounds like the associated object trick for ensuring the observer is deregistered isn't working. Does the Obj-C 32-bit runtime check for leaked KVO observers happen before clearing associated objects?

I think the quick fix here is to just update the relevant test to ensure the NSKeyValueObservation is released prior to the Dummy. FWIW if the associated objects stuff isn't working, that just means it's behaving like it did prior to this PR (since the old code never even attempted that), and so the breakage is just because this PR modified the test.

[lilyball]

I haven't tested it but I'd guess the following will work:

diff --git a/test/stdlib/KVOKeyPaths.swift b/test/stdlib/KVOKeyPaths.swift
index b3b302a367..32af1a4eac 100644
--- a/test/stdlib/KVOKeyPaths.swift
+++ b/test/stdlib/KVOKeyPaths.swift
@@ -127,13 +127,17 @@ class Target2 : NSObject, NSKeyValueObservingCustomization {
     // with the ability to look up the key path using the other.
     static func keyPathsAffectingValue(for key: AnyKeyPath) -> Set<AnyKeyPath> {
         print("keyPathsAffectingValue: key == \\.name:", key == \Target2.name)
-        _ = Dummy().observe(\.name) { (_, _) in }
+        withExtendedLifetime(Dummy()) { (dummy) in
+            _ = dummy.observe(\.name) { (_, _) in }
+        }
         return []
     }
 
     static func automaticallyNotifiesObservers(for key: AnyKeyPath) -> Bool {
         print("automaticallyNotifiesObservers: key == \\.name:", key == \Target2.name)
-        _ = Dummy().observe(\.name) { (_, _) in }
+        withExtendedLifetime(Dummy()) { (dummy) in
+            _ = dummy.observe(\.name) { (_, _) in }
+        }
         return true
     }
 }

[millenomi]

I need to check if automatic deregistration works on 32-bit. It really should, tho.

[millenomi]

@lilyball your suggestion works, and I will repropose your patch with the fix.

[iwasrobbed-ks]

Just an FYI for others: this fix still isn't released as of iOS 12.2 / Xcode 10.2.1, making Swift KVO unusable on background threads for now.

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 above #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.

[CLoutas]

Has there been any update on this recently? What's the status as of iOS 13, Xcode 11 and Swift 5.1?

[bobergj]

@CLoutas: git branch -r --contains 7c51419 shows this was merged into the branch "swift-5.1-branch". Meaning it was included in Swift 5.1.

Since the fixes affect the runtime libraries rather than the compiler, this means that it is fixed on iOS 13, macOS 10.15 but still requires the workaround on earlier OS versions.

[CLoutas]

@bobergj that's unfortunate, but at least we have a workaround. Thank you very much for clarifying that.