From bc99a32e4d9a8db1d8e524fbdf1a8279a8525834 Mon Sep 17 00:00:00 2001
From: Ramanpreet Nara <ramanpreet@fb.com>
Date: Tue, 14 Apr 2020 18:24:23 -0700
Subject: [PATCH] Control concurrent calls into TMMDelegate from TMM

Summary:
In D20659799, I improved `TurboModuleManager.getModule(moduleName)` thread-safety by ensuring that if two threads race to require the same NativeModule, only one thread creates the NativeModule, while the other one waits until it's created.

## The problem:
What I failed to realize was that when two threads race to require two different NativeModules, we can get concurrent calls into `TurboModuleManagerDelegate.getModule(moduleName)`, and `TurboModuleManagerDelegate.getLegacyCxxModule(moduleName)`, which don't have any thread-safe guarantees.

## The fix
`TurboModuleManagerDelegate` is supposed to be an input to the TurboModule system. So, rather than expecting that all TurboModuleManagerDelegates are thread-safe, which might be a reasonable ask (see T65532092), this diff has `TurboModuleManager` acquire the delegate's lock before calling into it. This ensures that we don't get concurrent access into the delegate, which could be reading from, or writing to, some data structure in these method calls. (This was the case with `ReactPackageTurboModuleManagerDelegate`, which is what Fb4a and Workplace use under the hood).

Changelog:
[Android][Fixed] - Control concurrent calls into TMMDelegate from TurboModuleManager

Reviewed By: mdvacca

Differential Revision: D21025965

fbshipit-source-id: d22c4abfe87f9e534717a06f186dde87d3cd24df
---
 .../turbomodule/core/TurboModuleManager.java  | 22 ++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/TurboModuleManager.java b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/TurboModuleManager.java
index 1b97c5bd62d..004c8e8a740 100644
--- a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/TurboModuleManager.java
+++ b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/TurboModuleManager.java
@@ -32,7 +32,7 @@ import java.util.*;
  */
 public class TurboModuleManager implements JSIModule, TurboModuleRegistry {
   private static volatile boolean sIsSoLibraryLoaded;
-  private final List<String> mEagerInitModuleNames;
+  private final List<String> mEagerInitModuleNames = new ArrayList<>();
   private final TurboModuleProvider mJavaModuleProvider;
   private final TurboModuleProvider mCxxModuleProvider;
 
@@ -64,8 +64,11 @@ public class TurboModuleManager implements JSIModule, TurboModuleRegistry {
             delegate);
     installJSIBindings();
 
-    mEagerInitModuleNames =
-        delegate == null ? new ArrayList<String>() : delegate.getEagerInitModuleNames();
+    if (delegate != null) {
+      synchronized (delegate) {
+        mEagerInitModuleNames.addAll(delegate.getEagerInitModuleNames());
+      }
+    }
 
     mJavaModuleProvider =
         new TurboModuleProvider() {
@@ -75,7 +78,10 @@ public class TurboModuleManager implements JSIModule, TurboModuleRegistry {
               return null;
             }
 
-            return delegate.getModule(moduleName);
+            /** TODO(T65532092): Should TurboModuleManagerDelegate be thread-safe? */
+            synchronized (delegate) {
+              return delegate.getModule(moduleName);
+            }
           }
         };
 
@@ -87,7 +93,13 @@ public class TurboModuleManager implements JSIModule, TurboModuleRegistry {
               return null;
             }
 
-            CxxModuleWrapper nativeModule = delegate.getLegacyCxxModule(moduleName);
+            CxxModuleWrapper nativeModule;
+
+            /** TODO(T65532092): Should TurboModuleManagerDelegate be thread-safe? */
+            synchronized (delegate) {
+              nativeModule = delegate.getLegacyCxxModule(moduleName);
+            }
+
             if (nativeModule != null) {
               // TurboModuleManagerDelegate must always return TurboModules
               Assertions.assertCondition(