diff --git a/docs/reference/rust-review-checklist.md b/docs/reference/rust-review-checklist.md index c154b452972..4f069f0798d 100644 --- a/docs/reference/rust-review-checklist.md +++ b/docs/reference/rust-review-checklist.md @@ -82,14 +82,12 @@ Rust types exposed to JavaScript via the JSG bindings follow these patterns: Methods with a `&self` receiver are registered as instance methods on the prototype; methods without a receiver are registered as static methods on the constructor. Any other receiver is a compile error, since JavaScript can call back into the resource while a method runs; state that - methods change belongs in `Cell` or `RefCell` fields. A method or accessor can take - `this: jsg::Rc` instead of `&self` to get a `jsg::Rc` of its resource, for example to - hand to a callback. Flag state that a method shares with a callback through an `std::rc::Rc`; + methods change belongs in `Cell` or `RefCell` fields or, when it holds a `jsg::Rc` or a + `TracedGlobal`, in `jsg::Cell` or `jsg::RefCell` fields (`src/rust/jsg/README.md` §Cells). A + method or accessor can take `this: jsg::Rc` instead of `&self` to get a `jsg::Rc` of its + resource, for example to hand to a callback. Flag state that a method shares with a callback through an `std::rc::Rc`; the callback should hold `this` (or `this.downgrade()`) instead. Flag a `RefCell` borrow held across a call that can run JavaScript: a re-entrant call then panics. - Also flag a `borrow_mut()` of a `RefCell` holding GC references (`TracedGlobal`, `jsg::Rc`) - held across a call that can allocate on the V8 heap, wrap the resource or create or drop a - `jsg::Rc` to it: tracing skips the cell, so those references can lose their values. Verify the converted name is correct and matches the intended API surface. - **`#[jsg_static_constant]`** on a `const` item inside a `#[jsg_resource]` impl block exposes it as a read-only numeric constant on both the constructor and prototype (Rust equivalent of @@ -152,10 +150,12 @@ some violations, and the README says which. Reviewers flag these: - An `std::rc::Rc` around a `TracedGlobal`, a `jsg::Rc`, or a type holding them, even if it is not shared yet. The fix is a `jsg::Rc` or `jsg::Weak` to the resource that owns the state, such as a method's `this`. -- A `jsg::Rc` moved out of a traced field (`Cell::take`, `Option::take`, `mem::replace`, popping - a traced `Vec`) and used after a call that can run JavaScript or allocate on the V8 heap, - without a `.clone()` first - ([#7976](https://github.com/cloudflare/workerd/issues/7976)). +- A `jsg::Rc` or `TracedGlobal` reached through interior mutability other than a `jsg::Cell` or + `jsg::RefCell`: `UnsafeCell`, `OnceCell`, a hand-written cell, or a std `Cell` or `RefCell` + that a hand-written `impl Traced` visits through `borrow()`. A `jsg::Rc` or `TracedGlobal` in + a std cell field of a `#[jsg_resource]` or `#[jsg_struct]` does not compile; these are the + paths the compiler cannot see ([#7976](https://github.com/cloudflare/workerd/issues/7976)). +- A hand-written `impl HoldsNoGcReferences` on a type whose `trace` visits something. - A `jsg::v8::Global` inside a type with a hand-written `impl Traced`. - New C++ JSG (`jsg::Lock`, `JSG_TRY`/`JSG_CATCH`, `jsg::check`, `JsExceptionThrown`, `createTunneledException`, `makeInternalError`, `jsg::Data`/`jsg::Value`) in a `.c++` or `.h` diff --git a/src/rust/AGENTS.md b/src/rust/AGENTS.md index e8291568f07..bdf41a5a5d2 100644 --- a/src/rust/AGENTS.md +++ b/src/rust/AGENTS.md @@ -35,18 +35,18 @@ _Snapshot — the set drifts as crates come and go; `bazel query //src/rust/...` - **Namespace**: `workerd::rust::*` for crates in this directory, except `python-parser` → `edgeworker::rust::python_parser`; a crate living beside C++ uses that C++'s namespace (e.g. `workerd::server::cli`) - **Errors**: `thiserror` for library crates; `jsg::Error` with `ExceptionType` for JSG-facing crates - **JSG nullish values**: `Option` is `T | undefined`; `Nullable` is `T | null` (coerces `undefined` to `Null`, but method arguments remain required). Prefer `NullOrUndefined` to `Option>` for three distinct `Some(T)`, `Null`, and `Undefined` states; omitted arguments become `Undefined`. `Lenient` additionally preserves shallow mismatches as `Unconvertable`; converting it to `NullOrUndefined` maps those to `Undefined`, and `Lenient::to_parts()` also returns the original JS value. See [Rust JSG nullish values](jsg/README.md#nullablet-and-nullorundefinedt). -- **JSG resources**: `#[jsg_resource]` on struct + impl block; `#[jsg_method]` auto-converts `snake_case` → `camelCase`; methods with `&self` become instance methods, methods without a receiver become static methods, and any other receiver is a compile error (JavaScript can re-enter a resource, so state that methods change goes in `Cell`/`RefCell` fields; never hold a `RefCell` borrow across a call that can run JavaScript, nor a `borrow_mut()` of one holding GC references across a call that can allocate on the V8 heap, wrap the resource or create or drop a `jsg::Rc` to it, since tracing skips a mutably borrowed `RefCell`); a method or accessor can take `this: jsg::Rc` as its first parameter instead of `&self` to get a `jsg::Rc` of its resource (like `JSG_THIS`) to hand to a callback; don't share the resource's state with a callback through an `std::rc::Rc` (see `jsg/README.md` §Reaching the resource from a callback); a `#[jsg_method]` is registered with the type's `v8::Signature`, so its `&self` callback resolves the receiver without the `WORKERD_RUST_WRAPPABLE_TAG` marker check (`WrappableRc::resolve_method_receiver`); callbacks without a signature (accessors, inspect properties, wildcard interceptors) use `resolve_receiver`, which reads the marker before unwrapping so that it accepts any value; an eligible `#[jsg_method]` also gets a V8 Fast API callback (types in `jsg/fast_api.rs`) and must not call into V8; a `#[jsg_constructor]` returns `Self` or `Result` (an `Err` is thrown) and throws `TypeError` when called without `new`; `#[jsg_static_constant]` on `const` items exposes read-only numeric constants on both constructor and prototype (name kept as-is, no camelCase); `#[jsg_resource(nested_types(Foo, Name = Bar))]` on the impl block puts other resources' constructors on the prototype (`JSG_NESTED_TYPE`); resources integrate with GC via `Traced` + `GarbageCollected`: every named field is traced via `Traced::trace(&self.field, visitor)` and all non-traceable types use no-op `Traced` impls +- **JSG resources**: `#[jsg_resource]` on struct + impl block; `#[jsg_method]` auto-converts `snake_case` → `camelCase`; methods with `&self` become instance methods, methods without a receiver become static methods, and any other receiver is a compile error (JavaScript can re-enter a resource, so state that methods change goes in `Cell`/`RefCell` fields, or in `jsg::Cell`/`jsg::RefCell` fields when it holds a `jsg::Rc` or a `TracedGlobal`, whose `take`/`replace`/`borrow_mut` take the lock and make the contents strong; never hold a `RefCell` borrow across a call that can run JavaScript); a method or accessor can take `this: jsg::Rc` as its first parameter instead of `&self` to get a `jsg::Rc` of its resource (like `JSG_THIS`) to hand to a callback; don't share the resource's state with a callback through an `std::rc::Rc` (see `jsg/README.md` §Reaching the resource from a callback); a `#[jsg_method]` is registered with the type's `v8::Signature`, so its `&self` callback resolves the receiver without the `WORKERD_RUST_WRAPPABLE_TAG` marker check (`WrappableRc::resolve_method_receiver`); callbacks without a signature (accessors, inspect properties, wildcard interceptors) use `resolve_receiver`, which reads the marker before unwrapping so that it accepts any value; an eligible `#[jsg_method]` also gets a V8 Fast API callback (types in `jsg/fast_api.rs`) and must not call into V8; a `#[jsg_constructor]` returns `Self` or `Result` (an `Err` is thrown) and throws `TypeError` when called without `new`; `#[jsg_static_constant]` on `const` items exposes read-only numeric constants on both constructor and prototype (name kept as-is, no camelCase); `#[jsg_resource(nested_types(Foo, Name = Bar))]` on the impl block puts other resources' constructors on the prototype (`JSG_NESTED_TYPE`); resources integrate with GC via `Traced` + `GarbageCollected`: every named field is traced via `Traced::trace(&self.field, visitor)` and all non-traceable types use no-op `Traced` impls - **Resources created by C++**: C++ creates a Rust resource through an `extern "Rust"` function of the crate that defines it, which returns the wrapper through `jsg::v8::run_ffi_callback`; C++ holds the wrapper (a `jsg::JsRef`), since it has no typed reference to a Rust resource, and checks a value's type through one that returns `jsg::resource::ffi_is::`, a `kj::Maybe` that is none when an exception is pending. See `jsg/README.md` §Creating resources from C++. - **Custom JSG registration**: `#[jsg_resource(custom_members)]` on an impl generates member factories but leaves `impl Resource` to the author. `Resource::members(registry, flags)` uses ordinary Rust to select members, order, and property placement, and also drives RTTI under the export's flags. Factories are `_member()` (constant names lowercased); ordinary property factories are `_member(PropertyPlacement)` (`Prototype`, `Instance` or `LazyInstance`) and include the paired setter. See `jsg-macros/README.md` §Custom member registration. - **JSG properties**: three property macros on `#[jsg_resource]` impl blocks — `#[jsg_property(prototype|instance|lazy_instance [, name = "..."] [, readonly])]` (`prototype` registers an accessor on the prototype, equivalent to `JSG_PROTOTYPE_PROPERTY`; `instance` registers an own data property via V8 `SetNativeDataProperty`, equivalent to `JSG_INSTANCE_PROPERTY`; `lazy_instance` registers an own data property via V8 `SetLazyDataProperty` whose getter runs once, on first access, equivalent to `JSG_LAZY_INSTANCE_PROPERTY`: it has no setter, is writable unless `readonly` (`JSG_LAZY_READONLY_INSTANCE_PROPERTY`); declaration order sets `Object.keys()` order for automatic registration; `readonly` is a compile-time check preventing a paired setter; `name = "..."` overrides the JS name; prefer `prototype` in almost all cases), `#[jsg_inspect_property]` (registered under a unique symbol, invisible to normal enumeration and string-key lookup, surfaced by `node:util` `inspect()`, equivalent to `JSG_INSPECT_PROPERTY`), and `#[jsg_wildcard_property]` (answers string-named properties the object and its prototypes lack, at most one per type, equivalent to `JSG_WILDCARD_PROPERTY`; see `jsg/README.md`); setter auto-detected from `set_` prefix; read-only when no setter present; getter/setter `.length` and `.name` are set correctly when `spec_compliant_property_attributes` compat flag is enabled -- **`jsg::v8::Global` / `TracedGlobal`**: persistent handles. A `Global` is for handles Rust owns outside resources: GC never visits it, so it is always strong, holds its handle inline (no box), has no `Traced` impl (it cannot be a resource field), and is `Send` and `Sync`. A `TracedGlobal` is for resource fields, which GC traces to collect cycles; it is `Send` (not `Sync`). Either may be dropped on any thread. Under the isolate lock its strong handle is reset at once; without the lock it is queued and reset at the next `IsolateBase::Lock`, through `IsolateBase::applyDeferredActions()`; once the `Realm` is dropped (under the lock, before `Dispose`) the handles are left to `Dispose`. See `jsg/isolate_state.rs`. Hand-written FFI that resets a V8 handle directly must hold the lock. Rust never resets a `TracedGlobal`'s traced reference: every GC visit lets go of it and makes the strong handle strong again (`ClearWeak`) before C++ installs a new one if needed, and a drop just lets go of it; V8 frees untraced nodes at the next full GC. See `GcVisitor::visit_traced_global`. A `TracedGlobal` keeps its handles in a `Box`, since V8 holds the address of a weak handle's slot; the slot of a `TracedGlobal` dropped after the `Realm` is kept until the isolate is disposed +- **`jsg::v8::Global` / `TracedGlobal`**: persistent handles. A `Global` is for handles Rust owns outside resources: GC never visits it, so it is always strong, holds its handle inline (no box), has no `Traced` impl (it cannot be a resource field), and is `Send` and `Sync`. A `TracedGlobal` is for resource fields, which GC traces to collect cycles; it is `Send` (not `Sync`). Either may be dropped on any thread. Under the isolate lock its strong handle is reset at once; without the lock it is queued and reset at the next `IsolateBase::Lock`, through `IsolateBase::applyDeferredActions()`; once the `Realm` is dropped (under the lock, before `Dispose`) the handles are left to `Dispose`. See `jsg/isolate_state.rs`. Hand-written FFI that resets a V8 handle directly must hold the lock. Rust never resets a `TracedGlobal`'s traced reference: every GC visit, and the detaching visit on the way out of a `jsg::` cell, lets go of it and makes the strong handle strong again (`ClearWeak`) before C++ installs a new one if needed, and a drop just lets go of it; V8 frees untraced nodes at the next full GC. See `GcVisitor::visit_traced_global`. A `TracedGlobal` keeps its handles in a `Box`, since V8 holds the address of a weak handle's slot; the slot of a `TracedGlobal` dropped after the `Realm` is kept until the isolate is disposed - **JSG parameters**: function `.length` is the Web IDL required-argument count under `spec_compliant_property_attributes` (`this: jsg::Rc` and a leading `&mut Lock` are not counted; counting stops at the first type whose `FromJS::is_optional()` returns true), so put optional parameters last -- **`Traced`**: core tracing trait in `jsg::wrappable`; built-ins include no-op impls for primitives/value types and delegating impls for wrappers/collections (`Option`, `Nullable`, `NullOrUndefined`, `Lenient`, `Vec`, maps/sets, `Cell`, `jsg::Rc`, `jsg::Weak`, `jsg::v8::TracedGlobal`); `jsg::v8::Global` deliberately has none +- **`Traced`**: core tracing trait in `jsg::wrappable`, with `trace`, which GC calls and which also detaches a value from its owner (`GcVisitor::is_detaching`: every GC reference in it becomes strong and forgets its parent, so it stays valid outside a traced field); built-ins include no-op impls for primitives/value types and delegating impls for wrappers/collections (`Option`, `Nullable`, `NullOrUndefined`, `Lenient`, `Vec`, `jsg::Dict`, maps/sets, `Cell`, `RefCell`, `jsg::Cell`, `jsg::RefCell`, `jsg::Rc`, `jsg::Weak`, `jsg::v8::TracedGlobal`); `jsg::v8::Global` deliberately has none. `std::cell::Cell` and `RefCell` are `Traced` only for `T: jsg::HoldsNoGcReferences` (no `jsg::Rc` or `TracedGlobal` inside); a `jsg::Rc` or `TracedGlobal` that is moved out of a field lives in a `jsg::Cell` or `jsg::RefCell` (`jsg/cell.rs`), which detach it before it leaves the cell - **Sequences to JavaScript**: `Vec` converts to an `Array`, numbers included (`vec![1u8, 2]` makes `[1, 2]`). A typed array or an `ArrayBuffer` is an explicit conversion of a slice: `to_js_typed_array(lock)` makes the typed array of the element type, and `to_js_array_buffer(lock)` makes an `ArrayBuffer` of bytes (`[u8]` only), as C++ JSG returns for a `kj::Array`. See `jsg/README.md` §Arrays, typed arrays and `ArrayBuffer`s - **`#[jsg_resource(custom_trace)]`**: suppresses the auto-generated `Traced` impl so the user can write their own; `GarbageCollected` (`memory_name`), `jsg::Type`, `jsg::ToJS`, and `jsg::FromJS` are still generated - **Rust JSG safety rules** (full text and examples in `src/rust/jsg/README.md` §Safety rules): 1. Never call a V8 API that takes a `v8::Local` while an exception or a termination is pending: V8 either clears the exception on entry, so the script runs on, or fails a debug check and in a release build runs on with it pending. Every C++ shim that passes a context to V8 first checks `exceptionPending()` (`jsg/ffi.h`) and fails with a pending error. Never throw while execution is terminating, and never discard a `jsg::Error` from an operation that can fail with a pending error (a call, `Object::get`/`set`, a coercion, `ToJS::to_js`); pass it on with `?`. Creating strings, numbers, objects, arrays, error objects and templates is fine at any time. - 2. Never put traced state (`TracedGlobal`, `jsg::Rc`, or a type holding them) behind `std::rc::Rc`. To give a callback or another party access, give it a `jsg::Rc` or `jsg::Weak` to the resource that owns the state (a method gets one as `this: jsg::Rc`). Clone a `jsg::Rc` right after moving it out of a traced field (`take`, `replace`, `pop`), since the moved one is neither strong nor traced ([#7976](https://github.com/cloudflare/workerd/issues/7976)). Don't put a `Global` in anything GC traces. A typed promise's reaction gets its value detached from the promise's wrapper, as a moved C++ `jsg::Ref` is; a `Traced` impl that visits through `GcVisitor::as_ffi` must handle the `None` it returns for such a visit (`GcVisitor::is_detaching()`). + 2. Never put traced state (`TracedGlobal`, `jsg::Rc`, or a type holding them) behind `std::rc::Rc`. To give a callback or another party access, give it a `jsg::Rc` or `jsg::Weak` to the resource that owns the state (a method gets one as `this: jsg::Rc`). Keep a `jsg::Rc` or `TracedGlobal` that is moved out of a field in a `jsg::Cell` or `jsg::RefCell`, which make it strong on the way out; a plain move out of a std cell would leave it neither strong nor traced, so std cells holding one do not compile ([#7976](https://github.com/cloudflare/workerd/issues/7976)). Don't put a `Global` in anything GC traces. A typed promise's reaction gets its value detached from the promise's wrapper, as a moved C++ `jsg::Ref` is, and so does a value leaving a `jsg::` cell; a `Traced` impl that visits through `GcVisitor::as_ffi` must handle the `None` it returns for such a visit (`GcVisitor::is_detaching()`). 3. The C++ shims call V8 directly, not C++ JSG (`JSG_TRY`, `jsg::check`, `jsg::Lock::from`, `createTunneledException`, `makeInternalError`, `jsg::Data`, ...). The one reviewed exception is `io_context_await_io_and_run()` in `src/workerd/io/io-context/ffi.c++`, which throws `jsg::JsExceptionThrown` to pass a pending exception to `IoContext::run()`; don't extend it. `//src/rust/jsg:cpp-jsg-allowlist-test` enforces this for the files in `CPP_SHIMS` in `jsg/BUILD.bazel`; add shim files outside `src/rust/jsg` there. 4. `Local<'s>` and `Lock<'s>` stay in their scope, and futures passed to async I/O are `'static`. - **Formatting**: `rustfmt.toml` — `group_imports = "StdExternalCrate"`, `imports_granularity = "Item"` (one `use` per import) diff --git a/src/rust/jsg-bench/lib.rs b/src/rust/jsg-bench/lib.rs index c021da9caa3..77fdb9a05d1 100644 --- a/src/rust/jsg-bench/lib.rs +++ b/src/rust/jsg-bench/lib.rs @@ -67,7 +67,7 @@ enum NumberOrString { #[jsg_resource] struct Node { value: Cell, - child: Cell>>, + child: jsg::Cell>>, } #[jsg_resource] @@ -76,7 +76,7 @@ impl Node { fn constructor(value: u32) -> Self { Self { value: Cell::new(value), - child: Cell::new(None), + child: jsg::Cell::new(None), } } @@ -91,8 +91,8 @@ impl Node { } #[jsg_property(prototype)] - fn get_child(&self) -> Option> { - let child = self.child.take(); + fn get_child(&self, lock: &mut jsg::Lock<'_>) -> Option> { + let child = self.child.take(lock); self.child.set(child.clone()); child } diff --git a/src/rust/jsg-compile-tests/gc-reference-in-std-cell-is-not-traced.rs b/src/rust/jsg-compile-tests/gc-reference-in-std-cell-is-not-traced.rs new file mode 100644 index 00000000000..c55751e95ce --- /dev/null +++ b/src/rust/jsg-compile-tests/gc-reference-in-std-cell-is-not-traced.rs @@ -0,0 +1,70 @@ +// Copyright (c) 2026 Cloudflare, Inc. +// Licensed under the Apache 2.0 license found in the LICENSE file or at: +// https://opensource.org/licenses/Apache-2.0 + +// A `jsg::Rc` or a `TracedGlobal` moved out of a `std::cell::Cell` or `std::cell::RefCell` +// after GC visited it there is neither strong nor traced. So those cells are `Traced` only for +// types that hold no GC references (`jsg::HoldsNoGcReferences`); a reference that a resource +// takes out of a field lives in a `jsg::Cell` or `jsg::RefCell`, which make it strong on the +// way out. +use std::cell::Cell; +use std::cell::RefCell; + +use jsg::v8; +use jsg_macros::jsg_resource; + +#[jsg_resource] +struct Leaf; + +#[jsg_resource] +impl Leaf {} + +#[jsg_resource] //~ ERROR E0277 +struct RcInCell { + pending: Cell>>, +} + +#[jsg_resource] +impl RcInCell {} + +#[jsg_resource] //~ ERROR E0277 +struct RcInRefCell { + queue: RefCell>>, +} + +#[jsg_resource] +impl RcInRefCell {} + +#[jsg_resource] //~ ERROR E0277 +struct GlobalInCell { + callback: Cell>>, +} + +#[jsg_resource] +impl GlobalInCell {} + +#[jsg_resource] //~ ERROR E0277 +struct GlobalInRefCell { + handles: RefCell>>, +} + +#[jsg_resource] +impl GlobalInRefCell {} + +// A `Lenient` holds a `TracedGlobal` for a value it could not convert. +#[jsg_resource] //~ ERROR E0277 +struct LenientInCell { + value: Cell>, +} + +#[jsg_resource] +impl LenientInCell {} + +// A `jsg::Cell` holding a `jsg::Rc` would be moved out whole, with its weak contents. +#[jsg_resource] //~ ERROR E0277 +struct JsgCellInCell { + nested: Cell>>>, +} + +#[jsg_resource] +impl JsgCellInCell {} diff --git a/src/rust/jsg-compile-tests/gc-reference-in-std-rc-is-not-traced.rs b/src/rust/jsg-compile-tests/gc-reference-in-std-rc-is-not-traced.rs new file mode 100644 index 00000000000..a1b070da7fa --- /dev/null +++ b/src/rust/jsg-compile-tests/gc-reference-in-std-rc-is-not-traced.rs @@ -0,0 +1,32 @@ +// Copyright (c) 2026 Cloudflare, Inc. +// Licensed under the Apache 2.0 license found in the LICENSE file or at: +// https://opensource.org/licenses/Apache-2.0 + +// Every owner of a shared `std::rc::Rc` would trace what it holds, but a GC reference has a +// single parent. So `std::rc::Rc` is `Traced` only for types that hold no GC references +// (`jsg::HoldsNoGcReferences`); state shared with another owner lives in a resource, reached +// through a `jsg::Rc` or `jsg::Weak`. +use jsg::v8; +use jsg_macros::jsg_resource; + +#[jsg_resource] +struct Leaf; + +#[jsg_resource] +impl Leaf {} + +#[jsg_resource] //~ ERROR E0277 +struct SharedRc { + leaf: std::rc::Rc>, +} + +#[jsg_resource] +impl SharedRc {} + +#[jsg_resource] //~ ERROR E0277 +struct SharedGlobal { + callback: std::rc::Rc>, +} + +#[jsg_resource] +impl SharedGlobal {} diff --git a/src/rust/jsg-compile-tests/ok-async-context-frame.rs b/src/rust/jsg-compile-tests/ok-async-context-frame.rs index b5774d64cb7..b15fe311756 100644 --- a/src/rust/jsg-compile-tests/ok-async-context-frame.rs +++ b/src/rust/jsg-compile-tests/ok-async-context-frame.rs @@ -5,7 +5,6 @@ // An `AsyncContextFrame` is `'static`, `Send` and `Sync`, so a future passed to async I/O can hold // it, and a `TracedAsyncContextFrame` is `Send` and can be a resource field. `enter` opens no // scope, so a handle created in the frame stays valid after it. -use std::cell::Cell; use jsg::v8; use jsg_macros::jsg_resource; @@ -15,7 +14,7 @@ fn assert_static_send() {} #[jsg_resource] struct Holder { - frame: Cell>, + frame: jsg::Cell>, } #[jsg_resource] @@ -27,7 +26,7 @@ pub fn t<'s>(lock: &mut jsg::Lock<'s>) -> v8::Local<'s, v8::Object> { let frame = jsg::AsyncContextFrame::current(lock); let object = frame.enter(lock, |lock| lock.new_object()); let holder = Holder { - frame: Cell::new(Some(frame.into())), + frame: jsg::Cell::new(Some(frame.into())), }; drop(holder); object diff --git a/src/rust/jsg-compile-tests/ok-gc-references-in-jsg-cells.rs b/src/rust/jsg-compile-tests/ok-gc-references-in-jsg-cells.rs new file mode 100644 index 00000000000..d9eea394aaf --- /dev/null +++ b/src/rust/jsg-compile-tests/ok-gc-references-in-jsg-cells.rs @@ -0,0 +1,43 @@ +// Copyright (c) 2026 Cloudflare, Inc. +// Licensed under the Apache 2.0 license found in the LICENSE file or at: +// https://opensource.org/licenses/Apache-2.0 + +// Traced state that a resource moves out of a field lives in a `jsg::Cell` or `jsg::RefCell`, +// whose `take`, `replace` and `borrow_mut` take the lock and make a reference strong before it +// leaves the cell. +// A `std::cell::Cell` still holds anything with no GC references in it, such as a `jsg::Weak`. +use std::cell::Cell; + +use jsg::v8; +use jsg_macros::jsg_method; +use jsg_macros::jsg_resource; + +#[jsg_resource] +struct Leaf; + +#[jsg_resource] +impl Leaf {} + +#[jsg_resource] +struct Holder { + pending: jsg::Cell>>, + queue: jsg::RefCell>>, + callback: jsg::Cell>>, + handles: jsg::RefCell>>, + weak: Cell>>, + runs: Cell, +} + +#[jsg_resource] +impl Holder { + #[jsg_method] + fn run(&self, lock: &mut jsg::Lock<'_>) { + let _pending: Option> = self.pending.take(lock); + let _next: Option> = self.queue.borrow_mut(lock).pop(); + let _count = self.queue.borrow().len(); + let _callback = self.callback.replace(lock, None); + let _handle = self.handles.borrow_mut(lock).pop(); + let _weak = self.weak.take(); + self.runs.set(self.runs.get() + 1); + } +} diff --git a/src/rust/jsg-compile-tests/ok-promise.rs b/src/rust/jsg-compile-tests/ok-promise.rs index 3889a0a4ea2..99e45932474 100644 --- a/src/rust/jsg-compile-tests/ok-promise.rs +++ b/src/rust/jsg-compile-tests/ok-promise.rs @@ -4,15 +4,13 @@ // A method returns a promise, keeps its resolver past the call, and reacts to another promise // in each of the three ways, without `unsafe`. -use std::cell::RefCell; - use jsg::v8; use jsg_macros::jsg_method; use jsg_macros::jsg_resource; #[jsg_resource] pub struct Waiter { - resolver: RefCell>>, + resolver: jsg::RefCell>>, } #[jsg_resource] @@ -24,7 +22,7 @@ impl Waiter { input: v8::Local<'s, v8::Promise>, ) -> Result, jsg::Error> { let resolver = v8::PromiseResolver::new(lock)?; - *self.resolver.borrow_mut() = Some(resolver.to_traced_global(lock)); + *self.resolver.borrow_mut(lock) = Some(resolver.to_traced_global(lock)); let wrapped = input.then(lock, String::from("data"), |lock, data, value| { let object = lock.new_object(); object.set(lock, data, value)?; diff --git a/src/rust/jsg-compile-tests/ok-typed-promise.rs b/src/rust/jsg-compile-tests/ok-typed-promise.rs index 4a3ed7a637d..f1b2469dfa8 100644 --- a/src/rust/jsg-compile-tests/ok-typed-promise.rs +++ b/src/rust/jsg-compile-tests/ok-typed-promise.rs @@ -5,8 +5,6 @@ // Methods take and return typed promises, also in containers, carry Rust values through // reactions, chain promises, keep a promise and its resolver in fields, and report errors by // rejecting, without `unsafe`. -use std::cell::RefCell; - use jsg::v8; use jsg_macros::jsg_method; use jsg_macros::jsg_resource; @@ -19,8 +17,8 @@ impl jsg::Traced for Carried {} #[jsg_resource] pub struct Waiter { - promise: RefCell>>, - resolver: RefCell>>, + promise: jsg::RefCell>>, + resolver: jsg::RefCell>>, } #[jsg_resource] @@ -28,14 +26,15 @@ impl Waiter { #[jsg_method] fn start(&self, lock: &mut jsg::Lock<'_>) -> Result<(), jsg::Error> { let (promise, resolver) = jsg::Promise::::new_with_resolver(lock)?; - *self.promise.borrow_mut() = Some(promise.into_traced(lock)); - *self.resolver.borrow_mut() = Some(resolver.into_traced(lock)); + *self.promise.borrow_mut(lock) = Some(promise.into_traced(lock)); + *self.resolver.borrow_mut(lock) = Some(resolver.into_traced(lock)); Ok(()) } #[jsg_method] fn finish(&self, lock: &mut jsg::Lock<'_>, count: u32) -> Result<(), jsg::Error> { - if let Some(resolver) = self.resolver.borrow_mut().take() { + let resolver = self.resolver.borrow_mut(lock).take(); + if let Some(resolver) = resolver { resolver.as_local(lock).resolve( lock, Carried { @@ -48,7 +47,7 @@ impl Waiter { #[jsg_method] fn count<'s>(&self, lock: &mut jsg::Lock<'s>) -> Result, jsg::Error> { - let promise = self.promise.borrow_mut().take(); + let promise = self.promise.borrow_mut(lock).take(); lock.eval_now(|lock| { let promise = promise .ok_or_else(|| jsg::Error::new_type_error("not started"))? diff --git a/src/rust/jsg-cpp/ffi.rs b/src/rust/jsg-cpp/ffi.rs index 6727897f367..3b56beaaa82 100644 --- a/src/rust/jsg-cpp/ffi.rs +++ b/src/rust/jsg-cpp/ffi.rs @@ -159,8 +159,8 @@ pub unsafe fn call_cpp_for_ref( /// parameter does, and runs no JavaScript to check it. /// - To JavaScript, it is the object's existing wrapper, or a new one for its dynamic type. /// - A resource field holding one is traced like a C++ `jsg::Ref` field. As for a `jsg::Rc`, a -/// `Ref` moved out of a traced field must be cloned before use; see `src/rust/jsg/README.md` -/// §Safety rules, rule 2. +/// `Ref` moved out of a field lives in a `jsg::Cell` or `jsg::RefCell`, which make it strong +/// on the way out; see `src/rust/jsg/README.md` §Safety rules, rule 2. /// /// It is neither `Send` nor `Sync`: cloning it changes a refcount that the isolate lock guards. /// It may be dropped on any thread, as a `jsg::Ref` may. diff --git a/src/rust/jsg-macros/README.md b/src/rust/jsg-macros/README.md index 2ac3a4c71d9..55a7f8363fe 100644 --- a/src/rust/jsg-macros/README.md +++ b/src/rust/jsg-macros/README.md @@ -57,12 +57,10 @@ Generates a V8 `FunctionCallback` for a method on a `#[jsg_resource]` type. - **Instance methods** (`&self`) are placed on the prototype. JavaScript can call back into a resource while one of its methods runs, so methods never get `&mut self`; keep state - they change in `Cell` or `RefCell` fields. Prefer `Cell`: a `RefCell` borrow held across a - call that can run JavaScript makes a re-entrant call panic, and tracing skips a mutably borrowed - `RefCell`, so while a `borrow_mut()` is held across a call that can allocate on the V8 heap, - wrap the resource or create or drop a `jsg::Rc` to it, the values of the `TracedGlobal`s in it - and the JS wrappers of the resources its `jsg::Rc`s point to can be collected (debug builds - abort instead). + they change in `Cell` or `RefCell` fields; state that holds a `jsg::Rc` or a `TracedGlobal` + goes in a `jsg::Cell` or `jsg::RefCell`, whose `take`, `replace` and `borrow_mut` take the lock + and make the contents strong (see "Cells" in `src/rust/jsg/README.md`). Prefer `Cell`: a + `RefCell` borrow held across a call that can run JavaScript makes a re-entrant call panic. - **Static methods** (no receiver) are placed on the constructor. - **`this: jsg::Rc`** as the first parameter, instead of `&self`, also makes an instance method. `this` is a `jsg::Rc` of the resource the method was called on, like `JSG_THIS` in @@ -109,7 +107,7 @@ impl DnsUtil { // Instance — obj.watch(…); `this` is a jsg::Rc of `obj`'s resource #[jsg_method] pub fn watch(this: jsg::Rc, lock: &mut Lock, watcher: jsg::Rc) { - watcher.targets.borrow_mut().push(this); + watcher.targets.borrow_mut(lock).push(this); // targets: jsg::RefCell>> } // Static — DnsUtil.create(…) @@ -505,14 +503,15 @@ use no-op `Traced` impls; containers/wrappers recurse to inner values. | `Option` / `jsg::Nullable` / `jsg::NullOrUndefined` | Delegates to `T` when `Some` | | `jsg::Lenient` | Traces `T` when `Some`, or the original JS value when `Unconvertable` | | `Vec`, `jsg::Dict`, `HashMap`, `BTreeMap`, `HashSet`, `BTreeSet` | Delegates recursively to contained values | -| `Cell` / `std::cell::Cell` | Delegates via `as_ptr()` read (safe under single-threaded, non-reentrant GC tracing) | -| `RefCell` | Delegates via `try_borrow()`; skipped while mutably borrowed (debug builds abort if `T` can hold GC references) | +| `std::cell::Cell` / `std::cell::RefCell` | Only for `T: jsg::HoldsNoGcReferences`: a `jsg::Rc` or a `TracedGlobal` inside is a compile error | +| `jsg::Cell` / `jsg::RefCell` | Delegates to `T`, for any `T: Traced` (a `Cell` through an `as_ptr()` read, safe because visits change nothing other than through the visitor and `replace` moves the value out before detaching it; a `RefCell` through `try_borrow()`, skipping a mutably borrowed cell, whose contents are strong); `take`, `replace` and `borrow_mut` make strong what leaves the cell (see "Cells" in `src/rust/jsg/README.md`) | | Plain data / primitives | No-op `Traced` | | `#[jsg_struct]` types | Delegates to each field, like a resource | | Any other `T: Traced` | Uses `T`'s implementation | -The `Cell<…>` variants are required whenever a traced field needs to be mutated -after construction, because `Traced::trace` receives `&self`. +A cell is required whenever a traced field needs to be mutated after construction, +because `Traced::trace` receives `&self`. A field that holds a `jsg::Rc` or a +`TracedGlobal` uses the `jsg::` cells. ### Manual tracing with `custom_trace` @@ -550,7 +549,6 @@ pub struct MySocket { ``` ```rust -use std::cell::Cell; use std::collections::HashMap; #[jsg_resource] @@ -563,7 +561,7 @@ pub struct EventRouter { // Interior-mutable callback set after construction; dual-mode TracedGlobal enables // cycle collection if the callback closes over this resource's own JS wrapper. - on_error: Cell>>, + on_error: jsg::Cell>>, // Weak — does not keep target alive. parent: jsg::Weak, diff --git a/src/rust/jsg-test/cpp_interop.rs b/src/rust/jsg-test/cpp_interop.rs index f427a327f3d..202432d3cec 100644 --- a/src/rust/jsg-test/cpp_interop.rs +++ b/src/rust/jsg-test/cpp_interop.rs @@ -11,7 +11,6 @@ //! lazy property that depends on the current request, and is held by a C++ resource. use std::cell::Cell; -use std::cell::RefCell; use jsg::ToJS; use jsg::v8; @@ -247,7 +246,7 @@ thread_local! { pub struct Host { pub label: String, /// A C++ resource, traced as a `jsg::Ref` field of a C++ resource is. - pub held: RefCell>>, + pub held: jsg::RefCell>>, } impl Drop for Host { @@ -308,8 +307,8 @@ impl Host { /// Keeps a C++ resource in a traced field. #[jsg_method] - pub fn hold(&self, holder: jsg_cpp::Ref) { - *self.held.borrow_mut() = Some(holder); + pub fn hold(&self, lock: &mut jsg::Lock<'_>, holder: jsg_cpp::Ref) { + *self.held.borrow_mut(lock) = Some(holder); } /// The held C++ resource, the same JavaScript object `hold()` was given. @@ -327,7 +326,7 @@ unsafe fn host_new(isolate: *mut ffi::Isolate, label: &str) -> ffi::MaybeLocal { v8::run_ffi_callback(isolate, |lock| { jsg::Rc::new(Host { label: label.to_owned(), - held: RefCell::new(None), + held: jsg::RefCell::new(None), }) .to_js(lock) }) diff --git a/src/rust/jsg-test/tests/async_context.rs b/src/rust/jsg-test/tests/async_context.rs index d51b243a5ac..8bc29539857 100644 --- a/src/rust/jsg-test/tests/async_context.rs +++ b/src/rust/jsg-test/tests/async_context.rs @@ -39,7 +39,7 @@ fn frame_storing(lock: &mut jsg::Lock, value: &str) -> AsyncContextFrame { /// Lets JavaScript read the current frame, and holds a frame for later calls. #[jsg_resource] struct Probe { - frame: Cell>, + frame: jsg::Cell>, /// Whether the frame current before `run_in_frame` entered its frame was current again /// after it. restored: Cell, @@ -51,7 +51,7 @@ impl Probe { /// A strong handle to the captured frame. The traced frame goes back into its field before /// anything can run JavaScript, which the caller may do with the handle it gets. fn frame(&self, lock: &mut jsg::Lock) -> AsyncContextFrame { - let traced = self.frame.take().unwrap(); + let traced = self.frame.take(lock).unwrap(); let frame = traced.to_async_context_frame(lock); self.frame.set(Some(traced)); frame @@ -109,7 +109,7 @@ impl Probe { fn install_probe(lock: &mut jsg::Lock, ctx: &crate::EvalContext<'_>) -> jsg::Rc { let probe = jsg::Rc::new(Probe { - frame: Cell::new(None), + frame: jsg::Cell::new(None), restored: Cell::new(false), saw_pending: Cell::new(false), }); @@ -345,7 +345,7 @@ static STORED_DROPS: AtomicUsize = AtomicUsize::new(0); #[jsg_resource] struct Stored { /// The probe whose frame stores this value, which closes a cycle through the frame. - owner: Cell>>, + owner: jsg::Cell>>, } impl Drop for Stored { @@ -370,7 +370,7 @@ fn frame_in_resource_is_traced() { let mut kept = None; harness.run_in_context(|lock, _ctx| { let probe = jsg::Rc::new(Probe { - frame: Cell::new(None), + frame: jsg::Cell::new(None), restored: Cell::new(false), saw_pending: Cell::new(false), }); @@ -378,7 +378,7 @@ fn frame_in_resource_is_traced() { // GC traces its frame through the wrapper. let wrapper = probe.clone().to_js(lock)?; let stored = jsg::Rc::new(Stored { - owner: Cell::new(Some(probe.clone())), + owner: jsg::Cell::new(Some(probe.clone())), }) .to_js(lock)?; probe.capture_in(lock, stored); diff --git a/src/rust/jsg-test/tests/cell.rs b/src/rust/jsg-test/tests/cell.rs new file mode 100644 index 00000000000..b5e61c1a40a --- /dev/null +++ b/src/rust/jsg-test/tests/cell.rs @@ -0,0 +1,304 @@ +// Copyright (c) 2026 Cloudflare, Inc. +// Licensed under the Apache 2.0 license found in the LICENSE file or at: +// https://opensource.org/licenses/Apache-2.0 + +//! Tests for `jsg::Cell` and `jsg::RefCell`, which make a reference strong before it leaves +//! the cell. +//! +//! Once GC has visited a `jsg::Rc` or a `TracedGlobal` in a field of a resource that has a +//! wrapper, the reference is weak and depends on the resource's trace. Moved out of the field +//! by a bitwise move, it would be neither strong nor traced, and the next GC would collect the +//! wrapper of the `jsg::Rc`'s resource, or the value of the `TracedGlobal`. These tests take +//! references out through the cells after such a visit, run a GC, and check that nothing was +//! lost. + +use std::sync::atomic::AtomicUsize; +use std::sync::atomic::Ordering; + +use jsg::FromJS; +use jsg::ToJS; +use jsg::v8; +use jsg_macros::jsg_constructor; +use jsg_macros::jsg_method; +use jsg_macros::jsg_resource; + +static TASK_DROPS: AtomicUsize = AtomicUsize::new(0); + +/// A resource that JavaScript creates and hands to a `Scheduler`, which then holds the only +/// reference to it. +#[jsg_resource] +struct Task { + /// A value that only this task keeps alive, once GC has put the handle in traced mode. + payload: jsg::Cell>>, +} + +impl Drop for Task { + fn drop(&mut self) { + TASK_DROPS.fetch_add(1, Ordering::SeqCst); + } +} + +#[jsg_resource] +impl Task { + #[jsg_constructor] + fn constructor() -> Self { + Self { + payload: jsg::Cell::new(None), + } + } + + #[jsg_method] + fn set_payload<'s>(&self, lock: &mut jsg::Lock<'s>, value: v8::Local<'s, v8::Value>) { + self.payload.set(Some(value.to_traced_global(lock))); + } +} + +/// A resource that holds `jsg::Rc`s of tasks in both kinds of cell. +#[jsg_resource] +struct Scheduler { + pending: jsg::Cell>>, + queue: jsg::RefCell>>, +} + +#[jsg_resource] +impl Scheduler { + #[jsg_constructor] + fn constructor() -> Self { + Self { + pending: jsg::Cell::new(None), + queue: jsg::RefCell::new(Vec::new()), + } + } + + #[jsg_method] + fn schedule(&self, task: jsg::Rc) { + self.pending.set(Some(task)); + } + + #[jsg_method] + fn enqueue(&self, lock: &mut jsg::Lock<'_>, task: jsg::Rc) { + self.queue.borrow_mut(lock).push(task); + } +} + +/// Creates the global `scheduler` with one scheduled and one queued task, which JavaScript +/// creates, marks with an own property of their wrappers and gives a payload, and then runs a GC. +/// The scheduler's wrapper is reachable, so the visit makes the `jsg::Rc`s in its fields weak, +/// and the tasks' `TracedGlobal`s traced. The scripts end in `undefined` so that no `Local` of a +/// task outlives them. +fn setup(lock: &mut jsg::Lock<'_>, ctx: &crate::EvalContext<'_>) -> jsg::Rc { + let task = jsg::resource::function_template_of::(lock).unwrap(); + ctx.set_global(lock, "Task", task); + let scheduler = jsg::resource::function_template_of::(lock).unwrap(); + ctx.set_global(lock, "Scheduler", scheduler); + ctx.eval_raw( + lock, + r" + globalThis.scheduler = new Scheduler(); + for (const [name, add] of [['scheduled', 'schedule'], ['queued', 'enqueue']]) { + const task = new Task(); + task.mark = name; + task.setPayload({ of: name }); + scheduler[add](task); + } + undefined + ", + ) + .expect("should evaluate"); + let scheduler: jsg::Rc = ctx.eval(lock, "scheduler").expect("should evaluate"); + crate::Harness::request_gc(lock); + scheduler +} + +/// The own property `mark` of the wrapper of `task`, which is `undefined` if the wrapper is a +/// new one. +fn mark<'s>(lock: &mut jsg::Lock<'s>, task: &jsg::Rc) -> v8::Local<'s, v8::Value> { + let wrapper = v8::Local::::try_from(task.clone().to_js(lock).unwrap()).unwrap(); + wrapper.get(lock, "mark").unwrap() +} + +fn assert_marked(lock: &mut jsg::Lock<'_>, task: &jsg::Rc, expected: &str) { + let mark = mark(lock, task); + assert!(mark.is_string(), "the wrapper was replaced: {mark:?}"); + assert_eq!(String::from_js(lock, mark).unwrap(), expected); +} + +/// A `jsg::Rc` taken out of a `jsg::Cell` after a visit made it weak is strong, so a GC leaves +/// its resource's wrapper alone. +#[test] +fn rc_taken_from_a_cell_keeps_its_wrapper() { + let harness = crate::Harness::new(); + harness.run_in_context(|lock, ctx| { + let scheduler = setup(lock, ctx); + let task = scheduler.pending.take(lock).expect("scheduled above"); + crate::Harness::request_gc(lock); + assert_marked(lock, &task, "scheduled"); + Ok(()) + }); +} + +/// `replace` makes the old value strong as `take` does. +#[test] +fn rc_replaced_in_a_cell_keeps_its_wrapper() { + let harness = crate::Harness::new(); + harness.run_in_context(|lock, ctx| { + let scheduler = setup(lock, ctx); + let task = scheduler + .pending + .replace(lock, None) + .expect("scheduled above"); + crate::Harness::request_gc(lock); + assert_marked(lock, &task, "scheduled"); + Ok(()) + }); +} + +/// `jsg::RefCell::borrow_mut` makes everything in the cell strong, so what the borrower pops out +/// is strong. +#[test] +fn rc_popped_from_a_ref_cell_keeps_its_wrapper() { + let harness = crate::Harness::new(); + harness.run_in_context(|lock, ctx| { + let scheduler = setup(lock, ctx); + let task = scheduler + .queue + .borrow_mut(lock) + .pop() + .expect("queued above"); + crate::Harness::request_gc(lock); + assert_marked(lock, &task, "queued"); + Ok(()) + }); +} + +/// `jsg::RefCell::try_borrow_mut` makes everything in the cell strong too. +#[test] +fn rc_popped_from_a_ref_cell_through_try_borrow_mut_keeps_its_wrapper() { + let harness = crate::Harness::new(); + harness.run_in_context(|lock, ctx| { + let scheduler = setup(lock, ctx); + let task = scheduler + .queue + .try_borrow_mut(lock) + .expect("not borrowed") + .pop() + .expect("queued above"); + crate::Harness::request_gc(lock); + assert_marked(lock, &task, "queued"); + Ok(()) + }); +} + +/// With its wrapper kept, the resource's own fields stay traced: its `TracedGlobal` keeps its +/// value. +#[test] +fn rc_taken_from_a_cell_keeps_the_values_its_resource_traces() { + let harness = crate::Harness::new(); + harness.run_in_context(|lock, ctx| { + let scheduler = setup(lock, ctx); + let task = scheduler.pending.take(lock).expect("scheduled above"); + crate::Harness::request_gc(lock); + let payload = task.payload.take(lock).expect("set above"); + let payload = v8::Local::::try_from(payload.as_local(lock)).unwrap(); + let of = payload.get(lock, "of").unwrap(); + assert_eq!(String::from_js(lock, of).unwrap(), "scheduled"); + Ok(()) + }); +} + +/// A `TracedGlobal` taken out of a `jsg::Cell` after a visit put it in traced mode is strong +/// again, so a GC leaves its value alone even though nothing traces the handle. +#[test] +fn traced_global_taken_from_a_cell_keeps_its_value() { + let harness = crate::Harness::new(); + harness.run_in_context(|lock, ctx| { + let scheduler = setup(lock, ctx); + // Taken from the task while the scheduler still holds and traces the task. + let payload = scheduler.queue.borrow()[0] + .payload + .take(lock) + .expect("set above"); + crate::Harness::request_gc(lock); + let payload = v8::Local::::try_from(payload.as_local(lock)).unwrap(); + let of = payload.get(lock, "of").unwrap(); + assert_eq!(String::from_js(lock, of).unwrap(), "queued"); + Ok(()) + }); +} + +/// A `jsg::Rc` taken out of one resource forgets which resource traced it, so another resource +/// can hold and trace it. (The C++ `Wrappable` asserts that a reference keeps one parent, and +/// aborts the process otherwise.) +#[test] +fn rc_taken_from_one_resource_is_traced_by_another() { + let harness = crate::Harness::new(); + harness.run_in_context(|lock, ctx| { + let scheduler = setup(lock, ctx); + let other: jsg::Rc = ctx + .eval(lock, "globalThis.other = new Scheduler()") + .expect("should evaluate"); + let task = scheduler.pending.take(lock).expect("scheduled above"); + other.pending.set(Some(task)); + crate::Harness::request_gc(lock); + // The second visit finds the reference with its new parent recorded. + crate::Harness::request_gc(lock); + let task = other.pending.take(lock).expect("set above"); + assert_marked(lock, &task, "scheduled"); + Ok(()) + }); +} + +/// A `jsg::Rc` put back after it was taken out goes back to depending on the owner's trace, so +/// the owner and the task are collected once JavaScript lets go of the owner. +#[test] +fn rc_put_back_into_a_cell_is_collected_with_its_owner() { + TASK_DROPS.store(0, Ordering::SeqCst); + + let harness = crate::Harness::new(); + harness.run_in_context(|lock, ctx| { + let scheduler = setup(lock, ctx); + let task = scheduler.pending.take(lock).expect("scheduled above"); + scheduler.pending.set(Some(task)); + let task = scheduler + .queue + .borrow_mut(lock) + .pop() + .expect("queued above"); + scheduler.queue.borrow_mut(lock).push(task); + crate::Harness::request_gc(lock); + assert_eq!(TASK_DROPS.load(Ordering::SeqCst), 0); + + ctx.eval_raw(lock, "globalThis.scheduler = undefined") + .expect("should evaluate"); + drop(scheduler); + Ok(()) + }); + // GC in a separate context so no `Local` of the scheduler's wrapper is left. + harness.run_in_context(|lock, _ctx| { + crate::Harness::request_gc(lock); + assert_eq!(TASK_DROPS.load(Ordering::SeqCst), 2); + Ok(()) + }); +} + +/// Everything in a mutably borrowed `jsg::RefCell` is strong, so a GC that runs during the +/// borrow, which cannot visit the cell, loses nothing. +#[test] +fn gc_during_a_mutable_borrow_loses_nothing() { + let harness = crate::Harness::new(); + harness.run_in_context(|lock, ctx| { + let scheduler = setup(lock, ctx); + { + let mut queue = scheduler.queue.borrow_mut(lock); + crate::Harness::request_gc(lock); + let task = queue.pop().expect("queued above"); + queue.push(task); + } + crate::Harness::request_gc(lock); + let task = scheduler.queue.borrow()[0].clone(); + assert_marked(lock, &task, "queued"); + let payload = task.payload.take(lock).expect("set above"); + assert!(payload.as_local(lock).is_object()); + Ok(()) + }); +} diff --git a/src/rust/jsg-test/tests/collections_gc.rs b/src/rust/jsg-test/tests/collections_gc.rs index d62405e9b10..37d196502c8 100644 --- a/src/rust/jsg-test/tests/collections_gc.rs +++ b/src/rust/jsg-test/tests/collections_gc.rs @@ -21,8 +21,6 @@ //! makes the references traced. //! 4. Move to a fresh context (global is gone) → GC must collect parent and all children. -use std::cell::Cell; -use std::cell::RefCell; use std::collections::BTreeMap; use std::collections::BTreeSet; use std::collections::HashMap; @@ -44,13 +42,13 @@ static LEAF_DROPS: AtomicUsize = AtomicUsize::new(0); struct Leaf { pub value: u32, /// The wrapper of the leaf's parent, which makes a cycle through the parent's field. - owner: RefCell>>, + owner: jsg::RefCell>>, } fn leaf(value: u32) -> jsg::Rc { jsg::Rc::new(Leaf { value, - owner: RefCell::new(None), + owner: jsg::RefCell::new(None), }) } @@ -62,7 +60,7 @@ fn keep_owner<'a>( ) { for leaf in leaves { let owner = jsg::v8::TracedGlobal::new(lock, owner); - *leaf.owner.borrow_mut() = Some(owner); + *leaf.owner.borrow_mut(lock) = Some(owner); } } @@ -573,14 +571,14 @@ fn dict_rc_values_kept_alive_through_gc() { } // ============================================================================= -// Cell>> +// jsg::Cell>> // ============================================================================= static CELL_VEC_PARENT_DROPS: AtomicUsize = AtomicUsize::new(0); #[jsg_resource] struct CellVecParent { - pub children: Cell>>, + pub children: jsg::Cell>>, } impl Drop for CellVecParent { @@ -604,7 +602,7 @@ fn cell_vec_rc_children_kept_alive_through_gc() { let c2 = leaf(22); let parent = jsg::Rc::new(CellVecParent { - children: Cell::new(vec![c1.clone(), c2.clone()]), + children: jsg::Cell::new(vec![c1.clone(), c2.clone()]), }); let wrapped = parent.clone().to_js(lock)?; keep_owner(lock, wrapped, [&c1, &c2]); @@ -636,7 +634,7 @@ fn cell_vec_rc_empty_does_not_crash_during_gc() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { let parent = jsg::Rc::new(CellVecParent { - children: Cell::new(vec![]), + children: jsg::Cell::new(vec![]), }); let wrapped = parent.clone().to_js(lock)?; ctx.set_global(lock, "parent", wrapped); @@ -655,14 +653,14 @@ fn cell_vec_rc_empty_does_not_crash_during_gc() { } // ============================================================================= -// Cell>> +// jsg::Cell>> // ============================================================================= static CELL_MAP_PARENT_DROPS: AtomicUsize = AtomicUsize::new(0); #[jsg_resource] struct CellHashMapParent { - pub children: Cell>>, + pub children: jsg::Cell>>, } impl Drop for CellHashMapParent { @@ -688,7 +686,7 @@ fn cell_hashmap_rc_values_kept_alive_through_gc() { map.insert("m".to_owned(), m.clone()); let parent = jsg::Rc::new(CellHashMapParent { - children: Cell::new(map), + children: jsg::Cell::new(map), }); let wrapped = parent.clone().to_js(lock)?; keep_owner(lock, wrapped, [&m]); diff --git a/src/rust/jsg-test/tests/cpp_interop.rs b/src/rust/jsg-test/tests/cpp_interop.rs index 96c31acdf52..8778befbb38 100644 --- a/src/rust/jsg-test/tests/cpp_interop.rs +++ b/src/rust/jsg-test/tests/cpp_interop.rs @@ -6,7 +6,6 @@ //! resources that take, return and hold it (`crate::cpp_interop`). use std::cell::Cell; -use std::cell::RefCell; use jsg::v8; @@ -34,7 +33,7 @@ impl Drop for InRequest { fn new_host() -> jsg::Rc { jsg::Rc::new(Host { label: "test".to_owned(), - held: RefCell::new(None), + held: jsg::RefCell::new(None), }) } @@ -333,7 +332,7 @@ fn cpp_resource_carried_by_a_typed_promise_can_be_kept() { let (promise, resolver) = jsg::Promise::>::new_with_resolver(lock)?; let kept: jsg::Promise<()> = promise.then(lock, host, |lock, host, holder| { - host.hold(holder); + host.hold(lock, holder); // Traces the host, and through it the reference, from its new owner. Harness::request_gc(lock); Ok(()) diff --git a/src/rust/jsg-test/tests/function_new.rs b/src/rust/jsg-test/tests/function_new.rs index a1156324389..579ebe0f78d 100644 --- a/src/rust/jsg-test/tests/function_new.rs +++ b/src/rust/jsg-test/tests/function_new.rs @@ -325,7 +325,7 @@ static CYCLE_DROPS: AtomicUsize = AtomicUsize::new(0); /// Data that can hold a handle back to its own function. struct CycleData { _drops: DropCounter, - stored: RefCell>>, + stored: jsg::RefCell>>, } impl jsg::Traced for CycleData { @@ -340,10 +340,10 @@ fn cycle_through_data_is_collected() { harness.run_in_context(|lock, ctx| { let data = CycleData { _drops: DropCounter(&CYCLE_DROPS), - stored: RefCell::new(None), + stored: jsg::RefCell::new(None), }; let function = v8::Function::new(lock, data, |lock, data, args| { - *data.stored.borrow_mut() = Some(args.get(0).to_traced_global(lock)); + *data.stored.borrow_mut(lock) = Some(args.get(0).to_traced_global(lock)); Ok(lock.undefined()) }) .unwrap(); @@ -395,7 +395,7 @@ static TEARDOWN_LEAF_DROPS: AtomicUsize = AtomicUsize::new(0); struct TeardownData { _drops: DropCounter, leaf: jsg::Rc, - stored: RefCell>>, + stored: jsg::RefCell>>, } impl jsg::Traced for TeardownData { @@ -416,10 +416,10 @@ fn data_of_reachable_function_is_dropped_with_its_isolate() { leaf: jsg::Rc::new(Leaf { _drops: DropCounter(&TEARDOWN_LEAF_DROPS), }), - stored: RefCell::new(None), + stored: jsg::RefCell::new(None), }; let function = v8::Function::new(lock, data, |lock, data, args| { - *data.stored.borrow_mut() = Some(args.get(0).to_traced_global(lock)); + *data.stored.borrow_mut(lock) = Some(args.get(0).to_traced_global(lock)); Ok(lock.undefined()) }) .unwrap(); diff --git a/src/rust/jsg-test/tests/gc.rs b/src/rust/jsg-test/tests/gc.rs index 0aee57c4b4c..0eef5009328 100644 --- a/src/rust/jsg-test/tests/gc.rs +++ b/src/rust/jsg-test/tests/gc.rs @@ -11,7 +11,6 @@ //! Note: Circular references through `Rc` are NOT collected, matching the behavior //! of C++ `jsg::Rc` which uses `kj::Own` cross-references. -use std::cell::Cell; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; @@ -909,6 +908,8 @@ struct NativeState { impl jsg::Traced for NativeState {} +impl jsg::HoldsNoGcReferences for NativeState {} + impl Drop for NativeState { fn drop(&mut self) { NATIVE_STATE_DROPS.fetch_add(1, Ordering::SeqCst); @@ -1058,16 +1059,16 @@ fn rc_native_object_dropped_on_minor_gc() { static CYCLIC_RESOURCE_DROPS: AtomicUsize = AtomicUsize::new(0); -/// A resource that stores a `TracedGlobal` via `Cell` so the back-reference +/// A resource that stores a `TracedGlobal` via `jsg::Cell` so the back-reference /// to the resource's own JS wrapper can be installed after wrapping. /// -/// `Cell>` provides interior mutability through `&self`, matching +/// `jsg::Cell>` provides interior mutability through `&self`, matching /// the access pattern of `Traced::trace(&self)`. #[jsg_resource] struct CyclicResource { - /// The stored "callback". Uses `Cell` so it can be set after `to_js()` + /// The stored "callback". Uses `jsg::Cell` so it can be set after `to_js()` /// returns the wrapper `Local`, without requiring `&mut self`. - callback: std::cell::Cell>>, + callback: jsg::Cell>>, } impl Drop for CyclicResource { @@ -1095,7 +1096,7 @@ fn global_value_back_ref_is_collected_by_gc() { let harness = crate::Harness::new(); harness.run_in_context(|lock, _ctx| { let resource = jsg::Rc::new(CyclicResource { - callback: std::cell::Cell::new(None), + callback: jsg::Cell::new(None), }); // Wrap to produce the JS object, then immediately promote to a @@ -1150,10 +1151,10 @@ impl CellChild {} static CELL_PARENT_DROPS: AtomicUsize = AtomicUsize::new(0); -/// Resource with a `Cell>` field. +/// Resource with a `jsg::Cell>` field. #[jsg_resource] struct CellParent { - pub child: Cell>, + pub child: jsg::Cell>, } impl Drop for CellParent { @@ -1165,10 +1166,10 @@ impl Drop for CellParent { #[jsg_resource] impl CellParent {} -/// Resource with a `Cell>>` field. +/// Resource with a `jsg::Cell>>` field. #[jsg_resource] struct CellOptionParent { - pub child: Cell>>, + pub child: jsg::Cell>>, } impl Drop for CellOptionParent { @@ -1180,10 +1181,10 @@ impl Drop for CellOptionParent { #[jsg_resource] impl CellOptionParent {} -/// Resource with a `Cell>>` field. +/// Resource with a `jsg::Cell>>` field. #[jsg_resource] struct CellNullableParent { - pub child: Cell>>, + pub child: jsg::Cell>>, } impl Drop for CellNullableParent { @@ -1195,21 +1196,6 @@ impl Drop for CellNullableParent { #[jsg_resource] impl CellNullableParent {} -/// Resource using the fully-qualified `std::cell::Cell>` syntax. -#[jsg_resource] -struct StdCellParent { - pub child: std::cell::Cell>, -} - -impl Drop for StdCellParent { - fn drop(&mut self) { - CELL_PARENT_DROPS.fetch_add(1, Ordering::SeqCst); - } -} - -#[jsg_resource] -impl StdCellParent {} - /// `Cell>` keeps the child alive through GC. #[test] fn cell_ref_keeps_child_alive_through_gc() { @@ -1220,7 +1206,7 @@ fn cell_ref_keeps_child_alive_through_gc() { harness.run_in_context(|lock, ctx| { let child = jsg::Rc::new(CellChild); let parent = jsg::Rc::new(CellParent { - child: Cell::new(child.clone()), + child: jsg::Cell::new(child.clone()), }); let wrapped = parent.clone().to_js(lock)?; ctx.set_global(lock, "parent", wrapped); @@ -1254,7 +1240,7 @@ fn cell_option_ref_some_keeps_child_alive_through_gc() { harness.run_in_context(|lock, ctx| { let child = jsg::Rc::new(CellChild); let parent = jsg::Rc::new(CellOptionParent { - child: Cell::new(Some(child.clone())), + child: jsg::Cell::new(Some(child.clone())), }); let wrapped = parent.clone().to_js(lock)?; ctx.set_global(lock, "parent", wrapped); @@ -1284,7 +1270,7 @@ fn cell_option_ref_none_does_not_crash_during_gc() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { let parent = jsg::Rc::new(CellOptionParent { - child: Cell::new(None), + child: jsg::Cell::new(None), }); let wrapped = parent.clone().to_js(lock)?; ctx.set_global(lock, "parent", wrapped); @@ -1313,7 +1299,7 @@ fn cell_nullable_ref_some_keeps_child_alive_through_gc() { harness.run_in_context(|lock, ctx| { let child = jsg::Rc::new(CellChild); let parent = jsg::Rc::new(CellNullableParent { - child: Cell::new(jsg::Nullable::Some(child.clone())), + child: jsg::Cell::new(jsg::Nullable::Some(child.clone())), }); let wrapped = parent.clone().to_js(lock)?; ctx.set_global(lock, "parent", wrapped); @@ -1343,7 +1329,7 @@ fn cell_nullable_ref_null_does_not_crash_during_gc() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { let parent = jsg::Rc::new(CellNullableParent { - child: Cell::new(jsg::Nullable::Null), + child: jsg::Cell::new(jsg::Nullable::Null), }); let wrapped = parent.clone().to_js(lock)?; ctx.set_global(lock, "parent", wrapped); @@ -1361,35 +1347,3 @@ fn cell_nullable_ref_null_does_not_crash_during_gc() { Ok(()) }); } - -/// `std::cell::Cell>` (fully-qualified path) keeps child alive through GC. -#[test] -fn std_cell_ref_keeps_child_alive_through_gc() { - CELL_CHILD_DROPS.store(0, Ordering::SeqCst); - CELL_PARENT_DROPS.store(0, Ordering::SeqCst); - - let harness = crate::Harness::new(); - harness.run_in_context(|lock, ctx| { - let child = jsg::Rc::new(CellChild); - let parent = jsg::Rc::new(StdCellParent { - child: std::cell::Cell::new(child.clone()), - }); - let wrapped = parent.clone().to_js(lock)?; - ctx.set_global(lock, "parent", wrapped); - - std::mem::drop(child); - std::mem::drop(parent); - - crate::Harness::request_gc(lock); - assert_eq!(CELL_PARENT_DROPS.load(Ordering::SeqCst), 0); - assert_eq!(CELL_CHILD_DROPS.load(Ordering::SeqCst), 0); - Ok(()) - }); - - harness.run_in_context(|lock, _ctx| { - crate::Harness::request_gc(lock); - assert_eq!(CELL_PARENT_DROPS.load(Ordering::SeqCst), 1); - assert_eq!(CELL_CHILD_DROPS.load(Ordering::SeqCst), 1); - Ok(()) - }); -} diff --git a/src/rust/jsg-test/tests/global.rs b/src/rust/jsg-test/tests/global.rs index b794c0e57af..9a203f7c31c 100644 --- a/src/rust/jsg-test/tests/global.rs +++ b/src/rust/jsg-test/tests/global.rs @@ -5,7 +5,6 @@ //! Tests for `Global`: its layout, dropping it under the isolate lock, without it, on //! another thread or after its isolate is gone, and turning it into a `TracedGlobal`. -use std::cell::Cell; use std::panic::AssertUnwindSafe; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; @@ -33,7 +32,7 @@ impl Drop for GlobalCounted { /// A resource that owns a `TracedGlobal`, which GC traces. #[jsg_resource] struct TracedHolder { - handle: Cell>>, + handle: jsg::Cell>>, } #[jsg_resource] @@ -263,7 +262,7 @@ fn global_into_traced_global_is_traced() { let harness = crate::Harness::new(); harness.run_in_context(|lock, _ctx| { let resource = jsg::Rc::new(TracedHolder { - handle: Cell::new(None), + handle: jsg::Cell::new(None), }); let wrapper = resource.clone().to_js(lock)?; let global = v8::Global::new(lock, wrapper); diff --git a/src/rust/jsg-test/tests/isolates.rs b/src/rust/jsg-test/tests/isolates.rs index 0e049c397b3..01dde5fc1d0 100644 --- a/src/rust/jsg-test/tests/isolates.rs +++ b/src/rust/jsg-test/tests/isolates.rs @@ -4,8 +4,6 @@ //! Tests that keep handles of one isolate out of another. -#[cfg(not(debug_assertions))] -use std::cell::Cell; use std::panic::AssertUnwindSafe; use jsg::FromJS; @@ -159,6 +157,31 @@ fn global_of_another_isolate_panics() { }); } +#[test] +fn taking_a_global_of_another_isolate_out_of_a_cell_panics() { + let a = crate::Harness::new(); + let b = crate::Harness::new(); + let mut global: Option> = None; + a.run_in_context(|lock, _ctx| { + let value = "from a".to_js(lock)?; + global = Some(v8::TracedGlobal::new(lock, value)); + Ok(()) + }); + let cell = jsg::Cell::new(global); + + let result = std::panic::catch_unwind(AssertUnwindSafe(|| { + b.run_in_context(|lock, _ctx| { + let _taken = cell.take(lock); + Ok(()) + }); + })); + let payload = result.expect_err("taking A's TracedGlobal out of a cell in B must panic"); + assert_eq!( + panic_message(&*payload), + "attempt to use a Global or TracedGlobal in an isolate other than the one it was created in" + ); +} + #[test] fn callback_entered_while_another_isolate_has_scopes_throws() { let a = crate::Harness::new(); @@ -283,7 +306,7 @@ fn resource_wrapped_in_another_isolate_panics_while_exception_pending() { #[cfg(not(debug_assertions))] #[jsg_resource] struct TracedGlobalHolder { - handle: Cell>>, + handle: jsg::Cell>>, } #[cfg(not(debug_assertions))] @@ -306,7 +329,7 @@ fn gc_of_another_isolate_leaves_a_global_alone() { Ok(()) }); let holder = jsg::Rc::new(TracedGlobalHolder { - handle: Cell::new(global), + handle: jsg::Cell::new(global), }); b.run_in_context(|lock, ctx| { @@ -318,7 +341,7 @@ fn gc_of_another_isolate_leaves_a_global_alone() { a.run_in_context(|lock, _ctx| { crate::Harness::request_gc(lock); - let global = holder.handle.take().expect("set above"); + let global = holder.handle.take(lock).expect("set above"); assert!(global.as_local(lock).is_object()); drop(global); Ok(()) diff --git a/src/rust/jsg-test/tests/local_value.rs b/src/rust/jsg-test/tests/local_value.rs index 2db3fb617fc..44da9ebd7f3 100644 --- a/src/rust/jsg-test/tests/local_value.rs +++ b/src/rust/jsg-test/tests/local_value.rs @@ -5,8 +5,6 @@ //! `Local` converts like C++ JSG's `v8::Local`: any value passes through //! unchanged in both directions. -use std::cell::RefCell; - use jsg::FromJS; use jsg::ToJS; use jsg::v8; @@ -15,7 +13,7 @@ use jsg_macros::jsg_resource; #[jsg_resource] struct Values { - stored: RefCell>>, + stored: jsg::RefCell>>, } #[jsg_resource] @@ -32,7 +30,8 @@ impl Values { #[jsg_method] pub fn store<'s>(&self, lock: &mut jsg::Lock<'s>, value: v8::Local<'s, v8::Value>) { - *self.stored.borrow_mut() = Some(value.to_traced_global(lock)); + let value = value.to_traced_global(lock); + *self.stored.borrow_mut(lock) = Some(value); } #[jsg_method] @@ -50,7 +49,7 @@ impl Values { fn install(lock: &mut jsg::Lock<'_>, ctx: &crate::EvalContext<'_>) { let values = jsg::Rc::new(Values { - stored: RefCell::new(None), + stored: jsg::RefCell::new(None), }) .to_js(lock) .unwrap(); diff --git a/src/rust/jsg-test/tests/mod.rs b/src/rust/jsg-test/tests/mod.rs index 73863987da3..b53a3c09a2d 100644 --- a/src/rust/jsg-test/tests/mod.rs +++ b/src/rust/jsg-test/tests/mod.rs @@ -8,6 +8,7 @@ mod async_context; mod boolean; mod buffer_types; mod builtin_module; +mod cell; mod coercion_safety; mod collections_gc; mod cpp_interop; diff --git a/src/rust/jsg-test/tests/promise.rs b/src/rust/jsg-test/tests/promise.rs index 6bd28690132..b5998f3cba2 100644 --- a/src/rust/jsg-test/tests/promise.rs +++ b/src/rust/jsg-test/tests/promise.rs @@ -1029,7 +1029,7 @@ fn promise_operations_while_terminating_keep_the_termination() { /// the resolver in between, as an API waiting on an event would. #[jsg_resource] struct Deferred { - resolver: RefCell>>, + resolver: jsg::RefCell>>, } #[jsg_resource] @@ -1037,7 +1037,7 @@ impl Deferred { #[jsg_method] fn wait<'s>(&self, lock: &mut jsg::Lock<'s>) -> Result, jsg::Error> { let resolver = v8::PromiseResolver::new(lock)?; - *self.resolver.borrow_mut() = Some(resolver.to_traced_global(lock)); + *self.resolver.borrow_mut(lock) = Some(resolver.to_traced_global(lock)); Ok(resolver.promise()) } @@ -1063,7 +1063,7 @@ impl Deferred { &self, lock: &mut jsg::Lock<'s>, ) -> Result, jsg::Error> { - let resolver = self.resolver.borrow_mut().take(); + let resolver = self.resolver.borrow_mut(lock).take(); resolver .map(|resolver| resolver.as_local(lock)) .ok_or_else(|| jsg::Error::new_type_error("nothing is waiting")) @@ -1074,7 +1074,7 @@ impl Deferred { fn resource_settles_promise_from_later_call() { crate::Harness::new().run_in_context(|lock, ctx| { let deferred = jsg::Rc::new(Deferred { - resolver: RefCell::new(None), + resolver: jsg::RefCell::new(None), }); ctx.set_global(lock, "deferred", deferred); let code = format!( @@ -1100,7 +1100,7 @@ static DEFERRED_DROPS: AtomicUsize = AtomicUsize::new(0); #[jsg_resource] struct DroppedDeferred { _drops: DropCounter, - resolver: RefCell>>, + resolver: jsg::RefCell>>, } #[jsg_resource] @@ -1108,7 +1108,7 @@ impl DroppedDeferred { #[jsg_method] fn wait<'s>(&self, lock: &mut jsg::Lock<'s>) -> Result, jsg::Error> { let resolver = v8::PromiseResolver::new(lock)?; - *self.resolver.borrow_mut() = Some(resolver.to_traced_global(lock)); + *self.resolver.borrow_mut(lock) = Some(resolver.to_traced_global(lock)); Ok(resolver.promise()) } } @@ -1119,7 +1119,7 @@ fn cycle_through_held_resolver_is_collected() { harness.run_in_context(|lock, ctx| { let deferred = jsg::Rc::new(DroppedDeferred { _drops: DropCounter(&DEFERRED_DROPS), - resolver: RefCell::new(None), + resolver: jsg::RefCell::new(None), }); ctx.set_global(lock, "deferred", deferred); // The promise's reaction holds the resource, which holds the resolver of the promise. @@ -1146,8 +1146,8 @@ fn cycle_through_held_resolver_is_collected() { /// its handles once, through it. #[jsg_resource] struct CacheStore { - entries: RefCell>>, - in_flight: RefCell>>, + entries: jsg::RefCell>>, + in_flight: jsg::RefCell>>, } #[jsg_resource] @@ -1219,10 +1219,10 @@ impl ReadThroughCache { }, |lock, fill, value| { let entry = value.to_traced_global(lock); - fill.store.in_flight.borrow_mut().remove(&fill.key); + fill.store.in_flight.borrow_mut(lock).remove(&fill.key); fill.store .entries - .borrow_mut() + .borrow_mut(lock) .insert(fill.key.clone(), entry); Ok(value) }, @@ -1234,14 +1234,14 @@ impl ReadThroughCache { key: key.clone(), }, |lock, fill, _reason| { - fill.store.in_flight.borrow_mut().remove(&fill.key); + fill.store.in_flight.borrow_mut(lock).remove(&fill.key); Ok(lock.undefined()) }, )?; let in_flight = filled.to_traced_global(lock); self.store .in_flight - .borrow_mut() + .borrow_mut(lock) .insert(key.clone(), in_flight); let key_js = key.as_str().to_js(lock)?; @@ -1253,7 +1253,7 @@ impl ReadThroughCache { Ok(Err(exception)) => resolver.reject(lock, exception)?, Err(err) => { // Execution is terminating, so nothing will settle the fill. - self.store.in_flight.borrow_mut().remove(&key); + self.store.in_flight.borrow_mut(lock).remove(&key); return Err(err); } } @@ -1264,8 +1264,8 @@ impl ReadThroughCache { fn install_cache(lock: &mut jsg::Lock<'_>, ctx: &crate::EvalContext<'_>) { let cache = jsg::Rc::new(ReadThroughCache { store: jsg::Rc::new(CacheStore { - entries: RefCell::new(HashMap::new()), - in_flight: RefCell::new(HashMap::new()), + entries: jsg::RefCell::new(HashMap::new()), + in_flight: jsg::RefCell::new(HashMap::new()), }), }); ctx.set_global(lock, "cache", cache); diff --git a/src/rust/jsg-test/tests/resource_this.rs b/src/rust/jsg-test/tests/resource_this.rs index 628ab8eb3cf..ac6d2f55295 100644 --- a/src/rust/jsg-test/tests/resource_this.rs +++ b/src/rust/jsg-test/tests/resource_this.rs @@ -5,7 +5,6 @@ //! Tests for `this: jsg::Rc` parameters, which give a method or accessor a `jsg::Rc` of //! the resource it was called on, in place of `&self`. -use std::cell::Cell; use std::cell::RefCell; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; @@ -28,7 +27,7 @@ struct Task { label: RefCell, /// The wrapper of the queue this task is in, which the task keeps as a resource keeps a /// callback it registered: through a `TracedGlobal`, so that GC can collect the cycle. - queue: Cell>>, + queue: jsg::Cell>>, } impl Drop for Task { @@ -44,7 +43,7 @@ impl Task { Self { name, label: RefCell::new(String::new()), - queue: Cell::new(None), + queue: jsg::Cell::new(None), } } @@ -89,7 +88,7 @@ impl Task { this.queue.set(Some( queue.clone().to_js(lock).unwrap().to_traced_global(lock), )); - queue.tasks.borrow_mut().push(this); + queue.tasks.borrow_mut(lock).push(this); } #[jsg_property] @@ -116,7 +115,7 @@ impl Task { /// A resource that holds `jsg::Rc`s of tasks, as a callback's data would. #[jsg_resource] struct Queue { - tasks: RefCell>>, + tasks: jsg::RefCell>>, } #[jsg_resource] @@ -124,13 +123,13 @@ impl Queue { #[jsg_constructor] fn constructor() -> Self { Self { - tasks: RefCell::new(Vec::new()), + tasks: jsg::RefCell::new(Vec::new()), } } #[jsg_method] - fn clear(&self) { - self.tasks.borrow_mut().clear(); + fn clear(&self, lock: &mut jsg::Lock<'_>) { + self.tasks.borrow_mut(lock).clear(); } #[jsg_method] diff --git a/src/rust/jsg-test/tests/to_js_errors.rs b/src/rust/jsg-test/tests/to_js_errors.rs index 2d2aeaa384d..9ad4a102423 100644 --- a/src/rust/jsg-test/tests/to_js_errors.rs +++ b/src/rust/jsg-test/tests/to_js_errors.rs @@ -6,8 +6,6 @@ //! the value, and from the return value of a method or a wildcard property to the JavaScript //! caller, which catches what JavaScript threw during the conversion. -use std::cell::RefCell; - use jsg::ToJS; use jsg::v8; use jsg_macros::jsg_method; @@ -93,7 +91,7 @@ impl Converter { /// Answers its `value` property with what calling its callback returns. #[jsg_resource] struct WildcardConverter { - callback: RefCell>>, + callback: jsg::RefCell>>, } #[jsg_resource] @@ -106,7 +104,7 @@ impl WildcardConverter { ) -> jsg::Result<()> { let callback = v8::Local::::try_from(callback) .map_err(|_| jsg::Error::new_type_error("expected a function"))?; - *self.callback.borrow_mut() = Some(v8::TracedGlobal::new(lock, callback)); + *self.callback.borrow_mut(lock) = Some(v8::TracedGlobal::new(lock, callback)); Ok(()) } @@ -126,7 +124,7 @@ fn install(lock: &mut jsg::Lock<'_>, ctx: &crate::EvalContext<'_>) { lock, "wildcard", jsg::Rc::new(WildcardConverter { - callback: RefCell::new(None), + callback: jsg::RefCell::new(None), }), ); } diff --git a/src/rust/jsg-test/tests/traceable_gc.rs b/src/rust/jsg-test/tests/traceable_gc.rs index 71e425f5b6b..c14b6241461 100644 --- a/src/rust/jsg-test/tests/traceable_gc.rs +++ b/src/rust/jsg-test/tests/traceable_gc.rs @@ -7,7 +7,6 @@ //! `#[jsg_resource]` now traces every field via `Traced::trace`, so nested //! non-resource helper structs/enums just need to implement `Traced`. -use std::cell::RefCell; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; @@ -175,12 +174,15 @@ jsg::jsg_traced!(PlainData); /// and a `#[jsg_struct]` type can when a field can. #[test] fn holds_gc_references_of_jsg_types() { + fn holds_no_gc_references() {} assert!(>::holds_gc_references()); assert!(Leaf::holds_gc_references()); assert!(!>::holds_gc_references()); + holds_no_gc_references::>(); assert!(!PlainStruct::holds_gc_references()); assert!(Link::holds_gc_references()); assert!(!PlainData::holds_gc_references()); + holds_no_gc_references::(); } // ============================================================================= @@ -198,7 +200,7 @@ struct Link { #[jsg_resource] struct Node { - pub link: RefCell>, + pub link: jsg::RefCell>, } impl Drop for Node { @@ -213,7 +215,7 @@ impl Node {} /// Holds a `Node`'s wrapper, which closes a cycle through the `Node`'s `Link`. #[jsg_resource] struct Back { - node: std::cell::Cell>>, + node: jsg::Cell>>, } impl Drop for Back { @@ -236,10 +238,10 @@ fn cycle_through_struct_field_is_collected() { let harness = crate::Harness::new(); harness.run_in_context(|lock, _ctx| { let back = jsg::Rc::new(Back { - node: std::cell::Cell::new(None), + node: jsg::Cell::new(None), }); let node = jsg::Rc::new(Node { - link: RefCell::new(Some(Link { + link: jsg::RefCell::new(Some(Link { target: back.clone(), })), }); diff --git a/src/rust/jsg-test/tests/traced_global_lifetime.rs b/src/rust/jsg-test/tests/traced_global_lifetime.rs index 8477eb961f9..0fae38ca224 100644 --- a/src/rust/jsg-test/tests/traced_global_lifetime.rs +++ b/src/rust/jsg-test/tests/traced_global_lifetime.rs @@ -3,11 +3,9 @@ // https://opensource.org/licenses/Apache-2.0 //! Tests for dropping a `TracedGlobal` without the isolate lock, on another thread, or after its -//! isolate is gone, and for moving one that GC has visited, in and out of the resource that -//! traces it. +//! isolate is gone, and for taking one that GC has visited out of the resource that traces it, +//! and putting it back. -use std::cell::Cell; -use std::cell::RefCell; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; @@ -20,7 +18,7 @@ use jsg_macros::jsg_resource; /// handle. #[jsg_resource] struct HandleHolder { - handle: Cell>>, + handle: jsg::Cell>>, } #[jsg_resource] @@ -67,14 +65,15 @@ impl HolderParent { /// A resource that collects `TracedGlobal`s in a `Vec`, which moves them whenever it grows. #[jsg_resource] struct HandleList { - handles: RefCell>>, + handles: jsg::RefCell>>, } #[jsg_resource] impl HandleList { #[jsg_method] fn push<'s>(&self, lock: &mut jsg::Lock<'s>, value: v8::Local<'s, v8::Value>) { - self.handles.borrow_mut().push(value.to_traced_global(lock)); + let value = value.to_traced_global(lock); + self.handles.borrow_mut(lock).push(value); } } @@ -126,7 +125,7 @@ fn wrapped_resource_dropped_after_its_isolate_is_disposed() { let mut holder: Option> = None; harness.run_in_context(|lock, ctx| { let resource = jsg::Rc::new(HandleHolder { - handle: Cell::new(None), + handle: jsg::Cell::new(None), }); resource .handle @@ -154,7 +153,7 @@ fn wrapped_resource_in_traced_mode_is_dropped_with_its_isolate() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { let resource = jsg::Rc::new(HandleHolder { - handle: Cell::new(None), + handle: jsg::Cell::new(None), }); let wrapper = resource.clone().to_js(lock)?; // The resource holds its own wrapper, and JavaScript holds it too. @@ -183,7 +182,7 @@ fn holder_in_traced_mode( ctx: &crate::EvalContext<'_>, ) -> (jsg::Rc, usize) { let resource = jsg::Rc::new(HandleHolder { - handle: Cell::new(None), + handle: jsg::Cell::new(None), }); ctx.set_global(lock, "holder", resource.clone()); // No `Local` of the value outlives the scripts, so that once `globalThis.value` lets go of @@ -207,7 +206,7 @@ fn traced_global_dropped_frees_its_traced_node_at_the_next_gc() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { let (resource, baseline) = holder_in_traced_mode(lock, ctx); - drop(resource.handle.take()); + resource.handle.set(None); assert!( crate::Harness::used_global_handles_size(lock) > baseline, "dropping leaves the traced node for the next full GC", @@ -218,10 +217,10 @@ fn traced_global_dropped_frees_its_traced_node_at_the_next_gc() { }); } -/// A `TracedGlobal` taken out of its resource is traced by nothing from then on, so the next full -/// GC frees its traced node, while its value lives on and the handle still reaches it. Dropping -/// the `TracedGlobal` afterwards must leave the freed node alone: freeing it again would corrupt -/// V8's accounting of traced nodes, if not its free list. +/// A `TracedGlobal` taken out of its resource is strong, and traced by nothing from then on, so +/// the next full GC frees its traced node, while its value lives on and the handle still reaches +/// it. Dropping the `TracedGlobal` afterwards must leave the freed node alone: freeing it again +/// would corrupt V8's accounting of traced nodes, if not its free list. #[test] fn global_taken_out_of_a_traced_resource_outlives_its_traced_node() { let harness = crate::Harness::new(); @@ -229,7 +228,7 @@ fn global_taken_out_of_a_traced_resource_outlives_its_traced_node() { let (resource, baseline) = holder_in_traced_mode(lock, ctx); let with_global = crate::Harness::used_global_handles_size(lock); - let taken = resource.handle.take().expect("set above"); + let taken = resource.handle.take(lock).expect("set above"); // Nothing traces the node now; the value stays reachable through the global object. crate::Harness::request_gc(lock); let without_node = crate::Harness::used_global_handles_size(lock); @@ -254,7 +253,7 @@ fn global_put_back_after_its_traced_node_was_freed_is_traced_again() { let (resource, baseline) = holder_in_traced_mode(lock, ctx); let with_global = crate::Harness::used_global_handles_size(lock); - let taken = resource.handle.take().expect("set above"); + let taken = resource.handle.take(lock).expect("set above"); crate::Harness::request_gc(lock); resource.handle.set(Some(taken)); crate::Harness::request_gc(lock); @@ -268,7 +267,7 @@ fn global_put_back_after_its_traced_node_was_freed_is_traced_again() { ctx.eval_raw(lock, "globalThis.value = undefined") .expect("should evaluate"); crate::Harness::request_gc(lock); - let taken = resource.handle.take().expect("set above"); + let taken = resource.handle.take(lock).expect("set above"); assert!(taken.as_local(lock).is_object()); drop(taken); @@ -279,15 +278,14 @@ fn global_put_back_after_its_traced_node_was_freed_is_traced_again() { } /// A resource without a wrapper that gains its first strong reference has its handles made -/// strong, by a visit outside any GC. A `TracedGlobal` put back into it after a GC freed its traced -/// node still has a weak strong handle, which that visit must make strong again along with -/// letting go of the node, or the value dies once nothing else holds it. +/// strong, by a visit outside any GC. That visit must make the weak handle of a `TracedGlobal` +/// in traced mode strong again, or the value dies once nothing else holds it. #[test] -fn global_put_back_into_a_resource_that_gains_a_strong_reference_is_strong() { +fn global_in_a_resource_that_gains_a_strong_reference_is_made_strong() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { let child = jsg::Rc::new(HandleHolder { - handle: Cell::new(None), + handle: jsg::Cell::new(None), }); let parent = jsg::Rc::new(HolderParent { child }); ctx.set_global(lock, "parent", parent.clone()); @@ -300,10 +298,6 @@ fn global_put_back_into_a_resource_that_gains_a_strong_reference_is_strong() { // goes to traced mode. crate::Harness::request_gc(lock); - let taken = parent.child.handle.take().expect("set above"); - // Nothing traces the node now, and the GC frees it. - crate::Harness::request_gc(lock); - parent.child.handle.set(Some(taken)); // The child's first strong reference visits the `TracedGlobal`. let child = parent.child.clone(); @@ -311,7 +305,7 @@ fn global_put_back_into_a_resource_that_gains_a_strong_reference_is_strong() { ctx.eval_raw(lock, "globalThis.value = undefined; undefined") .expect("should evaluate"); crate::Harness::request_gc(lock); - let taken = child.handle.take().expect("set above"); + let taken = child.handle.take(lock).expect("set above"); assert!(taken.as_local(lock).is_object()); Ok(()) }); @@ -393,9 +387,9 @@ fn used_global_handles_size_at_next_lock(harness: &crate::Harness) -> usize { at_next_lock - (at_lock_after - at_next_lock) } -/// A `TracedGlobal` in traced mode dropped without the isolate lock queues its strong handle and -/// lets go of its traced node, which a full GC frees while the handle waits. The next lock resets the -/// handle and leaves the freed node alone. +/// A `TracedGlobal` taken out of a resource in traced mode has let go of its traced node, which +/// a full GC frees. Dropped without the isolate lock, it queues its strong handle; the next lock +/// resets the handle and leaves the freed node alone. #[test] fn traced_global_dropped_without_the_lock_is_released_at_the_next_lock() { let harness = crate::Harness::new(); @@ -405,7 +399,7 @@ fn traced_global_dropped_without_the_lock_is_released_at_the_next_lock() { let (resource, b) = holder_in_traced_mode(lock, ctx); baseline = b; let with_global = crate::Harness::used_global_handles_size(lock); - let taken = resource.handle.take().expect("set above"); + let taken = resource.handle.take(lock).expect("set above"); crate::Harness::with_isolate_unlocked(lock, || drop(taken)); assert_eq!(lock.globals_awaiting_lock(), 1); crate::Harness::request_gc(lock); @@ -422,16 +416,18 @@ fn traced_global_dropped_without_the_lock_is_released_at_the_next_lock() { } /// Wraps a new `HandleList` that collects a value, then GCs, which makes the first `TracedGlobal`'s -/// strong handle weak, then has the list grow and leaves it unreachable. +/// strong handle weak, then has the list grow, which makes it strong again, and leaves the list +/// unreachable. fn fill_and_abandon_handle_list(lock: &mut jsg::Lock<'_>, ctx: &crate::EvalContext<'_>) { let list = jsg::Rc::new(HandleList { - handles: RefCell::new(Vec::new()), + handles: jsg::RefCell::new(Vec::new()), }); ctx.set_global(lock, "list", list); ctx.eval_raw(lock, "list.push({})") .expect("should evaluate"); crate::Harness::request_gc(lock); - // Growing the `Vec` moves the first `TracedGlobal`, and frees the buffer it was in. + // Growing the `Vec` moves the first `TracedGlobal`, and frees the buffer it was in. The + // `jsg::RefCell` makes it strong before the push. ctx.eval_raw(lock, "for (let i = 0; i < 64; i++) list.push({})") .expect("should evaluate"); ctx.eval_raw(lock, "globalThis.list = undefined") @@ -439,11 +435,13 @@ fn fill_and_abandon_handle_list(lock: &mut jsg::Lock<'_>, ctx: &crate::EvalConte } /// Once GC has visited a `TracedGlobal`, V8 holds the address of its weak handle's slot, to null it -/// when the value dies. A `Vec` that grows moves its `TracedGlobal`s, so the slot must not move -/// with them. The values must die in the first GC after the move: a GC that visited them would keep -/// them alive. +/// when the value dies. A `Vec` that grows moves its `TracedGlobal`s; it grows under a +/// `jsg::RefCell::borrow_mut`, which first makes them strong, letting go of their traced nodes, +/// and the box keeps the slot address valid in any case. The values must die in the first GC after +/// the move, with the list, and the freed nodes and reset handles bring the count back to the +/// baseline. #[test] -fn visited_globals_survive_being_moved() { +fn visited_globals_moved_by_a_growing_vec_are_released_with_it() { HANDLE_LIST_DROPS.store(0, Ordering::SeqCst); let harness = crate::Harness::new(); @@ -471,63 +469,33 @@ fn visited_globals_survive_being_moved() { }); } -/// A `TracedGlobal` taken out of its resource keeps its weak handle, and V8 nulls that handle's -/// slot when the value dies, which it does at the next GC when nothing else holds it. The slot must -/// still be the one V8 knows, although the `TracedGlobal` has moved, or the `TracedGlobal` would go -/// on to reset a node V8 already freed. -#[test] -fn visited_global_taken_out_of_its_resource_sees_its_value_die() { - let harness = crate::Harness::new(); - harness.run_in_context(|lock, ctx| { - let resource = jsg::Rc::new(HandleHolder { - handle: Cell::new(None), - }); - ctx.set_global(lock, "holder", resource.clone()); - crate::Harness::request_gc(lock); - let baseline = crate::Harness::used_global_handles_size(lock); - // The value is reachable through the resource's `TracedGlobal` alone. - ctx.eval_raw(lock, "holder.set({})") - .expect("should evaluate"); - // Tracing the wrapper puts the handle in traced mode: weak, with a traced node that - // the resource's trace keeps alive. - crate::Harness::request_gc(lock); - let taken = resource.handle.take().expect("set above"); - // Nothing traces the node now, so the value dies and V8 nulls the weak handle. - crate::Harness::request_gc(lock); - drop(taken); - assert_eq!(crate::Harness::used_global_handles_size(lock), baseline); - Ok(()) - }); -} - -/// GC can still run once the `Realm` is dropped, and V8 then still nulls the weak handle slot of -/// a visited `TracedGlobal` when its value dies. The slot of a `TracedGlobal` dropped after the -/// `Realm` must therefore outlive that GC, or V8 writes into freed memory, which a memory checker -/// reports. +/// GC can still run once the `Realm` is dropped. Here the resource that owns a visited +/// `TracedGlobal` is reachable only through its wrapper, which the realm's global object held, so +/// the GC after the realm collects the value, nulling the weak handle's slot, and then the +/// resource, dropping the `TracedGlobal` with the `Realm` gone. A `TracedGlobal` whose slot V8 +/// nulls only after the drop would need a weak one moved out of its resource, which only a `Drop` +/// impl can do; no test covers that. #[test] fn visited_global_dropped_after_its_realm_keeps_its_slot_through_a_gc() { let harness = crate::Harness::new(); - let mut taken = None; harness.run_in_context(|lock, ctx| { let resource = jsg::Rc::new(HandleHolder { - handle: Cell::new(None), + handle: jsg::Cell::new(None), }); - ctx.set_global(lock, "holder", resource.clone()); + ctx.set_global(lock, "holder", resource); ctx.eval_raw(lock, "holder.set({}); undefined") .expect("should evaluate"); // Tracing the wrapper makes the handle weak. crate::Harness::request_gc(lock); - taken = resource.handle.take(); Ok(()) }); - let taken = taken.expect("set in the context"); - // The weak handle is all that refers to the value, which dies in the GC. - harness.drop_realm_then_gc(|| drop(taken)); + harness.drop_realm_then_gc(|| {}); } /// A `TracedGlobal` is `Send`: another thread may drop it while this one holds the isolate lock and -/// creates handles. The drop is queued, and the next lock resets it. One of the `TracedGlobal`s is -/// in traced mode, so GC may write into its slot while it waits in the queue. +/// creates handles. The drop is queued, and the next lock resets it. One of the `TracedGlobal`s was +/// taken out of a resource in traced mode, and let go of a traced node that the GC frees while the +/// handle waits in the queue. #[test] fn drop_on_another_thread_is_deferred_until_the_next_lock() { COUNTED_DROPS.store(0, Ordering::SeqCst); @@ -542,7 +510,7 @@ fn drop_on_another_thread_is_deferred_until_the_next_lock() { drop(resource); let (resource, _) = holder_in_traced_mode(lock, ctx); - visited = resource.handle.take(); + visited = resource.handle.take(lock); holder = Some(resource); Ok(()) }); @@ -581,10 +549,11 @@ fn drop_on_another_thread_is_deferred_until_the_next_lock() { } /// `jsg::Rc::new` takes no `Lock`, so a thread without the isolate lock can put a `TracedGlobal` -/// into a new resource, whose first strong reference visits the `TracedGlobal`. That visit must -/// leave the handles alone while this thread holds the lock and creates handles: the strong handle -/// of a `TracedGlobal` in traced mode stays weak, so its value dies at the next GC once nothing -/// else holds it. +/// into a new resource, whose first strong reference visits the `TracedGlobal`, and drop that +/// resource, which queues the handle while this thread holds the lock and creates handles. The +/// `TracedGlobal` was taken out of a resource in traced mode, so it is strong and has no traced +/// node, and the visit finds nothing to change; the test checks that the drop is queued and that +/// the queued strong handle keeps the value alive until the next lock resets it. #[test] fn visit_on_another_thread_leaves_a_global_alone() { COUNTED_DROPS.store(0, Ordering::SeqCst); @@ -592,7 +561,7 @@ fn visit_on_another_thread_leaves_a_global_alone() { let harness = crate::Harness::new(); harness.run_in_context(|lock, ctx| { let resource = jsg::Rc::new(HandleHolder { - handle: Cell::new(None), + handle: jsg::Cell::new(None), }); ctx.set_global(lock, "holder", resource.clone()); ctx.eval_raw( @@ -600,19 +569,19 @@ fn visit_on_another_thread_leaves_a_global_alone() { "holder.set(globalThis.value = holder.newCounted()); undefined", ) .expect("should evaluate"); - // Tracing the wrapper puts the `TracedGlobal` in traced mode. + // Tracing the wrapper puts the `TracedGlobal` in traced mode; taking it out makes it + // strong. crate::Harness::request_gc(lock); - let taken = resource.handle.take().expect("set above"); + let taken = resource.handle.take(lock).expect("set above"); let barrier = std::sync::Arc::new(std::sync::Barrier::new(2)); let mover = std::thread::spawn({ let barrier = std::sync::Arc::clone(&barrier); move || { barrier.wait(); - let holder = jsg::Rc::new(HandleHolder { - handle: Cell::new(Some(taken)), - }); - holder.handle.take().expect("set above") + drop(jsg::Rc::new(HandleHolder { + handle: jsg::Cell::new(Some(taken)), + })); } }); barrier.wait(); @@ -620,17 +589,24 @@ fn visit_on_another_thread_leaves_a_global_alone() { for _ in 0..1000 { drop(lock.new_object().to_traced_global(lock)); } - let taken = mover.join().expect("the move does not panic"); + mover.join().expect("the move and the drop do not panic"); + assert_eq!(lock.globals_awaiting_lock(), 1); ctx.eval_raw(lock, "globalThis.value = undefined; undefined") .expect("should evaluate"); crate::Harness::request_gc(lock); assert_eq!( COUNTED_DROPS.load(Ordering::SeqCst), - 1, - "the strong handle is still weak" + 0, + "the queued strong handle roots the value" ); - drop(taken); + Ok(()) + }); + + harness.run_in_context(|lock, _ctx| { + assert_eq!(lock.globals_awaiting_lock(), 0); + crate::Harness::request_gc(lock); + assert_eq!(COUNTED_DROPS.load(Ordering::SeqCst), 1); Ok(()) }); } diff --git a/src/rust/jsg-test/tests/typed_promise.rs b/src/rust/jsg-test/tests/typed_promise.rs index fd4ab861165..81620aab3f0 100644 --- a/src/rust/jsg-test/tests/typed_promise.rs +++ b/src/rust/jsg-test/tests/typed_promise.rs @@ -5,7 +5,6 @@ //! Typed promises, `jsg::Promise<'s, T>`: carrying Rust values, reacting to them, and converting //! them where they cross into and out of JavaScript as C++ JSG's `jsg::Promise` does. -use std::cell::RefCell; use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; @@ -253,7 +252,7 @@ impl Holder {} /// Keeps what a reaction hands it, as a resource field. #[jsg_resource] struct Keeper { - kept: RefCell>>, + kept: jsg::RefCell>>, } #[jsg_resource] @@ -278,7 +277,7 @@ impl Keeper { fn carried_resource_can_be_kept_after_garbage_collection() { crate::Harness::new().run_in_context(|lock, ctx| { let keeper = jsg::Rc::new(Keeper { - kept: RefCell::new(None), + kept: jsg::RefCell::new(None), }); ctx.set_global(lock, "keeper", keeper.clone()); let object = lock.new_object(); @@ -289,7 +288,7 @@ fn carried_resource_can_be_kept_after_garbage_collection() { }); let (promise, resolver) = jsg::Promise::>::new_with_resolver(lock)?; let kept: jsg::Promise<()> = promise.then(lock, keeper, |lock, keeper, holder| { - *keeper.kept.borrow_mut() = Some(holder); + *keeper.kept.borrow_mut(lock) = Some(holder); // Traces the keeper, and through it the `jsg::Rc`, from its new owner. crate::Harness::request_gc(lock); Ok(()) @@ -856,8 +855,8 @@ fn carried_resource_survives_garbage_collection() { /// Hands out a typed promise and settles it from a later call, holding the resolver in between. #[jsg_resource] struct Deferred { - resolver: RefCell>>, - promise: RefCell>>, + resolver: jsg::RefCell>>, + promise: jsg::RefCell>>, } #[jsg_resource] @@ -865,14 +864,14 @@ impl Deferred { #[jsg_method] fn start(&self, lock: &mut jsg::Lock<'_>) -> Result<(), jsg::Error> { let (promise, resolver) = jsg::Promise::::new_with_resolver(lock)?; - *self.resolver.borrow_mut() = Some(resolver.into_traced(lock)); - *self.promise.borrow_mut() = Some(promise.into_traced(lock)); + *self.resolver.borrow_mut(lock) = Some(resolver.into_traced(lock)); + *self.promise.borrow_mut(lock) = Some(promise.into_traced(lock)); Ok(()) } #[jsg_method] fn name<'s>(&self, lock: &mut jsg::Lock<'s>) -> Result, jsg::Error> { - let promise = self.promise.borrow_mut().take(); + let promise = self.promise.borrow_mut(lock).take(); let promise = promise .ok_or_else(|| jsg::Error::new_type_error("not started"))? .into_local(lock); @@ -881,7 +880,7 @@ impl Deferred { #[jsg_method] fn finish(&self, lock: &mut jsg::Lock<'_>, name: String) -> Result<(), jsg::Error> { - let resolver = self.resolver.borrow_mut().take(); + let resolver = self.resolver.borrow_mut(lock).take(); let resolver = resolver .ok_or_else(|| jsg::Error::new_type_error("not started"))? .as_local(lock); @@ -893,8 +892,8 @@ impl Deferred { fn resource_keeps_typed_promise_and_resolver() { crate::Harness::new().run_in_context(|lock, ctx| { let deferred = jsg::Rc::new(Deferred { - resolver: RefCell::new(None), - promise: RefCell::new(None), + resolver: jsg::RefCell::new(None), + promise: jsg::RefCell::new(None), }); ctx.set_global(lock, "deferred", deferred); ctx.eval_raw(lock, "deferred.start()").unwrap(); @@ -1002,14 +1001,14 @@ fn eval_now_rejects_with_what_fails() { /// Settles a typed promise from a later call, for a callback that returns its promise. #[jsg_resource] struct Later { - resolver: RefCell>>, + resolver: jsg::RefCell>>, } #[jsg_resource] impl Later { #[jsg_method] fn finish(&self, lock: &mut jsg::Lock<'_>, name: String) -> Result<(), jsg::Error> { - let resolver = self.resolver.borrow_mut().take(); + let resolver = self.resolver.borrow_mut(lock).take(); let resolver = resolver .ok_or_else(|| jsg::Error::new_type_error("nothing is waiting"))? .as_local(lock); @@ -1018,7 +1017,7 @@ impl Later { #[jsg_method] fn fail(&self, lock: &mut jsg::Lock<'_>, message: String) -> Result<(), jsg::Error> { - let resolver = self.resolver.borrow_mut().take(); + let resolver = self.resolver.borrow_mut(lock).take(); let resolver = resolver .ok_or_else(|| jsg::Error::new_type_error("nothing is waiting"))? .as_local(lock); @@ -1030,14 +1029,14 @@ impl Later { /// A pending promise, which `finish` or `fail` settles. fn wait<'s>(&self, lock: &mut jsg::Lock<'s>) -> Result, jsg::Error> { let (promise, resolver) = jsg::Promise::::new_with_resolver(lock)?; - *self.resolver.borrow_mut() = Some(resolver.into_traced(lock)); + *self.resolver.borrow_mut(lock) = Some(resolver.into_traced(lock)); Ok(promise) } } fn install_later(lock: &mut jsg::Lock<'_>, ctx: &crate::EvalContext<'_>) -> jsg::Rc { let later = jsg::Rc::new(Later { - resolver: RefCell::new(None), + resolver: jsg::RefCell::new(None), }); ctx.set_global(lock, "later", later.clone()); later @@ -1159,14 +1158,14 @@ fn then_or_catch_promise_runs_the_callback_for_the_outcome() { /// holds the `Waiter`. #[jsg_resource] struct Source { - resolver: RefCell>>, + resolver: jsg::RefCell>>, } #[jsg_resource] impl Source { #[jsg_method] fn finish(&self, lock: &mut jsg::Lock<'_>, value: u32) -> Result<(), jsg::Error> { - let resolver = self.resolver.borrow_mut().take(); + let resolver = self.resolver.borrow_mut(lock).take(); let resolver = resolver .ok_or_else(|| jsg::Error::new_type_error("nothing is waiting"))? .as_local(lock); @@ -1202,7 +1201,7 @@ impl Waiter { source: jsg::Rc, ) -> Result, jsg::Error> { let (promise, resolver) = jsg::Promise::::new_with_resolver(lock)?; - *source.resolver.borrow_mut() = Some(resolver.into_traced(lock)); + *source.resolver.borrow_mut(lock) = Some(resolver.into_traced(lock)); Ok(promise) } @@ -1224,7 +1223,7 @@ fn returned_promise_keeps_its_creator_alive(method: &str) { WAITER_DROPS.store(0, Ordering::SeqCst); crate::Harness::new().run_in_context(|lock, ctx| { let source = jsg::Rc::new(Source { - resolver: RefCell::new(None), + resolver: jsg::RefCell::new(None), }); ctx.set_global(lock, "source", source); let make_waiter = v8::Function::new(lock, (), |lock, (), _args| { diff --git a/src/rust/jsg-test/tests/wildcard_property.rs b/src/rust/jsg-test/tests/wildcard_property.rs index 064f2deaa04..80001e89bef 100644 --- a/src/rust/jsg-test/tests/wildcard_property.rs +++ b/src/rust/jsg-test/tests/wildcard_property.rs @@ -5,8 +5,6 @@ //! `#[jsg_wildcard_property]` behaves as C++ JSG's `JSG_WILDCARD_PROPERTY`. The first test //! mirrors the "Named interceptor" test in `src/workerd/jsg/jsg-test.c++` case for case. -use std::cell::RefCell; - use jsg::ExceptionType; use jsg::MemberRegistry; use jsg::Resource; @@ -65,7 +63,7 @@ impl Echo { /// new error, as an error that replaces whatever the call threw. #[jsg_resource] struct Caller { - callback: RefCell>>, + callback: jsg::RefCell>>, } #[jsg_resource] @@ -78,7 +76,7 @@ impl Caller { ) -> Result<(), jsg::Error> { let callback = v8::Local::::try_from(callback) .map_err(|_| jsg::Error::new_type_error("not a function"))?; - *self.callback.borrow_mut() = Some(v8::TracedGlobal::new(lock, callback)); + *self.callback.borrow_mut(lock) = Some(v8::TracedGlobal::new(lock, callback)); Ok(()) } @@ -410,7 +408,7 @@ fn error_does_not_replace_termination() { lock, "caller", jsg::Rc::new(Caller { - callback: RefCell::new(None), + callback: jsg::RefCell::new(None), }), ); ctx.set_global(lock, "panicker", panicker()); diff --git a/src/rust/jsg/README.md b/src/rust/jsg/README.md index 16ad52daa11..f202aae0719 100644 --- a/src/rust/jsg/README.md +++ b/src/rust/jsg/README.md @@ -178,41 +178,52 @@ GC owner that traces it: a resource, or a `Wrappable` such as the data of a func of the wrapper detached, as a moved C++ `jsg::Ref` is: every `jsg::Rc` and `TracedGlobal` in it is strong again, with no owner, so the reaction can keep it or put it in another owner (see `GcVisitor::is_detaching`; `GcVisitor::as_ffi` returns `None` for such a visit, which a `Traced` -impl that visits through it must handle). +impl that visits through it must handle). The `jsg::` cells below detach a value the same way. - Never put traced state inside an `std::rc::Rc`. A clone of the `Rc` held by a second GC owner - traces the same state again. For a `jsg::Rc`, C++ asserts a single owner + would trace the same state again, so `std::rc::Rc` is `Traced` only for types that hold no GC + references (`jsg::HoldsNoGcReferences`). For a `jsg::Rc`, C++ asserts a single owner (`Wrappable::visitRef` in `src/workerd/jsg/wrappable.c++`), so the process aborts. For a `TracedGlobal`, nothing asserts, but whichever owner traces it last decides whether its handle is strong or weak. The value can then be collected while the other owner still uses it. To give a callback or another party access to the state, give it a `jsg::Rc` or `jsg::Weak` to the resource that owns the state. A method gets one with a `this: jsg::Rc` parameter; see [Reaching the resource from a callback](#reaching-the-resource-from-a-callback). -- Clone a `jsg::Rc` that you move out of a traced field (with `Cell::take`, `Option::take` or - `mem::replace`, or by popping it from a traced `Vec`), and use the clone. Once GC has visited a - `jsg::Rc` in a field, the `jsg::Rc` gives up its strong reference and relies on the field's - owner to trace it. Moved out of the field, it is neither strong nor traced, so GC can collect - the resource's JavaScript wrapper while Rust still holds it. The resource then gets a new - wrapper the next time it reaches JavaScript, and nothing traces the `TracedGlobal`s it holds, - so they can lose their values. C++ `jsg::Ref` doesn't have this problem because its move - constructor makes the reference strong again, but Rust has no hook on moves. A clone always - starts strong. Clone it right away, or at least before anything that can run JavaScript or - allocate on the V8 heap. See [#7976](https://github.com/cloudflare/workerd/issues/7976). +- Keep a `jsg::Rc` or a `TracedGlobal` that a resource moves out of a field in a `jsg::Cell` or + a `jsg::RefCell`, not in a `std::cell::Cell` or `RefCell`. Once GC has visited a `jsg::Rc` in + a field, the `jsg::Rc` gives up its strong reference and relies on the field's owner to trace + it; a `TracedGlobal` makes its handle weak the same way. Moved out of the field by a bitwise + move, the reference is neither strong nor traced, so GC can collect the resource's JavaScript + wrapper, or the `TracedGlobal`'s value, while Rust still holds it. The resource then gets a + new wrapper the next time it reaches JavaScript, and nothing traces the `TracedGlobal`s it + holds, so they can lose their values. C++ `jsg::Ref` doesn't have this problem because its + move constructor makes the reference strong again, but Rust has no hook on moves. + `jsg::Cell::take`, `jsg::Cell::replace` and `jsg::RefCell::borrow_mut` detach everything in + the cell before it can leave, which makes it strong; see [Cells](#cells). A resource is + reached only through `&self`, so a cell is the only way to move a reference out of a field, + and the std cells are `Traced` only for types that hold no GC references + (`jsg::HoldsNoGcReferences`). The exception is a resource's `Drop` impl, which has + `&mut self`: a reference it moves out of a field and keeps must be cloned or dropped there. + See [#7976](https://github.com/cloudflare/workerd/issues/7976). - Don't put a `jsg::v8::Global` in anything GC traces. A `Global` is always strong, so if it points back at its owner (for example function data that holds the function itself), neither is ever collected. Use a `TracedGlobal`. The compiler rejects a `Global` field in a resource, but not inside a type with a hand-written `impl Traced`, so check those by hand. Bad: two `Function::new` callbacks whose data are clones of one `std::rc::Rc`, where -`State` holds a `TracedGlobal`. Fix: a resource holds `State`, and each callback holds a -`jsg::Rc` or `jsg::Weak` to that resource. +`State` holds a `TracedGlobal`, which doesn't compile. Fix: a resource holds `State`, and each +callback holds a `jsg::Rc` or `jsg::Weak` to that resource. -Bad: `let task = self.pending.take().unwrap();`, then a call into JavaScript before `task` is -used. Fix: `let task = self.pending.take().unwrap().clone();`. +Bad: `pending: Cell>>` or +`callback: Cell>>`, which don't compile. Fix: +`pending: jsg::Cell>>`, taken out with `self.pending.take(lock)`. -Tested by: the `global-is-not-traced` compile test, for a `Global` field in a resource. No test -checks for traced state behind an `std::rc::Rc`, or for a `jsg::Rc` used after it was moved out -of a field; reviewers check those. +Tested by: the `global-is-not-traced` compile test, for a `Global` field in a resource, and the +`gc-reference-in-std-cell-is-not-traced` and `gc-reference-in-std-rc-is-not-traced` compile +tests, for a `jsg::Rc` or a `TracedGlobal` in a std cell or a std `Rc`. No test checks a +hand-written `impl Traced` that shares traced state some other way or reaches it through +another kind of cell, a std cell included, or a hand-written `impl HoldsNoGcReferences` on a type +whose `trace` visits something; reviewers check those. ### 3. Don't build on C++ JSG @@ -293,7 +304,6 @@ Rust resources integrate with V8's garbage collector through the existing C++ `W ```rust use jsg_macros::{jsg_resource, jsg_method}; -use std::cell::Cell; #[jsg_resource] struct MyResource { @@ -311,8 +321,8 @@ struct MyResource { // jsg::v8::TracedGlobal fields — JS value traced with strong↔weak dual-mode switching. // Allows GC to detect and collect back-reference cycles (e.g. a stored callback // that closes over the resource's own JS wrapper). - // Must be wrapped in Cell<_> for interior mutability (trace takes &self). - callback: Cell>>, + // Must be wrapped in jsg::Cell<_> for interior mutability (trace takes &self). + callback: jsg::Cell>>, } #[jsg_resource] @@ -355,17 +365,18 @@ struct Task { impl Task { /// Runs the task later: `scheduler` keeps a `jsg::Rc` of it until then. #[jsg_method] - fn schedule(this: jsg::Rc, scheduler: jsg::Rc) { + fn schedule(this: jsg::Rc, lock: &mut jsg::Lock<'_>, scheduler: jsg::Rc) { // `this` dereferences to the resource, like `&self`. this.scheduled.set(true); - scheduler.tasks.borrow_mut().push(this); + scheduler.tasks.borrow_mut(lock).push(this); } } ``` Whatever holds the `jsg::Rc` must visit it in its `Traced::trace`, just as a resource visits -its `jsg::Rc` fields. When it later moves the `jsg::Rc` out of that field, for example to run -the task, it must clone it before use. +its `jsg::Rc` fields. A field it later moves the `jsg::Rc` out of, for example to run the +task, is a `jsg::Cell` or `jsg::RefCell` (here `tasks: jsg::RefCell>>`), +which make the `jsg::Rc` strong on the way out; see [Cells](#cells). Don't give the callback the resource's state through an `std::rc::Rc` instead. Both rules are explained in [Safety rules](#safety-rules), rule 2. @@ -379,6 +390,57 @@ it a `jsg::Weak` (`this.downgrade()`) instead. Calling the method on something that isn't a resource of this type throws a `TypeError`, as it does for `&self` methods. A method that takes `this` has no Fast API callback. +### Cells + +A resource's methods take `&self`, so state that changes after construction lives in a cell. +For traced state, that is a `jsg::Cell` or a `jsg::RefCell`: + +- `jsg::Cell` moves its value in and out whole, like `std::cell::Cell`: `set(value)`, + `take(lock)` and `replace(lock, value)`. +- `jsg::RefCell` hands out borrows, like `std::cell::RefCell`: `borrow()` and + `borrow_mut(lock)`. + +`take`, `replace` and `borrow_mut` take the lock and detach what leaves the cell from its owner, as +a typed promise's reaction does with its value: every `jsg::Rc` and `TracedGlobal` in it is strong +again and forgets its owner. `borrow_mut` detaches the whole value, since the borrower can move any +of it out. GC visits a `jsg::Rc` or a `TracedGlobal` where it finds it, and once the owner is +reachable only through its wrapper the visit makes the reference weak and leaves it to the owner's +trace. Moved out of the owner by a plain Rust move, the reference would be neither strong nor +traced. The cells make it strong before it can leave, so whatever a method takes out, or pops from a +borrowed `Vec`, stays valid for as long as the method holds it. A reference that stays in the cell +is made weak again by the next GC visit. Everything in a mutably borrowed `jsg::RefCell` is strong, +so a GC that runs during the borrow, which cannot look inside, loses nothing. + +```rust +#[jsg_resource] +struct Scheduler { + pending: jsg::Cell>>, + queue: jsg::RefCell>>, +} + +#[jsg_resource] +impl Scheduler { + #[jsg_method] + fn run(&self, lock: &mut jsg::Lock<'_>) { + if let Some(task) = self.pending.take(lock) { + task.run(lock); // `task` is strong, whatever ran in between + } + // Moved out of the borrow first, so that `run`, which can run JavaScript, doesn't find + // the queue borrowed. + let tasks = std::mem::take(&mut *self.queue.borrow_mut(lock)); + for task in tasks { + task.run(lock); + } + } +} +``` + +`std::cell::Cell` and `std::cell::RefCell` are `Traced` only for types that hold no GC +references, which the marker trait `jsg::HoldsNoGcReferences` marks; a `jsg::Rc` or a +`TracedGlobal` in one is a compile error. So a `jsg::Rc` or `TracedGlobal` that leaves a traced +field of a live resource is strong, unless a hand-written `impl Traced` reaches it through a std +cell; see [Safety rules](#safety-rules), rule 2, for the exceptions. + ### GC Behavior - **No JS wrapper**: Dropping the last `Rc` immediately destroys the resource (no GC needed). @@ -392,33 +454,34 @@ does for `&self` methods. A method that takes `this` has no Fast API callback. | `jsg::Nullable>` | Yes — when `Some` | | | `jsg::NullOrUndefined>` | Yes — when `Some` | | | `jsg::Lenient>` | Yes — when `Some` or `Unconvertable` | | -| `Cell>` | Yes — strong edge | Use `Cell` when field needs interior mutability | -| `Cell>>` | Yes — when `Some` | | -| `Cell>>` | Yes — when `Some` | | -| `Cell>>` | Yes — when `Some` | | +| `jsg::Cell>>` | Yes — when `Some` | Use `jsg::Cell` when the field is set after construction; `take` and `replace` make the `Rc` strong | +| `jsg::Cell>>` | Yes — when `Some` | | +| `jsg::Cell>>` | Yes — when `Some` | | +| `jsg::RefCell>>` | Yes — each element | Use `jsg::RefCell` when the field is changed in place; `borrow_mut` makes every `Rc` strong | +| `Cell` / `RefCell` where `T` holds a `jsg::Rc` or a `TracedGlobal` | Compile error — no `Traced` impl | Use `jsg::Cell` / `jsg::RefCell` ([Cells](#cells)) | | `Vec>` | Yes — each element | Iterates and visits every `Rc` in the vec | | `HashMap>` | Yes — each value | Iterates `.values()` and visits each `Rc` | | `BTreeMap>` | Yes — each value | Iterates `.values()` and visits each `Rc` | | `jsg::Dict>` | Yes — each value | Visits the value of every entry | | `HashSet>` | Yes — each element | Iterates and visits every `Rc` | | `BTreeSet>` | Yes — each element | Iterates and visits every `Rc` | -| `Cell>>` | Yes — each element | `Cell` variant of above | -| `Cell>>` | Yes — each value | `Cell` variant of above | -| Same patterns with `jsg::v8::TracedGlobal` | Yes | All collection forms work with `TracedGlobal` too | +| `jsg::Cell>>` | Yes — each element | `jsg::Cell` variant of above | +| `jsg::Cell>>` | Yes — each value | `jsg::Cell` variant of above | +| Same patterns with `jsg::v8::TracedGlobal` | Yes | All collection forms work with `TracedGlobal` too, in `jsg::` cells | | `jsg::v8::TracedGlobal` | Yes — dual strong/traced | Enables cycle collection; see below | | `Option>` | Yes — when `Some` | | | `jsg::Nullable>` | Yes — when `Some` | | | `jsg::NullOrUndefined>` | Yes — when `Some` | | -| `Cell>` | Yes — dual strong/traced | Required when set after construction | -| `Cell>>` | Yes — when `Some` | | +| `jsg::Cell>` | Yes — dual strong/traced | Required when set after construction; `replace` makes the handle strong again | +| `jsg::Cell>>` | Yes — when `Some` | | | `jsg::Weak` | No | Doesn't keep target alive | | `jsg::v8::Global` | Compile error — no `Traced` impl | Use `TracedGlobal` in resource fields | | `jsg::TracedAsyncContextFrame` | Yes — like `TracedGlobal` | Nothing to trace for the root frame | | `jsg::AsyncContextFrame` | Compile error — no `Traced` impl | Use `TracedAsyncContextFrame` in resource fields | -| `std::rc::Rc` | Depends on `T` | Only for plain data; never around a `TracedGlobal` or `jsg::Rc` (Safety rules, rule 2) | +| `std::rc::Rc` where `T` holds a `jsg::Rc` or a `TracedGlobal` | Compile error — no `Traced` impl | Share a resource through a `jsg::Rc` or `jsg::Weak` instead (Safety rules, rule 2) | | Any `T: Traced` | Depends on `T` | `#[jsg_resource]` calls `Traced::trace` on every field | -- **`Cell` for interior mutability**: `Traced::trace` takes `&self`. Fields that need to be mutated after construction (e.g. a callback set in a method) can use `Cell`. `Cell` implements `Traced` by reading through `as_ptr()` during single-threaded GC tracing. +- **Cells for interior mutability**: `Traced::trace` takes `&self`. Fields that need to be mutated after construction (e.g. a callback set in a method) use a cell: `jsg::Cell` or `jsg::RefCell` for a field that holds a `jsg::Rc` or a `TracedGlobal`, and `std::cell::Cell` or `RefCell` for anything else; see [Cells](#cells). - **`Traced` drives field tracing**: `#[jsg_resource]` now traces every named field via `Traced::trace(&self.field, visitor)`. Types with no GC edges use no-op `Traced` impls; wrappers/collections delegate recursively. - **Nested wrappers are supported by composition**: `Option>>` works as long as each layer implements `Traced`. - **`#[jsg_resource(custom_trace)]`**: suppresses the generated `Traced` impl so you can write your own. `GarbageCollected` (`memory_name`), `Type`, `ToJS`, and `FromJS` are still generated. @@ -613,35 +676,36 @@ and it is dropped like a `Global`. A `TracedGlobal` is `Send`, and not `Sync`: e both leave its handles alone on a thread that does not hold the isolate lock. A `TracedGlobal` in traced mode also holds a `v8::TracedReference`, which Rust never resets. A -`TracedGlobal` taken out of its resource, for instance by `Cell::take()`, is traced by nothing, so -the next full GC may free its node, and nothing tells the `TracedGlobal` that it moved, the way C++ -JSG's `jsg::Data` move constructor re-strengthens its handle. So each GC visit lets go of the old -node and makes the strong handle strong again before C++ decides afresh whether to install a new -node, and a drop just lets go of it. V8 frees nodes that nothing traces at the next full GC; see -`GcVisitor::visit_traced_global` for the cost of this and how to avoid it. +`TracedGlobal` taken out of its resource is traced by nothing, so the next full GC may free its +node, and nothing tells the `TracedGlobal` that it moved, the way C++ JSG's `jsg::Data` move +constructor re-strengthens its handle. So each GC visit lets go of the old node and makes the +strong handle strong again before C++ decides afresh whether to install a new node, and a drop +just lets go of it. A detaching visit, which `jsg::Cell::take` and the other ways out of a +`jsg::` cell run, does the same, so a `TracedGlobal` leaves its resource strong. V8 frees nodes +that nothing traces at the next full GC; see `GcVisitor::visit_traced_global` for the cost of +this and how to avoid it. Bitwise moves are also why a `TracedGlobal` keeps its handles in a box. In traced mode the strong -handle is weak, and V8 keeps the address of its slot to null it when the value dies; the box -keeps that address valid however the `TracedGlobal` itself moves, for example when the `Vec` holding -it grows. The box lives until the strong handle has been reset. GC can still run after the -`Realm` is dropped, so the box of a `TracedGlobal` dropped then is kept until the isolate is -disposed. - -A `TracedGlobal` taken out of its resource is still weak, and nothing traces its value: unless -something else keeps the value alive, any GC from then on may free it, which leaves the -`TracedGlobal` empty, and `as_local` on it panics. Put the `TracedGlobal` back, or hold the value -through another handle, before a GC can run. Tracing likewise skips a `RefCell` while it is -mutably borrowed, so the `TracedGlobal`s in it are untraced for as long as the borrow lasts; debug -builds abort when that happens. +handle is weak, and V8 keeps the address of its slot to null it when the value dies; the box keeps +that address valid however the `TracedGlobal` itself moves. A `TracedGlobal` leaves a `jsg::` cell +strong, and a `std::cell::Cell` or `RefCell` cannot hold one, so a weak `TracedGlobal` moves only on +its way to being dropped (`jsg::Cell::set`) or in a resource's `Drop` impl, which has `&mut self`. +The box lives until the strong handle has been reset. GC can still run after the `Realm` is dropped, +so the box of a `TracedGlobal` dropped then is kept until the isolate is disposed. + +A `TracedGlobal` taken out of a `jsg::Cell` with `take(lock)` or `replace(lock, ..)`, or moved +out of a mutably borrowed `jsg::RefCell`, is strong and keeps its value; see [Cells](#cells). +Tracing skips a `jsg::RefCell` while it is mutably borrowed, which is safe because `borrow_mut` +made everything in it strong. `TracedGlobal` fields on `#[jsg_resource]` structs participate in GC tracing when visited via `GcVisitor::visit_traced_global`. This enables the garbage collector to detect and collect back-reference cycles — for example, a resource that stores a JS callback which closes over the resource's own JS wrapper: ```rust #[jsg_resource] struct EventEmitter { - // Cell> for interior mutability: the callback is set after + // jsg::Cell> for interior mutability: the callback is set after // construction, and trace() receives &self. - on_event: Cell>>, + on_event: jsg::Cell>>, } #[jsg_resource] @@ -748,9 +812,9 @@ As with `Global` and `TracedGlobal`, there are two types: so that GC collects a cycle through the frame's values. It is `Send`, not `Sync`, and must have exactly one owner that GC traces. `TracedAsyncContextFrame::from` converts an `AsyncContextFrame`, and `to_async_context_frame(lock)` goes the other way, for example to pass - a resource's frame to async I/O. Like a `TracedGlobal` taken out of its resource, a - `TracedAsyncContextFrame` taken out of its field is traced by nothing: enter, clone or convert - it before anything can run JavaScript or allocate on the V8 heap, and put it back. + a resource's frame to async I/O. Like a `TracedGlobal`, a `TracedAsyncContextFrame` that a + resource changes after construction lives in a `jsg::Cell` or `jsg::RefCell`, which make it + strong as it leaves; see [Cells](#cells). ```rust let frame = jsg::AsyncContextFrame::current(lock); diff --git a/src/rust/jsg/async_context.rs b/src/rust/jsg/async_context.rs index 06ad4ec5070..29f7c9fc277 100644 --- a/src/rust/jsg/async_context.rs +++ b/src/rust/jsg/async_context.rs @@ -106,9 +106,9 @@ impl AsyncContextFrame { /// to pass a resource's frame to async I/O. /// /// Once the resource has no strong reference from Rust, the garbage collector keeps the frame -/// alive only by tracing it through the resource, like any `TracedGlobal`. A frame taken out of -/// its field, for example with `Cell::take`, is traced by nothing, so enter it, clone it or -/// convert it before anything can run JavaScript or allocate on the V8 heap, and put it back. +/// alive only by tracing it through the resource, like any `TracedGlobal`. A field that the frame +/// is taken out of is a [`crate::Cell`] or [`crate::RefCell`], which make it strong as it leaves, +/// so it stays alive however long Rust holds it. pub struct TracedAsyncContextFrame { /// The frame's wrapper, or `None` for the root frame. value: Option>, diff --git a/src/rust/jsg/cell.rs b/src/rust/jsg/cell.rs new file mode 100644 index 00000000000..163d3352134 --- /dev/null +++ b/src/rust/jsg/cell.rs @@ -0,0 +1,195 @@ +// Copyright (c) 2026 Cloudflare, Inc. +// Licensed under the Apache 2.0 license found in the LICENSE file or at: +// https://opensource.org/licenses/Apache-2.0 + +#![allow( + unsafe_code, + reason = "reads the contents of a `std::cell::Cell` through its pointer" +)] + +//! Cells for traced state that a resource changes after construction: a [`Cell`] and a +//! [`RefCell`] that make a reference strong before it leaves the cell. +//! +//! GC visits a `jsg::Rc` or a `TracedGlobal` where it finds it. Once the owner of the field is +//! reachable only through its JavaScript wrapper, the visit makes the reference weak and relies +//! on the owner's trace to keep its target alive. Rust moves are bitwise, so a reference moved +//! out of the field, as `std::cell::Cell::take` or `std::cell::RefCell::borrow_mut` would do, +//! stays weak, and the owner no longer traces it: the next GC can collect the wrapper of a +//! `jsg::Rc`'s resource, or the value of a `TracedGlobal`. C++ `jsg::Ref` makes a moved +//! reference strong in its move constructor; these cells do it as a value leaves them instead, +//! with a detaching visit (see [`GcVisitor::is_detaching`]). The std cells are `Traced` +//! only for types that hold no GC references ([`HoldsNoGcReferences`]), so these cells are the +//! only way out of a field of a live resource. + +use std::cell::BorrowError; +use std::cell::BorrowMutError; +use std::cell::Ref; +use std::cell::RefMut; + +use crate::HoldsNoGcReferences; +use crate::Lock; +use crate::Traced; +use crate::v8::GcVisitor; + +/// A cell for traced state in a resource, such as an `Option>`, whose value is +/// moved in and out whole. +/// +/// Like [`std::cell::Cell`], it gives its value up only by moving it out, with [`take`](Self::take) +/// or [`replace`](Self::replace). Those detach the value they return from the cell's owner (see +/// [`GcVisitor::is_detaching`]), which makes every `jsg::Rc` and `TracedGlobal` in it strong, so +/// one that GC has visited in the cell stays valid outside it. They take the lock, since making a +/// reference strong touches V8 handles. +/// +/// Use a [`RefCell`] to read or change the value in place. +pub struct Cell { + inner: std::cell::Cell, +} + +impl Cell { + /// A cell holding `value`. + pub const fn new(value: T) -> Self { + Self { + inner: std::cell::Cell::new(value), + } + } + + /// Replaces the value with `value` and drops the old one. + /// + /// Dropping a weak reference needs no fix-up, so this takes no lock. + pub fn set(&self, value: T) { + self.inner.set(value); + } + + /// Replaces the value with `value` and returns the old one, made strong. + /// + /// # Panics + /// Panics if the value holds a `TracedGlobal` of an isolate other than the lock's. + pub fn replace(&self, lock: &mut Lock<'_>, value: T) -> T { + // Detached once out of the cell: making a `jsg::Rc` to a resource with no wrapper strong + // visits that resource, which can lead back to this cell, and that visit must not find + // the old value here still weak. + let old = self.inner.replace(value); + GcVisitor::detach(lock, &old); + old + } + + /// Takes the value, made strong, and leaves `T::default()` behind. + /// + /// # Panics + /// Panics if the value holds a `TracedGlobal` of an isolate other than the lock's. + pub fn take(&self, lock: &mut Lock<'_>) -> T + where + T: Default, + { + self.replace(lock, T::default()) + } +} + +impl Traced for Cell { + fn holds_gc_references() -> bool { + T::holds_gc_references() + } + + fn trace(&self, visitor: &mut GcVisitor) { + // SAFETY: visits are single-threaded within an isolate, create only shared references, and + // write only through the interior mutability of the GC references they reach (`trace` + // changes nothing other than through the visitor). The cell hands out no reference to its + // value, and `replace` moves the value out before it runs a visit, so no visit overlaps a + // write to the cell. + unsafe { (*self.inner.as_ptr()).trace(visitor) } + } +} + +impl HoldsNoGcReferences for Cell {} + +impl Default for Cell { + fn default() -> Self { + Self::new(T::default()) + } +} + +/// A cell for traced state in a resource that is read or changed in place, such as a +/// `Vec>`. +/// +/// Like [`std::cell::RefCell`], it hands out shared and exclusive borrows of its value. Unlike +/// `std::cell::RefCell`, [`borrow_mut`](Self::borrow_mut) first detaches the value from the +/// cell's owner (see [`GcVisitor::is_detaching`]), which makes everything in it strong, so that +/// whatever the borrower moves out, for example by popping a `Vec`, is strong. It takes the lock +/// for that. The borrow itself does not hold the lock. +/// +/// Everything in a mutably borrowed cell is strong, so a GC visit that finds the cell borrowed +/// safely skips it. The references left in the cell are made weak again by the next visit that +/// finds the owner reachable only through its wrapper. Holding the borrow across a call that can +/// run JavaScript is still a mistake: a method that borrows the cell again panics. +pub struct RefCell { + inner: std::cell::RefCell, +} + +impl RefCell { + /// A cell holding `value`. + pub const fn new(value: T) -> Self { + Self { + inner: std::cell::RefCell::new(value), + } + } + + /// Borrows the value. + /// + /// # Panics + /// Panics if the value is mutably borrowed, as [`std::cell::RefCell::borrow`] does. + pub fn borrow(&self) -> Ref<'_, T> { + self.inner.borrow() + } + + /// Borrows the value, or fails if it is mutably borrowed. + pub fn try_borrow(&self) -> Result, BorrowError> { + self.inner.try_borrow() + } + + /// Mutably borrows the value, after making everything in it strong. That visits every + /// element of a collection, so make a batch of changes under one borrow. + /// + /// # Panics + /// Panics if the value is borrowed, as [`std::cell::RefCell::borrow_mut`] does, or if it holds + /// a `TracedGlobal` of an isolate other than the lock's. + pub fn borrow_mut(&self, lock: &mut Lock<'_>) -> RefMut<'_, T> { + let guard = self.inner.borrow_mut(); + GcVisitor::detach(lock, &*guard); + guard + } + + /// Mutably borrows the value, after making everything in it strong, or fails if it is + /// borrowed. + /// + /// # Panics + /// Panics if the value holds a `TracedGlobal` of an isolate other than the lock's. + pub fn try_borrow_mut(&self, lock: &mut Lock<'_>) -> Result, BorrowMutError> { + let guard = self.inner.try_borrow_mut()?; + GcVisitor::detach(lock, &*guard); + Ok(guard) + } +} + +impl Traced for RefCell { + fn holds_gc_references() -> bool { + T::holds_gc_references() + } + + fn trace(&self, visitor: &mut GcVisitor) { + // A mutably borrowed cell is skipped, by a detaching visit too: `borrow_mut` made + // everything in it strong, and anything the borrower puts in is strong too, so the visit + // has nothing to keep alive or to make strong. The next visit after the borrow ends makes + // the contents weak again where they should be. + if let Ok(inner) = self.inner.try_borrow() { + inner.trace(visitor); + } + } +} + +impl HoldsNoGcReferences for RefCell {} + +impl Default for RefCell { + fn default() -> Self { + Self::new(T::default()) + } +} diff --git a/src/rust/jsg/lib.rs b/src/rust/jsg/lib.rs index 917a9ef57f2..567508ec132 100644 --- a/src/rust/jsg/lib.rs +++ b/src/rust/jsg/lib.rs @@ -9,6 +9,7 @@ use std::sync::Arc; mod async_context; mod buffer; +mod cell; mod dict; pub mod fast_api; pub mod feature_flags; @@ -34,6 +35,8 @@ pub use async_context::AsyncContextFrame; pub use async_context::TracedAsyncContextFrame; pub use buffer::ToJsArrayBuffer; pub use buffer::ToJsTypedArray; +pub use cell::Cell; +pub use cell::RefCell; pub use dict::Dict; pub use feature_flags::FeatureFlags; use isolate_state::IsolateState; @@ -93,6 +96,7 @@ pub use v8::GcVisitor; pub use v8::IsolatePtr; pub use v8::ffi::ExceptionType; pub use wrappable::FromJS; +pub use wrappable::HoldsNoGcReferences; pub use wrappable::ToJS; pub use wrappable::Traced; diff --git a/src/rust/jsg/macros.rs b/src/rust/jsg/macros.rs index 0b6609e19e8..88c8865d184 100644 --- a/src/rust/jsg/macros.rs +++ b/src/rust/jsg/macros.rs @@ -7,7 +7,8 @@ /// /// Types that contain only plain data (no `jsg::Rc`, `jsg::v8::TracedGlobal`, etc.) do not /// need to participate in GC tracing. This macro produces a `Traced` impl whose -/// `trace` method is a no-op and whose `holds_gc_references` returns `false`. +/// `trace` method is a no-op and whose `holds_gc_references` returns `false`, and a +/// [`HoldsNoGcReferences`](crate::HoldsNoGcReferences) implementation. /// /// # Example /// @@ -24,7 +25,9 @@ macro_rules! jsg_traced { fn holds_gc_references() -> bool { false } - })* + } + + impl $crate::HoldsNoGcReferences for $t {})* }; } diff --git a/src/rust/jsg/opaque.rs b/src/rust/jsg/opaque.rs index b2b4b801796..3c0e0b4b595 100644 --- a/src/rust/jsg/opaque.rs +++ b/src/rust/jsg/opaque.rs @@ -17,13 +17,12 @@ )] use std::any::TypeId; -use std::cell::RefCell; use std::rc::Rc; use crate::GarbageCollected; use crate::Lock; +use crate::RefCell; use crate::Traced; -use crate::v8::GcVisitor; use crate::v8::Local; use crate::v8::Value; use crate::v8::WrappableRc; @@ -87,7 +86,6 @@ pub fn unwrap_opaque<'s, T: Traced + 'static>( // SAFETY: `wrappable` keeps the Wrappable, and with it the `Opaque`, alive while the reference // is used. Only shared references to the `Opaque` are ever created. let opaque = unsafe { opaque.as_ref() }; - let value = opaque.0.try_borrow_mut().ok()?.take()?; - GcVisitor::detach(lock, &value); - Some(value) + // The borrow detaches the value before it is taken. + opaque.0.try_borrow_mut(lock).ok()?.take() } diff --git a/src/rust/jsg/promise.rs b/src/rust/jsg/promise.rs index 512fcf85e00..d0c0c7b14d5 100644 --- a/src/rust/jsg/promise.rs +++ b/src/rust/jsg/promise.rs @@ -10,8 +10,6 @@ reason = "settles and reacts to promises through the V8 FFI" )] -use std::cell::RefCell; - use crate::Error; use crate::FromJS; use crate::Lock; @@ -177,7 +175,7 @@ impl Promise { /// The data of a promise's reactions, which the first of them to run drops once it returns, as /// C++ JSG drops a `ThenCatchPair`: a promise settles once, so the other can no longer run. Until /// then, the garbage collector traces it through the reactions. -struct ReactionData(RefCell>); +struct ReactionData(crate::RefCell>); impl Traced for ReactionData { fn holds_gc_references() -> bool { @@ -213,7 +211,7 @@ where None => unreachable!("a promise reaction ran after another reaction to the promise"), }; // Dropped after the borrow ends, so that what its drop does cannot find the cell borrowed. - let data = data.0.borrow_mut().take(); + let data = data.0.borrow_mut(lock).take(); drop(data); result } @@ -380,7 +378,7 @@ impl<'s> Local<'s, Promise> { { self.debug_assert_isolate(lock); let _ = (on_fulfilled, on_rejected); - let data = ReactionData(RefCell::new(Some(data))); + let data = ReactionData(crate::RefCell::new(Some(data))); let [on_fulfilled, on_rejected] = Function::new_sharing_data(lock, data, [reaction::(), reaction::()])?; // SAFETY: lock guarantees the isolate is locked and a HandleScope is active; all three diff --git a/src/rust/jsg/resource.rs b/src/rust/jsg/resource.rs index 014dde80cb5..c88e65ee0f7 100644 --- a/src/rust/jsg/resource.rs +++ b/src/rust/jsg/resource.rs @@ -53,11 +53,13 @@ use crate::v8::ffi::Wrappable; /// declaration order. /// /// **Moving out of a traced field:** once GC has visited an `Rc` in a traced field, the `Rc` -/// gives up its strong reference and relies on the field's owner to trace it. An `Rc` moved out -/// of the field (`Cell::take`, `Option::take`, `mem::replace`) is then neither strong nor traced, -/// unlike a moved C++ `jsg::Ref`. Clone it and use the clone, which starts strong; see -/// `README.md` §Safety rules, rule 2, and -/// . +/// gives up its strong reference and relies on the field's owner to trace it. An `Rc` moved +/// out of the field by a bitwise move is then neither strong nor traced, unlike a moved C++ +/// `jsg::Ref`, whose move constructor makes it strong again. So a field that an `Rc` is moved +/// out of is a [`crate::Cell`] or [`crate::RefCell`], which make it strong before it leaves +/// the cell, as [`v8::GcVisitor::is_detaching`] describes; `std::cell::Cell` and +/// `std::cell::RefCell` are not `Traced` for types that hold an `Rc`. See `README.md` §Safety +/// rules, rule 2, and . pub struct Rc { /// Shared ownership of the Rust resource. handle: std::rc::Rc, @@ -376,6 +378,8 @@ impl Traced for Weak { } } +impl crate::HoldsNoGcReferences for Weak {} + impl GarbageCollected for Weak { fn memory_name(&self) -> &'static std::ffi::CStr { // jsgGetMemoryName is only called on live Wrappables, never on Weak. diff --git a/src/rust/jsg/v8.rs b/src/rust/jsg/v8.rs index 91a5b0642c4..ed421d84f25 100644 --- a/src/rust/jsg/v8.rs +++ b/src/rust/jsg/v8.rs @@ -4585,8 +4585,9 @@ pub struct GcVisitor { pub(crate) enum VisitMode { /// For the C++ `jsg::GcVisitor` of its owner's visit. Visit, - /// To detach the value from its owner; see [`GcVisitor::is_detaching`]. - Detach, + /// To detach the value from its owner, under the lock of the given isolate; see + /// [`GcVisitor::is_detaching`]. + Detach(*mut ffi::Isolate), } impl GcVisitor { @@ -4610,10 +4611,13 @@ impl GcVisitor { /// forgets its parent, and every `TracedGlobal` it traces lets go of its traced reference and /// makes its strong handle strong. The value is then safe to use and to put in another owner, /// however the garbage collector last visited it. - pub(crate) fn detach(_lock: &mut Lock<'_>, value: &T) { + /// + /// # Panics + /// Panics if `value` holds a `TracedGlobal` of an isolate other than the lock's. + pub(crate) fn detach(lock: &Lock<'_>, value: &T) { let mut visitor = Self { handle: ffi::GcVisitor { ptr: 0 }, - mode: VisitMode::Detach, + mode: VisitMode::Detach(lock.isolate().as_ffi()), }; value.trace(&mut visitor); } @@ -4623,7 +4627,7 @@ impl GcVisitor { /// `None`: a `Traced` impl that visits through it must instead make its reference strong /// again and forget its parent, as a moved C++ `jsg::Ref` does. pub fn is_detaching(&self) -> bool { - self.mode == VisitMode::Detach + matches!(self.mode, VisitMode::Detach(_)) } /// Visits a `jsg::Rc` field during GC tracing. @@ -4658,7 +4662,7 @@ impl GcVisitor { /// # Traced nodes /// /// Rust never resets a traced reference. A `TracedGlobal` taken out of its resource - /// (`Cell::take`, `mem::replace`) is traced by nothing until it is put back, and nothing + /// (in its `Drop` impl, say) is traced by nothing until it is put back, and nothing /// tells it that it moved, as a C++ move constructor would, so the next full GC may free /// its node while it still points to it. Every visit therefore first lets go of the traced /// reference: it clears the pointer without a reset, and makes the strong handle strong @@ -4677,9 +4681,10 @@ impl GcVisitor { /// node seen live before the collection in progress is still there until its marking /// completes. /// - /// A visit after the `TracedGlobal`'s isolate is closed does nothing, and neither does a visit - /// on behalf of another isolate, which debug builds assert against: a `TracedGlobal` belongs in - /// resources of its own isolate. A visit has no isolate when its resource has neither been + /// A visit after the `TracedGlobal`'s isolate is closed does nothing, and neither does a GC + /// visit on behalf of another isolate, which debug builds assert against; a detaching visit + /// under another isolate's lock panics. A `TracedGlobal` belongs in resources of its own + /// isolate. A visit has no isolate when its resource has neither been /// wrapped nor reached by a trace. Such a visit is not part of a GC: it comes from the /// resource's first strong reference, which keeps the handle strong, and `jsg::Rc::new` /// makes that reference without a `Lock`, possibly on a thread without the isolate lock. @@ -4698,13 +4703,21 @@ impl GcVisitor { /// last visit wins, and the value can be collected while the other owner still uses it. So /// a `TracedGlobal` must have exactly one GC owner, and must not be kept behind an /// `std::rc::Rc`; see `README.md` §Safety rules, rule 2. + /// + /// # Panics + /// Panics in a detaching visit (see [`GcVisitor::is_detaching`]) if `global` belongs to an + /// isolate other than the lock's. pub fn visit_traced_global(&mut self, global: &TracedGlobal) { if global.state.is_closed() { // The isolate is being torn down, and its handles are left to `Dispose`, as those // of a dropped `TracedGlobal` are. return; } - if self.is_detaching() { + if let VisitMode::Detach(isolate) = self.mode { + assert!( + global.state.belongs_to(isolate), + "attempt to use a Global or TracedGlobal in an isolate other than the one it was created in" + ); // SAFETY: see below, and `GlobalSlot`. let (strong, traced) = unsafe { ( @@ -4714,7 +4727,8 @@ impl GcVisitor { }; if traced.ptr != 0 { traced.ptr = 0; - // SAFETY: the strong handle is valid, and `detach` runs under the isolate lock. + // SAFETY: the strong handle is valid, and `detach` runs under the lock of its + // isolate, as checked above. unsafe { ffi::global_clear_weak(strong) }; } return; diff --git a/src/rust/jsg/wrappable-test.rs b/src/rust/jsg/wrappable-test.rs index 731db8b71e6..402a321cbaf 100644 --- a/src/rust/jsg/wrappable-test.rs +++ b/src/rust/jsg/wrappable-test.rs @@ -8,7 +8,6 @@ use std::collections::BTreeMap; use std::collections::BTreeSet; use std::collections::HashMap; use std::collections::HashSet; -use std::panic::AssertUnwindSafe; use super::*; @@ -40,9 +39,13 @@ fn holds_gc_references_follows_the_contents() { assert_forwards!(Vec); assert_forwards!(HashSet); assert_forwards!(BTreeSet); - assert_forwards!(Cell); - assert_forwards!(RefCell); - assert_forwards!(std::rc::Rc); + assert!(!>::holds_gc_references()); + assert!(!>::holds_gc_references()); + assert!(!>::holds_gc_references()); + assert!(>>::holds_gc_references()); + assert!(!>::holds_gc_references()); + assert!(>>::holds_gc_references()); + assert!(!>::holds_gc_references()); assert!(>>::holds_gc_references()); assert!(!>::holds_gc_references()); assert!(>>::holds_gc_references()); @@ -51,26 +54,25 @@ fn holds_gc_references_follows_the_contents() { assert!(>::holds_gc_references()); } +/// `HoldsNoGcReferences` follows the contents too. (That a `jsg::Rc`, a `TracedGlobal` or a +/// `Lenient` does not qualify is checked by the `gc-reference-in-std-cell-is-not-traced` +/// compile test.) #[test] -fn mutably_borrowed_refcell_without_gc_references_is_skipped() { - let cell = RefCell::new(String::from("label")); - let _borrow = cell.borrow_mut(); - cell.trace(&mut unused_visitor()); +fn holds_no_gc_references_follows_the_contents() { + fn holds_no_gc_references() {} + holds_no_gc_references::(); + holds_no_gc_references::>(); + holds_no_gc_references::>>(); + holds_no_gc_references::>>(); + holds_no_gc_references::>>(); + holds_no_gc_references::>(); + holds_no_gc_references::>>(); + holds_no_gc_references::>>(); } #[test] -#[cfg_attr( - not(debug_assertions), - ignore = "the borrow check is a debug assertion" -)] -fn mutably_borrowed_refcell_with_gc_references_panics_in_debug_builds() { - let cell: RefCell>> = RefCell::new(Vec::new()); +fn mutably_borrowed_refcell_without_gc_references_is_skipped() { + let cell = RefCell::new(String::from("label")); let _borrow = cell.borrow_mut(); - let payload = std::panic::catch_unwind(AssertUnwindSafe(|| { - cell.trace(&mut unused_visitor()); - })) - .expect_err("tracing a mutably borrowed RefCell of TracedGlobals must panic"); - assert!( - crate::panic_message(&*payload).starts_with("tracing found a mutably borrowed RefCell") - ); + cell.trace(&mut unused_visitor()); } diff --git a/src/rust/jsg/wrappable.rs b/src/rust/jsg/wrappable.rs index 28da728f676..b5f8bf693ae 100644 --- a/src/rust/jsg/wrappable.rs +++ b/src/rust/jsg/wrappable.rs @@ -111,16 +111,19 @@ use crate::v8; /// - **`Option`**, **`Nullable`**, **`NullOrUndefined`** — delegates to the inner value if present. /// - **Collections** (`Vec`, `Dict`, `HashMap`, `BTreeMap`, `HashSet`, /// `BTreeSet`) — iterates elements/values and traces each. -/// - **`Cell`** — reads through `as_ptr()` (sound under single-threaded GC). -/// - **`RefCell`** — traces through `try_borrow()`, so a mutably borrowed cell is skipped; -/// debug builds abort if it can hold GC references. +/// - **`std::cell::Cell`**, **`std::cell::RefCell`** — only for +/// `T: HoldsNoGcReferences`, so there is nothing in them to visit; see +/// [`HoldsNoGcReferences`]. +/// - **[`jsg::Cell`](crate::Cell)**, **[`jsg::RefCell`](crate::RefCell)** — delegate to the +/// value, and detach it from the cell's owner as it leaves the cell; see +/// [`GcVisitor::is_detaching`](crate::v8::GcVisitor::is_detaching). /// - **`jsg::Rc`** — visits the reference via `GcVisitor::visit_rc`. /// - **`jsg::Weak`** — no-op (weak refs don't keep targets alive). /// - **`jsg::v8::TracedGlobal`** — visits via `GcVisitor::visit_traced_global`. pub trait Traced { - /// Whether a value of this type can hold GC-visible references. Tracing a `RefCell` checks - /// it. It defaults to `true`, and wrappers and collections take it from what they hold. A - /// type whose `trace` is a no-op should return `false`, as `jsg_traced!` does. + /// Whether a value of this type can hold GC-visible references. It defaults to `true`, and + /// wrappers and collections take it from what they hold. A type whose `trace` is a no-op + /// should return `false`, as `jsg_traced!` does. fn holds_gc_references() -> bool where Self: Sized, @@ -140,10 +143,31 @@ pub trait Traced { /// not hold a [`v8::Global`](crate::v8::Global), which is always strong, so a cycle through /// it is never collected. Don't keep a type that holds GC references behind an /// `std::rc::Rc`, since a clone held by another GC owner would trace it too; see - /// `README.md` §Safety rules, rule 2. + /// `README.md` §Safety rules, rule 2. Other than through `visitor`, it must not change the + /// value or anything that holds it: a [`crate::Cell`] visits its value in place. fn trace(&self, _visitor: &mut crate::v8::GcVisitor) {} } +/// Marks a `Traced` type that holds no GC references: no `jsg::Rc` and no `TracedGlobal` that +/// its `trace` visits, directly or inside another type. (The keys of a map are not visited.) +/// +/// `std::cell::Cell` and `std::cell::RefCell` are `Traced` only for +/// `T: HoldsNoGcReferences`. A reference that GC has visited in a field is weak and depends on +/// the field's owner to trace it, and a move out of a std cell is bitwise and leaves it that +/// way. A resource is reached only through `&self`, except in its `Drop` impl, so a cell is the +/// only way to move a reference out of a field of a live resource. With the std cells closed to +/// them, every `jsg::Rc` and `TracedGlobal` that leaves a traced field of a live resource is +/// strong, unless a hand-written `impl Traced` reaches it through a std cell. A field that holds one and changes +/// after construction is a [`crate::Cell`] or [`crate::RefCell`], which take any `Traced` type. +/// +/// Every built-in `Traced` type implements it, except those that can hold GC references: +/// `jsg::Rc`, `TracedGlobal`, `Lenient`, `TracedPromise`, `TracedPromiseResolver` and +/// `TracedAsyncContextFrame`. Wrappers and collections implement it when their contents do. +/// `jsg_traced!` implements it for the types it covers; `#[jsg_struct]` and `#[jsg_oneof]` don't, +/// since their fields can hold GC references. Implement it for a hand-written `Traced` type, or +/// a `#[jsg_struct]` of plain data, only if its `trace` visits nothing. +pub trait HoldsNoGcReferences: Traced {} + /// Generates no-op `Traced` implementations for types with no GC-visible references. macro_rules! impl_traced_noop { ($($t:ty),* $(,)?) => { @@ -151,7 +175,9 @@ macro_rules! impl_traced_noop { fn holds_gc_references() -> bool { false } - })* + } + + impl HoldsNoGcReferences for $t {})* }; } @@ -189,6 +215,8 @@ impl Traced for Option { } } +impl HoldsNoGcReferences for Option {} + impl Traced for Lenient { // An unconvertable value is a `TracedGlobal`, whatever `T` is. fn holds_gc_references() -> bool { @@ -217,6 +245,8 @@ impl Traced for Nullable { } } +impl HoldsNoGcReferences for Nullable {} + impl Traced for NullOrUndefined { fn holds_gc_references() -> bool { T::holds_gc_references() @@ -229,6 +259,8 @@ impl Traced for NullOrUndefined { } } +impl HoldsNoGcReferences for NullOrUndefined {} + impl Traced for NonCoercible { fn holds_gc_references() -> bool { T::holds_gc_references() @@ -239,6 +271,8 @@ impl Traced for NonCoercible { } } +impl HoldsNoGcReferences for NonCoercible {} + impl Traced for Vec { fn holds_gc_references() -> bool { T::holds_gc_references() @@ -251,6 +285,8 @@ impl Traced for Vec { } } +impl HoldsNoGcReferences for Vec {} + impl Traced for Dict { fn holds_gc_references() -> bool { V::holds_gc_references() @@ -263,6 +299,8 @@ impl Traced for Dict { } } +impl HoldsNoGcReferences for Dict {} + impl Traced for std::collections::HashMap { fn holds_gc_references() -> bool { V::holds_gc_references() @@ -275,6 +313,11 @@ impl Traced for std::collections::HashM } } +impl HoldsNoGcReferences + for std::collections::HashMap +{ +} + impl Traced for std::collections::BTreeMap { fn holds_gc_references() -> bool { V::holds_gc_references() @@ -287,6 +330,8 @@ impl Traced for std::collections::BTreeMap { } } +impl HoldsNoGcReferences for std::collections::BTreeMap {} + impl Traced for std::collections::HashSet { fn holds_gc_references() -> bool { T::holds_gc_references() @@ -299,6 +344,11 @@ impl Traced for std::collections::HashSet< } } +impl HoldsNoGcReferences + for std::collections::HashSet +{ +} + impl Traced for std::collections::BTreeSet { fn holds_gc_references() -> bool { T::holds_gc_references() @@ -311,7 +361,11 @@ impl Traced for std::collections::BTreeSet { } } -impl Traced for std::cell::Cell { +impl HoldsNoGcReferences for std::collections::BTreeSet {} + +/// Only for `T: HoldsNoGcReferences`: a `jsg::Rc` or `TracedGlobal` taken out of a std `Cell` +/// would be neither strong nor traced. A cell that holds one is a [`crate::Cell`]. +impl Traced for std::cell::Cell { fn holds_gc_references() -> bool { T::holds_gc_references() } @@ -326,40 +380,33 @@ impl Traced for std::cell::Cell { } } -impl Traced for std::cell::RefCell { +impl HoldsNoGcReferences for std::cell::Cell {} + +/// Only for `T: HoldsNoGcReferences`: a `jsg::Rc` or `TracedGlobal` moved out through a std +/// `RefCell`'s mutable borrow would be neither strong nor traced. A cell that holds one is a +/// [`crate::RefCell`]. +impl Traced for std::cell::RefCell { fn holds_gc_references() -> bool { T::holds_gc_references() } fn trace(&self, visitor: &mut crate::v8::GcVisitor) { - // A mutable borrow is active here only if the code holding it calls something that visits - // the resource: an allocation on the V8 heap, which can start a GC, or wrapping the - // resource, or creating its first or dropping its last strong `jsg::Rc` while it has no - // wrapper, which switch its references between strong and traced. The contents cannot be - // read then, so the visit misses their references. A missed GC visit can cost a weak - // `TracedGlobal` its value and a `jsg::Rc` the JS wrapper of its resource; a missed switch - // can leave a reference weak that should be strong, for a later GC to free, or strong that - // should be weak, which leaks. Debug builds abort when that can happen (a panic cannot - // unwind out of a visit made from C++); release builds skip the cell. - // - // The real fix is a JSG cell type whose mutable borrow takes the lock and makes the - // references it holds strong until the borrow ends, so that a visit can skip a borrowed - // cell safely. That also covers a `TracedGlobal` taken out of a `Cell`, which nothing - // traces. - let Ok(inner) = self.try_borrow() else { - debug_assert!( - !T::holds_gc_references(), - "tracing found a mutably borrowed RefCell that can hold GC references, which it \ - cannot visit; do not hold a RefCell borrow_mut() across a call that can allocate \ - on the V8 heap, wrap the resource, or create or drop a jsg::Rc to it" - ); - return; - }; - inner.trace(visitor); + // `T` holds no GC references, so a mutably borrowed cell can be skipped: a visit would + // find nothing in it. (`crate::RefCell` holds references, and makes them strong when it + // is mutably borrowed, so a visit can skip it too.) + if let Ok(inner) = self.try_borrow() { + inner.trace(visitor); + } } } -impl Traced for std::rc::Rc { +impl HoldsNoGcReferences for std::cell::RefCell {} + +/// Only for `T: HoldsNoGcReferences`: every owner of a shared `Rc` would trace what it holds, and +/// a GC reference has a single parent. A `jsg::Rc` visited by two parents asserts, as a C++ +/// `jsg::Ref` does, and a `TracedGlobal` would stay alive only while the last one to visit it +/// does. Share a resource through a `jsg::Rc` or `jsg::Weak` to it instead. +impl Traced for std::rc::Rc { fn holds_gc_references() -> bool { T::holds_gc_references() } @@ -369,6 +416,8 @@ impl Traced for std::rc::Rc { } } +impl HoldsNoGcReferences for std::rc::Rc {} + // ============================================================================= // ToJS trait (Rust → JavaScript) // ============================================================================= diff --git a/src/workerd/io/io-context/lib.rs b/src/workerd/io/io-context/lib.rs index 6c5365c7dc1..c3991c02369 100644 --- a/src/workerd/io/io-context/lib.rs +++ b/src/workerd/io/io-context/lib.rs @@ -393,6 +393,8 @@ impl jsg::Traced for CallbackOutcome { } } +impl jsg::HoldsNoGcReferences for CallbackOutcome {} + /// The reaction to the promise that [`IoContext::await_io`] resolves once its callback has /// returned: returns the callback's value, or throws its error. Throwing in a reaction gives the /// exception an async stack trace, as C++ `throwOrReturnResult()` does. @@ -469,7 +471,13 @@ impl IoOwned { } /// Holds no JavaScript handles; see [`IoContext::add_object`]. -impl jsg::Traced for IoOwned {} +impl jsg::Traced for IoOwned { + fn holds_gc_references() -> bool { + false + } +} + +impl jsg::HoldsNoGcReferences for IoOwned {} /// The reference to an [`IoOwned`]'s value that its `IoContext` holds. struct SlotRef(Arc>>); diff --git a/src/workerd/io/io-context/trace.rs b/src/workerd/io/io-context/trace.rs index 45f2febcccf..ca7e2d29555 100644 --- a/src/workerd/io/io-context/trace.rs +++ b/src/workerd/io/io-context/trace.rs @@ -165,10 +165,22 @@ impl TraceContext { } /// Holds no JavaScript handles. -impl jsg::Traced for SpanBuilder {} +impl jsg::Traced for SpanBuilder { + fn holds_gc_references() -> bool { + false + } +} + +impl jsg::HoldsNoGcReferences for SpanBuilder {} /// Holds no JavaScript handles. -impl jsg::Traced for TraceContext {} +impl jsg::Traced for TraceContext { + fn holds_gc_references() -> bool { + false + } +} + +impl jsg::HoldsNoGcReferences for TraceContext {} /// A literal as the bridge passes it: the bytes of `str` with its NUL. fn literal(str: &'static CStr) -> &'static [u8] { diff --git a/types/rtti-fixture/main.rs b/types/rtti-fixture/main.rs index c38427b63da..19647f2e7b2 100644 --- a/types/rtti-fixture/main.rs +++ b/types/rtti-fixture/main.rs @@ -2,7 +2,6 @@ // Licensed under the Apache 2.0 license found in the LICENSE file or at: // https://opensource.org/licenses/Apache-2.0 -use std::cell::Cell; use std::io::Write; use jsg::ToJsTypedArray; @@ -43,7 +42,7 @@ enum Payload<'a> { #[jsg_resource(name = "RustRttiFixture", ts_root)] struct Fixture { label: String, - child: Cell>>, + child: jsg::Cell>>, } #[jsg_resource(nested_types(Rows = NestedChild))] @@ -52,7 +51,7 @@ impl Fixture { fn constructor(label: OptionalLabel) -> Self { Self { label: label.unwrap_or_default(), - child: Cell::new(None), + child: jsg::Cell::new(None), } } @@ -181,8 +180,8 @@ impl Fixture { } #[jsg_property(prototype)] - fn get_child(&self) -> jsg::Nullable> { - let child = self.child.take(); + fn get_child(&self, lock: &mut jsg::Lock<'_>) -> jsg::Nullable> { + let child = self.child.take(lock); let result = child.clone(); self.child.set(child); match result {