Hermes reference tracking (#563)

* explicit jhybrid class deconstructors

* move ownership of values to runtime

* fix isReleased

* runtime approach

* referenceTracker

* rename to RuntimeScope

* .

* somewhat working

* debugging + trackRef

* sort of working again

* ok

* do not include jni by default

* remove all the debug logs

* working without passing runtime for undefined/null

* reverted null/undefined now that runtime isn't needed and no longer tracked

* address some PR comments

* addressed error handling and null ptr

* revert primitive tracking back

* clear the scope maps after release

* filter tracking in scoped Value constructor too

* object deconstructor

---------

Co-authored-by: Jeremiah Zucker <[email protected]>

jvm/hermes/src/main/jni/BUILD

@@ -23,10 +23,12 @@ cc_library(
         "JHermesRuntime.cpp",
         "JJSIValue.cpp",
         "OnLoad.cpp",
+        "RuntimeScope.cpp",
     ],
     hdrs = [
         "JHermesRuntime.h",
         "JJSIValue.h",
+        "RuntimeScope.h",
     ],
     copts = CMAKE_BUILD_TYPE_COPTS + select({
         # TODO: Using React Natives JSI doesn't contain microtask - come back to this if we build it ourselves

jvm/hermes/src/main/jni/JHermesRuntime.h

@@ -49,28 +49,7 @@ class JHermesRuntime : public HybridClass<JHermesRuntime, JJSIRuntime> {
 
     // TODO: Add the rest of the HermesRuntime API (like loading bytecode)
     local_ref<JJSIValue::jhybridobject> evaluateJavaScriptWithSourceMap(std::string script, std::string sourceMap, std::string sourceURL) {
-        auto result = JJSIValue::newObjectCxxArgs(get_runtime().evaluateJavaScriptWithSourceMap(std::make_shared<StringBuffer>(script), std::make_shared<StringBuffer>(sourceMap), sourceURL));
-        trackRef(result);
-        return result;
-    }
-
-    void trackRef(alias_ref<JHybridClass::jhybridobject> ref) override {
-        // TODO: From my current understanding, holding a global prevents the class from being
-        //       GC'd when it goes out of scope in Java land, which would prevent the JSI Value reference
-        //       from being reset. I originally wanted to keep a weak_ref, but was getting intermittent
-        //       segfaults from untracked (or incorrectly tracked) references that were then not being
-        //       released because the weak_ref was out of scope. The assumption there was that if the
-        //       weak_ref was out of scope, it was already released. But this doesn't seem to be true,
-        //       otherwise we wouldn't be getting segfaults. Making them global does prevent the segfaults,
-        //       reaffirming the above. Since all the wrapper classes are holding are pointers
-        //       this probably isn't a huge deal, but is certainly an inefficiency we should look
-        //       to remove. I'd like to just create weak_ptrs on the JSI values themselves,
-        //       which requires the holders to use shared_ptrs and expose a getter for accessing.
-        //       Then we'd also need a way to hold arbitrary weak_ptrs. We could also maybe try
-        //       to get weak refs to _just_ the CXX parts of the hybrid class to avoid leaking things
-        //       on the JVM side?
-        // TODO: Maybe we could do a periodic pruning of the vector to remove obsolete refs?
-        scope_.push_back(make_global(ref));
+        return JJSIValue::newObjectCxxArgs(this->runtimeScope_, get_runtime().evaluateJavaScriptWithSourceMap(std::make_shared<StringBuffer>(script), std::make_shared<StringBuffer>(sourceMap), sourceURL));;
     }
 
     ~JHermesRuntime() override {
@@ -79,9 +58,26 @@ class JHermesRuntime : public HybridClass<JHermesRuntime, JJSIRuntime> {
     }
 
     void release() {
-        for (auto& ref : scope_) {
-            ref->cthis()->release();
+        for (auto& ptr_pair : *runtimeScope_->valueScope) {
+            ptr_pair.second.reset();
+        }
+        for (auto& ptr_pair : *runtimeScope_->objectScope) {
+            ptr_pair.second.reset();
+        }
+        for (auto& ptr_pair : *runtimeScope_->functionScope) {
+            ptr_pair.second.reset();
         }
+        for (auto& ptr_pair : *runtimeScope_->arrayScope) {
+            ptr_pair.second.reset();
+        }
+        for (auto& ptr_pair : *runtimeScope_->symbolScope) {
+            ptr_pair.second.reset();
+        }
+        runtimeScope_->valueScope->clear();
+        runtimeScope_->objectScope->clear();
+        runtimeScope_->functionScope->clear();
+        runtimeScope_->arrayScope->clear();
+        runtimeScope_->symbolScope->clear();
         if (jConfig_) jConfig_.reset();
         if (runtime_) runtime_.reset();
     }
@@ -96,6 +92,10 @@ class JHermesRuntime : public HybridClass<JHermesRuntime, JJSIRuntime> {
         throwNativeHandleReleasedException("HermesRuntime");
     }
 
+    shared_ptr<RuntimeScope> get_scope() override {
+        return runtimeScope_;
+    }
+
     operator facebook::jsi::Runtime&() { return get_runtime(); }
 
     local_ref<JHermesConfig::jhybridobject> get_config() {
@@ -108,11 +108,11 @@ class JHermesRuntime : public HybridClass<JHermesRuntime, JJSIRuntime> {
     friend HybridBase;
     std::unique_ptr<HermesRuntime> runtime_;
     global_ref<JHermesConfig::jhybridobject> jConfig_;
-    std::vector<global_ref<JHybridClass::jhybridobject>> scope_;
+    shared_ptr<RuntimeScope> runtimeScope_;
     explicit JHermesRuntime(
         std::unique_ptr<HermesRuntime> runtime,
         alias_ref<JHermesConfig::jhybridobject> jConfig
-    ) : HybridClass(), runtime_(std::move(runtime)), jConfig_(make_global(jConfig)), scope_() {}
+    ) : HybridClass(), runtime_(std::move(runtime)), jConfig_(make_global(jConfig)), runtimeScope_(make_shared<RuntimeScope>()) {}
 
     explicit JHermesRuntime(alias_ref<JHermesConfig::jhybridobject> jConfig) : JHermesRuntime(makeHermesRuntime(jConfig->cthis()->get_config()), jConfig) {}
     explicit JHermesRuntime() : JHermesRuntime(JHermesConfig::create(JHermesConfig::javaClassStatic())) {}