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 90f70ed

Browse filesBrowse files
joyeecheungaduh95
joyeecheung
authored and
aduh95
committed
src: use cppgc to manage ContextifyContext
This simplifies the memory management of ContextifyContext, making all references visible to V8. The destructors don't need to do anything because when the wrapper is going away, the context is already going away or otherwise it would've been holding the wrapper alive, so there's no need to reset the pointers in the context. Also, any global handles to the context would've been empty at this point, and the per-Environment context tracking code is capable of dealing with empty handles from contexts purged elsewhere. To this end, the context tracking code also purges empty handles from the list now, to prevent keeping too many empty handles around. PR-URL: #56522 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
1 parent 6762768 commit 90f70ed
Copy full SHA for 90f70ed

File tree

Expand file treeCollapse file tree

5 files changed

+115
-60
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

5 files changed

+115
-60
lines changed
Open diff view settings
Collapse file

‎src/env.cc‎

Copy file name to clipboardExpand all lines: src/env.cc
+6-1Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,12 @@ void AsyncHooks::InstallPromiseHooks(Local<Context> ctx) {
223223
: PersistentToLocal::Strong(js_promise_hooks_[3]));
224224
}
225225

226+
void Environment::PurgeTrackedEmptyContexts() {
227+
std::erase_if(contexts_, [&](auto&& el) { return el.IsEmpty(); });
228+
}
229+
226230
void Environment::TrackContext(Local<Context> context) {
231+
PurgeTrackedEmptyContexts();
227232
size_t id = contexts_.size();
228233
contexts_.resize(id + 1);
229234
contexts_[id].Reset(isolate_, context);
@@ -232,7 +237,7 @@ void Environment::TrackContext(Local<Context> context) {
232237

233238
void Environment::UntrackContext(Local<Context> context) {
234239
HandleScope handle_scope(isolate_);
235-
std::erase_if(contexts_, [&](auto&& el) { return el.IsEmpty(); });
240+
PurgeTrackedEmptyContexts();
236241
for (auto it = contexts_.begin(); it != contexts_.end(); it++) {
237242
if (Local<Context> saved_context = PersistentToLocal::Weak(isolate_, *it);
238243
saved_context == context) {
Collapse file

‎src/env.h‎

Copy file name to clipboardExpand all lines: src/env.h
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1093,6 +1093,7 @@ class Environment final : public MemoryRetainer {
10931093
const char* errmsg);
10941094
void TrackContext(v8::Local<v8::Context> context);
10951095
void UntrackContext(v8::Local<v8::Context> context);
1096+
void PurgeTrackedEmptyContexts();
10961097

10971098
std::list<binding::DLib> loaded_addons_;
10981099
v8::Isolate* const isolate_;
Collapse file

‎src/node_contextify.cc‎

Copy file name to clipboardExpand all lines: src/node_contextify.cc
+33-36Lines changed: 33 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -118,8 +118,9 @@ Local<Name> Uint32ToName(Local<Context> context, uint32_t index) {
118118

119119
} // anonymous namespace
120120

121-
BaseObjectPtr<ContextifyContext> ContextifyContext::New(
122-
Environment* env, Local<Object> sandbox_obj, ContextOptions* options) {
121+
ContextifyContext* ContextifyContext::New(Environment* env,
122+
Local<Object> sandbox_obj,
123+
ContextOptions* options) {
123124
Local<ObjectTemplate> object_template;
124125
HandleScope scope(env->isolate());
125126
CHECK_IMPLIES(sandbox_obj.IsEmpty(), options->vanilla);
@@ -140,41 +141,32 @@ BaseObjectPtr<ContextifyContext> ContextifyContext::New(
140141
if (!(CreateV8Context(env->isolate(), object_template, snapshot_data, queue)
141142
.ToLocal(&v8_context))) {
142143
// Allocation failure, maximum call stack size reached, termination, etc.
143-
return BaseObjectPtr<ContextifyContext>();
144+
return {};
144145
}
145146
return New(v8_context, env, sandbox_obj, options);
146147
}
147148

148-
void ContextifyContext::MemoryInfo(MemoryTracker* tracker) const {}
149+
void ContextifyContext::Trace(cppgc::Visitor* visitor) const {
150+
CppgcMixin::Trace(visitor);
151+
visitor->Trace(context_);
152+
}
149153

150154
ContextifyContext::ContextifyContext(Environment* env,
151155
Local<Object> wrapper,
152156
Local<Context> v8_context,
153157
ContextOptions* options)
154-
: BaseObject(env, wrapper),
155-
microtask_queue_(options->own_microtask_queue
158+
: microtask_queue_(options->own_microtask_queue
156159
? options->own_microtask_queue.release()
157160
: nullptr) {
161+
CppgcMixin::Wrap(this, env, wrapper);
162+
158163
context_.Reset(env->isolate(), v8_context);
159164
// This should only be done after the initial initializations of the context
160165
// global object is finished.
161166
DCHECK_NULL(v8_context->GetAlignedPointerFromEmbedderData(
162167
ContextEmbedderIndex::kContextifyContext));
163168
v8_context->SetAlignedPointerInEmbedderData(
164169
ContextEmbedderIndex::kContextifyContext, this);
165-
// It's okay to make this reference weak - V8 would create an internal
166-
// reference to this context via the constructor of the wrapper.
167-
// As long as the wrapper is alive, it's constructor is alive, and so
168-
// is the context.
169-
context_.SetWeak();
170-
}
171-
172-
ContextifyContext::~ContextifyContext() {
173-
Isolate* isolate = env()->isolate();
174-
HandleScope scope(isolate);
175-
176-
env()->UnassignFromContext(PersistentToLocal::Weak(isolate, context_));
177-
context_.Reset();
178170
}
179171

180172
void ContextifyContext::InitializeGlobalTemplates(IsolateData* isolate_data) {
@@ -251,19 +243,18 @@ MaybeLocal<Context> ContextifyContext::CreateV8Context(
251243
return scope.Escape(ctx);
252244
}
253245

254-
BaseObjectPtr<ContextifyContext> ContextifyContext::New(
255-
Local<Context> v8_context,
256-
Environment* env,
257-
Local<Object> sandbox_obj,
258-
ContextOptions* options) {
246+
ContextifyContext* ContextifyContext::New(Local<Context> v8_context,
247+
Environment* env,
248+
Local<Object> sandbox_obj,
249+
ContextOptions* options) {
259250
HandleScope scope(env->isolate());
260251
CHECK_IMPLIES(sandbox_obj.IsEmpty(), options->vanilla);
261252
// This only initializes part of the context. The primordials are
262253
// only initialized when needed because even deserializing them slows
263254
// things down significantly and they are only needed in rare occasions
264255
// in the vm contexts.
265256
if (InitializeContextRuntime(v8_context).IsNothing()) {
266-
return BaseObjectPtr<ContextifyContext>();
257+
return {};
267258
}
268259

269260
Local<Context> main_context = env->context();
@@ -300,7 +291,7 @@ BaseObjectPtr<ContextifyContext> ContextifyContext::New(
300291
info.origin = *origin_val;
301292
}
302293

303-
BaseObjectPtr<ContextifyContext> result;
294+
ContextifyContext* result;
304295
Local<Object> wrapper;
305296
{
306297
Context::Scope context_scope(v8_context);
@@ -315,7 +306,7 @@ BaseObjectPtr<ContextifyContext> ContextifyContext::New(
315306
ctor_name,
316307
static_cast<v8::PropertyAttribute>(v8::DontEnum))
317308
.IsNothing()) {
318-
return BaseObjectPtr<ContextifyContext>();
309+
return {};
319310
}
320311
}
321312

@@ -328,21 +319,23 @@ BaseObjectPtr<ContextifyContext> ContextifyContext::New(
328319
env->host_defined_option_symbol(),
329320
options->host_defined_options_id)
330321
.IsNothing()) {
331-
return BaseObjectPtr<ContextifyContext>();
322+
return {};
332323
}
333324

334325
env->AssignToContext(v8_context, nullptr, info);
335326

336327
if (!env->contextify_wrapper_template()
337328
->NewInstance(v8_context)
338329
.ToLocal(&wrapper)) {
339-
return BaseObjectPtr<ContextifyContext>();
330+
return {};
340331
}
341332

342-
result =
343-
MakeBaseObject<ContextifyContext>(env, wrapper, v8_context, options);
344-
// The only strong reference to the wrapper will come from the sandbox.
345-
result->MakeWeak();
333+
result = cppgc::MakeGarbageCollected<ContextifyContext>(
334+
env->isolate()->GetCppHeap()->GetAllocationHandle(),
335+
env,
336+
wrapper,
337+
v8_context,
338+
options);
346339
}
347340

348341
Local<Object> wrapper_holder =
@@ -352,7 +345,7 @@ BaseObjectPtr<ContextifyContext> ContextifyContext::New(
352345
->SetPrivate(
353346
v8_context, env->contextify_context_private_symbol(), wrapper)
354347
.IsNothing()) {
355-
return BaseObjectPtr<ContextifyContext>();
348+
return {};
356349
}
357350

358351
// Assign host_defined_options_id to the sandbox object or the global object
@@ -364,7 +357,7 @@ BaseObjectPtr<ContextifyContext> ContextifyContext::New(
364357
env->host_defined_option_symbol(),
365358
options->host_defined_options_id)
366359
.IsNothing()) {
367-
return BaseObjectPtr<ContextifyContext>();
360+
return {};
368361
}
369362
return result;
370363
}
@@ -438,7 +431,7 @@ void ContextifyContext::MakeContext(const FunctionCallbackInfo<Value>& args) {
438431
options.host_defined_options_id = args[6].As<Symbol>();
439432

440433
TryCatchScope try_catch(env);
441-
BaseObjectPtr<ContextifyContext> context_ptr =
434+
ContextifyContext* context_ptr =
442435
ContextifyContext::New(env, sandbox, &options);
443436

444437
if (try_catch.HasCaught()) {
@@ -469,6 +462,10 @@ ContextifyContext* ContextifyContext::ContextFromContextifiedSandbox(
469462

470463
template <typename T>
471464
ContextifyContext* ContextifyContext::Get(const PropertyCallbackInfo<T>& args) {
465+
// TODO(joyeecheung): it should be fine to simply use
466+
// args.GetIsolate()->GetCurrentContext() and take the pointer at
467+
// ContextEmbedderIndex::kContextifyContext, as V8 is supposed to
468+
// push the creation context before invoking these callbacks.
472469
return Get(args.This());
473470
}
474471

Collapse file

‎src/node_contextify.h‎

Copy file name to clipboardExpand all lines: src/node_contextify.h
+70-14Lines changed: 70 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -23,17 +23,73 @@ struct ContextOptions {
2323
bool vanilla = false;
2424
};
2525

26-
class ContextifyContext : public BaseObject {
26+
/**
27+
* The memory management of a vm context is as follows:
28+
*
29+
* user code
30+
* │
31+
* As global proxy or ▼
32+
* ┌──────────────┐ kSandboxObject embedder data ┌────────────────┐
33+
* ┌─► │ V8 Context │────────────────────────────────►│ Wrapper holder │
34+
* │ └──────────────┘ └───────┬────────┘
35+
* │ ▲ Object constructor/creation context │
36+
* │ │ │
37+
* │ ┌──────┴────────────┐ contextify_context_private_symbol │
38+
* │ │ ContextifyContext │◄────────────────────────────────────┘
39+
* │ │ JS Wrapper │◄──────────► ┌─────────────────────────┐
40+
* │ └───────────────────┘ cppgc │ node::ContextifyContext │
41+
* │ │ C++ Object │
42+
* └──────────────────────────────────► └─────────────────────────┘
43+
* v8::TracedReference / ContextEmbedderIndex::kContextifyContext
44+
*
45+
* There are two possibilities for the "wrapper holder":
46+
*
47+
* 1. When vm.constants.DONT_CONTEXTIFY is used, the wrapper holder is the V8
48+
* context's global proxy object
49+
* 2. Otherwise it's the arbitrary "sandbox object" that users pass into
50+
* vm.createContext() or a new empty object created internally if they pass
51+
* undefined.
52+
*
53+
* In 2, the global object of the new V8 context is created using
54+
* global_object_template with interceptors that perform any requested
55+
* operations on the global object in the context first on the sandbox object
56+
* living outside of the new context, then fall back to the global proxy of the
57+
* new context.
58+
*
59+
* It's critical for the user-accessible wrapper holder to keep the
60+
* ContextifyContext wrapper alive via contextify_context_private_symbol
61+
* so that the V8 context is always available to the user while they still
62+
* hold the vm "context" object alive.
63+
*
64+
* It's also critical for the V8 context to keep the wrapper holder
65+
* (specifically, the "sandbox object" if users pass one) as well as the
66+
* node::ContextifyContext C++ object alive, so that when the code
67+
* runs inside the object and accesses the global object, the interceptors
68+
* can still access the "sandbox object" and perform operations
69+
* on them, even if users already relinquish access to the outer
70+
* "sandbox object".
71+
*
72+
* The v8::TracedReference and the ContextEmbedderIndex::kContextifyContext
73+
* slot in the context only act as shortcuts between
74+
* the node::ContextifyContext C++ object and the V8 context.
75+
*/
76+
class ContextifyContext final : CPPGC_MIXIN(ContextifyContext) {
2777
public:
78+
SET_CPPGC_NAME(ContextifyContext)
79+
void Trace(cppgc::Visitor* visitor) const final;
80+
2881
ContextifyContext(Environment* env,
2982
v8::Local<v8::Object> wrapper,
3083
v8::Local<v8::Context> v8_context,
3184
ContextOptions* options);
32-
~ContextifyContext();
3385

34-
void MemoryInfo(MemoryTracker* tracker) const override;
35-
SET_MEMORY_INFO_NAME(ContextifyContext)
36-
SET_SELF_SIZE(ContextifyContext)
86+
// The destructors don't need to do anything because when the wrapper is
87+
// going away, the context is already going away or otherwise it would've
88+
// been holding the wrapper alive, so there's no need to reset the pointers
89+
// in the context. Also, any global handles to the context would've been
90+
// empty at this point, and the per-Environment context tracking code is
91+
// capable of dealing with empty handles from contexts purged elsewhere.
92+
~ContextifyContext() = default;
3793

3894
static v8::MaybeLocal<v8::Context> CreateV8Context(
3995
v8::Isolate* isolate,
@@ -48,7 +104,7 @@ class ContextifyContext : public BaseObject {
48104
Environment* env, const v8::Local<v8::Object>& wrapper_holder);
49105

50106
inline v8::Local<v8::Context> context() const {
51-
return PersistentToLocal::Default(env()->isolate(), context_);
107+
return context_.Get(env()->isolate());
52108
}
53109

54110
inline v8::Local<v8::Object> global_proxy() const {
@@ -75,14 +131,14 @@ class ContextifyContext : public BaseObject {
75131
static void InitializeGlobalTemplates(IsolateData* isolate_data);
76132

77133
private:
78-
static BaseObjectPtr<ContextifyContext> New(Environment* env,
79-
v8::Local<v8::Object> sandbox_obj,
80-
ContextOptions* options);
134+
static ContextifyContext* New(Environment* env,
135+
v8::Local<v8::Object> sandbox_obj,
136+
ContextOptions* options);
81137
// Initialize a context created from CreateV8Context()
82-
static BaseObjectPtr<ContextifyContext> New(v8::Local<v8::Context> ctx,
83-
Environment* env,
84-
v8::Local<v8::Object> sandbox_obj,
85-
ContextOptions* options);
138+
static ContextifyContext* New(v8::Local<v8::Context> ctx,
139+
Environment* env,
140+
v8::Local<v8::Object> sandbox_obj,
141+
ContextOptions* options);
86142

87143
static bool IsStillInitializing(const ContextifyContext* ctx);
88144
static void MakeContext(const v8::FunctionCallbackInfo<v8::Value>& args);
@@ -140,7 +196,7 @@ class ContextifyContext : public BaseObject {
140196
static void IndexedPropertyEnumeratorCallback(
141197
const v8::PropertyCallbackInfo<v8::Array>& args);
142198

143-
v8::Global<v8::Context> context_;
199+
v8::TracedReference<v8::Context> context_;
144200
std::unique_ptr<v8::MicrotaskQueue> microtask_queue_;
145201
};
146202

Collapse file

‎test/parallel/test-inspector-contexts.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-inspector-contexts.js
+5-9Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ common.skipIfInspectorDisabled();
88
const assert = require('assert');
99
const vm = require('vm');
1010
const { Session } = require('inspector');
11-
11+
const { gcUntil } = require('../common/gc');
1212
const session = new Session();
1313
session.connect();
1414

@@ -66,8 +66,7 @@ async function testContextCreatedAndDestroyed() {
6666

6767
// GC is unpredictable...
6868
console.log('Checking/waiting for GC.');
69-
while (!contextDestroyed)
70-
global.gc();
69+
await gcUntil('context destruction', () => contextDestroyed, Infinity, { type: 'major', execution: 'async' });
7170
console.log('Context destroyed.');
7271

7372
assert.strictEqual(contextDestroyed.params.executionContextId, id,
@@ -98,8 +97,7 @@ async function testContextCreatedAndDestroyed() {
9897

9998
// GC is unpredictable...
10099
console.log('Checking/waiting for GC again.');
101-
while (!contextDestroyed)
102-
global.gc();
100+
await gcUntil('context destruction', () => contextDestroyed, Infinity, { type: 'major', execution: 'async' });
103101
console.log('Other context destroyed.');
104102
}
105103

@@ -124,8 +122,7 @@ async function testContextCreatedAndDestroyed() {
124122

125123
// GC is unpredictable...
126124
console.log('Checking/waiting for GC a third time.');
127-
while (!contextDestroyed)
128-
global.gc();
125+
await gcUntil('context destruction', () => contextDestroyed, Infinity, { type: 'major', execution: 'async' });
129126
console.log('Context destroyed once again.');
130127
}
131128

@@ -148,8 +145,7 @@ async function testContextCreatedAndDestroyed() {
148145

149146
// GC is unpredictable...
150147
console.log('Checking/waiting for GC a fourth time.');
151-
while (!contextDestroyed)
152-
global.gc();
148+
await gcUntil('context destruction', () => contextDestroyed, Infinity, { type: 'major', execution: 'async' });
153149
console.log('Context destroyed a fourth time.');
154150
}
155151
}

0 commit comments

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