Skip to content

V
V8
Commit 86363753 authored by Camillo Bruni's avatar Camillo Bruni Committed by V8 LUCI CQ
Browse files
Options

[d8] Add mutex guard for AsyncHooksWrap::async_wraps_ 

Multiple threads can modify async_wraps_ in parallel, which is not ok.

Drive-by-fix:
- Use normal constructor/destructor for AsyncHooksWrap
- Use unique_ptr for storing AsyncHooksWrap

Bug: chromium:1278276
Change-Id: I667980151c775be29e603790e589b1de76fae05a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3338257Reviewed-by: 's avatarMarja Hölttä <marja@chromium.org>
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Auto-Submit: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#78375}
parent 5e268155
Show whitespace changes
Inline Side-by-side
Showing with 93 additions and 102 deletions
src/d8/async-hooks-wrapper.cc
View file @ 86363753
......@@ -13,6 +13,69 @@
namespace v8 {
namespace {
AsyncHooksWrap* UnwrapHook(const v8::FunctionCallbackInfo<v8::Value>& args) {
Isolate* isolate = args.GetIsolate();
HandleScope scope(isolate);
Local<Object> hook = args.This();
AsyncHooks* hooks = PerIsolateData::Get(isolate)->GetAsyncHooks();
if (!hooks->async_hook_ctor.Get(isolate)->HasInstance(hook)) {
isolate->ThrowError("Invalid 'this' passed instead of AsyncHooks instance");
return nullptr;
}
Local<External> wrap = hook->GetInternalField(0).As<External>();
void* ptr = wrap->Value();
return static_cast<AsyncHooksWrap*>(ptr);
}
void EnableHook(const v8::FunctionCallbackInfo<v8::Value>& args) {
AsyncHooksWrap* wrap = UnwrapHook(args);
if (wrap) wrap->Enable();
}
void DisableHook(const v8::FunctionCallbackInfo<v8::Value>& args) {
AsyncHooksWrap* wrap = UnwrapHook(args);
if (wrap) wrap->Disable();
}
} // namespace
AsyncHooks::AsyncHooks(Isolate* isolate) : isolate_(isolate) {
AsyncContext ctx;
ctx.execution_async_id = 1;
ctx.trigger_async_id = 0;
asyncContexts.push(ctx);
current_async_id = 1;
HandleScope handle_scope(isolate_);
async_hook_ctor.Reset(isolate_, FunctionTemplate::New(isolate_));
async_hook_ctor.Get(isolate_)->SetClassName(
String::NewFromUtf8Literal(isolate_, "AsyncHook"));
async_hooks_templ.Reset(isolate_,
async_hook_ctor.Get(isolate_)->InstanceTemplate());
async_hooks_templ.Get(isolate_)->SetInternalFieldCount(1);
async_hooks_templ.Get(isolate_)->Set(
isolate_, "enable", FunctionTemplate::New(isolate_, EnableHook));
async_hooks_templ.Get(isolate_)->Set(
isolate_, "disable", FunctionTemplate::New(isolate_, DisableHook));
async_id_smb.Reset(isolate_, Private::New(isolate_));
trigger_id_smb.Reset(isolate_, Private::New(isolate_));
isolate_->SetPromiseHook(ShellPromiseHook);
}
AsyncHooks::~AsyncHooks() {
isolate_->SetPromiseHook(nullptr);
base::RecursiveMutexGuard lock_guard(&async_wraps_mutex_);
async_wraps_.clear();
}
void AsyncHooksWrap::Enable() { enabled_ = true; }
void AsyncHooksWrap::Disable() { enabled_ = false; }
......@@ -43,38 +106,6 @@ void AsyncHooksWrap::set_promiseResolve_function(
promiseResolve_function_.Reset(isolate_, value);
}
static AsyncHooksWrap* UnwrapHook(
const v8::FunctionCallbackInfo<v8::Value>& args) {
Isolate* isolate = args.GetIsolate();
HandleScope scope(isolate);
Local<Object> hook = args.This();
AsyncHooks* hooks = PerIsolateData::Get(isolate)->GetAsyncHooks();
if (!hooks->async_hook_ctor.Get(isolate)->HasInstance(hook)) {
isolate->ThrowError("Invalid 'this' passed instead of AsyncHooks instance");
return nullptr;
}
Local<External> wrap = hook->GetInternalField(0).As<External>();
void* ptr = wrap->Value();
return static_cast<AsyncHooksWrap*>(ptr);
}
static void EnableHook(const v8::FunctionCallbackInfo<v8::Value>& args) {
AsyncHooksWrap* wrap = UnwrapHook(args);
if (wrap) {
wrap->Enable();
}
}
static void DisableHook(const v8::FunctionCallbackInfo<v8::Value>& args) {
AsyncHooksWrap* wrap = UnwrapHook(args);
if (wrap) {
wrap->Disable();
}
}
async_id_t AsyncHooks::GetExecutionAsyncId() const {
return asyncContexts.top().execution_async_id;
}
......@@ -95,7 +126,8 @@ Local<Object> AsyncHooks::CreateHook(
return Local<Object>();
}
AsyncHooksWrap* wrap = new AsyncHooksWrap(isolate);
std::unique_ptr<AsyncHooksWrap> wrap =
std::make_unique<AsyncHooksWrap>(isolate);
Local<Object> fn_obj = args[0].As<Object>();
......@@ -113,12 +145,15 @@ Local<Object> AsyncHooks::CreateHook(
SET_HOOK_FN(promiseResolve);
#undef SET_HOOK_FN
async_wraps_.push_back(wrap);
Local<Object> obj = async_hooks_templ.Get(isolate)
->NewInstance(currentContext)
.ToLocalChecked();
obj->SetInternalField(0, External::New(isolate, wrap));
obj->SetInternalField(0, External::New(isolate, wrap.get()));
{
base::RecursiveMutexGuard lock_guard(&async_wraps_mutex_);
async_wraps_.push_back(std::move(wrap));
}
return handle_scope.Escape(obj);
}
......@@ -184,8 +219,9 @@ void AsyncHooks::ShellPromiseHook(PromiseHookType type, Local<Promise> promise,
hooks->asyncContexts.pop();
}
if (!i::StackLimitCheck{i_isolate}.HasOverflowed()) {
for (AsyncHooksWrap* wrap : hooks->async_wraps_) {
PromiseHookDispatch(type, promise, parent, wrap, hooks);
base::RecursiveMutexGuard lock_guard(&hooks->async_wraps_mutex_);
for (const auto& wrap : hooks->async_wraps_) {
PromiseHookDispatch(type, promise, parent, *wrap, hooks);
if (try_catch.HasCaught()) break;
}
if (try_catch.HasCaught()) Shell::ReportException(isolate, &try_catch);
......@@ -196,39 +232,12 @@ void AsyncHooks::ShellPromiseHook(PromiseHookType type, Local<Promise> promise,
}
}
void AsyncHooks::Initialize() {
HandleScope handle_scope(isolate_);
async_hook_ctor.Reset(isolate_, FunctionTemplate::New(isolate_));
async_hook_ctor.Get(isolate_)->SetClassName(
String::NewFromUtf8Literal(isolate_, "AsyncHook"));
async_hooks_templ.Reset(isolate_,
async_hook_ctor.Get(isolate_)->InstanceTemplate());
async_hooks_templ.Get(isolate_)->SetInternalFieldCount(1);
async_hooks_templ.Get(isolate_)->Set(
isolate_, "enable", FunctionTemplate::New(isolate_, EnableHook));
async_hooks_templ.Get(isolate_)->Set(
isolate_, "disable", FunctionTemplate::New(isolate_, DisableHook));
async_id_smb.Reset(isolate_, Private::New(isolate_));
trigger_id_smb.Reset(isolate_, Private::New(isolate_));
isolate_->SetPromiseHook(ShellPromiseHook);
}
void AsyncHooks::Deinitialize() {
isolate_->SetPromiseHook(nullptr);
for (AsyncHooksWrap* wrap : async_wraps_) {
delete wrap;
}
}
void AsyncHooks::PromiseHookDispatch(PromiseHookType type,
Local<Promise> promise,
Local<Value> parent, AsyncHooksWrap* wrap,
Local<Value> parent,
const AsyncHooksWrap& wrap,
AsyncHooks* hooks) {
if (!wrap->IsEnabled()) return;
if (!wrap.IsEnabled()) return;
v8::Isolate* v8_isolate = hooks->isolate_;
HandleScope handle_scope(v8_isolate);
......@@ -239,35 +248,30 @@ void AsyncHooks::PromiseHookDispatch(PromiseHookType type,
.ToLocalChecked();
Local<Value> args[1] = {async_id};
// This is unused. It's here to silence the warning about
// not using the MaybeLocal return value from Call.
MaybeLocal<Value> result;
// Sacrifice the brevity for readability and debugfulness
switch (type) {
case PromiseHookType::kInit:
if (!wrap->init_function().IsEmpty()) {
if (!wrap.init_function().IsEmpty()) {
Local<Value> initArgs[4] = {
async_id, String::NewFromUtf8Literal(v8_isolate, "PROMISE"),
promise->GetPrivate(context, hooks->trigger_id_smb.Get(v8_isolate))
.ToLocalChecked(),
promise};
result = wrap->init_function()->Call(context, rcv, 4, initArgs);
USE(wrap.init_function()->Call(context, rcv, 4, initArgs));
}
break;
case PromiseHookType::kBefore:
if (!wrap->before_function().IsEmpty()) {
result = wrap->before_function()->Call(context, rcv, 1, args);
if (!wrap.before_function().IsEmpty()) {
USE(wrap.before_function()->Call(context, rcv, 1, args));
}
break;
case PromiseHookType::kAfter:
if (!wrap->after_function().IsEmpty()) {
result = wrap->after_function()->Call(context, rcv, 1, args);
if (!wrap.after_function().IsEmpty()) {
USE(wrap.after_function()->Call(context, rcv, 1, args));
}
break;
case PromiseHookType::kResolve:
if (!wrap->promiseResolve_function().IsEmpty()) {
result = wrap->promiseResolve_function()->Call(context, rcv, 1, args);
if (!wrap.promiseResolve_function().IsEmpty()) {
USE(wrap.promiseResolve_function()->Call(context, rcv, 1, args));
}
}
}
......
src/d8/async-hooks-wrapper.h
View file @ 86363753
......@@ -28,10 +28,8 @@ struct AsyncContext {
class AsyncHooksWrap {
public:
explicit AsyncHooksWrap(Isolate* isolate) {
enabled_ = false;
isolate_ = isolate;
}
explicit AsyncHooksWrap(Isolate* isolate)
: isolate_(isolate), enabled_(false) {}
void Enable();
void Disable();
bool IsEnabled() const { return enabled_; }
......@@ -58,18 +56,8 @@ class AsyncHooksWrap {
class AsyncHooks {
public:
explicit AsyncHooks(Isolate* isolate) {
isolate_ = isolate;
AsyncContext ctx;
ctx.execution_async_id = 1;
ctx.trigger_async_id = 0;
asyncContexts.push(ctx);
current_async_id = 1;
Initialize();
}
~AsyncHooks() { Deinitialize(); }
explicit AsyncHooks(Isolate* isolate);
~AsyncHooks();
async_id_t GetExecutionAsyncId() const;
async_id_t GetTriggerAsyncId() const;
......@@ -79,19 +67,18 @@ class AsyncHooks {
Persistent<FunctionTemplate> async_hook_ctor;
private:
std::vector<AsyncHooksWrap*> async_wraps_;
base::RecursiveMutex async_wraps_mutex_;
std::vector<std::unique_ptr<AsyncHooksWrap>> async_wraps_;
Isolate* isolate_;
Persistent<ObjectTemplate> async_hooks_templ;
Persistent<Private> async_id_smb;
Persistent<Private> trigger_id_smb;
void Initialize();
void Deinitialize();
static void ShellPromiseHook(PromiseHookType type, Local<Promise> promise,
Local<Value> parent);
static void PromiseHookDispatch(PromiseHookType type, Local<Promise> promise,
Local<Value> parent, AsyncHooksWrap* wrap,
Local<Value> parent,
const AsyncHooksWrap& wrap,
AsyncHooks* hooks);
std::stack<AsyncContext> asyncContexts;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment