Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw TypeError if new Exception or getArg would expose v128 to JS #309

Merged
merged 1 commit into from
Jun 14, 2024

Conversation

dschuff
Copy link
Member

@dschuff dschuff commented Jun 14, 2024

Fixes #308
Fixes #295

@dschuff dschuff requested a review from aheejin June 14, 2024 00:38
@aheejin
Copy link
Member

aheejin commented Jun 14, 2024

Fixes #308 and #295

By the way in order to close both issues you need to prepend "fixes" to each of the issue, like, "fixes #xxx and fixes #yyy"

Copy link
Member

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

I believe there are more places that need to protect against exnref now, e.g., the Table constructor, Table#set, Table#grow, and perhaps others.

Overall, it would be simpler and more robust to move these checks into ToWebAssemblyValue, ToJSValue, and DefaultValue instead of duplicating them everywhere.

@dschuff
Copy link
Member Author

dschuff commented Jun 14, 2024

OK yeah I'll look into that. I wasn't sure if there was some reason the checks weren't put into those places in the first place. Maybe we'll have change something to deal with the fact that they can throw now.

@dschuff
Copy link
Member Author

dschuff commented Jun 14, 2024

Actually exnref is already handled in those places: IIRC you added them in your initial version that included the core spec. Since this change makes a symmetric change to Exception objects, I'm going to land it, but as i mentioned in #308 (comment) and you mentioned here, maybe it does just make sense to sink those checks into ToJSValue/ToWebAssembly value etc.

@dschuff dschuff merged commit f30dfee into main Jun 14, 2024
6 checks passed
@dschuff
Copy link
Member Author

dschuff commented Jun 14, 2024

Regarding that change: it would be a refactoring that would affect both v128 and exnref and would be a no-op change compared to what's here. So, would it be better to do in the main spec repo after we merge this?

@rossberg
Copy link
Member

Actually exnref is already handled in those places

It definitely is missing in Table#grow. And in the Table constructor, but that presumably cannot encounter this case right now, since ToValueType cannot currently return exnref. However, it might in the future, so for the sake of robustness, it wouldn't harm to have a check there as well (which of course would be subsumed by the refactoring).

So, would it be better to do in the main spec repo after we merge this?

Does that make it easier? I think it doesn't matter, and we could just as well do it while we're at it (and otherwise introduce more redundancy).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants