From c8e000b19a3dc47d58bc3ea1ede8211a370d59bb Mon Sep 17 00:00:00 2001 From: Ram N Date: Fri, 27 Jul 2018 23:30:39 -0700 Subject: [PATCH] Prevent class loading for lazy native modules Summary: When native modules use `LazyReactPackage`, the modules themselves are not initialized. However, they still use the class names, causing the classes to load. This diff removes the need to perform any class loads. Any properties of the classes that are required are now populated in the `ReactModuleInfo` of that class. Note that this diff itself does not prevent class loading since any references to `*.class` in `LazyReactpackage` needs to be removed in a consequent diff Reviewed By: achen1 Differential Revision: D8950025 fbshipit-source-id: 80ddf7e1f33bf2af0db1bd262069795de77ec611 --- .../com/facebook/react/LazyReactPackage.java | 2 +- .../react/NativeModuleRegistryBuilder.java | 34 ++++++++----------- .../react/bridge/JavaModuleWrapper.java | 10 +++--- .../facebook/react/bridge/ModuleHolder.java | 10 ++++++ .../com/facebook/react/bridge/ModuleSpec.java | 12 ++++++- .../react/bridge/NativeModuleRegistry.java | 28 +++++++-------- .../react/module/model/ReactModuleInfo.java | 14 +++++++- .../processing/ReactModuleSpecProcessor.java | 26 +++++++++++++- .../react/bridge/BaseJavaModuleTest.java | 2 +- 9 files changed, 94 insertions(+), 44 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/LazyReactPackage.java b/ReactAndroid/src/main/java/com/facebook/react/LazyReactPackage.java index 68c184a0e..766f7c5b3 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/LazyReactPackage.java +++ b/ReactAndroid/src/main/java/com/facebook/react/LazyReactPackage.java @@ -74,7 +74,7 @@ public abstract class LazyReactPackage implements ReactPackage { .flush(); ReactMarker.logMarker( ReactMarkerConstants.CREATE_MODULE_START, - holder.getType().getSimpleName()); + holder.getClassName()); try { nativeModule = holder.getProvider().get(); } finally { diff --git a/ReactAndroid/src/main/java/com/facebook/react/NativeModuleRegistryBuilder.java b/ReactAndroid/src/main/java/com/facebook/react/NativeModuleRegistryBuilder.java index 49a150bb4..7550af230 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/NativeModuleRegistryBuilder.java +++ b/ReactAndroid/src/main/java/com/facebook/react/NativeModuleRegistryBuilder.java @@ -32,8 +32,8 @@ public class NativeModuleRegistryBuilder { private final ReactInstanceManager mReactInstanceManager; private final boolean mLazyNativeModulesEnabled; - private final Map, ModuleHolder> mModules = new HashMap<>(); - private final Map> namesToType = new HashMap<>(); + private final Map mModules = new HashMap<>(); + private final Map namesToType = new HashMap<>(); public NativeModuleRegistryBuilder( ReactApplicationContext reactApplicationContext, @@ -53,16 +53,10 @@ public class NativeModuleRegistryBuilder { lazyReactPackage.getReactModuleInfoProvider().getReactModuleInfos(); for (ModuleSpec moduleSpec : moduleSpecs) { - Class type = moduleSpec.getType(); - ReactModuleInfo reactModuleInfo = reactModuleInfoMap.get(type.getCanonicalName()); + String className = moduleSpec.getClassName(); + ReactModuleInfo reactModuleInfo = reactModuleInfoMap.get(className); ModuleHolder moduleHolder; if (reactModuleInfo == null) { - if (BaseJavaModule.class.isAssignableFrom(type)) { - throw new IllegalStateException( - "Native Java module " - + type.getSimpleName() - + " should be annotated with @ReactModule and added to a @ReactModuleList."); - } NativeModule module; ReactMarker.logMarker( ReactMarkerConstants.CREATE_MODULE_START, @@ -78,7 +72,7 @@ public class NativeModuleRegistryBuilder { } String name = moduleHolder.getName(); - putModuleTypeAndHolderToModuleMaps(type, name, moduleHolder); + putModuleTypeAndHolderToModuleMaps(className, name, moduleHolder); } } else { FLog.d( @@ -103,20 +97,20 @@ public class NativeModuleRegistryBuilder { public void addNativeModule(NativeModule nativeModule) { String name = nativeModule.getName(); Class type = nativeModule.getClass(); - putModuleTypeAndHolderToModuleMaps(type, name, new ModuleHolder(nativeModule)); + putModuleTypeAndHolderToModuleMaps(type.getName(), name, new ModuleHolder(nativeModule)); } private void putModuleTypeAndHolderToModuleMaps( - Class type, String underName, ModuleHolder moduleHolder) + String className, String underName, ModuleHolder moduleHolder) throws IllegalStateException { if (namesToType.containsKey(underName)) { - Class existingNativeModule = namesToType.get(underName); + String existingNativeModule = namesToType.get(underName); if (!moduleHolder.getCanOverrideExistingModule()) { throw new IllegalStateException( "Native module " - + type.getSimpleName() + + className + " tried to override " - + existingNativeModule.getSimpleName() + + existingNativeModule + " for module name " + underName + ". Check the getPackages() method in MainApplication.java, it might be " @@ -127,14 +121,14 @@ public class NativeModuleRegistryBuilder { mModules.remove(existingNativeModule); } - namesToType.put(underName, type); - mModules.put(type, moduleHolder); + namesToType.put(underName, className); + mModules.put(className, moduleHolder); } public NativeModuleRegistry build() { ArrayList batchCompleteListenerModules = new ArrayList<>(); - for (Map.Entry, ModuleHolder> entry : mModules.entrySet()) { - if (OnBatchCompleteListener.class.isAssignableFrom(entry.getKey())) { + for (Map.Entry entry : mModules.entrySet()) { + if (entry.getValue().hasOnBatchCompleteListener()) { batchCompleteListenerModules.add(entry.getValue()); } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaModuleWrapper.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaModuleWrapper.java index ae91556b2..ecc701ee0 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaModuleWrapper.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/JavaModuleWrapper.java @@ -49,14 +49,14 @@ public class JavaModuleWrapper { private final JSInstance mJSInstance; private final ModuleHolder mModuleHolder; - private final Class mModuleClass; + private final String mClassName; private final ArrayList mMethods; private final ArrayList mDescs; - public JavaModuleWrapper(JSInstance jsInstance, Class moduleClass, ModuleHolder moduleHolder) { + public JavaModuleWrapper(JSInstance jsInstance, String className, ModuleHolder moduleHolder) { mJSInstance = jsInstance; mModuleHolder = moduleHolder; - mModuleClass = moduleClass; + mClassName = className; mMethods = new ArrayList<>(); mDescs = new ArrayList(); } @@ -76,9 +76,9 @@ public class JavaModuleWrapper { Systrace.beginSection(TRACE_TAG_REACT_JAVA_BRIDGE, "findMethods"); Set methodNames = new HashSet<>(); - Class classForMethods = mModuleClass; + Class classForMethods = mModuleHolder.getModule().getClass(); Class superClass = - (Class) mModuleClass.getSuperclass(); + (Class) classForMethods.getSuperclass(); if (ReactModuleWithSpec.class.isAssignableFrom(superClass)) { // For java module that is based on generated flow-type spec, inspect the // spec abstract class instead, which is the super class of the given java diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ModuleHolder.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ModuleHolder.java index e438d06c4..f1b67e7a8 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/ModuleHolder.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/ModuleHolder.java @@ -40,6 +40,8 @@ public class ModuleHolder { private final String mName; private final boolean mCanOverrideExistingModule; private final boolean mHasConstants; + private final boolean mIsCxxModule; + private final boolean mHasOnBatchCompleteListener; private @Nullable Provider mProvider; // Outside of the constructur, these should only be checked or set when synchronized on this @@ -55,6 +57,8 @@ public class ModuleHolder { mCanOverrideExistingModule = moduleInfo.canOverrideExistingModule(); mHasConstants = moduleInfo.hasConstants(); mProvider = provider; + mHasOnBatchCompleteListener = moduleInfo.hasOnBatchCompleteListener(); + mIsCxxModule = moduleInfo.isCxxModule(); if (moduleInfo.needsEagerInit()) { mModule = create(); } @@ -64,6 +68,8 @@ public class ModuleHolder { mName = nativeModule.getName(); mCanOverrideExistingModule = nativeModule.canOverrideExistingModule(); mHasConstants = true; + mIsCxxModule = CxxModuleWrapper.class.isAssignableFrom(nativeModule.getClass()); + mHasOnBatchCompleteListener = OnBatchCompleteListener.class.isAssignableFrom(nativeModule.getClass()); mModule = nativeModule; PrinterHolder.getPrinter() .logMessage(ReactDebugOverlayTags.NATIVE_MODULE, "NativeModule init: %s", mName); @@ -113,6 +119,10 @@ public class ModuleHolder { return mHasConstants; } + public boolean isCxxModule() {return mIsCxxModule; } + + public boolean hasOnBatchCompleteListener() {return mHasOnBatchCompleteListener; } + @DoNotStrip public NativeModule getModule() { NativeModule module; diff --git a/ReactAndroid/src/main/java/com/facebook/react/bridge/ModuleSpec.java b/ReactAndroid/src/main/java/com/facebook/react/bridge/ModuleSpec.java index 537a610e5..8bb5d461b 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/ModuleSpec.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/ModuleSpec.java @@ -25,6 +25,7 @@ public class ModuleSpec { private final @Nullable Class mType; private final Provider mProvider; + private final String mClassName; /** * Simple spec for modules with a default constructor. @@ -66,19 +67,28 @@ public class ModuleSpec { public static ModuleSpec nativeModuleSpec( Class type, Provider provider) { - return new ModuleSpec(type, provider); + return new ModuleSpec(provider, type.getName()); } private ModuleSpec( @Nullable Class type, Provider provider) { mType = type; mProvider = provider; + mClassName = type == null ? null : type.getName(); + } + + public ModuleSpec(Provider provider, String name) { + mType = null; + mProvider = provider; + mClassName = name; } public @Nullable Class getType() { return mType; } + public String getClassName(){return mClassName;} + public Provider getProvider() { return mProvider; } 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 66c73e6af..ffd2e8ecf 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/bridge/NativeModuleRegistry.java +++ b/ReactAndroid/src/main/java/com/facebook/react/bridge/NativeModuleRegistry.java @@ -22,12 +22,12 @@ import com.facebook.systrace.Systrace; public class NativeModuleRegistry { private final ReactApplicationContext mReactApplicationContext; - private final Map, ModuleHolder> mModules; + private final Map mModules; private final ArrayList mBatchCompleteListenerModules; public NativeModuleRegistry( ReactApplicationContext reactApplicationContext, - Map, ModuleHolder> modules, + Map modules, ArrayList batchCompleteListenerModules) { mReactApplicationContext = reactApplicationContext; mModules = modules; @@ -37,7 +37,7 @@ public class NativeModuleRegistry { /** * Private getters for combining NativeModuleRegistrys */ - private Map, ModuleHolder> getModuleMap() { + private Map getModuleMap() { return mModules; } @@ -52,9 +52,10 @@ public class NativeModuleRegistry { /* package */ Collection getJavaModules( JSInstance jsInstance) { ArrayList javaModules = new ArrayList<>(); - for (Map.Entry, ModuleHolder> entry : mModules.entrySet()) { - Class type = entry.getKey(); - if (!CxxModuleWrapperBase.class.isAssignableFrom(type)) { + for (Map.Entry entry : mModules.entrySet()) { + String type = entry.getKey(); + if (!entry.getValue().isCxxModule()) { + //if (!CxxModuleWrapperBase.class.isAssignableFrom(entry.getValue().getModule().getClass())) { javaModules.add(new JavaModuleWrapper(jsInstance, type, entry.getValue())); } } @@ -63,9 +64,8 @@ public class NativeModuleRegistry { /* package */ Collection getCxxModules() { ArrayList cxxModules = new ArrayList<>(); - for (Map.Entry, ModuleHolder> entry : mModules.entrySet()) { - Class type = entry.getKey(); - if (CxxModuleWrapperBase.class.isAssignableFrom(type)) { + for (Map.Entry entry : mModules.entrySet()) { + if (entry.getValue().isCxxModule()) { cxxModules.add(entry.getValue()); } } @@ -80,11 +80,11 @@ public class NativeModuleRegistry { Assertions.assertCondition(mReactApplicationContext.equals(newRegister.getReactApplicationContext()), "Extending native modules with non-matching application contexts."); - Map, ModuleHolder> newModules = newRegister.getModuleMap(); + Map newModules = newRegister.getModuleMap(); ArrayList batchCompleteListeners = newRegister.getBatchCompleteListenerModules(); - for (Map.Entry, ModuleHolder> entry : newModules.entrySet()) { - Class key = entry.getKey(); + for (Map.Entry entry : newModules.entrySet()) { + String key = entry.getKey(); if (!mModules.containsKey(key)) { ModuleHolder value = entry.getValue(); if (batchCompleteListeners.contains(value)) { @@ -137,12 +137,12 @@ public class NativeModuleRegistry { } public boolean hasModule(Class moduleInterface) { - return mModules.containsKey(moduleInterface); + return mModules.containsKey(moduleInterface.getName()); } public T getModule(Class moduleInterface) { return (T) Assertions.assertNotNull( - mModules.get(moduleInterface), moduleInterface.getSimpleName()).getModule(); + mModules.get(moduleInterface.getName()), moduleInterface.getSimpleName()).getModule(); } public List getAllModules() { diff --git a/ReactAndroid/src/main/java/com/facebook/react/module/model/ReactModuleInfo.java b/ReactAndroid/src/main/java/com/facebook/react/module/model/ReactModuleInfo.java index b980df1f4..5cafdc44e 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/module/model/ReactModuleInfo.java +++ b/ReactAndroid/src/main/java/com/facebook/react/module/model/ReactModuleInfo.java @@ -15,16 +15,22 @@ public class ReactModuleInfo { private final boolean mCanOverrideExistingModule; private final boolean mNeedsEagerInit; private final boolean mHasConstants; + private final boolean mIsCxxModule; + private final boolean mHasOnBatchCompleteListener; public ReactModuleInfo( String name, boolean canOverrideExistingModule, boolean needsEagerInit, - boolean hasConstants) { + boolean hasConstants, + boolean isCxxModule, + boolean hasOnBatchCompleteListener) { mName = name; mCanOverrideExistingModule = canOverrideExistingModule; mNeedsEagerInit = needsEagerInit; mHasConstants = hasConstants; + mIsCxxModule = isCxxModule; + mHasOnBatchCompleteListener = hasOnBatchCompleteListener; } public String name() { @@ -42,4 +48,10 @@ public class ReactModuleInfo { public boolean hasConstants() { return mHasConstants; } + + public boolean isCxxModule() {return mIsCxxModule; } + + public boolean hasOnBatchCompleteListener() { + return mHasOnBatchCompleteListener; + } } diff --git a/ReactAndroid/src/main/java/com/facebook/react/module/processing/ReactModuleSpecProcessor.java b/ReactAndroid/src/main/java/com/facebook/react/module/processing/ReactModuleSpecProcessor.java index 1d5b4ae59..ab5dbe85e 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/module/processing/ReactModuleSpecProcessor.java +++ b/ReactAndroid/src/main/java/com/facebook/react/module/processing/ReactModuleSpecProcessor.java @@ -5,6 +5,8 @@ package com.facebook.react.module.processing; +import com.facebook.react.bridge.CxxModuleWrapper; +import com.facebook.react.bridge.OnBatchCompleteListener; import javax.annotation.processing.AbstractProcessor; import javax.annotation.processing.Filer; import javax.annotation.processing.Messager; @@ -21,6 +23,7 @@ import javax.lang.model.element.TypeElement; import javax.lang.model.type.MirroredTypesException; import javax.lang.model.type.TypeMirror; import javax.lang.model.util.Elements; +import javax.lang.model.util.Types; import java.io.IOException; import java.util.ArrayList; @@ -72,6 +75,7 @@ public class ReactModuleSpecProcessor extends AbstractProcessor { private Elements mElements; @SuppressFieldNotInitialized private Messager mMessager; + private Types mTypes; @Override public synchronized void init(ProcessingEnvironment processingEnv) { @@ -80,6 +84,7 @@ public class ReactModuleSpecProcessor extends AbstractProcessor { mFiler = processingEnv.getFiler(); mElements = processingEnv.getElementUtils(); mMessager = processingEnv.getMessager(); + mTypes = processingEnv.getTypeUtils(); } @Override @@ -154,6 +159,9 @@ public class ReactModuleSpecProcessor extends AbstractProcessor { } else { builder.addStatement("$T map = new $T()", MAP_TYPE, INSTANTIATED_MAP_TYPE); + TypeMirror cxxModuleWrapperTypeMirror = mElements.getTypeElement(CxxModuleWrapper.class.getName()).asType(); + TypeMirror onBatchCompleteListenerTypeMirror = mElements.getTypeElement(OnBatchCompleteListener.class.getName()).asType(); + for (String nativeModule : nativeModules) { String keyString = nativeModule; @@ -163,6 +171,7 @@ public class ReactModuleSpecProcessor extends AbstractProcessor { keyString + " not found by ReactModuleSpecProcessor. " + "Did you misspell the module?"); } + ReactModule reactModule = typeElement.getAnnotation(ReactModule.class); if (reactModule == null) { throw new ReactModuleSpecException( @@ -182,12 +191,27 @@ public class ReactModuleSpecProcessor extends AbstractProcessor { name -> name.contentEquals("getConstants") || name.contentEquals("getTypedExportedConstants")); } + boolean isCxxModule = mTypes.isAssignable(typeElement.asType(), cxxModuleWrapperTypeMirror); + boolean hasOnBatchCompleteListener = false; + try { + hasOnBatchCompleteListener = mTypes.isAssignable(typeElement.asType(), onBatchCompleteListenerTypeMirror); + } catch (RuntimeException e) { + // This is SUPER ugly, but we need to do this, especially for AsyncStorageModule which implements ModuleDataCleaner + // In the case of that specific class, we get the exception + // com.sun.tools.javac.code.Symbol$CompletionFailure: class file for ModuleDataCleaner not found. + // The exception is caused because the class is not loaded the first time. However, catching it and + // running it again the second time loads the class and does what the following statement originally intended + hasOnBatchCompleteListener = mTypes.isAssignable(typeElement.asType(), onBatchCompleteListenerTypeMirror); + } + String valueString = new StringBuilder() .append("new ReactModuleInfo(") .append("\"").append(reactModule.name()).append("\"").append(", ") .append(reactModule.canOverrideExistingModule()).append(", ") .append(reactModule.needsEagerInit()).append(", ") - .append(hasConstants) + .append(hasConstants).append(", ") + .append(isCxxModule).append(", ") + .append(hasOnBatchCompleteListener) .append(")") .toString(); diff --git a/ReactAndroid/src/test/java/com/facebook/react/bridge/BaseJavaModuleTest.java b/ReactAndroid/src/test/java/com/facebook/react/bridge/BaseJavaModuleTest.java index 637bd812b..215b52771 100644 --- a/ReactAndroid/src/test/java/com/facebook/react/bridge/BaseJavaModuleTest.java +++ b/ReactAndroid/src/test/java/com/facebook/react/bridge/BaseJavaModuleTest.java @@ -41,7 +41,7 @@ public class BaseJavaModuleTest { @Before public void setup() { ModuleHolder moduleHolder = new ModuleHolder(new MethodsModule()); - mWrapper = new JavaModuleWrapper(null, MethodsModule.class, moduleHolder); + mWrapper = new JavaModuleWrapper(null, MethodsModule.class.getName(), moduleHolder); mMethods = mWrapper.getMethodDescriptors(); PowerMockito.mockStatic(SoLoader.class); mArguments = PowerMockito.mock(ReadableNativeArray.class);