sbooth / CAAudioHardware

The Swift-friendly Core Audio HAL
MIT License
2 stars 1 forks source link

Crash in whenSelectorChanges and Xcode 16 beta 2 #19

Closed andykent closed 20 hours ago

andykent commented 2 weeks ago

I think this might be a Swift bug but wanted to check here first to make sure I wasn't missing something silly.

I tested building our project on Xcode 16 today and I am seeing a crash in the CAAudioHardware main branch. I've narrowed the crash down and I am able to reproduce it by adding a single line to a stock File > New Project SwiftUI App.

import SwiftUI
import CAAudioHardware

@main
struct AudioHardwareCrashApp: App {

  init() {
    try? AudioSystemObject.instance.whenSelectorChanges(.defaultOutputDevice, on: .main) { _ in }
  }

  var body: some Scene {
    WindowGroup {
      ContentView()
    }
  }
}
Screenshot 2024-06-20 at 15 09 54

It seems to me like propertyListeners.removeValue(forKey:) is operating on a Dictionary that isn't properly initialised? Maybe it's related to the shared instance initialisation somehow?

I tried switching swift language modes between 5 and 6 but it seems to make no difference. The same code runs fine when compiled from Xcode 15.

I tried reproducing it outside of CAAudioHardware with the following simplified example but that seems to run fine so it's more subtle than this...

public class Example {
  public static let instance = Example()
  private var dict = [String: Bool]()

  public final func remove(key: String) {
    if let value = dict.removeValue(forKey: key) {
      print("removed")
    } else {
      print("not found")
    }
  }
}

Example.instance.remove(key: "missing")
sbooth commented 2 weeks ago

I've experienced the same issue and I agree that it appears to be a Swift bug. The problem seems to be related to Hashable and structs containing C structures but I haven't done a lot of testing.

I can reproduce the problem with the following code:

import CoreAudio

struct PropertyAddress: RawRepresentable {
    public let rawValue: AudioObjectPropertyAddress
}

extension PropertyAddress: Hashable {
    public static func == (lhs: PropertyAddress, rhs: PropertyAddress) -> Bool {
        lhs.rawValue.mSelector == rhs.rawValue.mSelector && lhs.rawValue.mScope == rhs.rawValue.mScope && lhs.rawValue.mElement == rhs.rawValue.mElement
    }

    public func hash(into hasher: inout Hasher) {
        hasher.combine(rawValue.mSelector)
        hasher.combine(rawValue.mScope)
        hasher.combine(rawValue.mElement)
    }
}

func crashes() {
    var d = [PropertyAddress: Int]()
    d.removeValue(forKey: PropertyAddress(rawValue: AudioObjectPropertyAddress()))
}

When called, crashes will crash with EXC_BAD_ACCESS on the call to removeValue.

Edit: works in Xcode 15.4, crashes in Xcode 16 beta

sbooth commented 2 weeks ago

I've done more testing and the problem seems to be related to RawRepresentable.

The following code works in Xcode 16 beta:

import CoreAudio

struct PropertyAddress {
    public let rawValue: AudioObjectPropertyAddress
}

extension PropertyAddress: Hashable {
    public static func == (lhs: PropertyAddress, rhs: PropertyAddress) -> Bool {
        lhs.rawValue.mSelector == rhs.rawValue.mSelector && lhs.rawValue.mScope == rhs.rawValue.mScope && lhs.rawValue.mElement == rhs.rawValue.mElement
    }

    public func hash(into hasher: inout Hasher) {
        hasher.combine(rawValue.mSelector)
        hasher.combine(rawValue.mScope)
        hasher.combine(rawValue.mElement)
    }
}

func no_crash() {
    var d = [PropertyAddress: Int]()
    d.removeValue(forKey: PropertyAddress(rawValue: AudioObjectPropertyAddress()))
}

So the addition of RawRepresentable conformance to PropertyAddress triggers the behavior.

andykent commented 2 weeks ago

Glad I'm not alone here! 🤣

Nice job narrowing it down. Seems like it might be worth raising this as a Swift issue then I guess?

sbooth commented 2 weeks ago

Yes, I think so. I'll file a bug with Apple in the next day or two.

andykent commented 2 weeks ago

Thanks!

On a somewhat related note, whilst looking into this issue I realised that whenPropertyChanges is not thread-safe, mostly due to the accesses to propertyListeners. This is why I thought at first this crash was my bad for doing concurrent calls whenPropertyChanges at some point. Is this a bug or is there are main actor expectation here?

I don't see a lot of documentation on the Apple side around thread safety of the coreaudio APIs, do happen to know if they are thread safe, presumably the expectation isn't that they are all called from the main thread?

sbooth commented 2 weeks ago

I had a similar thought and couldn't find a definitive answer either. I've asked on the Core Audio mailing list: https://www.mail-archive.com/coreaudio-api@lists.apple.com/msg01860.html and hopefully I'll get a response.

Semi-related to #17

sbooth commented 2 weeks ago

I fear that the coreaudio-api mailing list might be dead so I posted the same question in the Apple forums. Hopefully someone at Apple will respond.

andykent commented 2 weeks ago

...tumbleweed...

Fingers crossed for an answer on one of them!

I just tried Xcode 16 beta 2 and the situation is the same.

sbooth commented 1 week ago

Indeed. I just created #21 for the concurrency issues.

sbooth commented 1 week ago

@andykent Does the problem occur for you only when using the Xcode debugger or also if you run an archived/exported copy of the application?

Apple said they're unable to reproduce the problem and asked for a complete sample project and crash log, and interestingly enough when I archive the test application and run it from Finder it does not crash. So it seems to be limited to running from within Xcode.

Perhaps you could file a duplicate report and reference my feedback ID FB14003009?

andykent commented 1 week ago

Interesting, I did actually file feedback yesterday but haven't heard anything back: FB14071598

I just tested and it does indeed work for me from a release build too so seems to be only debug builds that are affected.

I'm not able to test it right now but I do wonder if it's possibly also only the combo of Xcode 16 + macOS 14 that triggers it. Would be interesting to test with the macOS 15 beta.

Not sure if they are related or not but there are few reports on the Swift issues tracker that touch on runtime crashes around protocol conformance:

The good news is if it is related to that then it might be fixed by https://github.com/swiftlang/swift/pull/74604

🤞

sbooth commented 1 day ago

@andykent It looks like this package will become obsolete in macOS 15 and later: https://developer.apple.com/documentation/coreaudio/audiohardwareobject

andykent commented 1 day ago

Oh Wow! good find, I hadn't noticed those new APIs. Maybe this project needs to become a backport / polyfill of the new APIs? It's not a million miles off!

Interesting that none of those APIs are marked as Sendable or @MainActor I guess that suggests the function calls are thread safe but the objects themselves aren't sendable? Either that or they have created a brand new API without Swift 6 concurrency annotations I suppose.

andykent commented 1 day ago

I filed FB14247375 "Please add better documentation for new CoreAudio Swift APIs" 😉

sbooth commented 21 hours ago

I've been attempting some detective work to try and figure out whether the AudioObject APIs are thread-safe. I think based on looking at the open-source portion of IOKit that under the hood audio objects are IOService objects which means they should (?) be thread-safe since their messages are passed using mach ports. But I still have no inkling whether it's safe, for example, to add two property listeners to the same object ID on different threads simultaneously.

sbooth commented 20 hours ago

I believe I've "fixed" the issue by removing RawRepresentable conformance from PropertyAddress (which was of dubious value anyway). For me the crash no longer occurs with Xcode 16 beta 2 after merging the linked PR.

I'm happy to leave this issue open for chat or we can migrate the discussion to #17 or #21.

andykent commented 20 hours ago

Amazing work. Thanks.

Very happy to close this and move the discussion to more relevant places.