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

Inconsistent presence of property in contextified object #52720

Closed
lachrist opened this issue Apr 27, 2024 · 6 comments · Fixed by #53172
Closed

Inconsistent presence of property in contextified object #52720

lachrist opened this issue Apr 27, 2024 · 6 comments · Fixed by #53172
Labels
vm Issues and PRs related to the vm subsystem.

Comments

@lachrist
Copy link

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

@benjamingr
Copy link
Member

@nodejs/vm

@targos
Copy link
Member

targos commented Apr 30, 2024

I understand why it happens:

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

node/src/node_contextify.cc

Lines 477 to 478 in 6aa9047

maybe_rv =
ctx->global_proxy()->GetRealNamedProperty(context, property);

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
Copy link
Member

targos commented Apr 30, 2024

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

@targos targos self-assigned this May 27, 2024
targos added a commit to targos/node that referenced this issue May 27, 2024
@targos
Copy link
Member

targos commented May 27, 2024

I opened #53172

@targos targos removed their assignment May 29, 2024
@targos
Copy link
Member

targos commented Jun 5, 2024

Reopening as a revert of #53172 is necessary (#53348).

If we fix this, #53346 shows a use case to take care of.

@targos targos reopened this Jun 5, 2024
sophonieb pushed a commit to sophonieb/node that referenced this issue Jun 20, 2024
Fixes: nodejs#52720
PR-URL: nodejs#53172
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
Fixes: nodejs#52720
PR-URL: nodejs#53172
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@legendecas legendecas reopened this Jul 1, 2024
@legendecas
Copy link
Member

Re-landing at #53517

nodejs-github-bot pushed a commit that referenced this issue Jul 9, 2024
Distinguish named property and indexed property when enumerating keys
and handle query interceptions.

Co-Authored-By: Michaël Zasso <targos@protonmail.com>
PR-URL: #53517
Fixes: #52720
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
aduh95 pushed a commit that referenced this issue Jul 12, 2024
PR-URL: #53517
Fixes: #52720
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
aduh95 pushed a commit that referenced this issue Jul 12, 2024
Distinguish named property and indexed property when enumerating keys
and handle query interceptions.

Co-Authored-By: Michaël Zasso <targos@protonmail.com>
PR-URL: #53517
Fixes: #52720
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
aduh95 pushed a commit that referenced this issue Jul 16, 2024
PR-URL: #53517
Fixes: #52720
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
aduh95 pushed a commit that referenced this issue Jul 16, 2024
Distinguish named property and indexed property when enumerating keys
and handle query interceptions.

Co-Authored-By: Michaël Zasso <targos@protonmail.com>
PR-URL: #53517
Fixes: #52720
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vm Issues and PRs related to the vm subsystem.
5 participants