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

Fallback to Perception on iOS 17 beta builds #66

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

ollieatkinson
Copy link
Contributor

@ollieatkinson ollieatkinson commented Apr 30, 2024

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:

  • iOS 17.0.0
  • watchOS 10.0.0
  • tvOS 17.0.0

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

@iOSdevALDI
Copy link

+1

@stephencelis
Copy link
Member

@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
Copy link
Contributor Author

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.

@ollieatkinson ollieatkinson changed the title Revert 17.0.1 availability, add a boolean to allow clients to force Perceptionover Observation Allow defaulting to the implementation of Perception over Observation on platforms over iOS 17 Apr 30, 2024
@stephencelis
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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)
}

@ollieatkinson ollieatkinson changed the title Allow defaulting to the implementation of Perception over Observation on platforms over iOS 17 Fallback to Perception on iOS 17 beta builds May 1, 2024
Copy link
Member

@stephencelis stephencelis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for figuring out the fix! Made a few small changes and I think we're good to merge!

  • Changed the computed vars to a let so the logic will only run once
  • Moved the sysctlbyname calls into the #ifs where it's needed
  • Pruned the comments a bit

@stephencelis
Copy link
Member

@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
Copy link
Contributor Author

@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
Copy link
Member

@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
Copy link
Contributor Author

ollieatkinson commented May 7, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants