Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

src: fix pointer compression build #50680

Closed
wants to merge 2 commits into from
Closed

src: fix pointer compression build #50680

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7923103
deps: V8: cherry-pick 475c8cdf9a95
kvakil Nov 12, 2023
b2b9acb
src: add IsolateScopes before using isolates
kvakil Nov 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/api/environment.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,8 @@ void SetIsolateMiscHandlers(v8::Isolate* isolate, const IsolateSettings& s) {

void SetIsolateUpForNode(v8::Isolate* isolate,
const IsolateSettings& settings) {
Isolate::Scope isolate_scope(isolate);

SetIsolateErrorHandlers(isolate, settings);
SetIsolateMiscHandlers(isolate, settings);
}
Expand Down Expand Up @@ -354,6 +356,9 @@ Isolate* NewIsolate(Isolate::CreateParams* params,

SetIsolateCreateParamsForNode(params);
Isolate::Initialize(isolate, *params);

Isolate::Scope isolate_scope(isolate);

if (snapshot_data == nullptr) {
// If in deserialize mode, delay until after the deserialization is
// complete.
Expand Down Expand Up @@ -428,6 +433,8 @@ Environment* CreateEnvironment(
ThreadId thread_id,
std::unique_ptr<InspectorParentHandle> inspector_parent_handle) {
Isolate* isolate = isolate_data->isolate();

Isolate::Scope isolate_scope(isolate);
HandleScope handle_scope(isolate);

const bool use_snapshot = context.IsEmpty();
Expand Down
3 changes: 3 additions & 0 deletions src/env.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,6 +349,8 @@ IsolateDataSerializeInfo IsolateData::Serialize(SnapshotCreator* creator) {

void IsolateData::DeserializeProperties(const IsolateDataSerializeInfo* info) {
size_t i = 0;

v8::Isolate::Scope isolate_scope(isolate_);
HandleScope handle_scope(isolate_);

if (per_process::enabled_debug_list.enabled(DebugCategory::MKSNAPSHOT)) {
Expand Down Expand Up @@ -431,6 +433,7 @@ void IsolateData::CreateProperties() {
// One byte because our strings are ASCII and we can safely skip V8's UTF-8
// decoding step.

v8::Isolate::Scope isolate_scope(isolate_);
HandleScope handle_scope(isolate_);

#define V(PropertyName, StringValue) \
Expand Down
3 changes: 3 additions & 0 deletions src/json_parser.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ bool JSONParser::Parse(const std::string& content) {
DCHECK(!parsed_);

Isolate* isolate = isolate_.get();
v8::Isolate::Scope isolate_scope(isolate);
v8::HandleScope handle_scope(isolate);

Local<Context> context = Context::New(isolate);
Expand Down Expand Up @@ -45,6 +46,7 @@ bool JSONParser::Parse(const std::string& content) {
std::optional<std::string> JSONParser::GetTopLevelStringField(
std::string_view field) {
Isolate* isolate = isolate_.get();
v8::Isolate::Scope isolate_scope(isolate);
v8::HandleScope handle_scope(isolate);

Local<Context> context = context_.Get(isolate);
Expand All @@ -70,6 +72,7 @@ std::optional<std::string> JSONParser::GetTopLevelStringField(

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

Local<Context> context = context_.Get(isolate);
Expand Down
2 changes: 1 addition & 1 deletion src/json_parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class JSONParser {
private:
// We might want a lighter-weight JSON parser for this use case. But for now
// using V8 is good enough.
RAIIIsolate isolate_;
RAIIIsolateWithoutEntering isolate_;

v8::Global<v8::Context> context_;
v8::Global<v8::Object> content_;
Expand Down
2 changes: 2 additions & 0 deletions src/node_sea.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,9 @@ std::optional<std::string> GenerateCodeCache(std::string_view main_path,
RAIIIsolate raii_isolate(SnapshotBuilder::GetEmbeddedSnapshotData());
Isolate* isolate = raii_isolate.get();

v8::Isolate::Scope isolate_scope(isolate);
RaisinTen marked this conversation as resolved.
Show resolved Hide resolved
HandleScope handle_scope(isolate);

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

Expand Down
9 changes: 7 additions & 2 deletions src/util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,7 @@ Local<String> UnionBytes::ToStringChecked(Isolate* isolate) const {
}
}

RAIIIsolate::RAIIIsolate(const SnapshotData* data)
RAIIIsolateWithoutEntering::RAIIIsolateWithoutEntering(const SnapshotData* data)
: allocator_{ArrayBuffer::Allocator::NewDefaultAllocator()} {
isolate_ = Isolate::Allocate();
CHECK_NOT_NULL(isolate_);
Expand All @@ -692,9 +692,14 @@ RAIIIsolate::RAIIIsolate(const SnapshotData* data)
Isolate::Initialize(isolate_, params);
}

RAIIIsolate::~RAIIIsolate() {
RAIIIsolateWithoutEntering::~RAIIIsolateWithoutEntering() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name is icky. Better suggestions are welcome - maybe RAIIIsolate needs to be renamed too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this special case? Isn't re-entering of isolates allowed anyways?

Copy link
Member

@joyeecheung joyeecheung Nov 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is only for the theoretical interference with outer Isolate scopes, I think we can simply CHECK_NULL(Isolate::TryGetCurrent()) in the JSONParser constructor and call it a day, or just add a comment to JSONParser "For now, don't use this when there's an entered Isolate while JSONParser is alive". The only usage of this class doesn't have outer isolate scopes and we can probably just switch to simdjson later. There's no need to make it super robust for a theoretical case that probably would not happen any time soon / will just be rewritten anyway.

per_process::v8_platform.Platform()->UnregisterIsolate(isolate_);
isolate_->Dispose();
}

RAIIIsolate::RAIIIsolate(const SnapshotData* data)
: isolate_{data}, isolate_scope_{isolate_.get()} {}

RAIIIsolate::~RAIIIsolate() {}

} // namespace node
22 changes: 18 additions & 4 deletions src/util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -969,11 +969,11 @@ void SetConstructorFunction(v8::Isolate* isolate,
SetConstructorFunctionFlag flag =
SetConstructorFunctionFlag::SET_CLASS_NAME);

// Simple RAII class to spin up a v8::Isolate instance.
class RAIIIsolate {
// Like RAIIIsolate, except doesn't enter the isolate while it's in scope.
class RAIIIsolateWithoutEntering {
public:
explicit RAIIIsolate(const SnapshotData* data = nullptr);
~RAIIIsolate();
explicit RAIIIsolateWithoutEntering(const SnapshotData* data = nullptr);
~RAIIIsolateWithoutEntering();

v8::Isolate* get() const { return isolate_; }

Expand All @@ -982,6 +982,20 @@ class RAIIIsolate {
v8::Isolate* isolate_;
};

// Simple RAII class to spin up a v8::Isolate instance and enter it
// immediately.
class RAIIIsolate {
public:
explicit RAIIIsolate(const SnapshotData* data = nullptr);
~RAIIIsolate();

v8::Isolate* get() const { return isolate_.get(); }

private:
RAIIIsolateWithoutEntering isolate_;
v8::Isolate::Scope isolate_scope_;
};

} // namespace node

#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
Expand Down