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.

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

new vm.Script only evaluates cachedData once #39009

Open
Ilmarinen100 opened this issue Jun 11, 2021 · 9 comments
Open

new vm.Script only evaluates cachedData once #39009

Ilmarinen100 opened this issue Jun 11, 2021 · 9 comments
Labels
v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem.

Comments

@Ilmarinen100
Copy link

Ilmarinen100 commented Jun 11, 2021

new vm.Script() only evaluates cachedData once when called multiple times with the same js-code. It uses an undocumented, internal cache.

  • 16.3.0:
  • Darwin XXXXXXX 19.6.0 Darwin Kernel Version 19.6.0: Thu May 6 00:48:39 PDT 2021; root:xnu-6153.141.33~1/RELEASE_X86_64 x86_64 i386 MacBookPro15,1 Darwin:
  • vm:

What steps will reproduce the bug?

run the following code in a file or repl:

const vm = require('vm');
const b = Buffer.alloc(0);
console.log(new vm.Script("", {cachedData: b});
console.log(new vm.Script("", {cachedData: b});

Reproducible: always

What is the expected behavior?

Script { cachedDataRejected: true }
Script { cachedDataRejected: true }

What do you see instead?

Script { cachedDataRejected: true }
Script { cachedDataRejected: false }

Additional information

Also reproducible in versions as far back as NodeJs 8.16 and on Linux

@targos
Copy link
Member

targos commented Jun 12, 2021

I don't know exactly how V8 determines whether two scripts are the same (and therefore uses its internal cache), but I think passing a different filename option to vm.Script should force it to recompile the code.

@targos targos added v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem. labels Jun 12, 2021
@joyeecheung
Copy link
Member

The second time V8 should be using it's internal per-isolate compilation cache so it won't be rejected (since it wasn't used at all), although at the V8 API level I don't think there is a way for us to tell the exact cache behavior (I see that there is a TODO there to expose this to the API for Blink though)

@joyeecheung
Copy link
Member

joyeecheung commented Jun 12, 2021

I'd say if there is a fix then we should be returning something more detailed than a boolean of cachedDataRejected, so this looks more like a feature request to me (if that solves your issue)

@targos
Copy link
Member

targos commented Jun 12, 2021

@joyeecheung is it mainly a feature request for us (i.e V8 gives us the reason for the rejection but we don't expose it) or for V8 ?

@Ilmarinen100
Copy link
Author

I'd say if there is a fix then we should be returning something more detailed than a boolean of cachedDataRejected, so this looks more like a feature request to me (if that solves your issue)

In my case I wanted to validate 2 distinct files with cachedData for the same string of js-code. I did not manage to have my code reach the "both rejected" case. (without spawning child processes). Likely I will have to implement the feature in a different way, possibly by validating some aspects of the bytecode on our own or by adding another checksum.

@joyeecheung
Copy link
Member

is it mainly a feature request for us (i.e V8 gives us the reason for the rejection but we don't expose it) or for V8 ?

I'd say both, V8 needs to expose more info about cache behavior in the embedder API and we need to expose that in our JS API, right now all we have is a boolean when internally it is more like a enum.

Likely I will have to implement the feature in a different way, possibly by validating some aspects of the bytecode on our own or by adding another checksum.

Does targo's suggestion of changing the filename work for you? That gives the script a different origin so the second try won't hit the compilation cache.

@Ilmarinen100
Copy link
Author

Ilmarinen100 commented Jun 14, 2021

@joyeecheung @targos : I made a typo in my initial trial. Using a filename indeed helps, even if the same filename is used again:

> console.log(new vm.Script("", {cachedData: b, filename: 'a.js'}))
Script { cachedDataRejected: true }
undefined
> console.log(new vm.Script("", {cachedData: b, filename: 'a.js'}))
Script { cachedDataRejected: true }
undefined

@targos
Copy link
Member

targos commented Jun 14, 2021

even if the same filename is used again

that's weird. filename is also the same when you don't specify it (it defaults to 'evalmachine.<anonymous>')

@Ilmarinen100
Copy link
Author

Ilmarinen100 commented Jun 14, 2021

@targos : very weird indeed, I built a little script and got these results:

valid: false filename: undefined            =>    RIGHT   Script { cachedDataRejected: true }
valid: false filename: undefined            =>    WRONG   Script { cachedDataRejected: false }
valid: true filename: undefined             =>    RIGHT   Script { cachedDataRejected: false }
valid: false filename: undefined            =>    WRONG   Script { cachedDataRejected: false }
valid: false filename: a.js                 =>    RIGHT   Script { cachedDataRejected: true }
valid: false filename: a.js                 =>    RIGHT   Script { cachedDataRejected: true }
valid: true filename: a.js                  =>    RIGHT   Script { cachedDataRejected: false }
valid: false filename: a.js                 =>    RIGHT   Script { cachedDataRejected: true }

basically it seems the strange behaviour only happens without a filename
[edit: the second last entry is RIGHT since node 16 but was WRONG in 14.15.0]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants