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. Introduce a new private inner class
HandleAndContextScope to reduce boilerplate.

Refs: #50680 (comment)
  • Loading branch information
kvakil committed Nov 12, 2023
1 parent 9134f4a commit 8eabb5b
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 7 deletions.
24 changes: 19 additions & 5 deletions src/json_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,28 @@

namespace node {
using v8::Context;
using v8::Global;
using v8::Isolate;
using v8::Local;
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::HandleAndContextScope::HandleAndContextScope(
v8::Isolate* isolate, const v8::Global<v8::Context>& context)
: handle_scope_(isolate), context_scope_(context.Get(isolate)) {}

JSONParser::HandleAndContextScope::~HandleAndContextScope() {}

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 +40,22 @@ 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();
HandleAndContextScope handle_and_context_scope(isolate, context_);

Local<Context> context = context_.Get(isolate);
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,6 +74,8 @@ std::optional<std::string> JSONParser::GetTopLevelStringField(

std::optional<bool> JSONParser::GetTopLevelBoolField(std::string_view field) {
Isolate* isolate = isolate_.get();
HandleAndContextScope handle_and_context_scope(isolate, context_);

Local<Context> context = context_.Get(isolate);
Local<Object> content_object = content_.Get(isolate);
Local<Value> value;
Expand Down
14 changes: 12 additions & 2 deletions src/json_parser.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,22 @@ class JSONParser {
std::optional<bool> GetTopLevelBoolField(std::string_view field);

private:
class HandleAndContextScope {
public:
HandleAndContextScope(v8::Isolate* isolate,
const v8::Global<v8::Context>& context);
~HandleAndContextScope();

private:
v8::HandleScope handle_scope_;
v8::Context::Scope context_scope_;
};

// 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 8eabb5b

Please sign in to comment.