From 5c5410459e042c24db555db3e40fc761daba465e Mon Sep 17 00:00:00 2001 From: Kathy Gray Date: Wed, 21 Jun 2017 11:56:08 -0700 Subject: [PATCH] Setting bridge up for sharing: allowing native modules to register after init Reviewed By: javache Differential Revision: D4945784 fbshipit-source-id: 80e7236e9ccd5d5c9a7fba7c96b98fc38b43a2fc --- .../facebook/react/ReactInstanceManager.java | 127 +++++++++++------- .../react/ReactInstanceManagerBuilder.java | 4 +- .../react/bridge/CatalystInstance.java | 6 + .../react/bridge/CatalystInstanceImpl.java | 20 +++ .../react/bridge/NativeModuleRegistry.java | 39 ++++++ .../jni/react/jni/CatalystInstanceImpl.cpp | 27 +++- .../main/jni/react/jni/CatalystInstanceImpl.h | 5 + .../jni/react/jni/ModuleRegistryBuilder.cpp | 8 +- .../jni/react/jni/ModuleRegistryBuilder.h | 2 +- ReactCommon/cxxreact/Instance.cpp | 5 +- ReactCommon/cxxreact/Instance.h | 1 + ReactCommon/cxxreact/ModuleRegistry.cpp | 32 ++++- ReactCommon/cxxreact/ModuleRegistry.h | 8 ++ 13 files changed, 217 insertions(+), 67 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java index 9339282e983..3b6e4676aac 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java +++ b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManager.java @@ -287,6 +287,7 @@ public class ReactInstanceManager { * * Called from UI thread. */ + @ThreadConfined(UI) public void createReactContextInBackground() { Log.d(ReactConstants.TAG, "ReactInstanceManager.createReactContextInBackground()"); Assertions.assertCondition( @@ -299,6 +300,33 @@ public class ReactInstanceManager { recreateReactContextInBackgroundInner(); } + @ThreadConfined(UI) + public void registerAdditionalPackages(List packages) { + if (packages == null || packages.isEmpty()) { + return; + } + + // CatalystInstance hasn't been created, so add packages for later evaluation + if (!hasStartedCreatingInitialContext()) { + for (ReactPackage p : packages) { + if (!mPackages.contains(p)) { + mPackages.add(p); + } + } + return; + } + + ReactContext context = getCurrentReactContext(); + CatalystInstance catalystInstance = context != null ? context.getCatalystInstance() : null; + + Assertions.assertNotNull(catalystInstance, "CatalystInstance null after hasStartedCreatingInitialContext true."); + + final ReactApplicationContext reactContext = new ReactApplicationContext(mApplicationContext); + + NativeModuleRegistry nativeModuleRegistry = processPackages(reactContext, packages, true); + catalystInstance.extendNativeModules(nativeModuleRegistry); + } + /** * Recreate the react application and context. This should be called if configuration has * changed or the developer has requested the app to be reloaded. It should only be called after @@ -806,7 +834,6 @@ public class ReactInstanceManager { if (!mSetupReactContextInBackgroundEnabled) { UiThreadUtil.assertOnUiThread(); } - Assertions.assertCondition(mCurrentReactContext == null); mCurrentReactContext = Assertions.assertNotNull(reactContext); CatalystInstance catalystInstance = Assertions.assertNotNull(reactContext.getCatalystInstance()); @@ -922,56 +949,23 @@ public class ReactInstanceManager { Log.d(ReactConstants.TAG, "ReactInstanceManager.createReactContext()"); ReactMarker.logMarker(CREATE_REACT_CONTEXT_START); final ReactApplicationContext reactContext = new ReactApplicationContext(mApplicationContext); - NativeModuleRegistryBuilder nativeModuleRegistryBuilder = new NativeModuleRegistryBuilder( - reactContext, - this, - mLazyNativeModulesEnabled); + if (mUseDeveloperSupport) { reactContext.setNativeModuleCallExceptionHandler(mDevSupportManager); } - ReactMarker.logMarker(PROCESS_PACKAGES_START); - Systrace.beginSection( - TRACE_TAG_REACT_JAVA_BRIDGE, - "createAndProcessCoreModulesPackage"); - try { - CoreModulesPackage coreModulesPackage = - new CoreModulesPackage( - this, - mBackBtnHandler, - mUIImplementationProvider, - mLazyViewManagersEnabled); - processPackage(coreModulesPackage, nativeModuleRegistryBuilder); - } finally { - Systrace.endSection(TRACE_TAG_REACT_JAVA_BRIDGE); - } - - // TODO(6818138): Solve use-case of native/js modules overriding - for (ReactPackage reactPackage : mPackages) { - Systrace.beginSection( - TRACE_TAG_REACT_JAVA_BRIDGE, - "createAndProcessCustomReactPackage"); - try { - processPackage(reactPackage, nativeModuleRegistryBuilder); - } finally { - Systrace.endSection(TRACE_TAG_REACT_JAVA_BRIDGE); - } - } - ReactMarker.logMarker(PROCESS_PACKAGES_END); - - ReactMarker.logMarker(BUILD_NATIVE_MODULE_REGISTRY_START); - Systrace.beginSection(TRACE_TAG_REACT_JAVA_BRIDGE, "buildNativeModuleRegistry"); - NativeModuleRegistry nativeModuleRegistry; - try { - nativeModuleRegistry = nativeModuleRegistryBuilder.build(); - } finally { - Systrace.endSection(TRACE_TAG_REACT_JAVA_BRIDGE); - ReactMarker.logMarker(BUILD_NATIVE_MODULE_REGISTRY_END); - } + CoreModulesPackage coreModulesPackage = + new CoreModulesPackage( + this, + mBackBtnHandler, + mUIImplementationProvider, + mLazyViewManagersEnabled); + mPackages.add(coreModulesPackage); + NativeModuleRegistry nativeModuleRegistry = processPackages(reactContext, mPackages, false); NativeModuleCallExceptionHandler exceptionHandler = mNativeModuleCallExceptionHandler != null - ? mNativeModuleCallExceptionHandler - : mDevSupportManager; + ? mNativeModuleCallExceptionHandler + : mDevSupportManager; CatalystInstanceImpl.Builder catalystInstanceBuilder = new CatalystInstanceImpl.Builder() .setReactQueueConfigurationSpec(mUseSeparateUIBackgroundThread ? ReactQueueConfigurationSpec.createWithSeparateUIBackgroundThread() : @@ -1006,6 +1000,49 @@ public class ReactInstanceManager { return reactContext; } + private NativeModuleRegistry processPackages( + ReactApplicationContext reactContext, + List packages, + boolean checkAndUpdatePackageMembership) { + NativeModuleRegistryBuilder nativeModuleRegistryBuilder = new NativeModuleRegistryBuilder( + reactContext, + this, + mLazyNativeModulesEnabled); + + ReactMarker.logMarker(PROCESS_PACKAGES_START); + + // TODO(6818138): Solve use-case of native modules overriding + for (ReactPackage reactPackage : packages) { + if (checkAndUpdatePackageMembership && mPackages.contains(reactPackage)) { + continue; + } + Systrace.beginSection( + TRACE_TAG_REACT_JAVA_BRIDGE, + "createAndProcessCustomReactPackage"); + try { + if (checkAndUpdatePackageMembership) { + mPackages.add(reactPackage); + } + processPackage(reactPackage, nativeModuleRegistryBuilder); + } finally { + Systrace.endSection(TRACE_TAG_REACT_JAVA_BRIDGE); + } + } + ReactMarker.logMarker(PROCESS_PACKAGES_END); + + ReactMarker.logMarker(BUILD_NATIVE_MODULE_REGISTRY_START); + Systrace.beginSection(TRACE_TAG_REACT_JAVA_BRIDGE, "buildNativeModuleRegistry"); + NativeModuleRegistry nativeModuleRegistry; + try { + nativeModuleRegistry = nativeModuleRegistryBuilder.build(); + } finally { + Systrace.endSection(TRACE_TAG_REACT_JAVA_BRIDGE); + ReactMarker.logMarker(BUILD_NATIVE_MODULE_REGISTRY_END); + } + + return nativeModuleRegistry; + } + private void processPackage( ReactPackage reactPackage, NativeModuleRegistryBuilder nativeModuleRegistryBuilder) { diff --git a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerBuilder.java b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerBuilder.java index 3ceac7ebece..76e602e0b17 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerBuilder.java +++ b/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerBuilder.java @@ -197,8 +197,8 @@ public class ReactInstanceManagerBuilder { public ReactInstanceManagerBuilder setUseSeparateUIBackgroundThread( boolean useSeparateUIBackgroundThread) { - mUseSeparateUIBackgroundThread = useSeparateUIBackgroundThread; - return this; + mUseSeparateUIBackgroundThread = useSeparateUIBackgroundThread; + return this; } public ReactInstanceManagerBuilder setMinNumShakes(int minNumShakes) { diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstance.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstance.java index c6bab4da62b..5799365ab12 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstance.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstance.java @@ -68,6 +68,12 @@ public interface CatalystInstance T getNativeModule(Class nativeModuleInterface); Collection getNativeModules(); + /** + * This method permits a CatalystInstance to extend the known + * Native modules. This provided registry contains only the new modules to load. + */ + void extendNativeModules(NativeModuleRegistry modules); + /** * Adds a idle listener for this Catalyst instance. The listener will receive notifications * whenever the bridge transitions from idle to busy and vice-versa, where the busy state is diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java index 3a2f82659b7..f57ef2e3065 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/CatalystInstanceImpl.java @@ -157,6 +157,26 @@ public class CatalystInstanceImpl implements CatalystInstance { } } + /** + * This method and the native below permits a CatalystInstance to extend the known + * Native modules. This registry contains only the new modules to load. The + * registry {@code mNativeModuleRegistry} updates internally to contain all the new modules, and generates + * the new registry for extracting just the new collections. + */ + @Override + public void extendNativeModules(NativeModuleRegistry modules) { + //Extend the Java-visible registry of modules + mNativeModuleRegistry.registerModules(modules); + Collection javaModules = modules.getJavaModules(this); + Collection cxxModules = modules.getCxxModules(); + //Extend the Cxx-visible registry of modules wrapped in appropriate interfaces + jniExtendNativeModules(javaModules, cxxModules); + } + + private native void jniExtendNativeModules( + Collection javaModules, + Collection cxxModules); + private native void initializeBridge( ReactCallback callback, JavaScriptExecutor jsExecutor, diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/NativeModuleRegistry.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/NativeModuleRegistry.java index 9e2f5692ef9..b17521a1309 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/NativeModuleRegistry.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/NativeModuleRegistry.java @@ -13,6 +13,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.Map; +import java.util.HashMap; import com.facebook.infer.annotation.Assertions; import com.facebook.systrace.Systrace; @@ -35,6 +36,21 @@ public class NativeModuleRegistry { mBatchCompleteListenerModules = batchCompleteListenerModules; } + /** + * Private getters for combining NativeModuleRegistrys + */ + private Map, ModuleHolder> getModuleMap() { + return mModules; + } + + private ReactApplicationContext getReactApplicationContext() { + return mReactApplicationContext; + } + + private ArrayList getBatchCompleteListenerModules() { + return mBatchCompleteListenerModules; + } + /* package */ Collection getJavaModules( JSInstance jsInstance) { ArrayList javaModules = new ArrayList<>(); @@ -58,6 +74,29 @@ public class NativeModuleRegistry { return cxxModules; } + /* + * Adds any new modules to the current module regsitry + */ + /* package */ void registerModules(NativeModuleRegistry newRegister) { + + Assertions.assertCondition(mReactApplicationContext.equals(newRegister.getReactApplicationContext()), + "Extending native modules with non-matching application contexts."); + + Map, ModuleHolder> newModules = newRegister.getModuleMap(); + ArrayList batchCompleteListeners = newRegister.getBatchCompleteListenerModules(); + + for (Map.Entry, ModuleHolder> entry : newModules.entrySet()) { + Class key = entry.getKey(); + if (!mModules.containsKey(key)) { + ModuleHolder value = entry.getValue(); + if (batchCompleteListeners.contains(value)) { + mBatchCompleteListenerModules.add(value); + } + mModules.put(key, value); + } + } + } + /* package */ void notifyJSInstanceDestroy() { mReactApplicationContext.assertOnNativeModulesQueueThread(); Systrace.beginSection( diff --git a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp index 47a38c8e7d2..e10662f8396 100644 --- a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp +++ b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.cpp @@ -97,6 +97,7 @@ void CatalystInstanceImpl::registerNatives() { registerHybrid({ makeNativeMethod("initHybrid", CatalystInstanceImpl::initHybrid), makeNativeMethod("initializeBridge", CatalystInstanceImpl::initializeBridge), + makeNativeMethod("jniExtendNativeModules", CatalystInstanceImpl::extendNativeModules), makeNativeMethod("jniSetSourceURL", CatalystInstanceImpl::jniSetSourceURL), makeNativeMethod("jniLoadScriptFromAssets", CatalystInstanceImpl::jniLoadScriptFromAssets), makeNativeMethod("jniLoadScriptFromFile", CatalystInstanceImpl::jniLoadScriptFromFile), @@ -147,18 +148,32 @@ void CatalystInstanceImpl::initializeBridge( // don't need jsModuleDescriptions any more, all the way up and down the // stack. + moduleRegistry_ = std::make_shared( + buildNativeModuleList( + std::weak_ptr(instance_), + javaModules, + cxxModules, + moduleMessageQueue_, + uiBackgroundMessageQueue_)); + instance_->initializeBridge( folly::make_unique( callback, uiBackgroundMessageQueue_ != NULL ? uiBackgroundMessageQueue_ : moduleMessageQueue_), jseh->getExecutorFactory(), folly::make_unique(jsQueue), - buildModuleRegistry( - std::weak_ptr(instance_), - javaModules, - cxxModules, - moduleMessageQueue_, - uiBackgroundMessageQueue_)); + moduleRegistry_); +} + +void CatalystInstanceImpl::extendNativeModules( + jni::alias_ref::javaobject> javaModules, + jni::alias_ref::javaobject> cxxModules) { + moduleRegistry_->registerModules(buildNativeModuleList( + std::weak_ptr(instance_), + javaModules, + cxxModules, + moduleMessageQueue_, + uiBackgroundMessageQueue_)); } void CatalystInstanceImpl::jniSetSourceURL(const std::string& sourceURL) { diff --git a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h index 9679339c38c..f4aefb01c1e 100644 --- a/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h +++ b/ReactAndroid/src/main/jni/react/jni/CatalystInstanceImpl.h @@ -52,6 +52,10 @@ class CatalystInstanceImpl : public jni::HybridClass { jni::alias_ref::javaobject> javaModules, jni::alias_ref::javaobject> cxxModules); + void extendNativeModules( + jni::alias_ref::javaobject> javaModules, + jni::alias_ref::javaobject> cxxModules); + /** * Sets the source URL of the underlying bridge without loading any JS code. */ @@ -74,6 +78,7 @@ class CatalystInstanceImpl : public jni::HybridClass { // This should be the only long-lived strong reference, but every C++ class // will have a weak reference. std::shared_ptr instance_; + std::shared_ptr moduleRegistry_; std::shared_ptr moduleMessageQueue_; std::shared_ptr uiBackgroundMessageQueue_; }; diff --git a/ReactAndroid/src/main/jni/react/jni/ModuleRegistryBuilder.cpp b/ReactAndroid/src/main/jni/react/jni/ModuleRegistryBuilder.cpp index 73ad22a13ee..9706a43dc1c 100644 --- a/ReactAndroid/src/main/jni/react/jni/ModuleRegistryBuilder.cpp +++ b/ReactAndroid/src/main/jni/react/jni/ModuleRegistryBuilder.cpp @@ -29,7 +29,7 @@ xplat::module::CxxModule::Provider ModuleHolder::getProvider() const { }; } -std::unique_ptr buildModuleRegistry( +std::vector> buildNativeModuleList( std::weak_ptr winstance, jni::alias_ref::javaobject> javaModules, jni::alias_ref::javaobject> cxxModules, @@ -60,11 +60,7 @@ std::unique_ptr buildModuleRegistry( winstance, cm->getName(), cm->getProvider(), moduleMessageQueue)); } } - if (modules.empty()) { - return nullptr; - } else { - return folly::make_unique(std::move(modules)); - } + return modules; } }} diff --git a/ReactAndroid/src/main/jni/react/jni/ModuleRegistryBuilder.h b/ReactAndroid/src/main/jni/react/jni/ModuleRegistryBuilder.h index e295ebc55ef..6e31adc898d 100644 --- a/ReactAndroid/src/main/jni/react/jni/ModuleRegistryBuilder.h +++ b/ReactAndroid/src/main/jni/react/jni/ModuleRegistryBuilder.h @@ -23,7 +23,7 @@ class ModuleHolder : public jni::JavaClass { xplat::module::CxxModule::Provider getProvider() const; }; -std::unique_ptr buildModuleRegistry( +std::vector> buildNativeModuleList( std::weak_ptr winstance, jni::alias_ref::javaobject> javaModules, jni::alias_ref::javaobject> cxxModules, diff --git a/ReactCommon/cxxreact/Instance.cpp b/ReactCommon/cxxreact/Instance.cpp index f7a55801a0a..8da3d3cebf8 100644 --- a/ReactCommon/cxxreact/Instance.cpp +++ b/ReactCommon/cxxreact/Instance.cpp @@ -32,11 +32,12 @@ void Instance::initializeBridge( std::shared_ptr jsQueue, std::shared_ptr moduleRegistry) { callback_ = std::move(callback); + moduleRegistry_ = std::move(moduleRegistry); jsQueue->runOnQueueSync( - [this, &jsef, moduleRegistry, jsQueue] () mutable { + [this, &jsef, jsQueue] () mutable { nativeToJsBridge_ = folly::make_unique( - jsef.get(), moduleRegistry, jsQueue, callback_); + jsef.get(), moduleRegistry_, jsQueue, callback_); std::lock_guard lock(m_syncMutex); m_syncReady = true; diff --git a/ReactCommon/cxxreact/Instance.h b/ReactCommon/cxxreact/Instance.h index ca875c56901..46ae7144919 100644 --- a/ReactCommon/cxxreact/Instance.h +++ b/ReactCommon/cxxreact/Instance.h @@ -73,6 +73,7 @@ class Instance { std::shared_ptr callback_; std::unique_ptr nativeToJsBridge_; + std::shared_ptr moduleRegistry_; std::mutex m_syncMutex; std::condition_variable m_syncCV; diff --git a/ReactCommon/cxxreact/ModuleRegistry.cpp b/ReactCommon/cxxreact/ModuleRegistry.cpp index 98042f40118..907fce9291e 100644 --- a/ReactCommon/cxxreact/ModuleRegistry.cpp +++ b/ReactCommon/cxxreact/ModuleRegistry.cpp @@ -30,15 +30,36 @@ std::string normalizeName(std::string name) { ModuleRegistry::ModuleRegistry(std::vector> modules) : modules_(std::move(modules)) {} -void ModuleRegistry::registerModules(std::vector> modules) { - // TODO: consider relaxing this restriction - CHECK(modulesByName_.empty()) << "Can only register additional modules before NativeModules have been accessed"; +void ModuleRegistry::updateModuleNamesFromIndex(size_t index) { + for (; index < modules_.size(); index++ ) { + std::string name = normalizeName(modules_[index]->getName()); + modulesByName_[name] = index; + } +} - if (modules_.empty()) { +void ModuleRegistry::registerModules(std::vector> modules) { + if (modules_.empty() && unknownModules_.empty()) { modules_ = std::move(modules); } else { - modules_.reserve(modules_.size() + modules.size()); + size_t modulesSize = modules_.size(); + size_t addModulesSize = modules.size(); + bool addToNames = !modulesByName_.empty(); + modules_.reserve(modulesSize + addModulesSize); std::move(modules.begin(), modules.end(), std::back_inserter(modules_)); + if (!unknownModules_.empty()) { + for (size_t index = modulesSize; index < modulesSize + addModulesSize; index++) { + std::string name = normalizeName(modules_[index]->getName()); + auto it = unknownModules_.find(name); + if (it != unknownModules_.end()) { + throw std::runtime_error( + folly::to("module ", name, " was required without being registered and is now being registered.")); + } else if (addToNames) { + modulesByName_[name] = index; + } + } + } else if (addToNames) { + updateModuleNamesFromIndex(modulesSize); + } } } @@ -62,6 +83,7 @@ folly::Optional ModuleRegistry::getConfig(const std::string& name) auto it = modulesByName_.find(name); if (it == modulesByName_.end()) { + unknownModules_.insert(name); return nullptr; } diff --git a/ReactCommon/cxxreact/ModuleRegistry.h b/ReactCommon/cxxreact/ModuleRegistry.h index 909b3c2db65..a9b7b2de380 100644 --- a/ReactCommon/cxxreact/ModuleRegistry.h +++ b/ReactCommon/cxxreact/ModuleRegistry.h @@ -4,6 +4,7 @@ #include #include +#include #include #include @@ -42,8 +43,15 @@ class ModuleRegistry { // This is always populated std::vector> modules_; + // This is used to extend the population of modulesByName_ if registerModules is called after moduleNames + void updateModuleNamesFromIndex(size_t size); + // This is only populated if moduleNames() is called. Values are indices into modules_. std::unordered_map modulesByName_; + + // This is populated with modules that are requested via getConfig but are unknown. + // An error will be thrown if they are subsquently added to the registry. + std::unordered_set unknownModules_; }; }