Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 70 additions & 56 deletions crates/perry-runtime/src/object/field_get_set/has_property.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,66 +229,38 @@ pub extern "C" fn js_object_has_property(obj: f64, key: f64) -> f64 {

// A Web Fetch / zlib handle-band value (Headers/Request/Response, zlib
// streams) at or above the fetch band is a registry id, not a heap object —
// the pointer paths below would dereference the id and segfault, so this
// arm must resolve `key in <handle>` on its own and return.
//
// #6363: it used to report a flat `false`. That was a guard, not a route:
// a handle DOES have own properties — anything the user attached, via
// `handle.foo = v` or `Object.defineProperty(handle, …)` — and it has the
// typed native surface (`"status" in response`) on top. `hasOwnProperty`
// reports both, so a bare `false` here had `in` contradicting it. Answer
// from the expando table first (authoritative for own properties, and it
// sees a `{ value: undefined }` define that a [[Get]] probe cannot
// distinguish from absent), then fall back to the property dispatcher for
// the typed surface — which is exactly what the common/small-handle band
// already does further down this function.
// the pointer paths below would dereference the id and segfault. A blanket
// `false` was wrong, though: a `Request`/`Response` DOES have `body` /
// `method` / `url` / `headers` / … properties. Auth.js's request-body parser
// gates on `"body" in request` (`if(!("body" in e) || !e.body …) return`),
// so reporting `false` made it skip parsing the credentials POST body — the
// `csrfToken` field never reached the CSRF check and every login failed with
// `MissingCSRF`. Delegate a STRING key to the same handle property dispatcher
// that property *reads* use (safe for these ids — no heap deref): the
// property exists if it resolves to a non-undefined value. A symbol key has
// no own-property meaning on these handles, so it still reports `false`.
// Common/small handles (below the fetch band) are intentionally NOT caught
// here: they fall through to the registered small-handle property path later
// in this function.
if obj_val.is_pointer() {
let addr = (obj_val.bits() & crate::value::POINTER_MASK) as usize;
// A Buffer is an ordinary Uint8Array in Node, so `"k" in buf` must see both
// the own properties user code hangs on it and the inherited
// `Buffer.prototype` methods. Perry keeps buffers outside the object model
// (a raw `BufferHeader`), so the generic pointer path below reads the header
// as an `ObjectHeader` and reported `false` for both.
if crate::buffer::is_registered_buffer(addr) {
if let Some(name) = unsafe { crate::object::metadata_key_to_string(key) } {
if crate::buffer::buffer_get_own_prop(addr, &name).is_some()
|| crate::object::buffer_dispatch::is_buffer_method_name(&name)
{
return nanbox_true;
}
// A numeric key is an index into the bytes: present iff in range.
if let Ok(idx) = name.parse::<usize>() {
let len = unsafe { (*(addr as *const crate::buffer::BufferHeader)).length };
return if idx < len as usize {
nanbox_true
} else {
nanbox_false
};
}
return nanbox_false;
}
}
if addr >= crate::value::addr_class::COMMON_HANDLE_BAND_END
&& crate::value::addr_class::is_handle_band(addr)
{
if unsafe { crate::symbol::js_is_symbol(key) } != 0 {
return if unsafe { crate::symbol::js_object_has_own_symbol(obj, key) } {
nanbox_true
} else {
nanbox_false
};
}
if let Some(name) = unsafe { crate::object::metadata_key_to_string(key) } {
if crate::object::handle_expando::handle_expando_has(addr as i64, &name) {
return nanbox_true;
}
if key_val.is_any_string() {
if let Some(dispatch) = super::super::class_registry::handle_property_dispatch() {
let found = unsafe {
dispatch(addr as i64, name.as_ptr(), name.len()).to_bits()
!= crate::value::TAG_UNDEFINED
};
if found {
return nanbox_true;
unsafe {
let key_ptr = crate::value::js_get_string_pointer_unified(key)
as *const crate::StringHeader;
if !key_ptr.is_null() {
let name_ptr = (key_ptr as *const u8)
.add(std::mem::size_of::<crate::StringHeader>());
let name_len = (*key_ptr).byte_len as usize;
let result = dispatch(addr as i64, name_ptr, name_len);
if result.to_bits() != crate::value::TAG_UNDEFINED {
return nanbox_true;
}
}
}
}
}
Expand Down Expand Up @@ -406,6 +378,37 @@ pub extern "C" fn js_object_has_property(obj: f64, key: f64) -> f64 {
}

let obj_addr = obj_val.bits() & 0x0000_FFFF_FFFF_FFFF;

// A `class X extends Request/Response` instance is a heap object whose native
// members (`body`/`method`/`url`/`headers`/…) live on an underlying fetch
// handle, not the JS prototype chain — property *reads* forward through the
// stashed `__perry_fetch_handle__`. The `in` operator must forward too, or
// `"body" in <Request subclass>` is `false`. Next.js's `NextRequest` extends
// `Request`, and Auth.js gates request-body parsing on `"body" in request`
// (`if(!("body" in e) || …) return`), so without this the credentials POST
// body was never parsed and every login failed with `MissingCSRF`. Only a
// STRING key forwards (native members are string-keyed); a miss falls through
// to the generic own-property scan below so real expandos still resolve.
if key_val.is_any_string() {
if let Some(handle_id) = unsafe { super::fetch_subclass_handle_id(obj_addr as usize) } {
if let Some(dispatch) = super::super::class_registry::handle_property_dispatch() {
unsafe {
let key_ptr = crate::value::js_get_string_pointer_unified(key)
as *const crate::StringHeader;
if !key_ptr.is_null() {
let name_ptr =
(key_ptr as *const u8).add(std::mem::size_of::<crate::StringHeader>());
let name_len = (*key_ptr).byte_len as usize;
let result = dispatch(handle_id, name_ptr, name_len);
if result.to_bits() != crate::value::TAG_UNDEFINED {
return nanbox_true;
}
}
}
}
}
}

Comment on lines +392 to +411

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift

Significant performance overhead for all in operator checks.

Invoking fetch_subclass_handle_id here creates a substantial performance regression. Because this block evaluates for any heap pointer with a string key, every standard in check (e.g., "foo" in my_object) now incurs the cost of a full property lookup for __perry_fetch_handle__. On a miss, this lookup will also traverse the object's entire prototype chain before falling through to the ordinary presence check.

To mitigate this overhead, consider adding a fast-path guard before calling fetch_subclass_handle_id. For example, you could check an internal bit flag on the object header, verify a specific class_id if one exists for subclasses, or limit this lookup to only known native fetch properties (like "body", "method", "url").

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/perry-runtime/src/object/field_get_set/has_property.rs` around lines
392 - 411, Reduce the overhead in the string-key path before calling
fetch_subclass_handle_id by adding a reliable fast-path guard that identifies
objects capable of subclass property dispatch. Update the surrounding logic in
has_property.rs, preserving dispatch behavior for eligible subclass objects
while bypassing fetch_subclass_handle_id for ordinary heap objects and retaining
the existing fallback property check.

// Date / RegExp / Error exotic instances: own expando props + builtin
// slots + prototype methods. The generic pointer path below would
// bit-cast the cell as an `ObjectHeader`.
Expand Down Expand Up @@ -541,8 +544,7 @@ pub extern "C" fn js_object_has_property(obj: f64, key: f64) -> f64 {
// prototype members (`subarray`, `map`, `join`, `toString`, …)
// count. `typed_array_prototype_chain_has` builds the shared
// prototype intrinsic on demand, so this is order-independent
// (#6164). Buffer-specific `Buffer.prototype` methods
// (`readUInt8`, …) are not covered here.
// (#6164).
if unsafe {
crate::typedarray_props::typed_array_prototype_chain_has(
obj_addr as usize,
Expand All @@ -551,6 +553,18 @@ pub extern "C" fn js_object_has_property(obj: f64, key: f64) -> f64 {
} {
return nanbox_true;
}
// #6406: the Buffer-specific surface the %TypedArray% chain
// above does NOT cover — a user own-property (`buf.foo = v`)
// and the `Buffer.prototype` methods (`readUInt8`,
// `writeInt8`, …). Perry keeps buffers outside the object
// model, so both live in the buffer side tables, not on a
// prototype the chain scan can reach. Without this,
// `"writeInt8" in buf` and `"foo" in buf` reported false.
if crate::buffer::buffer_get_own_prop(obj_addr as usize, name).is_some()
|| crate::object::buffer_dispatch::is_buffer_method_name(name)
{
return nanbox_true;
}
Comment on lines +563 to +567

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win

Swap the order of conditions to avoid an unnecessary mutex lock.

Based on the provided codebase context, buffer_get_own_prop acquires a global mutex (buffer_props().lock()). Since the || operator short-circuits, place is_buffer_method_name first to avoid acquiring the lock when the property is a known buffer method name.

⚡ Proposed fix
-                    if crate::buffer::buffer_get_own_prop(obj_addr as usize, name).is_some()
-                        || crate::object::buffer_dispatch::is_buffer_method_name(name)
+                    if crate::object::buffer_dispatch::is_buffer_method_name(name)
+                        || crate::buffer::buffer_get_own_prop(obj_addr as usize, name).is_some()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if crate::buffer::buffer_get_own_prop(obj_addr as usize, name).is_some()
|| crate::object::buffer_dispatch::is_buffer_method_name(name)
{
return nanbox_true;
}
if crate::object::buffer_dispatch::is_buffer_method_name(name)
|| crate::buffer::buffer_get_own_prop(obj_addr as usize, name).is_some()
{
return nanbox_true;
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/perry-runtime/src/object/field_get_set/has_property.rs` around lines
563 - 567, In the property check surrounding buffer_get_own_prop, evaluate
is_buffer_method_name(name) before the mutex-acquiring buffer_get_own_prop call
so known buffer methods short-circuit without locking; preserve the existing
true result and fallback behavior.

}
return nanbox_false;
}
Expand Down
39 changes: 39 additions & 0 deletions test-files/test_gap_fetch_handle_in_operator.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Regression: the `in` operator on a Web Fetch `Request` / `Response` handle
// must report its real properties.
//
// Perry represents `Request` / `Response` / `Headers` as native handle-band ids
// (not heap objects). `js_object_has_property` (the `in` operator) took a
// blanket shortcut and reported `false` for ALL handle-band values to avoid
// dereferencing the id as a pointer — but a `Request` genuinely has `body` /
// `method` / `url` / `headers`. Auth.js's request-body parser gates on
// `"body" in request` (`if (!("body" in e) || !e.body …) return`), so the blanket
// `false` made it skip parsing the credentials POST body — the `csrfToken` never
// reached the CSRF check and every login failed with `MissingCSRF`. The fix
// delegates a string key to the same handle property dispatcher property *reads*
// use. Byte-identical to `node --experimental-strip-types`.

const req = new Request("http://example.com/", {
method: "POST",
body: "csrfToken=abc123",
headers: { "content-type": "application/x-www-form-urlencoded" },
});
console.log("body in req: " + ("body" in req));
console.log("method in req: " + ("method" in req));
console.log("url in req: " + ("url" in req));
console.log("headers in req: " + ("headers" in req));
console.log("bodyUsed in req: " + ("bodyUsed" in req));
console.log("nonexistent in req: " + ("zzTotallyNotAProp" in req));

const res = new Response("hello", { status: 201 });
console.log("body in res: " + ("body" in res));
console.log("status in res: " + ("status" in res));
console.log("ok in res: " + ("ok" in res));
console.log("nonexistent in res: " + ("zzTotallyNotAProp" in res));

// Next.js's NextRequest extends Request — the `in` check must forward through
// the subclass's underlying native handle too.
class MyReq extends Request {}
const sub = new MyReq("http://example.com/", { method: "POST", body: "csrfToken=abc123", headers: {} });
console.log("body in subclass: " + ("body" in sub));
console.log("method in subclass: " + ("method" in sub));
console.log("nonexistent in subclass: " + ("zzTotallyNotAProp" in sub));