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

KVO: fix possible duplicate observing of key #2947

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

triplef
Copy link
Contributor

@triplef triplef commented Aug 19, 2021

Could happen when using dependent key paths, as the dependency handling can have added an observer for restOfKeypath already.

Following is a test case to reproduce the issue, which resulted in a KVO notification after removing the observer. This happened because number was observed twice internally, but only one observer was being removed internally on -removeObserver:forKeyPath:.

@interface Child : NSObject
@property (nonatomic, assign) NSUInteger number;
@end

@implementation Child
@end

@interface Parent : NSObject
@property (nonatomic, assign) BOOL useChild2;
@property (nonatomic, readonly) Child *child;
@property (nonatomic, strong) Child *child1;
@property (nonatomic, strong) Child *child2;
@end

@implementation Parent

- (id)init
{
	self = [super init];
	if (self) {
		self.child1 = [[Child alloc] init];
		self.child1.number = 1;
		self.child2 = [[Child alloc] init];
		self.child2.number = 2;;
	}
	return self;
}

+ (NSSet *)keyPathsForValuesAffectingChild
{
	return [NSSet setWithObject:@"useChild2"];
}

- (Child *)child
{
	return self.useChild2 ? self.child2 : self.child1;
}

@end

@interface Test : NSObject
@end

@implementation Test

- (void)test
{
	Parent *parent = [Parent new];

	[parent addObserver:self forKeyPath:@"child.number" options:NSKeyValueObservingOptionInitial context:NULL];
	[parent removeObserver:self forKeyPath:@"child.number"];

	// *** BUG: incorrectly triggers KVO ***
	parent.child.number = 999;
	// ***
}

- (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(NSDictionary *)change context:(void *)context
{
	NSLog(@"%@.%@ (change %@) = %@", object, keyPath, change, [object valueForKeyPath:keyPath]);
}

@end

While I don’t expect this to be merged given the state of the project, I wanted to post this here for posterity in case anyone else is using this KVO implementation.

Could happen when using dependent key paths, as the dependency handling can have added an observer for restOfKeypath already.
@DHowett
Copy link
Member

DHowett commented Aug 27, 2021

Wow, good fix. It's been a long time since I've thought about the KVO code.

I won't ask you to write a unit test for our abandoned project; I'm just glad somebody is still able to build it.

_dispatchWillChange() was not removing nested observers and optionally dependents if keypaths were in the process of changing. Now mirrors _dispatchDidChange() implementation.
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.

2 participants