From 3886f27acd04501083b1418c9a0faaedf45b16cf Mon Sep 17 00:00:00 2001 From: Ilya Kulakov Date: Sat, 14 Jul 2012 19:32:25 +0700 Subject: [PATCH] =?UTF-8?q?Fix=20crash=20if=20an=20observer=20is=20set=20b?= =?UTF-8?q?etween=20willChange=E2=80=A6/didChange=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When you add an observer to an object first time, its class is implicitly changed to a KVO_originalClassName (subclass of original class). This subclass adds willChange…/didChange… methods for observable properties. If you send willChange… before you add an observer, it does nothing. But if you send didChange… just after, the app will crash, because new KVO_originalClassName nerver receives willChange… The idea is to maintain counter of all received willChange… messages (per key) and decrease it in didChange… When KVO_originalClassName is created and didChange… is received (without opening willChange… to new class), exception is not thrown immediately, but the counter is checked first. If it's greater than 0, then didChange… just closes an unboserved willChange… Otherwise exception is thrown, as expected. --- Foundation/CPKeyValueObserving.j | 96 +++++++++++++++++++++++++++++++- Tests/Foundation/CPKVOTest.j | 45 +++++++++++++++ 2 files changed, 140 insertions(+), 1 deletion(-) diff --git a/Foundation/CPKeyValueObserving.j b/Foundation/CPKeyValueObserving.j index 2ab51e5df..ecf7e1c1d 100644 --- a/Foundation/CPKeyValueObserving.j +++ b/Foundation/CPKeyValueObserving.j @@ -31,26 +31,107 @@ - (void)willChangeValueForKey:(CPString)aKey { + if (!aKey) + return; + + if (!self[KVOProxyKey]) + { + if (!self._willChangeMessageCounter) + self._willChangeMessageCounter = new Object(); + + if (!self._willChangeMessageCounter[aKey]) + self._willChangeMessageCounter[aKey] = 1; + else + self._willChangeMessageCounter[aKey] += 1; + } } - (void)didChangeValueForKey:(CPString)aKey { + if (!aKey) + return; + + if (!self[KVOProxyKey]) + { + if (self._willChangeMessageCounter && self._willChangeMessageCounter[aKey]) + { + self._willChangeMessageCounter[aKey] -= 1; + + if (!self._willChangeMessageCounter[aKey]) + delete self._willChangeMessageCounter[aKey]; + } + else + [CPException raise:@"CPKeyValueObservingException" reason:@"'didChange...' message called without prior call of 'willChange...'"]; + } } - (void)willChange:(CPKeyValueChange)aChange valuesAtIndexes:(CPIndexSet)indexes forKey:(CPString)aKey { + if (!aKey) + return; + + if (!self[KVOProxyKey]) + { + if (!self._willChangeMessageCounter) + self._willChangeMessageCounter = new Object(); + + if (!self._willChangeMessageCounter[aKey]) + self._willChangeMessageCounter[aKey] = 1; + else + self._willChangeMessageCounter[aKey] += 1; + } } - (void)didChange:(CPKeyValueChange)aChange valuesAtIndexes:(CPIndexSet)indexes forKey:(CPString)aKey { + if (!aKey) + return; + + if (!self[KVOProxyKey]) + { + if (self._willChangeMessageCounter && self._willChangeMessageCounter[aKey]) + { + self._willChangeMessageCounter[aKey] -= 1; + + if (!self._willChangeMessageCounter[aKey]) + delete self._willChangeMessageCounter[aKey]; + } + else + [CPException raise:@"CPKeyValueObservingException" reason:@"'didChange...' message called without prior call of 'willChange...'"]; + } } - (void)willChangeValueForKey:(CPString)aKey withSetMutation:(CPKeyValueSetMutationKind)aMutationKind usingObjects:(CPSet)objects { + if (!aKey) + return; + + if (!self[KVOProxyKey]) + { + if (!self._willChangeMessageCounter) + self._willChangeMessageCounter = new Object(); + + if (!self._willChangeMessageCounter[aKey]) + self._willChangeMessageCounter[aKey] = 1; + else + self._willChangeMessageCounter[aKey] += 1; + } } - (void)didChangeValueForKey:(CPString)aKey withSetMutation:(CPKeyValueSetMutationKind)aMutationKind usingObjects:(CPSet)objects { + if (!self[KVOProxyKey]) + { + if (self._willChangeMessageCounter && self._willChangeMessageCounter[aKey]) + { + self._willChangeMessageCounter[aKey] -= 1; + + if (!self._willChangeMessageCounter[aKey]) + delete self._willChangeMessageCounter[aKey]; + } + else + [CPException raise:@"CPKeyValueObservingException" reason:@"'didChange...' message called without prior call of 'willChange...'"]; + } } - (void)addObserver:(id)anObserver forKeyPath:(CPString)aPath options:(unsigned)options context:(id)aContext @@ -804,7 +885,20 @@ var kvoNewAndOld = CPKeyValueObservingOptionNew | CPKeyValueObservingOpti { var level = _nestingForKey[aKey]; if (!changes || !level) - [CPException raise:@"CPKeyValueObservingException" reason:@"'didChange...' message called without prior call of 'willChange...'"]; + { + if (_targetObject._willChangeMessageCounter && _targetObject._willChangeMessageCounter[aKey]) + { + // Close unobserved willChange for a given key. + _targetObject._willChangeMessageCounter[aKey] -= 1; + + if (!_targetObject._willChangeMessageCounter[aKey]) + delete _targetObject._willChangeMessageCounter[aKey]; + + return; + } + else + [CPException raise:@"CPKeyValueObservingException" reason:@"'didChange...' message called without prior call of 'willChange...'"]; + } _nestingForKey[aKey] = level - 1; if (level - 1 > 0) diff --git a/Tests/Foundation/CPKVOTest.j b/Tests/Foundation/CPKVOTest.j index 04104f33e..e05fd9785 100644 --- a/Tests/Foundation/CPKVOTest.j +++ b/Tests/Foundation/CPKVOTest.j @@ -14,11 +14,14 @@ id focus; CPInteger observationCount; + + CPInteger testNestedNotificationsBobCount; } - (void)setUp { _sawObservation = NO; + testNestedNotificationsBobCount = 0; } - (void)testAddObserver @@ -465,6 +468,44 @@ [self assertTrue:newImp === oldImp]; } +- (void)testNestedNotifications +{ + var bob = [[PersonTester alloc] init]; + + [bob willChangeValueForKey:@"name"]; + [self assertTrue:bob._willChangeMessageCounter[@"name"] === 1]; + [bob didChangeValueForKey:@"name"]; + [self assertTrue:!bob._willChangeMessageCounter[@"name"]]; + + + [bob willChangeValueForKey:@"name"]; + [self assertTrue:bob._willChangeMessageCounter[@"name"] === 1]; + [bob willChangeValueForKey:@"phoneNumber"] + [self assertTrue:bob._willChangeMessageCounter[@"phoneNumber"] === 1]; + [bob didChangeValueForKey:@"phoneNumber"]; + [self assertTrue:!bob._willChangeMessageCounter[@"phoneNumber"]]; + [bob didChangeValueForKey:@"name"]; + [self assertTrue:!bob._willChangeMessageCounter[@"name"]]; + + [bob willChangeValueForKey:@"name"]; + [self assertTrue:bob._willChangeMessageCounter[@"name"] === 1]; + [bob addObserver:self forKeyPath:@"name" options:nil context:@"testNestedNotifications"]; + [bob addObserver:self forKeyPath:@"phoneNumber" options:nil context:@"testNestedNotifications"]; + [bob willChangeValueForKey:@"phoneNumber"]; + [self assertTrue:!bob._willChangeMessageCounter[@"phoneNumber"]]; + [bob didChangeValueForKey:@"phoneNumber"]; + [self assertTrue:!bob._willChangeMessageCounter[@"phoneNumber"]]; + [self assertTrue:testNestedNotificationsBobCount === 1]; + [bob didChangeValueForKey:@"name"]; + [self assertTrue:!bob._willChangeMessageCounter[@"name"]]; + + [bob willChangeValueForKey:@"name"]; + [self assertTrue:!bob._willChangeMessageCounter[@"name"]]; + [bob didChangeValueForKey:@"name"]; + [self assertTrue:!bob._willChangeMessageCounter[@"name"]]; + [self assertTrue:testNestedNotificationsBobCount === 2]; +} + - (void)observeValueForKeyPath:(CPString)aKeyPath ofObject:(id)anObject change:(CPDictionary)changes context:(id)aContext { var oldValue = [changes objectForKey:CPKeyValueChangeOldKey], @@ -669,6 +710,10 @@ [self assert:newValue equals:@"Jo Bob Ray"]; break; + case "testNestedNotifications": + testNestedNotificationsBobCount += 1; + break; + default: [self assertFalse:YES message:@"unhandled observation, must be an error"]; return;