Skip to content

Restrict JSG_STRUCT from having v8 local members#4916

Merged
jasnell merged 1 commit intomainfrom
jasnell/jsg_struct_no_v8_locals
Aug 29, 2025
Merged

Restrict JSG_STRUCT from having v8 local members#4916
jasnell merged 1 commit intomainfrom
jasnell/jsg_struct_no_v8_locals

Conversation

@jasnell
Copy link
Copy Markdown
Collaborator

@jasnell jasnell commented Aug 27, 2025

Make it a compile time error for a JSG_STRUCT to have members that are v8 locals (including JsValues).

It's not entirely safe for a JSG_STRUCT to use v8 locals because the struct may outlive the v8 HandleScope in which the locals were created. Let's enforce it at compile time.

Refs: #4834 (comment)

@jasnell jasnell requested review from a team as code owners August 27, 2025 14:19
@jasnell jasnell requested a review from a team August 27, 2025 14:19
@jasnell jasnell requested a review from a team as a code owner August 27, 2025 14:19
@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 27, 2025

The generated output of @cloudflare/workers-types matches the snapshot in types/generated-snapshot 🎉

@jasnell jasnell force-pushed the jasnell/jsg_struct_no_v8_locals branch from 8c760ff to 89f0792 Compare August 27, 2025 14:43
@jasnell jasnell marked this pull request as draft August 27, 2025 15:21
@jasnell jasnell force-pushed the jasnell/jsg_struct_no_v8_locals branch from 89f0792 to 60bef82 Compare August 28, 2025 02:17
@jasnell jasnell requested a review from kentonv August 28, 2025 03:05
@jasnell jasnell marked this pull request as ready for review August 28, 2025 03:05
@jasnell jasnell force-pushed the jasnell/jsg_struct_no_v8_locals branch from d52b02a to 6dfea8c Compare August 29, 2025 15:13
Make it a compile time error for a JSG_STRUCT to have
members that are v8 locals (includig JsValues).

It's not entirely safe for a JSG_STRUCT to use v8 locals
because the struct may outlive the v8 HandleScope in
which the locals were created. Let's enforce it at compile
time.

Update src/workerd/jsg/struct-test.c++
@jasnell jasnell force-pushed the jasnell/jsg_struct_no_v8_locals branch from 6dfea8c to 8514ca1 Compare August 29, 2025 15:19
@jasnell jasnell enabled auto-merge August 29, 2025 15:23
@jasnell jasnell disabled auto-merge August 29, 2025 16:35
@jasnell jasnell merged commit 6630b45 into main Aug 29, 2025
21 of 22 checks passed
@jasnell jasnell deleted the jasnell/jsg_struct_no_v8_locals branch August 29, 2025 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants