Skip to content

Conversation

@maraf
Copy link
Member

@maraf maraf commented Apr 28, 2022

A follow up on the #65994.

Migrate rest of the JS interop functions to out params instead of return.

  • mono_wasm_invoke_js_with_args
  • mono_wasm_cancel_promise
  • mono_wasm_compile_function

@maraf maraf added this to the 7.0.0 milestone Apr 28, 2022
@maraf maraf requested review from kg, lewing and pavelsavara April 28, 2022 10:09
@maraf maraf requested a review from marek-safar as a code owner April 28, 2022 10:09
@maraf maraf self-assigned this Apr 28, 2022
@ghost
Copy link

ghost commented Apr 28, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

A follow up on the #65994.

Migrate rest of the JS interop functions to out params instead of return.

  • mono_wasm_invoke_js_with_args
  • mono_wasm_cancel_promise
  • mono_wasm_compile_function
Author: maraf
Assignees: maraf
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript

Milestone: 7.0.0

@maraf
Copy link
Member Author

maraf commented Apr 28, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member

@kg please have look as well.

@kg
Copy link
Member

kg commented Apr 28, 2022

Any icalls we change the signature of like these need to have their name changed, because any third party callers (like blazor, uno etc) will crash or corrupt the heap by passing the wrong number of arguments (or reading a return value that isn't there). The convention I selected is a 'Ref' suffix at the end of the name for C#/C methods, and for JS methods either 'ref' (if they take a MonoObjectRef) or 'root' (if they take a WasmRoot)

Copy link
Member

@kg kg left a comment

Choose a reason for hiding this comment

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

Thanks for doing the work of spotting these issues and updating the code!

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 28, 2022
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 28, 2022
@maraf
Copy link
Member Author

maraf commented Apr 28, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@maraf maraf requested a review from kg April 28, 2022 11:51
@maraf
Copy link
Member Author

maraf commented Apr 28, 2022

Any icalls we change the signature of like these need to have their name changed, because any third party callers (like blazor, uno etc) will crash or corrupt the heap by passing the wrong number of arguments (or reading a return value that isn't there). The convention I selected is a 'Ref' suffix at the end of the name for C#/C methods, and for JS methods either 'ref' (if they take a MonoObjectRef) or 'root' (if they take a WasmRoot)

Thanks! Now I see it in your PR, I should have read it before.

@maraf maraf merged commit 8058bbe into dotnet:main Apr 29, 2022
@maraf maraf deleted the WasmMigrateToOutParams branch April 29, 2022 08:24
@ghost ghost locked as resolved and limited conversation to collaborators May 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants