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

[SR-9075] NSKeyValueObservation is likely not thread-safe on deinit #3608

Closed
lilyball mannequin opened this issue Oct 25, 2018 · 4 comments
Closed

[SR-9075] NSKeyValueObservation is likely not thread-safe on deinit #3608

lilyball mannequin opened this issue Oct 25, 2018 · 4 comments
Assignees

Comments

@lilyball
Copy link
Mannequin

lilyball mannequin commented Oct 25, 2018

Previous ID SR-9075
Radar None
Original Reporter @lilyball
Type Bug
Status Resolved
Resolution Done
Environment

Current Swift master (14d4235b571d7a4e2ae51854f0d665f96959d182)

Additional Detail from JIRA
Votes 0
Component/s Foundation
Labels Bug
Assignee @lilyball
Priority Medium

md5: 83e96e9c5b2276eba39cabcf7e32f97b

Issue Description:

NSKeyValueObservation unregisters its observation in deinit (assuming nobody calls invalidate()). This is a problem because it opens up a race condition whereby a second thread sends a KVO change notification in between the NSKeyValueObservation object decrementing its retain count to zero and it actually executing its deinit. If this race is hit, the NSKeyValueObservation.observeValue(forKeyPath:of:change:context:) method will be invoked on a dead object, which will potentially behave badly or crash.

Given the desire to unregister on deinit, I believe the only proper solution is as follows:

  1. Use a second helper object as the actual observing object and host of the observeValue(forKeyPath:of:change:context:) method. The NSKeyValueObservation object holds onto this second object and calls invalidate() when the NSKeyValueObservation object enters deinit.

  2. Add this second object as an associated value on the observed object (and then remove it in invalidate()). This way the second object will be guaranteed to be alive while the observed object is sending its KVO change notice. Note that it's not sufficient merely to ensure the second object is alive throughout the invalidate() call, as a KVO change notice may have been triggered on a background thread but not yet completed.

This approach still has the potential issue of someone calling objc_removeAssociatedObjects(_:) on the observed object, but in this rather unlikely event, the behavior would simply degrade to what we already have today.

@belkadan
Copy link

cc @millenomi

@lilyball
Copy link
Mannequin Author

lilyball mannequin commented Oct 25, 2018

I've got a proof-of-concept patch written for this already, but without tests yet. If I have time I'll publish it tonight.

That said, I'm not sure how to go about testing this, since it's a race condition. Do we have precedent for tests on threading-related race conditions?

@lilyball
Copy link
Mannequin Author

lilyball mannequin commented Oct 27, 2018

I've submitted PR #20103 for this issue.

@millenomi
Copy link
Contributor

Merged: apple/swift@7c51419

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from apple/swift May 5, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants