[Author: dmaclach]

Added some KVO debugging help. Specifically forces can access instance variables by KVO to NO
in debug mode.

R=thomasvl
DELTA=340  (269 added, 21 deleted, 50 changed)

1 files changed, 245 insertions, 72 deletions

diff --git a/Foundation/GTMNSObject+KeyValueObserving.m b/Foundation/GTMNSObject+KeyValueObserving.m
index a665abd..dde77e5 100644
--- a/Foundation/GTMNSObject+KeyValueObserving.m
+++ b/Foundation/GTMNSObject+KeyValueObserving.m
@@ -1,5 +1,5 @@
 //
-//  GTMNSObject+KeyValueObserving.h
+//  GTMNSObject+KeyValueObserving.m
 //
 //  Copyright 2009 Google Inc.
 //
@@ -25,11 +25,12 @@
 
 // This code is based on code by Michael Ash. 
 // See comment in header.
-
+#import "GTMDefines.h"
 #import "GTMNSObject+KeyValueObserving.h"
 #import "GTMDefines.h"
 #import "GTMDebugSelectorValidation.h"
 #import "GTMObjC2Runtime.h"
+#import "GTMMethodCheck.h"
 
 // A singleton that works as a dispatch center for KVO 
 // -[NSObject observeValueForKeyPath:ofObject:change:context:] and turns them
@@ -37,14 +38,14 @@
 // GTMKeyValueObservingHelpers, and keys them via the key generated by
 // -dictionaryKeyForObserver:ofObject:forKeyPath:selector.
 @interface GTMKeyValueObservingCenter : NSObject {
-@private
+ @private
   NSMutableDictionary *observerHelpers_;
 }
 
 + (id)defaultCenter;
 
 - (void)addObserver:(id)observer 
-             ofObject:(id)target 
+           ofObject:(id)target 
          forKeyPath:(NSString *)keyPath 
            selector:(SEL)selector 
            userInfo:(id)userInfo 
@@ -64,7 +65,7 @@
   __weak id observer_;
   SEL selector_;
   id userInfo_;
-  id target_;
+  __weak id target_;
   NSString* keyPath_;
 }
 
@@ -120,7 +121,23 @@ static char* GTMKeyValueObservingHelperContext
           NSStringFromSelector(selector_)];
 }
 
+#if GTM_SUPPORT_GC
+
+- (void)finalize {
+  if (target_) {
+    _GTMDevLog(@"Didn't deregister %@", self);
+    [self deregister];
+  }
+  [super finalize];
+}
+
+#endif  // GTM_SUPPORT_GC
+
 - (void)dealloc {
+  if (target_) {
+    _GTMDevLog(@"Didn't deregister %@", self);
+    [self deregister];
+  }
   [userInfo_ release];
   [keyPath_ release];
   [super dealloc];
@@ -152,6 +169,7 @@ static char* GTMKeyValueObservingHelperContext
 
 - (void)deregister {
   [target_ removeObserver:self forKeyPath:keyPath_];
+  target_ = nil;
 }
 
 @end
@@ -194,8 +212,13 @@ static char* GTMKeyValueObservingHelperContext
                       ofObject:(id)target 
                     forKeyPath:(NSString *)keyPath 
                       selector:(SEL)selector {
-  NSString *key = [NSString stringWithFormat:@"%p:%p:%@:%p", 
-                   observer, target, keyPath, selector];
+  NSString *key = nil;
+  if (!target && !keyPath && !selector) {
+    key = [NSString stringWithFormat:@"%p:", observer];
+  } else {
+    key = [NSString stringWithFormat:@"%p:%@:%p:%p", 
+           observer, keyPath, selector, target];
+  }
   return key;
 }
 
@@ -222,6 +245,7 @@ static char* GTMKeyValueObservingHelperContext
     if (oldHelper) {
       _GTMDevLog(@"%@ already observing %@ forKeyPath %@", 
                  observer, target, keyPath);
+      [oldHelper deregister];
     }
 #endif // DEBUG
     [observerHelpers_ setObject:helper forKey:key];
@@ -237,19 +261,27 @@ static char* GTMKeyValueObservingHelperContext
                                  ofObject:target
                                forKeyPath:keyPath 
                                  selector:selector];
-  GTMKeyValueObservingHelper *helper = nil;
+  NSMutableArray *allValidHelperKeys = [NSMutableArray array];
+  NSArray *allValidHelpers = nil;
   @synchronized(self) {
-    helper = [[observerHelpers_ objectForKey:key] retain];
+    
+    NSString *helperKey;
+    GTM_FOREACH_OBJECT(helperKey, [observerHelpers_ allKeys]) {
+      if ([helperKey hasPrefix:key]) {
+        [allValidHelperKeys addObject:helperKey];
+      }
+    }
 #if DEBUG
-    if (!helper) {
+    if ([allValidHelperKeys count] == 0) {
       _GTMDevLog(@"%@ was not observing %@ with keypath %@", 
                  observer, target, keyPath);
     }
 #endif // DEBUG
-    [observerHelpers_ removeObjectForKey:key];
+    allValidHelpers = [observerHelpers_ objectsForKeys:allValidHelperKeys 
+                                        notFoundMarker:[NSNull null]];
+    [observerHelpers_ removeObjectsForKeys:allValidHelperKeys];
   }
-  [helper deregister];
-  [helper release];
+  [allValidHelpers makeObjectsPerformSelector:@selector(deregister)];
 }
 
 @end
@@ -265,10 +297,11 @@ static char* GTMKeyValueObservingHelperContext
                 @"Missing observer, keyPath, or selector");
   GTMKeyValueObservingCenter *center 
     = [GTMKeyValueObservingCenter defaultCenter];
-  GTMAssertSelectorNilOrImplementedWithArguments(observer,
-                                                 selector, 
-                                                 @encode(GTMKeyValueChangeNotification *), 
-                                                 NULL);
+  GTMAssertSelectorNilOrImplementedWithArguments(
+      observer,
+      selector, 
+      @encode(GTMKeyValueChangeNotification *), 
+      NULL);
   [center addObserver:observer 
              ofObject:self 
            forKeyPath:keyPath 
@@ -284,16 +317,26 @@ static char* GTMKeyValueObservingHelperContext
                 @"Missing observer, keyPath, or selector");
   GTMKeyValueObservingCenter *center 
     = [GTMKeyValueObservingCenter defaultCenter];
-  GTMAssertSelectorNilOrImplementedWithArguments(observer,
-                                                 selector, 
-                                                 @encode(GTMKeyValueChangeNotification *), 
-                                                 NULL);  
+  GTMAssertSelectorNilOrImplementedWithArguments(
+      observer,
+      selector, 
+      @encode(GTMKeyValueChangeNotification *), 
+      NULL);  
   [center removeObserver:observer 
                 ofObject:self 
               forKeyPath:keyPath 
                 selector:selector];
 }
-   
+
+- (void)gtm_stopObservingAllKeyPaths {
+  GTMKeyValueObservingCenter *center 
+    = [GTMKeyValueObservingCenter defaultCenter];
+  [center removeObserver:self 
+                ofObject:nil 
+              forKeyPath:nil 
+                selector:Nil];
+} 
+
 @end
 
 
@@ -362,13 +405,82 @@ static char* GTMKeyValueObservingHelperContext
 
 #ifdef DEBUG
 
-// This category exists to attempt to help debug tricky KVO issues.
-// Set the GTMDebugKVO environment variable to 1 and you will get a whole
-// pile of KVO notifications that may help you track down problems.
+static void SwizzleMethodsInClass(Class cls, SEL sel1, SEL sel2) {
+  Method m1 = class_getInstanceMethod(cls, sel1);
+  Method m2 = class_getInstanceMethod(cls, sel2);
+  method_exchangeImplementations(m1, m2);
+}
+
+static void SwizzleClassMethodsInClass(Class cls, SEL sel1, SEL sel2) {
+  Method m1 = class_getClassMethod(cls, sel1);
+  Method m2 = class_getClassMethod(cls, sel2);
+  method_exchangeImplementations(m1, m2);
+}
+
+// This category exists to attempt to help deal with tricky KVO issues.
+// KVO is a wonderful technology in some ways, but is extremely fragile and
+// allows developers a lot of freedom to shoot themselves in the foot.
+// Refactoring an app that uses a lot of KVO can be really difficult, as can
+// debugging it.
+// These are some tools that we have found useful when working with KVO. Note
+// that these tools are only on in Debug builds.
+//
+// We have divided these tools up into two categories: Checks and Debugs.
+//
+// Debugs
+// Debugs are mainly for logging all the KVO/KVC that is occurring in your
+// application. To enable our KVO debugging, set the GTMDebugKVO environment 
+// variable to 1 and you will get a whole pile of KVO logging that may help you 
+// track down problems.
+// bash - export GTMDebugKVO=1
+// csh/tcsh - setenv GTMDebugKVO 1
+//
+// Checks
+// First we believe that instance variables should be private by default,
+// and that any KVO should be done via accessors. Apple by default allows KVO
+// to get at instance variables directly. Since our coding standards define
+// that instance variables should be @private, we feel that KVO shouldn't be
+// breaking this encapsulation. Unfortunately the @private, @protected
+// designators are a compile time convention only, and don't get carried over
+// into the runtime, so there's no way to check on an individual iVar what
+// it's visibility is. We therefore assume that an instance variable is private,
+// and disallow KVO access to instance variables. The problem with most KVO
+// issues is that they occur at runtime and unless you execute that case you
+// may never see the bug until it's too late. We try to force KVO issues to
+// rear their head at the time of the observing if at all possible.
+// Checks are on by default in debug builds. They can be turned off by defining
+// the compile flag GTM_PERFORM_KVO_CHECKS to 0 
+// i.e. #define GTM_PERFORM_KVO_CHECKS 0, or set it 
+// in GCC_PREPROCESSOR_DEFINITIONS.
+//
+// Checks work at a couple of different levels.
+// The most restrictive of the checks is that we set 
+// |accessInstanceVariablesDirectly| to NO by default. This means that if you
+// attempt to perform KVO on an instance variable, you will get an exception
+// thrown.
+// Also, when adding an observer, we check to see if any member of the path 
+// starts or ends with _ which by convention denotes an instance variable. If so 
+// we warn you about attempting to access a ivar directly.
+
 @interface NSObject (GTMDebugKeyValueObserving)
 @end
 
 @implementation NSObject (GTMDebugKeyValueObserving)
+GTM_METHOD_CHECK(NSObject, _gtmDebugAddObserver:forKeyPath:options:context:);
+GTM_METHOD_CHECK(NSObject, _gtmDebugRemoveObserver:forKeyPath:);
+GTM_METHOD_CHECK(NSObject, _gtmDebugWillChangeValueForKey:);
+GTM_METHOD_CHECK(NSObject, _gtmDebugDidChangeValueForKey:);
+GTM_METHOD_CHECK(NSArray, 
+    _gtmDebugArrayAddObserver:toObjectsAtIndexes:forKeyPath:options:context:);
+GTM_METHOD_CHECK(NSArray, 
+    _gtmDebugArrayRemoveObserver:fromObjectsAtIndexes:forKeyPath:);
+GTM_METHOD_CHECK(NSObject, 
+    _gtmCheckAddObserver:forKeyPath:options:context:);
+GTM_METHOD_CHECK(NSArray, 
+    _gtmCheckAddObserver:toObjectsAtIndexes:forKeyPath:options:context:);
+GTM_METHOD_CHECK(NSObject, 
+    _gtmAccessInstanceVariablesDirectly);
+
 + (void)load {
   NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
   NSDictionary *env = [[NSProcessInfo processInfo] environment];
@@ -380,48 +492,43 @@ static char* GTMKeyValueObservingHelperContext
     debug = ([debugKeyValue hasPrefix:@"Y"] || [debugKeyValue hasPrefix:@"T"] ||
              [debugKeyValue intValue]);
   }
+  Class cls = Nil;
   if (debug) {
-    Class cls = [NSObject class];
-    SEL addSelector 
-      = NSSelectorFromString(@"addObserver:forKeyPath:options:context:");
-    SEL removeSelector = NSSelectorFromString(@"removeObserver:forKeyPath:");
-    SEL debugAddSelector 
-      = NSSelectorFromString(@"_gtmDebugAddObserver:forKeyPath:options:context:");
-    SEL debugRemoveSelector 
-      = NSSelectorFromString(@"_gtmDebugRemoveObserver:forKeyPath:");
-    SEL willChangeValueSelector = NSSelectorFromString(@"willChangeValueForKey:");
-    SEL didChangeValueSelector = NSSelectorFromString(@"didChangeValueForKey:");
-    SEL debugWillChangeValueSelector 
-      = NSSelectorFromString(@"_gtmDebugWillChangeValueForKey:");
-    SEL debugDidChangeValueSelector 
-      = NSSelectorFromString(@"_gtmDebugDidChangeValueForKey:");
-    
-    Method m1 = class_getInstanceMethod(cls, addSelector);
-    Method m2 = class_getInstanceMethod(cls, debugAddSelector);
-    method_exchangeImplementations(m1, m2);
-    m1 = class_getInstanceMethod(cls, removeSelector);
-    m2 = class_getInstanceMethod(cls, debugRemoveSelector);
-    method_exchangeImplementations(m1, m2);
-    m1 = class_getInstanceMethod(cls, willChangeValueSelector);
-    m2 = class_getInstanceMethod(cls, debugWillChangeValueSelector);
-    method_exchangeImplementations(m1, m2);
-    m1 = class_getInstanceMethod(cls, didChangeValueSelector);
-    m2 = class_getInstanceMethod(cls, debugDidChangeValueSelector);
-    method_exchangeImplementations(m1, m2);
-    
-    debugAddSelector 
-      = NSSelectorFromString(@"_gtmDebugArrayAddObserver:forKeyPath:options:context:");
-    debugRemoveSelector 
-      = NSSelectorFromString(@"_gtmDebugArrayRemoveObserver:forKeyPath:");
-    
+    cls = [NSObject class];
+    SwizzleMethodsInClass(cls, 
+        @selector(addObserver:forKeyPath:options:context:),
+        @selector(_gtmDebugAddObserver:forKeyPath:options:context:));
+    SwizzleMethodsInClass(cls, 
+        @selector(removeObserver:forKeyPath:),
+        @selector(_gtmDebugRemoveObserver:forKeyPath:));
+    SwizzleMethodsInClass(cls, 
+        @selector(willChangeValueForKey:),
+        @selector(_gtmDebugWillChangeValueForKey:));
+    SwizzleMethodsInClass(cls, 
+        @selector(didChangeValueForKey:),
+        @selector(_gtmDebugDidChangeValueForKey:));
     cls = [NSArray class];
-    m1 = class_getInstanceMethod(cls, addSelector);
-    m2 = class_getInstanceMethod(cls, debugAddSelector);
-    method_exchangeImplementations(m1, m2);
-    m1 = class_getInstanceMethod(cls, removeSelector);
-    m2 = class_getInstanceMethod(cls, debugRemoveSelector);
-    method_exchangeImplementations(m1, m2);
+    SwizzleMethodsInClass(cls, 
+        @selector(addObserver:toObjectsAtIndexes:forKeyPath:options:context:),
+        @selector(_gtmDebugArrayAddObserver:toObjectsAtIndexes:forKeyPath:options:context:));
+    SwizzleMethodsInClass(cls, 
+        @selector(removeObserver:fromObjectsAtIndexes:forKeyPath:),
+        @selector(_gtmDebugArrayRemoveObserver:fromObjectsAtIndexes:forKeyPath:));
   }
+#if GTM_PERFORM_KVO_CHECKS
+  cls = [NSObject class];
+  SwizzleMethodsInClass(cls,
+      @selector(addObserver:forKeyPath:options:context:),
+      @selector(_gtmCheckAddObserver:forKeyPath:options:context:));  
+  SwizzleClassMethodsInClass(cls, 
+      @selector(accessInstanceVariablesDirectly), 
+      @selector(_gtmAccessInstanceVariablesDirectly));
+  cls = [NSArray class];
+  SwizzleMethodsInClass(cls, 
+      @selector(addObserver:toObjectsAtIndexes:forKeyPath:options:context:),
+      @selector(_gtmCheckAddObserver:toObjectsAtIndexes:forKeyPath:options:context:));
+  
+#endif  // GTM_PERFORM_KVO_CHECKS
   [pool release];
 }
 
@@ -434,25 +541,34 @@ static char* GTMKeyValueObservingHelperContext
                      options:options context:context];
 }
 
-- (void)_gtmDebugArrayAddObserver:(NSObject *)observer 
+- (void)_gtmDebugArrayAddObserver:(NSObject *)observer
+               toObjectsAtIndexes:(NSIndexSet *)indexes
                        forKeyPath:(NSString *)keyPath 
                           options:(NSKeyValueObservingOptions)options
                           context:(void *)context {
-  _GTMDevLog(@"Array adding observer %@ to %@ keypath '%@'", observer, self, keyPath);
-  [self _gtmDebugArrayAddObserver:observer forKeyPath:keyPath 
+  _GTMDevLog(@"Array adding observer %@ to indexes %@ of %@ keypath '%@'", 
+             observer, indexes, self, keyPath);
+  [self _gtmDebugArrayAddObserver:observer 
+               toObjectsAtIndexes:indexes
+                       forKeyPath:keyPath 
                           options:options context:context];
 }
 
-- (void)_gtmDebugRemoveObserver:(NSObject *)observer 
+- (void)_gtmDebugRemoveObserver:(NSObject *)observer
                      forKeyPath:(NSString *)keyPath {
-  _GTMDevLog(@"Removing observer %@ from %@ keypath '%@'", observer, self, keyPath);
+  _GTMDevLog(@"Removing observer %@ from %@ keypath '%@'", 
+             observer, self, keyPath);
   [self _gtmDebugRemoveObserver:observer forKeyPath:keyPath];
 }
 
 - (void)_gtmDebugArrayRemoveObserver:(NSObject *)observer 
+                fromObjectsAtIndexes:(NSIndexSet *)indexes
                           forKeyPath:(NSString *)keyPath {
-  _GTMDevLog(@"Array removing observer %@ from %@ keypath '%@'", observer, self, keyPath);
-  [self _gtmDebugArrayRemoveObserver:observer forKeyPath:keyPath];
+  _GTMDevLog(@"Array removing observer %@ from indexes %@ of %@ keypath '%@'", 
+             indexes, observer, self, keyPath);
+  [self _gtmDebugArrayRemoveObserver:observer 
+                fromObjectsAtIndexes:indexes 
+                          forKeyPath:keyPath];
 }
 
 - (void)_gtmDebugWillChangeValueForKey:(NSString*)key {
@@ -465,7 +581,64 @@ static char* GTMKeyValueObservingHelperContext
   [self _gtmDebugDidChangeValueForKey:key];
 }
 
+#if GTM_PERFORM_KVO_CHECKS
+
+- (void)_gtmCheckAddObserver:(NSObject *)observer
+                  forKeyPath:(NSString *)keyPath
+                     options:(NSKeyValueObservingOptions)options
+                     context:(void *)context {
+  NSArray *keyPathElements = [keyPath componentsSeparatedByString:@"."];
+  NSString *element;
+  GTM_FOREACH_OBJECT(element, keyPathElements) {
+    if ([element hasPrefix:@"_"] || [element hasSuffix:@"_"]) {
+      _GTMDevLog(@"warning: %@ is registering an observation on what appears "
+                 @"to be a private ivar of %@ (or a sub keyed object) with "
+                 @"element %@ of keyPath %@.", observer, self, element,
+                 keyPath);
+    }
+  }
+  [self _gtmCheckAddObserver:observer 
+                  forKeyPath:keyPath 
+                     options:options 
+                     context:context];
+}
+
+- (void)_gtmCheckAddObserver:(NSObject *)observer
+          toObjectsAtIndexes:(NSIndexSet *)indexes
+                  forKeyPath:(NSString *)keyPath
+                     options:(NSKeyValueObservingOptions)options
+                     context:(void *)context {
+  NSArray *keyPathElements = [keyPath componentsSeparatedByString:@"."];
+  NSString *element;
+  GTM_FOREACH_OBJECT(element, keyPathElements) {
+    if ([element hasPrefix:@"_"] || [element hasSuffix:@"_"]) {
+      _GTMDevLog(@"warning: %@ is registering an observation on what appears "
+                 @"to be a private ivar of %@ (or a sub keyed object) with "
+                 @"element %@ of keyPath %@.", observer, self, element,
+                 keyPath);
+    }
+  }
+  [self _gtmCheckAddObserver:observer
+          toObjectsAtIndexes:indexes
+                  forKeyPath:keyPath 
+                     options:options 
+                     context:context];
+}
+
+
++ (BOOL)_gtmAccessInstanceVariablesDirectly {
+  // Apple has lots of "bad" direct instance variable accesses, so we
+  // only want to check our code, as opposed to library code.
+  
+  // If this turns out to be slow, we may want to consider a cache to speed
+  // things up.
+  NSBundle *bundle = [NSBundle bundleForClass:self];
+  NSString *path = [bundle bundlePath];
+  return [path rangeOfString:@"/Library/"].location != NSNotFound;
+}
+
+#endif  // GTM_PERFORM_KVO_CHECKS
+
 @end
 
 #endif  // DEBUG
-