From 0a6f4dd8aaf4c8ff883ff762c409e30032d23d89 Mon Sep 17 00:00:00 2001 From: Joshua Gross Date: Fri, 11 Oct 2019 07:13:37 -0700 Subject: [PATCH] UIManagerBinding JSI function lambdas should not retain UIManager Summary: Ensure that on Android, only Scheduler and UIManagerBinder directly retain UIManager. These existing JSI function-retained lambdas will not have owning share_ptr refs to UIManager, just raw pointers. By the time the VM deallocates all JSI objects and UIManagerBinding goes away, it should be able to deallocate UIManager synchronously when UIManagerBinding is deallocated. The VM should not be executing any of these JSI functions at that time, so this should ensure that the raw pointer access is safe. This should also provide some extremely small perf wins, and make our memory model easier to reason about, since we're using shared_ptr less often and now `UIManager` is only owned in two places: Scheduler and UIManagerBinding. So, there are now only two places where UIManager can be deallocated: 1) Hermes is deallocated first, and UIManagerBinding is deallocated before Scheduler. Scheduler holds onto UIManager. Scheduler later is deallocated and frees UIManager; this would cause a crash if it's not completed before Hermes is torn down, but we currently ensure that Scheduler is torn down before Hermes. This is fine but we should document this contract, or make Scheduler not own UIManager. 2) Scheduler is deallocated first, decrementing the shared_ptr to UIManager. Later Hermes is torn down, which deallocates UIManagerBinding via the JSI pointer table, which synchronously deallocates UIManager before Hermes has shut down. This is the desired outcome. Changelog: [[Internal]] Reviewed By: shergin Differential Revision: D17872586 fbshipit-source-id: cbb25b5cd025f5f8597dc7a66073fe64edc57aa8 --- ReactCommon/fabric/uimanager/UIManagerBinding.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ReactCommon/fabric/uimanager/UIManagerBinding.cpp b/ReactCommon/fabric/uimanager/UIManagerBinding.cpp index edb6015aa65..c625156b9c8 100644 --- a/ReactCommon/fabric/uimanager/UIManagerBinding.cpp +++ b/ReactCommon/fabric/uimanager/UIManagerBinding.cpp @@ -155,7 +155,7 @@ jsi::Value UIManagerBinding::get( jsi::Runtime &runtime, const jsi::PropNameID &name) { auto methodName = name.utf8(runtime); - auto uiManager = uiManager_; + UIManager *uiManager = uiManager_.get(); // Semantic: Creates a new node with given pieces. if (methodName == "createNode") {