Skip to content

Commit

Permalink
src: fix JSONParser leaking internal V8 scopes
Browse files Browse the repository at this point in the history
JSONParser uses V8's JSON.parse (for now), meaning that its uses handles
and contexts. JSONParser was leaking its internal HandleScope and
Context::Scope.

Move the scope construction to the member functions to prevent those
scopes from leaking.

Refs: nodejs#50680 (comment)
PR-URL: nodejs#50688
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
  • Loading branch information
kvakil authored and martenrichter committed Nov 26, 2023
1 parent 032d766 commit e9c6d17
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 7 deletions.
22 changes: 17 additions & 5 deletions src/json_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,16 @@ using v8::Object;
using v8::String;
using v8::Value;

JSONParser::JSONParser()
: handle_scope_(isolate_.get()),
context_(isolate_.get(), Context::New(isolate_.get())),
context_scope_(context_.Get(isolate_.get())) {}
JSONParser::JSONParser() {}

bool JSONParser::Parse(const std::string& content) {
DCHECK(!parsed_);

Isolate* isolate = isolate_.get();
Local<Context> context = context_.Get(isolate);
v8::HandleScope handle_scope(isolate);

Local<Context> context = Context::New(isolate);
Context::Scope context_scope(context);

// It's not a real script, so don't print the source line.
errors::PrinterTryCatch bootstrapCatch(
Expand All @@ -34,16 +34,24 @@ bool JSONParser::Parse(const std::string& content) {
!result_value->IsObject()) {
return false;
}

context_.Reset(isolate, context);
content_.Reset(isolate, result_value.As<Object>());
parsed_ = true;

return true;
}

std::optional<std::string> JSONParser::GetTopLevelStringField(
std::string_view field) {
Isolate* isolate = isolate_.get();
v8::HandleScope handle_scope(isolate);

Local<Context> context = context_.Get(isolate);
Context::Scope context_scope(context);

Local<Object> content_object = content_.Get(isolate);

Local<Value> value;
// It's not a real script, so don't print the source line.
errors::PrinterTryCatch bootstrapCatch(
Expand All @@ -62,7 +70,11 @@ std::optional<std::string> JSONParser::GetTopLevelStringField(

std::optional<bool> JSONParser::GetTopLevelBoolField(std::string_view field) {
Isolate* isolate = isolate_.get();
v8::HandleScope handle_scope(isolate);

Local<Context> context = context_.Get(isolate);
Context::Scope context_scope(context);

Local<Object> content_object = content_.Get(isolate);
Local<Value> value;
bool has_field;
Expand Down
3 changes: 1 addition & 2 deletions src/json_parser.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,8 @@ class JSONParser {
// We might want a lighter-weight JSON parser for this use case. But for now
// using V8 is good enough.
RAIIIsolate isolate_;
v8::HandleScope handle_scope_;

v8::Global<v8::Context> context_;
v8::Context::Scope context_scope_;
v8::Global<v8::Object> content_;
bool parsed_ = false;
};
Expand Down

0 comments on commit e9c6d17

Please sign in to comment.