Fix KVO bugs related to cyclical observer dependencies (#608) (#612)

* Fix KVO bugs related to cyclical observer dependencies (#608)

- Replaced fragile dependent-keypath visited tracking with node keys `(object pointer, key name)` in dependency expansion.
- Added explicit ancestor/on-stack tracking to distinguish true cycles from safe revisits
- On revisit:
  - if node is on ancestor stack: treat as cycle and stop expansion
  - if visited but not on stack: allow expansion to continue
- This prevents infinite recursive rebuilds on cyclic dependencies while preserving correct branch wiring after nil-to-non-nil rematerialization and shared-path alias cases.

* Refactor and fix gcc errors

* Disabled tests on non-clang platforms

---------

Co-authored-by: Hendrik Hübner <hhuebner@MacBookPro.localdomain>

Author: HendrikHuebner

Source/NSKVOInternal.h

@@ -106,7 +106,8 @@
   NSMutableDictionary<NSString *, NSMutableArray<_NSKVOKeyObserver *> *>
                            *_keyObserverMap;
   NSInteger                 _dependencyDepth;
-  NSMutableSet<NSString *> *_existingDependentKeys;
+  NSMutableSet<id>         *_existingDependentKeys;
+  NSMutableSet<id>         *_dependencyAncestorKeys;
   gs_mutex_t                _lock;
 }
 - (instancetype)init;

Source/NSKVOSupport.m

@@ -207,38 +207,83 @@ - (void) dealloc
   [super dealloc];
 }
 
-- (void) pushDependencyStack
+// While exploring the observer graph, for dependencies, we use this to 
+// detect cycles to prevent infinite recursion. 
+- (void) beginDependencyExpansionScope
 {
   GS_MUTEX_LOCK(_lock);
   if (_dependencyDepth == 0)
     {
       _existingDependentKeys = [NSMutableSet new];
+      _dependencyAncestorKeys = [NSMutableSet new];
     }
   ++_dependencyDepth;
   GS_MUTEX_UNLOCK(_lock);
 }
 
-- (BOOL) lockDependentKeypath: (NSString *)keypath
+/* Nodes (observable values) in the observer graph are uniquely identified
+ * using a combination of the object pointer and the name of the value */
+- (id) dependencyKeyForObject: (id)object key: (NSString *)key
 {
+  return [NSArray arrayWithObjects:
+                   (object ?: [NSNull null]),
+                   (key ?: @""),
+                   nil];
+}
+
+- (void) pushObserverToCurrentAncestorStack: (_NSKVOKeyObserver *)keyObserver
+{
+  id ancestorKey = [self dependencyKeyForObject: keyObserver.object
+                                            key: keyObserver.key];
+  GS_MUTEX_LOCK(_lock);
+  [_dependencyAncestorKeys addObject: ancestorKey];
+  GS_MUTEX_UNLOCK(_lock);
+}
+
+- (void) popObserverFromCurrentAncestorStack: (_NSKVOKeyObserver *)keyObserver
+{
+  id ancestorKey = [self dependencyKeyForObject: keyObserver.object
+                                            key: keyObserver.key];
   GS_MUTEX_LOCK(_lock);
-  if ([_existingDependentKeys containsObject:keypath])
+  [_dependencyAncestorKeys removeObject: ancestorKey];
+  GS_MUTEX_UNLOCK(_lock);
+}
+
+/// Mark keypath as visited in the current dependency-expansion scope.
+- (BOOL) checkDependencyForCycle: (NSString *)keypath
+                             forNode: (_NSKVOKeyObserver *)keyObserver
+{
+  NSString *dependentKey;
+  NSString *unusedRemainder;
+  id visitToken;
+
+  dependentKey = _NSKVCSplitKeypath(keypath, &unusedRemainder);
+  visitToken = [self dependencyKeyForObject: keyObserver.object
+                                        key: dependentKey];
+  GS_MUTEX_LOCK(_lock);
+  if ([_existingDependentKeys containsObject:visitToken])
     {
+      BOOL isCycle = [_dependencyAncestorKeys containsObject: visitToken];
       GS_MUTEX_UNLOCK(_lock);
-      return NO;
+      // If it's on the current ancestor stack, treat as cycle and dedup.
+      // If it's already visited but not on stack, allow expansion to continue.
+      return !isCycle;
     }
-  [_existingDependentKeys addObject:keypath];
+  [_existingDependentKeys addObject:visitToken];
   GS_MUTEX_UNLOCK(_lock);
   return YES;
 }
 
-- (void) popDependencyStack
+- (void) endDependencyExpansionScope
 {
   GS_MUTEX_LOCK(_lock);
   --_dependencyDepth;
   if (_dependencyDepth == 0)
     {
       [_existingDependentKeys release];
       _existingDependentKeys = nil;
+      [_dependencyAncestorKeys release];
+      _dependencyAncestorKeys = nil;
     }
   GS_MUTEX_UNLOCK(_lock);
 }
@@ -344,28 +389,40 @@ - (bool) isEmpty
           NSArray 		*affectedKeyObservers;
           NSMutableArray 	*dependentObservers;
 
+          affectedKeyObservers = keyObserver.affectedObservers;
+
           /* affectedKeyObservers is the list of observers that must be notified
            * of changes. If we have descendants, we have to add ourselves to the
            * growing list of affected keys. If not, we must pass it along
            * unmodified. (This is a minor optimization: we don't need to signal
-           * for our own reconstruction
-           *  if we have no subpath observers.)
-	   */
-          affectedKeyObservers = (keyObserver.restOfKeypath
-	    ? ([keyObserver.affectedObservers arrayByAddingObject:keyObserver]
-	    ?: [NSArray arrayWithObject:keyObserver])
-	    : keyObserver.affectedObservers);
-
-          [observationInfo pushDependencyStack];
-          /* Don't allow our own key to be recreated.
-	   */
-          [observationInfo lockDependentKeypath:keyObserver.key];
+           * for our own reconstruction if we have no subpath observers.)
+           */
+          if (keyObserver.restOfKeypath)
+          {
+            if (affectedKeyObservers)
+              {
+                affectedKeyObservers =
+                  [affectedKeyObservers arrayByAddingObject:keyObserver];
+              }
+            else
+              {
+                affectedKeyObservers = [NSArray arrayWithObject:keyObserver];
+              }
+          }
 
+          [observationInfo beginDependencyExpansionScope];
+          [observationInfo pushObserverToCurrentAncestorStack: keyObserver];
+          /* Don't allow our own key to be recreated. */
+          [observationInfo checkDependencyForCycle:keyObserver.key
+                                               forNode:keyObserver];
+
+          /* The observers, which affect us */
           dependentObservers =
             [NSMutableArray arrayWithCapacity:[valueInfluencingKeys count]];
           for (NSString *dependentKeypath in valueInfluencingKeys)
             {
-              if ([observationInfo lockDependentKeypath:dependentKeypath])
+              if ([observationInfo checkDependencyForCycle:dependentKeypath
+                                                       forNode:keyObserver])
                 {
                   _NSKVOKeyObserver *dependentObserver
                     = _addKeypathObserver(object, dependentKeypath,
@@ -379,7 +436,8 @@ - (bool) isEmpty
             }
           keyObserver.dependentObservers = dependentObservers;
 
-          [observationInfo popDependencyStack];
+          [observationInfo popObserverFromCurrentAncestorStack: keyObserver];
+          [observationInfo endDependencyExpansionScope];
         }
     }
   else
@@ -458,6 +516,7 @@ - (bool) isEmpty
     {
       _addNestedObserversAndOptionallyDependents(keyObserver, true);
       _addKeyObserver(keyObserver);
+    } else {
     }
 
   return keyObserver;
@@ -847,7 +906,8 @@ - (void) removeObserver: (id)observer forKeyPath:(NSString *)keyPath
 {
   _NSKVOObservationInfo *observationInfo
     = (__bridge _NSKVOObservationInfo *) [notifyingObject observationInfo];
-  for (_NSKVOKeyObserver *keyObserver in [observationInfo observersForKey:key])
+  NSArray<_NSKVOKeyObserver *> *observers = [observationInfo observersForKey:key];
+  for (_NSKVOKeyObserver *keyObserver in observers)
     {
       _NSKVOKeypathObserver *keypathObserver;
 

Tests/base/NSKVOSupport/ComplexNilAndCycleTest.m

@@ -0,0 +1,179 @@
+#import <Foundation/Foundation.h>
+#import "ObjectTesting.h"
+
+@class BNode;
+
+@interface CNode : NSObject
+{
+  NSInteger _f;
+  NSObject *_d;
+  BNode *_b;
+}
+@property (nonatomic, retain) NSObject *d;
+@property (nonatomic, retain) BNode *b;
+@property (nonatomic, assign) NSInteger f;
+@end
+@implementation CNode
+@synthesize d = _d;
+@synthesize b = _b;
+- (NSInteger) f
+{
+  return _f;
+}
+- (void) setF: (NSInteger)f
+{
+  BOOL notifyD = (self.d != nil);
+  [self willChangeValueForKey: @"f"];
+  if (notifyD)
+    {
+      [self willChangeValueForKey: @"d"];
+    }
+  _f = f;
+  if (notifyD)
+    {
+      [self didChangeValueForKey: @"d"];
+    }
+  [self didChangeValueForKey: @"f"];
+}
+@end
+
+@interface BNode : NSObject
+{
+  CNode *_c;
+  NSInteger _e;
+}
+@property (nonatomic, retain) CNode *c;
+@property (nonatomic, assign) NSInteger e;
+@end
+@implementation BNode
+@synthesize c = _c;
+@synthesize e = _e;
+@end
+
+@interface ANode : NSObject
+{
+  BNode *_b;
+  CNode *_c;
+}
+@property (nonatomic, retain) BNode *b;
+@property (nonatomic, retain) CNode *c;
+@end
+@implementation ANode
+@synthesize b = _b;
+@synthesize c = _c;
+@end
+
+@interface RootNode : NSObject
+{
+  ANode *_a;
+}
+@property (nonatomic, retain) ANode *a;
+@property (nonatomic, readonly) BOOL x;
+@end
+@implementation RootNode
+@synthesize a = _a;
++ (NSSet *) keyPathsForValuesAffectingX
+{
+  return [NSSet setWithObjects: @"a.b.c.d", @"a.c.b.e", nil];
+}
+- (BOOL) x
+{
+  return (self.a.b.c.d != nil) || (self.a.c.b.e > 0);
+}
+@end
+
+@interface TestObserver : NSObject
+{
+@public
+  NSUInteger calls;
+}
+@end
+@implementation TestObserver
+- (void) observeValueForKeyPath: (NSString *)keyPath
+                       ofObject: (id)object
+                         change: (NSDictionary *)change
+                        context: (void *)context
+{
+  (void)keyPath;
+  (void)object;
+  (void)change;
+  (void)context;
+  calls++;
+}
+@end
+
+int main(void)
+{
+  CREATE_AUTORELEASE_POOL(pool);
+
+#if !defined(clang)
+  testHopeful = YES;
+#endif
+
+// testcases here
+  BNode *b = AUTORELEASE([BNode new]);
+  b.e = 1;
+
+  CNode *cForA = AUTORELEASE([CNode new]);
+  cForA.b = b;
+
+  CNode *cForB = AUTORELEASE([CNode new]);
+  cForB.d = nil;
+  cForB.f = 10;
+
+  ANode *a = AUTORELEASE([ANode new]);
+  a.b = b;
+  a.c = cForA;
+  b.c = cForB;
+
+  RootNode *root = AUTORELEASE([RootNode new]);
+  root.a = a;
+
+  TestObserver *obs = AUTORELEASE([TestObserver new]);
+  [root addObserver: obs forKeyPath: @"x" options: 0 context: NULL];
+
+  obs->calls = 0;
+  b.e = 2;
+  PASS(obs->calls == 1, "updating e should trigger notification");
+
+  CNode *newCForB = AUTORELEASE([CNode new]);
+  newCForB.d = AUTORELEASE([NSObject new]);
+  newCForB.f = 100;
+  obs->calls = 0;
+  b.c = newCForB;
+  PASS(obs->calls == 1,
+       "updating c with a new object where d is non-nil should trigger notification");
+
+  obs->calls = 0;
+  b.e = 3;
+  PASS(obs->calls == 1, "updating e should still notify");
+
+  obs->calls = 0;
+  newCForB.f = 101;
+  PASS(obs->calls == 1, "updating f should notify");
+
+  obs->calls = 0;
+  newCForB.d = nil;
+  PASS(obs->calls == 1, "setting d to nil again should notify");
+
+  obs->calls = 0;
+  newCForB.f = 102;
+  PASS(obs->calls == 0, "updating f should not notify after d became nil");
+
+  obs->calls = 0;
+  b.c = nil;
+  PASS(obs->calls == 1, "setting c to nil should notify");
+
+  obs->calls = 0;
+  b.e = 4;
+  PASS(obs->calls == 1, "updating e should still notify");
+
+  [root removeObserver: obs forKeyPath: @"x"];
+  
+#if !defined(clang)
+  testHopeful = NO;
+#endif
+
+  DESTROY(pool);
+  return 0;
+}