Skip to content

Commit

Permalink
Stop requiring TurboModuleManager JSIModule on CatalystInstance cleanup
Browse files Browse the repository at this point in the history
Summary:
In `CatalystInstanceImpl.destroy()`, we require the TurboModuleManager using the [following lines](https://fburl.com/diffusion/a4y6wbft):

```
final JSIModule turboModuleManager =
    ReactFeatureFlags.useTurboModules
        ? mJSIModuleRegistry.getModule(JSIModuleType.TurboModuleManager)
        : null;
```

For some strange reason, even though `ReactFeatureFlags.useTurboModules` is true, the TurboModuleManager isn't registered with mJSIModuleRegistry. I spent some time looking through the code, but I couldn't figure out why. These lines actually aren't necessary, so it's possible to fix the issue by simply working around it, which is what this diff does. We shouldn't have been double requiring the TurboModuleManager anyways, since `CatalystInstance.java` has a method to set the TurboModuleManager, which we call in `ReactInstanceManager.createReactContext`.

## Alternative approach
I could push this diff to the next cut, and instead land a diff that adds debug information to the native crash. At the cost of a week, it may help us figure out why we're seeing the crash. Thoughts? cc fkgozali

Reviewed By: fkgozali

Differential Revision: D17636604

fbshipit-source-id: ecfff593dc6eb4ec4d5e331348b308bc7ab37966
  • Loading branch information
RSNara authored and facebook-github-bot committed Sep 28, 2019
1 parent 1452954 commit 42dcfab
Showing 1 changed file with 6 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import com.facebook.react.bridge.queue.ReactQueueConfigurationSpec;
import com.facebook.react.common.ReactConstants;
import com.facebook.react.common.annotations.VisibleForTesting;
import com.facebook.react.config.ReactFeatureFlags;
import com.facebook.react.module.annotations.ReactModule;
import com.facebook.react.turbomodule.core.CallInvokerHolderImpl;
import com.facebook.react.turbomodule.core.interfaces.TurboModule;
Expand Down Expand Up @@ -101,6 +100,7 @@ public String toString() {

private JavaScriptContextHolder mJavaScriptContextHolder;
private @Nullable TurboModuleRegistry mTurboModuleRegistry = null;
private @Nullable JSIModule mTurboModuleManagerJSIModule = null;

// C++ parts
private final HybridData mHybridData;
Expand Down Expand Up @@ -353,20 +353,15 @@ public void run() {
}
}

final JSIModule turboModuleManager =
ReactFeatureFlags.useTurboModules
? mJSIModuleRegistry.getModule(JSIModuleType.TurboModuleManager)
: null;

getReactQueueConfiguration()
.getJSQueueThread()
.runOnQueue(
new Runnable() {
@Override
public void run() {
// We need to destroy the TurboModuleManager on the JS Thread
if (turboModuleManager != null) {
turboModuleManager.onCatalystInstanceDestroy();
if (mTurboModuleManagerJSIModule != null) {
mTurboModuleManagerJSIModule.onCatalystInstanceDestroy();
}

getReactQueueConfiguration()
Expand Down Expand Up @@ -563,8 +558,9 @@ public void run() {
}
}

public void setTurboModuleManager(JSIModule getter) {
mTurboModuleRegistry = (TurboModuleRegistry) getter;
public void setTurboModuleManager(JSIModule module) {
mTurboModuleRegistry = (TurboModuleRegistry) module;
mTurboModuleManagerJSIModule = module;
}

private void decrementPendingJSCalls() {
Expand Down

0 comments on commit 42dcfab

Please sign in to comment.