From 4020fe427cf71f616e00bc39f8155b9bed8fd47c Mon Sep 17 00:00:00 2001
From: Thomas Van Lenten <thomasvl@google.com>
Date: Thu, 2 Mar 2017 14:50:10 -0500
Subject: Handing threading race resolving methods.

- Don't prune the extension registry as that can lead to failures when two
  threads are racing.
- If adding the method fails, check and see if it already is bound to decide
  the return result. Deals with threading races binding the methods.
---
 objectivec/GPBMessage.m                  | 13 +++++++++++--
 objectivec/GPBRootObject.m               | 21 ++++++++++++++-------
 objectivec/GPBUtilities.m                | 19 +++++++++++++++++++
 objectivec/GPBUtilities_PackagePrivate.h |  2 ++
 4 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/objectivec/GPBMessage.m b/objectivec/GPBMessage.m
index 9660f1ed..58a10fdb 100644
--- a/objectivec/GPBMessage.m
+++ b/objectivec/GPBMessage.m
@@ -3152,8 +3152,17 @@ static void ResolveIvarSet(GPBFieldDescriptor *field,
   if (result.impToAdd) {
     const char *encoding =
         GPBMessageEncodingForSelector(result.encodingSelector, YES);
-    BOOL methodAdded = class_addMethod(descriptor.messageClass, sel,
-                                       result.impToAdd, encoding);
+    Class msgClass = descriptor.messageClass;
+    BOOL methodAdded = class_addMethod(msgClass, sel, result.impToAdd, encoding);
+    // class_addMethod() is documented as also failing if the method was already
+    // added; so we check if the method is already there and return success so
+    // the method dispatch will still happen.  Why would it already be added?
+    // Two threads could cause the same method to be bound at the same time,
+    // but only one will actually bind it; the other still needs to return true
+    // so things will dispatch.
+    if (!methodAdded) {
+      methodAdded = GPBClassHasSel(msgClass, sel);
+    }
     return methodAdded;
   }
   return [super resolveInstanceMethod:sel];
diff --git a/objectivec/GPBRootObject.m b/objectivec/GPBRootObject.m
index 4570716f..585d205a 100644
--- a/objectivec/GPBRootObject.m
+++ b/objectivec/GPBRootObject.m
@@ -184,11 +184,10 @@ static id ExtensionForName(id self, SEL _cmd) {
   dispatch_semaphore_wait(gExtensionSingletonDictionarySemaphore,
                           DISPATCH_TIME_FOREVER);
   id extension = (id)CFDictionaryGetValue(gExtensionSingletonDictionary, key);
-  if (extension) {
-    // The method is getting wired in to the class, so no need to keep it in
-    // the dictionary.
-    CFDictionaryRemoveValue(gExtensionSingletonDictionary, key);
-  }
+  // We can't remove the key from the dictionary here (as an optimization),
+  // two threads could have gone into +resolveClassMethod: for the same method,
+  // and ended up here; there's no way to ensure both return YES without letting
+  // both try to wire in the method.
   dispatch_semaphore_signal(gExtensionSingletonDictionarySemaphore);
   return extension;
 }
@@ -212,9 +211,17 @@ BOOL GPBResolveExtensionClassMethod(Class self, SEL sel) {
 #pragma unused(obj)
       return extension;
     });
-    if (class_addMethod(metaClass, sel, imp, encoding)) {
-      return YES;
+    BOOL methodAdded = class_addMethod(metaClass, sel, imp, encoding);
+    // class_addMethod() is documented as also failing if the method was already
+    // added; so we check if the method is already there and return success so
+    // the method dispatch will still happen.  Why would it already be added?
+    // Two threads could cause the same method to be bound at the same time,
+    // but only one will actually bind it; the other still needs to return true
+    // so things will dispatch.
+    if (!methodAdded) {
+      methodAdded = GPBClassHasSel(metaClass, sel);
     }
+    return methodAdded;
   }
   return NO;
 }
diff --git a/objectivec/GPBUtilities.m b/objectivec/GPBUtilities.m
index 68aadb77..78d85eaa 100644
--- a/objectivec/GPBUtilities.m
+++ b/objectivec/GPBUtilities.m
@@ -1774,6 +1774,25 @@ NSString *GPBDecodeTextFormatName(const uint8_t *decodeData, int32_t key,
 
 #pragma clang diagnostic pop
 
+BOOL GPBClassHasSel(Class aClass, SEL sel) {
+  // NOTE: We have to use class_copyMethodList, all other runtime method
+  // lookups actually also resolve the method implementation and this
+  // is called from within those methods.
+
+  BOOL result = NO;
+  unsigned int methodCount = 0;
+  Method *methodList = class_copyMethodList(aClass, &methodCount);
+  for (unsigned int i = 0; i < methodCount; ++i) {
+    SEL methodSelector = method_getName(methodList[i]);
+    if (methodSelector == sel) {
+      result = YES;
+      break;
+    }
+  }
+  free(methodList);
+  return result;
+}
+
 #pragma mark - GPBMessageSignatureProtocol
 
 // A series of selectors that are used solely to get @encoding values
diff --git a/objectivec/GPBUtilities_PackagePrivate.h b/objectivec/GPBUtilities_PackagePrivate.h
index 274351b7..16859d48 100644
--- a/objectivec/GPBUtilities_PackagePrivate.h
+++ b/objectivec/GPBUtilities_PackagePrivate.h
@@ -345,4 +345,6 @@ GPB_MESSAGE_SIGNATURE_ENTRY(int32_t, Enum)
 + (id)getClassValue;
 @end
 
+BOOL GPBClassHasSel(Class aClass, SEL sel);
+
 CF_EXTERN_C_END
-- 
cgit v1.2.3