Function unmarshaling with .dump - #264
Closed
amoffat wants to merge 2 commits into
Closed
Conversation
Owner
|
Thanks for your interest in contributing to quickjs-emscripten. I'm closing this PR because I don't want to add security-sensitive "magic". Feel free to write and distribute your own helper libraries on top of quickjs-emscripten. |
Author
|
Hi @justjake thanks for taking a look. This can't be implemented as a helper library because it needs access to internals. I understand your security/sandbox concerns. What if I preserved the existing behavior and this code path was only engaged with: vm.dump(handle, { unmarshalFunctions: true })or vm.dumpWithFunctions(handle)This way the default behavior is non-magic. There is also the deserializer/OOM improvement in this PR f5d292e. It makes it so giant client-side objects don't trip OOM. Are you interested in that improvement? |
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.
Purpose
To allow
.dumpto unmarshal client-side functions that behave like host-side functions:Use case
I have client-side code that calls a host-side function and passes a callback as an argument. The host-side function then does some work, and eventually calls the callback. The brdige/shim on the host-side that facilitates calls from the client to the host does not care about the specific arguments being passed around. It just
.dumpsthem before calling the host function.Correctly unmarshalling functions with
.dumpallows me to pass callbacks.Changes made
Refactored
.dumpto do host-side unmarshalling, instead of client-sideJS_JSONStringify(viaQTS_Dump) followed byJSON.parse. This allows us to recursively unmarshal individual properties (in the case of nested objects), so that we handle functions that might be buried within those objects.Added a
.wrapmethod, which acts as sort of the inverse of.dump. It takes a host-side object and converts it to a client-side handle. It handles most of the common objects..wrapis needed because in order for the unmarshalled functions to behave like host-side functions, they need to transparently convert their arguments into client-side handles. To achieve this,.wrap()is called on them automatically.Side-effects
The test for OOM is no longer needed (and I was unable to trip it) because the objects are no longer being allocated on the vm heap before being transferred back to the host.
Additional notes
Exposed a
QTS_IsErrorhelper, so that.dumpcan preserve the behavior of unmarshalling error objects correctly (which have non-enumerable properties likename,message, etc).This PR depends on #262 because of the memory corruption issue.