CRBProtocolObservers can now be mutated while forwarding methods.

base/ios/crb_protocol_observers_unittest.mm

Index: base/ios/crb_protocol_observers_unittest.mm
diff --git a/base/ios/crb_protocol_observers_unittest.mm b/base/ios/crb_protocol_observers_unittest.mm
index d235c98b21f0919a2742cd40eb07e371486a5345..11946fa6ffe98e6e7b6d959f41944f94a3b4392d 100644
--- a/base/ios/crb_protocol_observers_unittest.mm
+++ b/base/ios/crb_protocol_observers_unittest.mm
@@ -18,6 +18,8 @@
 
 @optional
 - (void)optionalMethod;
+- (void)mutateByAddingObserver:(id<TestObserver>)observer;
+- (void)mutateByRemovingObserver:(id<TestObserver>)observer;
 
 @end
 
@@ -31,6 +33,13 @@
 @property(nonatomic, readonly) BOOL optionalMethodInvoked;
 @end
 
+@interface TestMutateObserver : TestCompleteObserver
+
+- (instancetype)initWithObserver:(CRBProtocolObservers*)observer
+    NS_DESIGNATED_INITIALIZER;
+
+@end
+
 namespace {
 
 class CRBProtocolObserversTest : public PlatformTest {
@@ -50,11 +59,16 @@ class CRBProtocolObserversTest : public PlatformTest {
     complete_observer_.reset([[TestCompleteObserver alloc] init]);
     EXPECT_FALSE([complete_observer_ requiredMethodInvoked]);
     EXPECT_FALSE([complete_observer_ optionalMethodInvoked]);
+
+    mutate_observer_.reset(
+        [[TestMutateObserver alloc] initWithObserver:observers_.get()]);
+    EXPECT_FALSE([mutate_observer_ requiredMethodInvoked]);
   }
 
   base::scoped_nsobject<id> observers_;
   base::scoped_nsobject<TestPartialObserver> partial_observer_;
   base::scoped_nsobject<TestCompleteObserver> complete_observer_;
+  base::scoped_nsobject<TestMutateObserver> mutate_observer_;
 };
 
 // Verifies basic functionality of -[CRBProtocolObservers addObserver:] and
@@ -119,6 +133,34 @@ TEST_F(CRBProtocolObserversTest, WeakReference) {
   EXPECT_FALSE(weak_observer.get());
 }
 
+// Verifies that - [CRBProtocolObservers addObserver:] and
+// - [CRBProtocolObservers removeObserver:] can be called while methods are
+// being forwarded.
+TEST_F(CRBProtocolObserversTest, MutateObservers) {
+  // Indirectly add an observer while forwarding an observer method.
+  [observers_ addObserver:mutate_observer_];
+
+  [observers_ mutateByAddingObserver:partial_observer_];
+  EXPECT_FALSE([partial_observer_ requiredMethodInvoked]);
+
+  // Check that methods are correctly forwared to the indirectly added observer.
+  [mutate_observer_ reset];
+  [observers_ requiredMethod];
+  EXPECT_TRUE([mutate_observer_ requiredMethodInvoked]);
+  EXPECT_TRUE([partial_observer_ requiredMethodInvoked]);
+
+  [mutate_observer_ reset];
+  [partial_observer_ reset];
+
+  // Indirectly remove an observer while forwarding an observer method.
+  [observers_ mutateByRemovingObserver:partial_observer_];
+
+  // Check that method is not forwared to the indirectly removed observer.
+  [observers_ requiredMethod];
+  EXPECT_TRUE([mutate_observer_ requiredMethodInvoked]);
+  EXPECT_FALSE([partial_observer_ requiredMethodInvoked]);
+}
+

[droger, 2015/06/04 10:04:23]

We should also probably test the case where _invocation_depth is > 1.

[jbbegue, 2015/06/04 12:13:03]

I added a test for the nested case.
I also added another test for the case of an observer removing itself while
being notified.

 }  // namespace
 
 @implementation TestPartialObserver {
@@ -157,3 +199,25 @@ TEST_F(CRBProtocolObserversTest, WeakReference) {
 }
 
 @end
+
+@implementation TestMutateObserver {
+  __weak CRBProtocolObservers* _observers;
+}
+
+- (instancetype)initWithObserver:(CRBProtocolObservers*)observers {
+  self = [super init];
+  if (self) {
+    _observers = observers;
+  }
+  return self;
+}
+
+- (void)mutateByAddingObserver:(id<TestObserver>)observer {
+  [_observers addObserver:observer];
+}
+
+- (void)mutateByRemovingObserver:(id<TestObserver>)observer {
+  [_observers removeObserver:observer];
+}
+
+@end