Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit a33cc22

Browse filesBrowse files
ywave620danielleadams
authored andcommitted
src,worker: fix race of WorkerHeapSnapshotTaker
Any WorkerHeapSnapshotTaker instance should be fully owned by main thread. Remove buggy access to it from the worker thread. PR-URL: #44745 Fixes: #44515 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
1 parent 4c869c8 commit a33cc22
Copy full SHA for a33cc22

File tree

Expand file treeCollapse file tree

2 files changed

+37
-14
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

2 files changed

+37
-14
lines changed
Open diff view settings
Collapse file

‎src/base_object.h‎

Copy file name to clipboardExpand all lines: src/base_object.h
+3-1Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,9 @@ inline T* Unwrap(v8::Local<v8::Value> obj) {
232232
// circumstances such as the GC or Environment cleanup.
233233
// If weak, destruction behaviour is not affected, but the pointer will be
234234
// reset to nullptr once the BaseObject is destroyed.
235-
// The API matches std::shared_ptr closely.
235+
// The API matches std::shared_ptr closely. However, this class is not thread
236+
// safe, that is, we can't have different BaseObjectPtrImpl instances in
237+
// different threads refering to the same BaseObject instance.
236238
template <typename T, bool kIsWeak>
237239
class BaseObjectPtrImpl final {
238240
public:
Collapse file

‎src/node_worker.cc‎

Copy file name to clipboardExpand all lines: src/node_worker.cc
+34-13Lines changed: 34 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -771,28 +771,49 @@ void Worker::TakeHeapSnapshot(const FunctionCallbackInfo<Value>& args) {
771771
->NewInstance(env->context()).ToLocal(&wrap)) {
772772
return;
773773
}
774-
BaseObjectPtr<WorkerHeapSnapshotTaker> taker =
775-
MakeDetachedBaseObject<WorkerHeapSnapshotTaker>(env, wrap);
774+
775+
// The created WorkerHeapSnapshotTaker is an object owned by main
776+
// thread's Isolate, it can not be accessed by worker thread
777+
std::unique_ptr<BaseObjectPtr<WorkerHeapSnapshotTaker>> taker =
778+
std::make_unique<BaseObjectPtr<WorkerHeapSnapshotTaker>>(
779+
MakeDetachedBaseObject<WorkerHeapSnapshotTaker>(env, wrap));
776780

777781
// Interrupt the worker thread and take a snapshot, then schedule a call
778782
// on the parent thread that turns that snapshot into a readable stream.
779-
bool scheduled = w->RequestInterrupt([taker, env](Environment* worker_env) {
780-
heap::HeapSnapshotPointer snapshot {
781-
worker_env->isolate()->GetHeapProfiler()->TakeHeapSnapshot() };
783+
bool scheduled = w->RequestInterrupt([taker = std::move(taker),
784+
env](Environment* worker_env) mutable {
785+
heap::HeapSnapshotPointer snapshot{
786+
worker_env->isolate()->GetHeapProfiler()->TakeHeapSnapshot()};
782787
CHECK(snapshot);
788+
789+
// Here, the worker thread temporarily owns the WorkerHeapSnapshotTaker
790+
// object.
791+
783792
env->SetImmediateThreadsafe(
784-
[taker, snapshot = std::move(snapshot)](Environment* env) mutable {
793+
[taker = std::move(taker),
794+
snapshot = std::move(snapshot)](Environment* env) mutable {
785795
HandleScope handle_scope(env->isolate());
786796
Context::Scope context_scope(env->context());
787797

788-
AsyncHooks::DefaultTriggerAsyncIdScope trigger_id_scope(taker.get());
789-
BaseObjectPtr<AsyncWrap> stream = heap::CreateHeapSnapshotStream(
790-
env, std::move(snapshot));
791-
Local<Value> args[] = { stream->object() };
792-
taker->MakeCallback(env->ondone_string(), arraysize(args), args);
793-
}, CallbackFlags::kUnrefed);
798+
AsyncHooks::DefaultTriggerAsyncIdScope trigger_id_scope(taker->get());
799+
BaseObjectPtr<AsyncWrap> stream =
800+
heap::CreateHeapSnapshotStream(env, std::move(snapshot));
801+
Local<Value> args[] = {stream->object()};
802+
taker->get()->MakeCallback(
803+
env->ondone_string(), arraysize(args), args);
804+
// implicitly delete `taker`
805+
},
806+
CallbackFlags::kUnrefed);
807+
808+
// Now, the lambda is delivered to the main thread, as a result, the
809+
// WorkerHeapSnapshotTaker object is delivered to the main thread, too.
794810
});
795-
args.GetReturnValue().Set(scheduled ? taker->object() : Local<Object>());
811+
812+
if (scheduled) {
813+
args.GetReturnValue().Set(wrap);
814+
} else {
815+
args.GetReturnValue().Set(Local<Object>());
816+
}
796817
}
797818

798819
void Worker::LoopIdleTime(const FunctionCallbackInfo<Value>& args) {

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.