Fix structuredClone() under enhanced_error_serialization.#5627
Merged
Conversation
kentonv
commented
Dec 2, 2025
jasnell
reviewed
Dec 2, 2025
Member
Author
|
Luckily |
jasnell
approved these changes
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).
bb241be to
153b6e9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 whentreatClassInstancesAsPlainObjectswas false. This inadvertently neutralized the bug, because all objects with internal fields are also "class instances" (i.e. their prototype is something other thanObject.prototype). So all API objects continued to be treated as host objects.But
enhanced_error_serializationintroduced another way thatHasCustomHostObject()could return true, even whentreatClassInstancesAsPlainObjectsis 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
DOMExceptionserialization injs-rpc-test.jsmanaged to depend on this bug.enhanced_error_serializationas implemented doesn't apply toDOMException, 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 underenhanced_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 fromjs-rpc-testand recreated it inrpc-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).