fix(driver): fix multi thead conflict when destroy engine

Tencent · Oct 17, 2023 · 0658dc3 · 0658dc3
1 parent c609991
commit 0658dc3
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 24 deletions.
diff --git a/driver/js/include/driver/js_driver_utils.h b/driver/js/include/driver/js_driver_utils.h
@@ -53,8 +53,8 @@ class JsDriverUtils {
                            const string_view& global_config,
                            std::function<void(std::shared_ptr<Scope>)>&& scope_initialized_callback,
                            const JsCallback& call_host_callback);
-  static void DestroyInstance(const std::shared_ptr<Engine>& engine,
-                              const std::shared_ptr<Scope>& scope,
+  static void DestroyInstance(std::shared_ptr<Engine>&& engine,
+                              std::shared_ptr<Scope>&& scope,
                               const std::function<void(bool)>& callback,
                               bool is_reload);
   static bool RunScript(const std::shared_ptr<Scope>& scope,

diff --git a/driver/js/src/js_driver_utils.cc b/driver/js/src/js_driver_utils.cc
@@ -379,28 +379,10 @@ bool JsDriverUtils::RunScript(const std::shared_ptr<Scope>& scope,
   return flag;
 }
 
-void JsDriverUtils::DestroyInstance(const std::shared_ptr<Engine>& engine,
-                                    const std::shared_ptr<Scope>& scope,
+void JsDriverUtils::DestroyInstance(std::shared_ptr<Engine>&& engine,
+                                    std::shared_ptr<Scope>&& scope,
                                     const std::function<void(bool)>& callback,
                                     bool is_reload) {
-  auto scope_destroy_callback = [engine, scope, is_reload, callback] {
-#if defined(JS_V8) && defined(ENABLE_INSPECTOR) && !defined(V8_WITHOUT_INSPECTOR)
-    auto v8_vm = std::static_pointer_cast<V8VM>(engine->GetVM());
-    if (v8_vm->IsDebug()) {
-      auto inspector_client = v8_vm->GetInspectorClient();
-      if (inspector_client) {
-        auto inspector_context = scope->GetInspectorContext();
-        inspector_client->DestroyInspectorContext(is_reload, inspector_context);
-      }
-    } else {
-      scope->WillExit();
-    }
-#else
-    scope->WillExit();
-#endif
-    FOOTSTONE_LOG(INFO) << "js destroy end";
-    callback(true);
-  };
   auto vm = engine->GetVM();
   auto group = vm->GetGroupId();
   if (vm->IsDebug()) {
@@ -423,6 +405,33 @@ void JsDriverUtils::DestroyInstance(const std::shared_ptr<Engine>& engine,
     }
   }
   auto runner = engine->GetJsTaskRunner();
+
+  // DestroyInstance is called in the bridge thread, while the
+  // scope_destroy_callback is pushed into the task_runner to run in the DOM
+  // thread. When DestroyInstance is called, it holds the scope and engine.
+  // However, executing scope_destroy_callback may add tasks (e.g., setTimeout)
+  // holding the scope, even when the engine is destroyed. To prevent this, move
+  // the scope and engine into the scope_destroy_callback, which will be released
+  // when the callback is executed, ensuring no other tasks can be added to the
+  // task runner.
+  auto scope_destroy_callback = [engine = std::move(engine), scope = std::move(scope), is_reload, callback] {
+#if defined(JS_V8) && defined(ENABLE_INSPECTOR) && !defined(V8_WITHOUT_INSPECTOR)
+    auto v8_vm = std::static_pointer_cast<V8VM>(engine->GetVM());
+    if (v8_vm->IsDebug()) {
+      auto inspector_client = v8_vm->GetInspectorClient();
+      if (inspector_client) {
+        auto inspector_context = scope->GetInspectorContext();
+        inspector_client->DestroyInspectorContext(is_reload, inspector_context);
+      }
+    } else {
+      scope->WillExit();
+    }
+#else
+    scope->WillExit();
+#endif
+    FOOTSTONE_LOG(INFO) << "js destroy end";
+    callback(true);
+  };
   runner->PostTask(std::move(scope_destroy_callback));
   FOOTSTONE_DLOG(INFO) << "destroy, group = " << group;
 }

diff --git a/framework/android/connector/driver/js/src/main/cpp/src/js_driver_jni.cc b/framework/android/connector/driver/js/src/main/cpp/src/js_driver_jni.cc
@@ -51,6 +51,8 @@
 #include "vfs/uri.h"
 #include "vfs/vfs_resource_holder.h"
 
+#include <unistd.h>
+
 #ifdef JS_V8
 #include "driver/vm/v8/v8_vm.h"
 #endif
@@ -368,14 +370,13 @@ void DestroyJsDriver(__unused JNIEnv* j_env,
   auto scope_id = footstone::checked_numeric_cast<jint, uint32_t>(j_scope_id);
   auto flag = hippy::global_data_holder.Erase(scope_id);
   FOOTSTONE_CHECK(flag);
-  JsDriverUtils::DestroyInstance(engine, scope, [bridge_callback_object](bool ret) {
+  JsDriverUtils::DestroyInstance(std::move(engine), std::move(scope), [bridge_callback_object](bool ret) {
       if (ret) {
         hippy::bridge::CallJavaMethod(bridge_callback_object->GetObj(),INIT_CB_STATE::SUCCESS);
       } else {
         hippy::bridge::CallJavaMethod(bridge_callback_object->GetObj(),INIT_CB_STATE::DESTROY_ERROR);
       }
     }, static_cast<bool>(j_is_reload));
-  scope = nullptr;
 }
 
 void LoadInstance(JNIEnv* j_env,