-
Notifications
You must be signed in to change notification settings - Fork 58
Remove ODWReachability and iOS 11 macOS 10.14 code with EOL for those platforms #1371
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
base: main
Are you sure you want to change the base?
Conversation
|
@absaroj - Can you help with the review. thanks. |
|
|
||
| m_reach = [ODWReachability reachabilityForInternetConnection]; | ||
| void (^block)(NSNotification*) = ^(NSNotification*) | ||
| m_monitor = nw_path_monitor_create(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should handle scenario where nw_path_monitor_create fails, and return nil - may be rare scenario but under system resource exhaustion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is existing code, so probably not blocker here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API docs describe this return value as non-optional so it should never be nil - https://developer.apple.com/documentation/network/nw_path_monitor_create()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take that back, I checked the docs in Xcode and it does indeed look like this can be nil. I'm finding a hard time finding a reference to these docs online. Here's what it says:
/*!
* @function nw_path_monitor_create
*
* @abstract
* Create a default path monitor, that will allow the enumeration of all available
* interfaces on the system.
*
* @result
* Returns an allocated nw_path_monitor_t object on success.
* Callers are responsible for deallocating using nw_release(obj) or [obj release].
* These objects support ARC.
* Returns NULL on failure. Fails due to invalid parameters.
*/
API_AVAILABLE(macos(10.14), ios(12.0), watchos(5.0), tvos(12.0))
NW_RETURNS_RETAINED nw_path_monitor_t
nw_path_monitor_create(void);
|
One question: This change raises the minimum iOS version from 10.0 to 12.0. Are there any concerns about existing customer devices still running iOS 10/11, or is this acceptable given that Apple has long since ended support for these versions? |
I can only speak for iOS Outlook, but our min version is 18.0. |
App Store apps are required to be built with Xcode 16 or later as of April 2025, and only supports iOS 15 or later, so we should be okay. |
Started investigating the crash from #1370 and realized that ODWReachability should only be applicable for platforms below iOS 11 and macOS 10.14. However, a crash stack from an iOS 18 device clearly shows these classes being deallocated.
Seems like there was a bug where these reachability classes were being initialized without guarded availability checks (see NetworkInformation::SetupNetDetect:108) when they should only be used for lower platforms.
Regardless, given the EOL of these versions, this PR remove references to ODWReachability and fixes the crash.
Testing
Verified with build-ios.sh.