chore: bump v8 crate to 152.2.0 - #36665
Open
bartlomieju wants to merge 5 commits into
Open
Conversation
- `Global::open` is now `unsafe`; migrate call sites to the safe `v8::Local::new(scope, handle)`. - Interceptor setter/definer callbacks now take `ReturnValue<Boolean>`. - V8 removed asm.js validation, so `--no-validate-asm` is gone. Leaving it in made V8 reject the flag string and silently drop every flag after it. - Bootstrap no longer fits in a 5MB heap; raise the heap-limit test caps.
`MicrotaskQueue::new` now returns an owning `MicrotaskQueueHandle` instead of a `UniqueRef`, so the leak that hands the queue to the context is now explicit. Indexed/named setter and definer interceptors take `ReturnValue<Boolean>`.
The setter/definer `ReturnValue` payload changed to `Boolean` in v8 152, but the QuickJS backend vendors an older rusty_v8 that still expects `()`. Add a `PropertyInterceptorReturnValue` alias to the deno_v8 facade, which is where backend differences belong, and name it from the interceptors.
v8 152 replaced the `UniqueRef<MicrotaskQueue>` from `MicrotaskQueue::new`
with an owning `MicrotaskQueueHandle`. Two things followed from that which the
first pass got wrong.
`MicrotaskQueue` no longer implements `Drop` — only the handle does — so the
`drop_in_place` in `ContextifyContext::drop` silently became a no-op, and
forgetting the handle to recover a raw pointer leaked it outright. Under the
default `v8_cppgc_microtask_queue` setting the handle is a root into the
isolate's heap that contexts outlive on their own, so it wants to be held and
dropped, not leaked.
`ContextifyContext` now stores an `Option<OwnedMicrotaskQueue>` and lets
ordinary ownership release it, which drops the hand-written `Drop` impl, the
`mem::forget`, and the `unsafe { drop_in_place }` along with it.
`ContextifyModule` keeps borrowing the queue as a raw alias, which is now
sounder: a context keeps its queue alive independently of the handle, so the
alias survives even if the owning `ContextifyContext` is collected first.
`OwnedMicrotaskQueue` is aliased in the deno_v8 facade because the QuickJS
backend still returns `UniqueRef<MicrotaskQueue>`; both are RAII wrappers that
deref to `MicrotaskQueue`, so callers just name the alias.
Member
Author
|
Blocking on v8x release for now |
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
1 participant
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.
Updates the
v8crate from 150.4.0 to 152.2.0, which brings V8 15.2.124.1.Four things in the new version needed adapting to.
Global::openis nowunsafe, because the reference it hands back is tied tothe lifetime of the
Globalrather than to a handle scope. Every call site inthis repo was of the same shape — open a
Globaland immediately call a methodon it — so rather than wrapping them in
unsafeblocks they now go throughv8::Local::new(scope, handle), which is safe and is what upstream recommends.That covers 25 sites in
libs/coreplus a handful inext/webgpuandext/node.MicrotaskQueue::newnow returns an owningMicrotaskQueueHandlethat freesthe queue on drop, where it previously returned a
UniqueRefwhoseinto_rawleaked it.
ext/node/ops/vm.rsrelies on that leak: the queue is handed to acontext as a raw pointer and has to outlive it, and V8 offers no hook for when
the context goes away. The leak is now spelled out in a small helper with a
comment explaining why it is deliberate, instead of being implied by
into_raw.The named and indexed setter and definer interceptors now take
ReturnValue<Boolean>instead ofReturnValue<()>, matching the deleterinterceptor, which already used
Boolean. All the affected callbacks eitherignore the return value or forward it unchanged, so this is a type change only.
Finally, V8 removed asm.js validation and with it the
--no-validate-asmflag.This one is worth calling out: the flag sat first in
base_flags, and V8 stopsparsing at the first unrecognized flag, so leaving it in caused every flag after
it —
--turbo_fast_api_calls,--harmony-temporal,--js-float16array,--js-explicit-resource-management,--js-source-phase-imports,--js-defer-import-evaland--enable-queue-microtask— to be silentlydropped. The
no_validate_asmintegration test stays as-is and still passes,since removing the validator means there is no output to suppress either way.
The two heap-limit tests in
libs/corecapped the isolate at 5MB, which V8152 can no longer bootstrap within; since the callbacks under test are
registered after
JsRuntime::newreturns, there was nothing to raise the limitand the test aborted the process on OOM. Both caps are raised to 20MB, which
leaves what the tests actually assert untouched.
cargo test -p deno_corepasses (451 tests) and the full runtime unit suitepasses (104 test files), as do the
node:vmtests that exercise the microtaskqueue change.
One wrinkle worth flagging for review: the
quickjsbackend behind thedeno_v8facade vendors an older rusty_v8 which still expectsReturnValue<()>for the setter and definer interceptors, and there is nov8xrelease tracking v8 152 to bump to. So rather than writing the payloadtype directly in
ext/node/ops/vm.rs, the interceptors now name aPropertyInterceptorReturnValuealias defined in the facade, which resolves toBooleanon the v8 backend and()on QuickJS. Papering over backenddifferences is what the facade is for, and this keeps both backends building
from one set of callbacks.
cargo check -p deno -p denort --no-default-features --features quickjspasses alongside the default build.