Fix references passed to async functions #3188
Merged
Conversation
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
Fixes rustwasm#3167 rustwasm#3167 was happening because references to `JsValue`s work by pushing the JS value onto the JS stack, and then popping it off again when the function returns. This breaks for async functions, because the `JsValue` needs to remain valid for the lifetime of the future, not just the initial synchronous call. To fix this, I introduced a new descriptor, `LONGREF`, which is used instead of `REF` in async functions and marks that the reference can't be invalidated after the function returns. I then made a `LONGREF` to a `JsValue` behave the same as an owned `JsValue`, while working like a regular `REF` for everything else. I also had to add a new version of `RefFromWasmAbi`, `LongRefFromWasmAbi`, to accomodate for `LONGREF`s to `JsValue`s requiring that the `JsValue` is destroyed from the Rust side. While working on that, I also noticed that mutable references to slices were broken in async functions. They work by copying the data from JS to Rust, and then copying it back after the function returns to apply the changes. For async functions, that means that it's copied back before the future is actually finished, and so the changes won't be applied properly. To fix that, I changed it so that the data is copied back from the Rust side when the anchor is dropped, which happens when the future resolves for async functions. I also bumped the schema version, since this is a breaking change in the interface between the macro and the CLI, even though the schema itself is unchanged.
`Instruction::MutableSliceToMemory` previously manually lowered the reference to the type array using `addHeapObject`, but that broke when reference types were enabled. The most direct solution would have been to check whether reference types were enabled and use `addToExternrefTable` instead if so, but using a separate instruction means that it gets picked up by `wasm-bindgen-externref-xform`, and an `externref` is passed directly to wasm.
alexcrichton
approved these changes
Dec 12, 2022
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.
Fixes #3167
#3167 was happening because references to
JsValues work by pushing the JS value onto the JS stack, and then popping it off again when the function returns. This breaks for async functions, because theJsValueneeds to remain valid for the lifetime of the future, not just the initial synchronous call.To fix this, I introduced a new descriptor,
LONGREF, which is used instead ofREFin async functions and marks that the reference can't be invalidated after the function returns. I then made aLONGREFto aJsValuebehave the same as an ownedJsValue, while working like a regularREFfor everything else.I also had to add a new version of
RefFromWasmAbi,LongRefFromWasmAbi, to accomodate forLONGREFs toJsValues requiring that theJsValueis destroyed from the Rust side.While working on that, I also noticed that mutable references to slices were broken in async functions. They work by copying the data from JS to Rust, and then copying it back after the function returns to apply the changes. For async functions, that means that it's copied back before the future is actually finished, and so the changes won't be applied properly. To fix that, I changed it so that the data is copied back from the Rust side when the anchor is dropped, which happens when the future resolves for async functions.
I also bumped the schema version, since this is a breaking change in the interface between the macro and the CLI, even though the schema itself is unchanged.