pointfreeco / swift-perception

Observable tools, backported.
MIT License
575 stars 44 forks source link

Fallback to Perception on iOS 17 beta builds #66

Closed ollieatkinson closed 5 months ago

ollieatkinson commented 7 months ago

Early beta versions used the @_marker protocol for Observable. Changes to the protocol's memory layout in these versions disrupt dynamic casting, e.g. subject as? any Observable. This occurs because the Swift runtime expects a specific protocol metadata layout for casting, and this mismatch leads to a crash.

As a result, the following check has been implemented to disable Observation on any of the following beta OS versions:

This safeguard makes sure that Perception is used in place of Observation.

iOSdevALDI commented 7 months ago

+1

stephencelis commented 7 months ago

@ollieatkinson Thanks for this. I'm doing the revert and release separately here: #67.

I think we need to do more thinking. While we'd love to avoid these crashes, it also doesn't seem like we should create too much library uncertainty and problems to accommodate beta users.

I wonder if there is a more targeted fix that could be done? Maybe a way of detecting that the user has a beta OS that is affected and avoid the cast?

Also I think we need a bit more of a write-up in this PR to consider adding the property. I haven't fully wrapped my head around what it does :)

ollieatkinson commented 7 months ago

Also I think we need a bit more of a write-up in this PR to consider adding the property. I haven't fully wrapped my head around what it does :)

The purpose of the property is to force the use of Perception over Observation even when on iOS 17 and above.

stephencelis commented 7 months ago

@ollieatkinson If we go this route I think documentation that explains why a user may enable this flag would be helpful, and example code of how/where to do so.

But also I wonder if there's a way to do this in a more targeted fashion that doesn't require configuration? Could this boolean be detected based off an OS check at runtime?

ollieatkinson commented 7 months ago

But also I wonder if there's a way to do this in a more targeted fashion that doesn't require configuration? Could this boolean be detected based off an OS check at runtime?

Yes, totally - I understand the hesitation. We would need to use sysctlbyname to get the kern.osversion and ban specific versions we know to cause the bug. In this case, every iOS 17.0 beta before public release.

21A5248v (Developer Beta 1)
21A5268h (Developer Beta 2)
21A5277h (Developer Beta 3)
21A5277j (Developer Beta 3 Update/Public Beta 1)
21A5291h (Developer Beta 4)
21A5291j (Developer Beta 4 Update/Public Beta 2)
21A5303d (Developer Beta 5/Public Beta 3)
21A5312c (Developer Beta 6/Public Beta 4)
21A5319a (Developer Beta 7/Public Beta 5)
21A5326a (Developer Beta 8/Public Beta 6)

My original intention for adding this boolean was to provide a configuration escape hatch for consumers if they ever found another issue and this helped them work around it. Please let me know if you would prefer a runtime get check like I described above over configuration with documentation. I don't have strong opinions here other than being able to provide a workaround for users on the broken versions of iOS 17. As small and insignificant as it may feel to support these users - I appreciate your diligence.

ollieatkinson commented 7 months ago

e.g.

import Foundation
#if os(iOS) || os(tvOS)
import UIKit
#elseif os(watchOS)
import WatchKit
#endif

var forcePerceptionChecking: Bool { isBeta && isSystemVersionObservationBeta }

private var isBeta: Bool { kernelVersion.last?.isLowercase == true }

private var isSystemVersionObservationBeta: Bool {
  #if os(iOS) || os(tvOS)
  return UIDevice.current.systemVersion == "17.0"
  #elseif os(watchOS)
  return WKInterfaceDevice.current().systemVersion == "10.0"
  #elseif os(macOS)
  return false
  #endif
}

private var kernelVersion: String {
  var size = 0
  sysctlbyname("kern.osversion", nil, &size, nil, 0)
  var version = [CChar](repeating: 0, count: size)
  sysctlbyname("kern.osversion", &version, &size, nil, 0)
  return String(cString: version)
}
stephencelis commented 6 months ago

@ollieatkinson Before merging, can you confirm that pointing your project to this branch works fine for you? Everything building/compiling and working as expected, even for the beta bucket? I'm hoping even things like @Bindable/@Perception.Bindable work just fine?

ollieatkinson commented 6 months ago

@stephencelis

Everything is building compiling as expected, the one caveat is that I have been unable to test this since kern.osversion returns the macOS details when running a simulator build - I do not have a physical iOS 17 beta to test it on.

When running on my physical device I can confirm the kern.osversion is correct as highlighted by this printout, therefore the assumption that this would stay true and be correct for on-device beta.

17.4.1: https://betawiki.net/wiki/IOS_17.4.1_build_21E237

(lldb) po String(cString: version)
"21E237"
(lldb) po ProcessInfo.processInfo.operatingSystemVersionString
"Version 17.4.1 (Build 21E237)"

Beta: https://betawiki.net/wiki/IOS_17.0_build_21A5277j


If you are happy to move forward with this assumption I would be too - if not, I can look to try ship this branch into a release and await for customer feedback.

stephencelis commented 6 months ago

@ollieatkinson Huh! In the above code sample does ProcessInfo.processInfo.operatingSystemVersionString always return the right thing, even in a simulator? If so, I wonder if we should simply parse that value out instead of using the lower level kern.osversion APIs?

And since this fix was brought up because of your user base, I think it'd definitely be a good idea to roll it out to you users and verify the fix before merging. Assuming everything looks good on your end after the rollout, we'll be happy to merge!

ollieatkinson commented 6 months ago

In the above code sample does ProcessInfo.processInfo.operatingSystemVersionString always return the right thing, even in a simulator?

From limited testing, yes, however I would not want to rely on this behaviour.

If so, I wonder if we should simply parse that value out instead of using the lower level kern.osversion APIs?

Unfortunately not, the documentation for this property states "This string is not appropriate for parsing."

And since this fix was brought up because of your user base, I think it'd definitely be a good idea to roll it out to you users and verify the fix before merging. Assuming everything looks good on your end after the rollout, we'll be happy to merge!

This makes sense, let me go ahead to make this change and report back

ollieatkinson commented 5 months ago

Hey @stephencelis - looping back on this, since merging and having the fix in customers hands for ~3 weeks we have not observed any crashes!