node icon indicating copy to clipboard operation
node copied to clipboard

Inconsistent presence of property in contextified object

Open lachrist opened this issue 1 year ago • 3 comments

Version

v22.0.0

Platform

Darwin Laurents-MacBook-Air.local 23.2.0 Darwin Kernel Version 23.2.0: Wed Nov 15 21:59:33 PST 2023; root:xnu-10002.61.3~2/RELEASE_ARM64_T8112 arm64

Subsystem

vm

What steps will reproduce the bug?

import { createContext, runInContext } from "vm";
console.log(
  runInContext(
    `
      "use strict";
      ({
        hasOwn: Object.hasOwn(globalThis, "toLocaleString"),
        descriptor: Reflect.getOwnPropertyDescriptor(globalThis, "toLocaleString"),
      });
    `,
    createContext(),
  ),
); // { hasOwn: true, descriptor: undefined }

Tested in both v22.0.0 and v20.12.2.

How often does it reproduce? Is there a required condition?

No required condition.

What is the expected behavior? Why is that the expected behavior?

Either the property is present or it is not. So either { hasOwn: false, descriptor: undefined} and { hasOwn: true, descriptor: { ... } } would be better.

What do you see instead?

{ hasOwn: true, descriptor: undefined }

Additional information

No response

lachrist avatar Apr 27 '24 11:04 lachrist

@nodejs/vm

benjamingr avatar Apr 28 '24 20:04 benjamingr

I understand why it happens:

  • Object.has calls our PropertyGetterCallback interceptor
  • We call GetRealNamedProperty, which follows the prototype chain

https://github.com/nodejs/node/blob/6aa9047f960e2385dac807e0661a159324b00b0f/src/node_contextify.cc#L477-L478

I don't know how to fix it though. There are cases where we want to follow the protoype and other where we don't, but I'm not sure V8 gives us enough information to know in which case we are.

targos avatar Apr 30 '24 07:04 targos

Maybe the solution is to set a NamedPropertyQueryCallback (we currently don't).

targos avatar Apr 30 '24 07:04 targos

I opened https://github.com/nodejs/node/pull/53172

targos avatar May 27 '24 13:05 targos

Reopening as a revert of #53172 is necessary (https://github.com/nodejs/node/pull/53348).

If we fix this, https://github.com/nodejs/node/issues/53346 shows a use case to take care of.

targos avatar Jun 05 '24 12:06 targos

Re-landing at https://github.com/nodejs/node/pull/53517

legendecas avatar Jul 01 '24 21:07 legendecas

Reopened for https://github.com/nodejs/node/pull/54463

legendecas avatar Aug 20 '24 08:08 legendecas