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

vm: enhance cached data handling for empty functions and add validation tests #56442

Closed
wants to merge 4 commits into from
Closed

vm: enhance cached data handling for empty functions and add validation tests #56442

wants to merge 4 commits into from
+39 −2

Conversation

gurgunday
Copy link
Contributor

Fixes #56366

@gurgunday gurgunday changed the title vm: enhance cached data handling for empty functions and add validati… vm: enhance cached data handling for empty functions and add validation tests Jan 2, 2025
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem. labels Jan 2, 2025
@codecov Codecov
Copy link

codecov bot commented Jan 2, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.54%. Comparing base (35742a2) to head (e1b97b8).
Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
src/node_contextify.cc 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #56442      +/-   ##
==========================================
- Coverage   88.54%   88.54%   -0.01%     
==========================================
  Files         657      657              
  Lines      190742   190744       +2     
  Branches    36606    36617      +11     
==========================================
- Hits       168901   168893       -8     
- Misses      15027    15029       +2     
- Partials     6814     6822       +8
Files with missing lines Coverage Δ
src/node_contextify.cc 81.41% <83.33%> (+0.04%) ⬆️

... and 37 files with indirect coverage changes

legendecas
legendecas requested changes Jan 3, 2025
View reviewed changes
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

The issue is that the code cache of vm.Script and vm.compileFunction can not be interchanged. I think this is rather a V8 issue that it didn't reject the incorrect code cache.

src/node_contextify.cc
if (produce_cached_data) {
new_cached_data.reset(ScriptCompiler::CreateCodeCacheForFunction(fn));
if (produce_cached_data && !fn.IsEmpty()) {
TryCatchScope try_cache(env);
Copy link
Member

Choose a reason for hiding this comment

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

There is no JavaScript execution in this scope and this TryCacheScope is redundant.

src/node_contextify.cc
new_cached_data.reset(ScriptCompiler::CreateCodeCacheForFunction(fn));
if (produce_cached_data && !fn.IsEmpty()) {
TryCatchScope try_cache(env);
if (options != ScriptCompiler::kConsumeCodeCache) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this guard is the correct solution. When produceCachedData is true, a new cached data should always be returned regardless of whether the old one is consumed or not.

src/node_contextify.cc
@@ -1564,9 +1564,13 @@ Local<Object> ContextifyContext::CompileFunctionAndCacheResult(
return Object::New(env->isolate());

std::unique_ptr<ScriptCompiler::CachedData> new_cached_data;
if (produce_cached_data) {
new_cached_data.reset(ScriptCompiler::CreateCodeCacheForFunction(fn));
if (produce_cached_data && !fn.IsEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

The fn here is always not empty.

@gurgunday
Copy link
Contributor Author

The issue is that the code cache of vm.Script and vm.compileFunction can not be interchanged. I think this is rather a V8 issue that it didn't reject the incorrect code cache.

I see, so you say it's better to signal this to the V8 team and wait? This also fixes it

@legendecas
Copy link
Member

legendecas commented Jan 6, 2025 •
edited
Loading

v8/v8@96ee9bb should fix the issue.

@gurgunday gurgunday closed this Jan 6, 2025
@gurgunday gurgunday deleted the fix-issue branch January 6, 2025 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legendecas legendecas legendecas requested changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

vm.compileFunction results in an abort
3 participants
@gurgunday @legendecas @nodejs-github-bot