Cherrypick for #3984 along with CHANGELOG update. (#3988)

* Fix race condition in FIRComponentContainer instance creation (#3984)

* Fix race condition in FIRComponentContainer instance creation

Reported in #3967. Users sometimes seeing a crash when multiple threads
concurrently try to create Firestore instances. By inspection I found
that there's a race here where two threads can check for presence in the
cache, see nothing, proceed to creation, both store a value in the cache,
and both return their copies. This would lead to two Firestores being
created for a single App.

This replaces `dispatch_sync` on a serial queue with good old
`@synchronized` blocks. `dispatch_sync` will self-deadlock if called
from a thread already on the queue while `@synchronized` allows
recursion.

No new tests are added because several tests like
`testDependencyDoesntBlock` already cover this case.

* format

* Update Core CHANGELOG

* Update Core CHANGELOG again

Author: ryanwilson

Firebase/Core/CHANGELOG.md

@@ -1,3 +1,6 @@
+# v6.3.1 -- M57
+- [fixed] Fixed race condition in component container. (#3967, #3924)
+
 # v6.3.0 -- M56
 - [changed] Transitive GoogleDataTransport dependency incremented to v2.0.0. (#3729)
 - [fixed] Fixed "expiclitlySet" typo. (#3853)

Firebase/Core/FIRComponentContainer.m

@@ -23,9 +23,7 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
-@interface FIRComponentContainer () {
-  dispatch_queue_t _containerQueue;
-}
+@interface FIRComponentContainer ()
 
 /// The dictionary of components that are registered for a particular app. The key is an NSString
 /// of the protocol.
@@ -69,8 +67,6 @@ - (instancetype)initWithApp:(FIRApp *)app registrants:(NSMutableSet<Class> *)all
     _app = app;
     _cachedInstances = [NSMutableDictionary<NSString *, id> dictionary];
     _components = [NSMutableDictionary<NSString *, FIRComponentCreationBlock> dictionary];
-    _containerQueue =
-        dispatch_queue_create("com.google.FirebaseComponentContainer", DISPATCH_QUEUE_SERIAL);
 
     [self populateComponentsFromRegisteredClasses:allRegistrants forApp:app];
   }
@@ -103,7 +99,10 @@ - (void)populateComponentsFromRegisteredClasses:(NSSet<Class> *)classes forApp:(
           (component.instantiationTiming == FIRInstantiationTimingEagerInDefaultApp &&
            [app isDefaultApp]);
       if (shouldInstantiateEager || shouldInstantiateDefaultEager) {
-        [self instantiateInstanceForProtocol:component.protocol withBlock:component.creationBlock];
+        @synchronized(self) {
+          [self instantiateInstanceForProtocol:component.protocol
+                                     withBlock:component.creationBlock];
+        }
       }
     }
   }
@@ -116,6 +115,8 @@ - (void)populateComponentsFromRegisteredClasses:(NSSet<Class> *)classes forApp:(
 ///   - Call the block to create an instance if possible,
 ///   - Validate that the instance returned conforms to the protocol it claims to,
 ///   - Cache the instance if the block requests it
+///
+/// Note that this method assumes the caller already has @sychronized on self.
 - (nullable id)instantiateInstanceForProtocol:(Protocol *)protocol
                                     withBlock:(FIRComponentCreationBlock)creationBlock {
   if (!creationBlock) {
@@ -140,9 +141,7 @@ - (nullable id)instantiateInstanceForProtocol:(Protocol *)protocol
 
   // The instance is ready to be returned, but check if it should be cached first before returning.
   if (shouldCache) {
-    dispatch_sync(_containerQueue, ^{
-      self.cachedInstances[protocolName] = instance;
-    });
+    self.cachedInstances[protocolName] = instance;
   }
 
   return instance;
@@ -153,47 +152,35 @@ - (nullable id)instantiateInstanceForProtocol:(Protocol *)protocol
 - (nullable id)instanceForProtocol:(Protocol *)protocol {
   // Check if there is a cached instance, and return it if so.
   NSString *protocolName = NSStringFromProtocol(protocol);
-  __block id cachedInstance;
-  dispatch_sync(_containerQueue, ^{
-    cachedInstance = self.cachedInstances[protocolName];
-  });
 
-  if (cachedInstance) {
-    return cachedInstance;
+  id cachedInstance;
+  @synchronized(self) {
+    cachedInstance = self.cachedInstances[protocolName];
+    if (!cachedInstance) {
+      // Use the creation block to instantiate an instance and return it.
+      FIRComponentCreationBlock creationBlock = self.components[protocolName];
+      cachedInstance = [self instantiateInstanceForProtocol:protocol withBlock:creationBlock];
+    }
   }
-
-  // Use the creation block to instantiate an instance and return it.
-  FIRComponentCreationBlock creationBlock = self.components[protocolName];
-  return [self instantiateInstanceForProtocol:protocol withBlock:creationBlock];
+  return cachedInstance;
 }
 
 #pragma mark - Lifecycle
 
 - (void)removeAllCachedInstances {
-  // Loop through the cache and notify each instance that is a maintainer to clean up after itself.
-  // Design note: we're getting a copy here, unlocking the cached instances, iterating over the
-  // copy, then locking and removing all cached instances. A race condition *could* exist where a
-  // new cached instance is created between the copy and the removal, but the chances are slim and
-  // side-effects are significantly smaller than including the entire loop in the `dispatch_sync`
-  // block (access to the cache from inside the block would deadlock and crash).
-  __block NSDictionary<NSString *, id> *instancesCopy;
-  dispatch_sync(_containerQueue, ^{
-    instancesCopy = [self.cachedInstances copy];
-  });
-
-  for (id instance in instancesCopy.allValues) {
-    if ([instance conformsToProtocol:@protocol(FIRComponentLifecycleMaintainer)] &&
-        [instance respondsToSelector:@selector(appWillBeDeleted:)]) {
-      [instance appWillBeDeleted:self.app];
+  @synchronized(self) {
+    // Loop through the cache and notify each instance that is a maintainer to clean up after
+    // itself.
+    for (id instance in self.cachedInstances.allValues) {
+      if ([instance conformsToProtocol:@protocol(FIRComponentLifecycleMaintainer)] &&
+          [instance respondsToSelector:@selector(appWillBeDeleted:)]) {
+        [instance appWillBeDeleted:self.app];
+      }
     }
-  }
 
-  instancesCopy = nil;
-
-  // Empty the cache.
-  dispatch_sync(_containerQueue, ^{
+    // Empty the cache.
     [self.cachedInstances removeAllObjects];
-  });
+  }
 }
 
 @end