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

Define node::StreamBase's "onread" property as a JavaScript accessor #52758

Open
isheludko opened this issue Apr 30, 2024 · 0 comments
Open

Define node::StreamBase's "onread" property as a JavaScript accessor #52758

isheludko opened this issue Apr 30, 2024 · 0 comments
Labels
deprecations Issues and PRs related to deprecations. stream Issues and PRs related to the stream subsystem. v8 engine Issues and PRs related to the V8 dependency.

Comments

@isheludko
Copy link
Contributor

isheludko commented Apr 30, 2024

Version

v22.0.0-pre

Platform

No response

Subsystem

No response

What steps will reproduce the bug?

V8 is deprecating v8::ObjectTemplate::SetAccessor(..) (see https://crbug.com/336325111) which is used by node::StreamBase for defining "onread" property.
V8 suggests to use v8::ObjectTemplate::SetNativeDataProperty(..) instead but it has a slightly different behaviour and Node is currently relying on the old one.

The SetAccessor(..) defines a JavaScript data property which violates the JavaScript spec for the following use case:

  var proto = stream;
  var obj = Object.create(proto);
  obj.onread = 42; // [*]

According to the spec [*] should set the onread property on obj while for a SetAccessor-defined property V8 would trigger the property setter callback.
The SetNativeDataProperty-defined property would behave like a regular data property according to the spec.

Node defines "onread" property on the stream's prototype object while it sets the value via stream instance objects. This approach heavily depends on non-standard SetAccessor's semantics.
The right fix would be to define "onread" property as a JavaScript accessor using v8::ObjectTemplate::SetAccessorProperty(..).

The following fix for Node-ci seems to work: v8#183

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

No response

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

No response

What do you see instead?

n/a

Additional information

No response

@RedYetiDev RedYetiDev added stream Issues and PRs related to the stream subsystem. v8 engine Issues and PRs related to the V8 dependency. deprecations Issues and PRs related to deprecations. labels May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecations Issues and PRs related to deprecations. stream Issues and PRs related to the stream subsystem. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

2 participants