Skip to content

Commit

Permalink
Make TurboModuleProviderFunctionType not depend on TurboModuleManager…
Browse files Browse the repository at this point in the history
… instance

Summary:
## Description
The C++ lambda that JS invokes to create TurboModules uses `TurboModuleManager`. It is possible for this lambda to outlive the `TurboModuleManager` instance because we delete `TurboModuleManager` on the JS Thread before we schedule the deletion of `CatalystInstanceImpl` object on a neutral third-party background thread. `CatalystInstanceImpl` owns the JS VM instance that owns the C++ lambda.

## [CatalystInstanceImpl.java](https://fburl.com/diffusion/vt4pwjwa)
```
public void destroy() {
  // ...

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

            getReactQueueConfiguration()
                .getUIQueueThread()
                .runOnQueue(
                    new Runnable() {
                      Override
                      public void run() {
                        // AsyncTask.execute must be executed from the UI Thread
                        AsyncTask.execute(
                            new Runnable() {
                              Override
                              public void run() {
                                // Kill non-UI threads from neutral third party
                                // potentially expensive, so don't run on UI thread

                                // contextHolder is used as a lock to guard against
                                // other users of the JS VM having the VM destroyed
                                // underneath them, so notify them before we reset
                                // Native
                                mJavaScriptContextHolder.clear();

                                mHybridData.resetNative();
                                getReactQueueConfiguration().destroy();
                                Log.d(
                                    ReactConstants.TAG,
                                    "CatalystInstanceImpl.destroy() end");
                                ReactMarker.logMarker(
                                    ReactMarkerConstants.DESTROY_CATALYST_INSTANCE_END);
                              }
                            });
                      }
                    });
          }
        });
    }
  });

  // ...
}
```

The JS thread is also terminated in the neutral third-party thread. Therefore, it should be possible for JS to request a `TurboModule` after `TurboModuleManager` has been destroyed (i.e: JS can try to access memory that was freed). This is why I think we're getting a segfault in T54298358.

## Fix
The fix was to wrap all the member variables of TurboModuleManager we use in `TurboModuleProviderFunctionType` in weak references. This way, we can make sure that the memory is valid before using it.

Reviewed By: fkgozali

Differential Revision: D17539761

fbshipit-source-id: fe527383458a019a4cb9107ec5c3ddd6295ae41c
  • Loading branch information
RSNara authored and facebook-github-bot committed Sep 26, 2019
1 parent a2aa008 commit 5e68a98
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 34 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ TurboModuleManager::TurboModuleManager(
runtime_(rt),
jsCallInvoker_(jsCallInvoker),
nativeCallInvoker_(nativeCallInvoker),
delegate_(jni::make_global(delegate))
delegate_(jni::make_global(delegate)),
turboModuleCache_(std::make_shared<TurboModuleCache>())
{}

jni::local_ref<TurboModuleManager::jhybriddata> TurboModuleManager::initHybrid(
Expand All @@ -58,39 +59,57 @@ void TurboModuleManager::installJSIBindings() {
if (!runtime_) {
return; // Runtime doesn't exist when attached to Chrome debugger.
}

TurboModuleBinding::install(*runtime_, std::make_shared<TurboModuleBinding>(
[this](const std::string &name) -> std::shared_ptr<TurboModule> {
auto turboModuleLookup = turboModuleCache_.find(name);
if (turboModuleLookup != turboModuleCache_.end()) {
return turboModuleLookup->second;
}

auto cxxModule = delegate_->cthis()->getTurboModule(name, jsCallInvoker_);
if (cxxModule) {
turboModuleCache_.insert({name, cxxModule});
return cxxModule;
}

static auto getLegacyCxxModule = delegate_->getClass()->getMethod<jni::alias_ref<CxxModuleWrapper::javaobject>(const std::string&)>("getLegacyCxxModule");
auto legacyCxxModule = getLegacyCxxModule(delegate_.get(), name);

if (legacyCxxModule) {
auto turboModule = std::make_shared<react::TurboCxxModule>(legacyCxxModule->cthis()->getModule(), jsCallInvoker_);
turboModuleCache_.insert({name, turboModule});
return turboModule;
}

static auto getJavaModule = javaClassStatic()->getMethod<jni::alias_ref<JTurboModule>(const std::string&)>("getJavaModule");
auto moduleInstance = getJavaModule(javaPart_.get(), name);

if (moduleInstance) {
auto turboModule = delegate_->cthis()->getTurboModule(name, moduleInstance, jsCallInvoker_, nativeCallInvoker_);
turboModuleCache_.insert({name, turboModule});
return turboModule;
}

return std::shared_ptr<TurboModule>(nullptr);
})
[
turboModuleCache_ = std::weak_ptr<TurboModuleCache>(turboModuleCache_),
jsCallInvoker_ = std::weak_ptr<CallInvoker>(jsCallInvoker_),
nativeCallInvoker_ = std::weak_ptr<CallInvoker>(nativeCallInvoker_),
delegate_ = jni::make_weak(delegate_),
javaPart_ = jni::make_weak(javaPart_)
]
(const std::string &name) -> std::shared_ptr<TurboModule> {
auto turboModuleCache = turboModuleCache_.lock();
auto jsCallInvoker = jsCallInvoker_.lock();
auto nativeCallInvoker = nativeCallInvoker_.lock();
auto delegate = delegate_.lockLocal();
auto javaPart = javaPart_.lockLocal();

if (!turboModuleCache || !jsCallInvoker || !nativeCallInvoker || !delegate || !javaPart) {
return nullptr;
}

auto turboModuleLookup = turboModuleCache->find(name);
if (turboModuleLookup != turboModuleCache->end()) {
return turboModuleLookup->second;
}

auto cxxModule = delegate->cthis()->getTurboModule(name, jsCallInvoker);
if (cxxModule) {
turboModuleCache->insert({name, cxxModule});
return cxxModule;
}

static auto getLegacyCxxModule = delegate->getClass()->getMethod<jni::alias_ref<CxxModuleWrapper::javaobject>(const std::string&)>("getLegacyCxxModule");
auto legacyCxxModule = getLegacyCxxModule(delegate.get(), name);

if (legacyCxxModule) {
auto turboModule = std::make_shared<react::TurboCxxModule>(legacyCxxModule->cthis()->getModule(), jsCallInvoker);
turboModuleCache->insert({name, turboModule});
return turboModule;
}

static auto getJavaModule = javaPart->getClass()->getMethod<jni::alias_ref<JTurboModule>(const std::string&)>("getJavaModule");
auto moduleInstance = getJavaModule(javaPart.get(), name);

if (moduleInstance) {
auto turboModule = delegate->cthis()->getTurboModule(name, moduleInstance, jsCallInvoker, nativeCallInvoker);
turboModuleCache->insert({name, turboModule});
return turboModule;
}

return nullptr;
})
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,15 @@ class TurboModuleManager : public jni::HybridClass<TurboModuleManager> {
std::shared_ptr<CallInvoker> nativeCallInvoker_;
jni::global_ref<TurboModuleManagerDelegate::javaobject> delegate_;

using TurboModuleCache = std::unordered_map<std::string, std::shared_ptr<react::TurboModule>>;

/**
* TODO(T48018690):
* All modules are currently long-lived.
* We need to come up with a mechanism to allow modules to specify whether
* they want to be long-lived or short-lived.
*/
std::unordered_map<std::string, std::shared_ptr<react::TurboModule>> turboModuleCache_;
std::shared_ptr<TurboModuleCache> turboModuleCache_;

void installJSIBindings();
explicit TurboModuleManager(
Expand Down

0 comments on commit 5e68a98

Please sign in to comment.