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

inspector: Runtime.evaluate with throwOnSideEffect & contextId of vm.createContext throws when it shouldn't #27518

Closed
rsify opened this issue May 1, 2019 · 6 comments
Labels
inspector Issues and PRs related to the V8 inspector protocol

Comments

@rsify
Copy link

rsify commented May 1, 2019

  • Version: v12.1.0, although seems to apply to older versions too
  • Platform: macOS
  • Subsystem: inspector or vm

Demonstrating the issue from the title:

const inspector = require('inspector')
const vm = require('vm')

const session = new inspector.Session()
session.connect()

const ctx = vm.createContext({
	a: 100
})

a = 100

session.post('Runtime.evaluate', {
	expression: 'a',
	throwOnSideEffect: true,
	contextId: 2 // ctx's id
}, (error, res) => {
	console.log(res)
	if (
		res.exceptionDetails &&
		res.exceptionDetails.exception.description.startsWith('EvalError: Possible side-effect')
	) {
		process.exit(1)
	}
})

^ exits with code 1, even though the provided expression a is clearly causing no side effect.

Weirdly enough, changing the contextId from 2 to 1 (1 being the global one) makes the snippet exit with 0, hence working as expected.

@rsify rsify changed the title inspector: Runtime.evaluate with throwOnSideEffect & contextId of vm.createContext throws it shouldn't inspector: Runtime.evaluate with throwOnSideEffect & contextId of vm.createContext throws it shouldn't May 1, 2019
@targos
Copy link
Member

targos commented May 1, 2019

@nodejs/v8-inspector

@targos targos added the inspector Issues and PRs related to the V8 inspector protocol label May 1, 2019
@devsnek
Copy link
Member

devsnek commented May 1, 2019

it actually does cause a side effect. the VM context is a c++ defined proxy-like object.

@rsify
Copy link
Author

rsify commented May 1, 2019

Is this intended behaviour then @devsnek? If so, are there any workarounds?

@addaleax
Copy link
Member

addaleax commented May 1, 2019

@devsnek Um … what’s the side effect? Yes, it’s a proxy, but accessing variables doesn’t have side effects, right?

I think we’d need to extend NamedPropertyHandlerConfiguration and IndexedPropertyHandlerConfiguration to be able to actually mark the relevant functions as side-effect free in order for this to work, though.

@alexkozy
Copy link
Member

alexkozy commented May 1, 2019

I believe that it calls some C++ code, as @addalex mentioned - any C++ callback should be explicitly marked as nosideeffects: https://cs.chromium.org/chromium/src/v8/include/v8.h?rcl=ea50a032a6e5e9c796e408bf2dafc14b767d9896&l=6033 otherwise V8 considers it as side effect.

@addaleax
Copy link
Member

addaleax commented May 1, 2019

Oh, thanks @ak239 – I missed that kHasNoSideEffect exists. That’s nice! :)

addaleax added a commit to addaleax/node that referenced this issue May 1, 2019
@rsify rsify changed the title inspector: Runtime.evaluate with throwOnSideEffect & contextId of vm.createContext throws it shouldn't inspector: Runtime.evaluate with throwOnSideEffect & contextId of vm.createContext throws when it shouldn't May 1, 2019
targos pushed a commit that referenced this issue May 14, 2019
Fixes: #27518

PR-URL: #27523
Reviewed-By: Aleksei Koziatinskii <ak239spb@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants