Skip to content

Commit

Permalink
Ensure C++ default tmmd can access the CxxReactPackage (facebook#41673)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: facebook#41673

## The Problem
The cxxReactPackage property isn't initialized by the time initHybrid was executed. So, initHybrid was being called with null as the cxxReactPackage.

Why;
1. The default turbomodule manager delegate receives cxxReactPackage as a constructor param.
2. Java then executes all the constructors of the class hierarchy. This executes initHybrid.
3. Java finally initializes the properties of the derived class: default tmmd.cxxReactPackage (i.e: the parameter to initHybrid). **This is too late**

## The Fix
Refactor the code such that hybrid data creation doesn't depend on property initialization:
1. Create a static initHybrid method in default tmmd.
2. Call this static method with the cxxReactPackage, and assign the resultant HybridData to mHybridData (in tmmd).

Changelog: [Internal]

Reviewed By: javache

Differential Revision: D51550623

fbshipit-source-id: ed2b7587351cfca408cda3c8cef4dcf7547e5f1e
  • Loading branch information
RSNara authored and facebook-github-bot committed Nov 28, 2023
1 parent 4c1bdea commit 129bd9d
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import androidx.annotation.Nullable;
import com.facebook.common.logging.FLog;
import com.facebook.infer.annotation.Assertions;
import com.facebook.jni.HybridData;
import com.facebook.react.bridge.CxxModuleWrapper;
import com.facebook.react.bridge.ModuleSpec;
import com.facebook.react.bridge.NativeModule;
Expand Down Expand Up @@ -53,13 +54,22 @@ interface ModuleProvider {
private List<ReactPackage> mPackages;
private ReactApplicationContext mReactContext;

protected ReactPackageTurboModuleManagerDelegate() {
protected ReactPackageTurboModuleManagerDelegate(
ReactApplicationContext reactApplicationContext, List<ReactPackage> packages) {
super();
initialize(reactApplicationContext, packages);
}

protected ReactPackageTurboModuleManagerDelegate(
ReactApplicationContext reactApplicationContext,
List<ReactPackage> packages,
HybridData hybridData) {
super(hybridData);
initialize(reactApplicationContext, packages);
}

private void initialize(
ReactApplicationContext reactApplicationContext, List<ReactPackage> packages) {
super();
if (mIsLazy) {
mPackages = packages;
mReactContext = reactApplicationContext;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,13 @@ private constructor(
context: ReactApplicationContext,
packages: List<ReactPackage>,
private val eagerlyInitializedModules: List<String>,
private val cxxReactPackage: CxxReactPackage?,
) : ReactPackageTurboModuleManagerDelegate(context, packages) {
cxxReactPackage: CxxReactPackage?,
) : ReactPackageTurboModuleManagerDelegate(context, packages, initHybrid(cxxReactPackage)) {

@DoNotStrip override fun initHybrid() = initHybrid(cxxReactPackage)

external fun initHybrid(cxxReactPackage: CxxReactPackage?): HybridData?
override fun initHybrid(): HybridData? {
throw UnsupportedOperationException(
"DefaultTurboModuleManagerDelegate.initHybrid() must never be called!")
}

override fun getEagerInitModuleNames(): List<String> {
if (unstable_isLazyTurboModuleDelegate()) {
Expand Down Expand Up @@ -62,8 +63,11 @@ private constructor(
DefaultTurboModuleManagerDelegate(context, packages, eagerInitModuleNames, cxxReactPackage)
}

@Synchronized
override fun maybeLoadOtherSoLibraries() {
DefaultSoLoader.maybeLoadSoLibrary()
companion object {
init {
DefaultSoLoader.maybeLoadSoLibrary()
}

@DoNotStrip @JvmStatic external fun initHybrid(cxxReactPackage: CxxReactPackage?): HybridData?
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ protected TurboModuleManagerDelegate() {
mHybridData = initHybrid();
}

protected TurboModuleManagerDelegate(HybridData hybridData) {
maybeLoadOtherSoLibraries();
mHybridData = hybridData;
}

/**
* Create and return a TurboModule Java object with name `moduleName`. If `moduleName` isn't a
* TurboModule, return null.
Expand Down Expand Up @@ -77,5 +82,7 @@ public boolean unstable_enableSyncVoidMethods() {
return false;
}

// TODO(T171231381): Consider removing this method: could we just use the static initializer
// of derived classes instead?
protected synchronized void maybeLoadOtherSoLibraries() {}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ std::function<std::shared_ptr<TurboModule>(

jni::local_ref<DefaultTurboModuleManagerDelegate::jhybriddata>
DefaultTurboModuleManagerDelegate::initHybrid(
jni::alias_ref<jhybridobject>,
jni::alias_ref<jclass> jClass,
jni::alias_ref<CxxReactPackage::javaobject> cxxReactPackage) {
return makeCxxInstance(cxxReactPackage);
}
Expand All @@ -43,9 +43,12 @@ std::shared_ptr<TurboModule> DefaultTurboModuleManagerDelegate::getTurboModule(
const std::string& name,
const std::shared_ptr<CallInvoker>& jsInvoker) {
if (cxxReactPackage_) {
auto module = cxxReactPackage_->cthis()->getModule(name, jsInvoker);
if (module) {
return module;
auto cppPart = cxxReactPackage_->cthis();
if (cppPart) {
auto module = cppPart->getModule(name, jsInvoker);
if (module) {
return module;
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class DefaultTurboModuleManagerDelegate : public jni::HybridClass<
"Lcom/facebook/react/defaults/DefaultTurboModuleManagerDelegate;";

static jni::local_ref<jhybriddata> initHybrid(
jni::alias_ref<jhybridobject>,
jni::alias_ref<jclass>,
jni::alias_ref<CxxReactPackage::javaobject>);

static void registerNatives();
Expand Down Expand Up @@ -53,7 +53,7 @@ class DefaultTurboModuleManagerDelegate : public jni::HybridClass<
jni::global_ref<CxxReactPackage::javaobject> cxxReactPackage_;

DefaultTurboModuleManagerDelegate(
jni::alias_ref<CxxReactPackage::javaobject>);
jni::alias_ref<CxxReactPackage::javaobject> cxxReactPackage);
};

} // namespace facebook::react

0 comments on commit 129bd9d

Please sign in to comment.