Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Oct 17, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions driver/js/include/driver/js_driver_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
49 changes: 29 additions & 20 deletions driver/js/src/js_driver_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand All @@ -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;
}
Expand Down
5 changes: 3 additions & 2 deletions framework/android/connector/driver/js/src/main/cpp/src/js_driver_jni.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
Loading