Skip to content

Fix structuredClone() under enhanced_error_serialization.#5627

Merged
jasnell merged 1 commit intomainfrom
kenton/fix-enhanced_error_serialization-hostobject
Dec 2, 2025
Merged

Fix structuredClone() under enhanced_error_serialization.#5627
jasnell merged 1 commit intomainfrom
kenton/fix-enhanced_error_serialization-hostobject

Conversation

@kentonv
Copy link
Copy Markdown
Member

@kentonv kentonv commented Dec 2, 2025

The comments in ser.c++ suggested that V8 would always treat objects with internal fields as host objects, not calling the IsHostObject() callback even when it was enabled. This turns out to be wrong: if HasCustomHostObject() returns true, then IsHostObject() will be called for all objects, internal fields or otherwise, and the result respected.

Prior to the introduction of enhanced_error_serialization, though, HasCustomHostObject() would only return true when treatClassInstancesAsPlainObjects was false. This inadvertently neutralized the bug, because all objects with internal fields are also "class instances" (i.e. their prototype is something other than Object.prototype). So all API objects continued to be treated as host objects.

But enhanced_error_serialization introduced another way that HasCustomHostObject() could return true, even when treatClassInstancesAsPlainObjects is also true. Unfortuntaley, in this case, API objects (objects with internal fields) stopped being treated as host objects. Hence, all API objects started being serialized as empty objects, since the default serialization behavior for all objects is to ignore the prototype and just serialize the own properties.

The fix is to make sure we check for internal fields in IsHostObject().

Ironically, the test for DOMException serialization in js-rpc-test.js managed to depend on this bug. enhanced_error_serialization as implemented doesn't apply to DOMException, since it's a custom API object. But the bug caused it to be serialized as a plain object, which ended up causing its own properties to be preserved, as they should be under enhanced_error_serialization. However, its type was not preserved, but this was not actually tested, so the test passed by accident. I have removed this test from js-rpc-test and recreated it in rpc-error-test, where I also adjusted it to verify the type but not the own properties (leaving the latter as a TODO to fix later).

@kentonv kentonv requested a review from jasnell December 2, 2025 19:44
@kentonv kentonv requested review from a team as code owners December 2, 2025 19:44
@kentonv
Copy link
Copy Markdown
Member Author

kentonv commented Dec 2, 2025

Luckily enhanced_error_serialization is experimental so we should be able to make this fix without risking breaking people.

The comments in ser.c++ suggested that V8 would always treat objects with internal fields as host objects, not calling the `IsHostObject()` callback even when it was enabled. This turns out to be wrong: if HasCustomHostObject() returns true, then IsHostObject() will be called for *all* objects, internal fields or otherwise, and the result respected.

Prior to the introduction of `enhanced_error_serialization`, though, `HasCustomHostObject()` would only return true when `treatClassInstancesAsPlainObjects` was false. This inadvertently neutralized the bug, because all objects with internal fields are also "class instances" (i.e. their prototype is something other than `Object.prototype`). So all API objects continued to be treated as host objects.

But `enhanced_error_serialization` introduced another way that `HasCustomHostObject()` could return true, even when `treatClassInstancesAsPlainObjects` is also true. Unfortuntaley, in this case, API objects (objects with internal fields) _stopped_ being treated as host objects. Hence, all API objects started being serialized as empty objects, since the default serialization behavior for all objects is to ignore the prototype and just serialize the own properties.

The fix is to make sure we check for internal fields in `IsHostObject()`.

Ironically, the test for `DOMException` serialization in `js-rpc-test.js` managed to depend on this bug. `enhanced_error_serialization` as implemented doesn't apply to `DOMException`, since it's a custom API object. But the bug caused it to be serialized as a plain object, which ended up causing its own properties to be preserved, as they should be under `enhanced_error_serialization`. However, its _type_ was not preserved, but this was not actually tested, so the test passed by accident. I have removed this test from `js-rpc-test` and recreated it in `rpc-error-test`, where I also adjusted it to verify the type but not the own properties (leaving the latter as a TODO to fix later).
@kentonv kentonv force-pushed the kenton/fix-enhanced_error_serialization-hostobject branch from bb241be to 153b6e9 Compare December 2, 2025 19:56
@jasnell jasnell merged commit 5656e29 into main Dec 2, 2025
54 of 57 checks passed
@jasnell jasnell deleted the kenton/fix-enhanced_error_serialization-hostobject branch December 2, 2025 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants