From 9e2ee0029ac89801f07fed9cb9be169bc57dc9d8 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Mon, 14 Jul 2025 21:11:12 -0400 Subject: [PATCH] script: Reduce usage of Trusted in Node::insert. (#37762) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These changes introduce a new kind of task that uses a variation of the `task!` syntax. Existing `task!` usages create task structs that have a `Send` bound, which requires the use of `Trusted` to reference a DOM object T inside of the task closure. The new syntax replaces the `Send` bound with a `JSTraceable` bound, which requires explicit capture clauses with types. This looks like: ```rust task!(ScriptPrepare: {script: DomRoot} |script| { script.prepare(CanGc::note()); }), ``` The capture clauses must list every value that will be referenced from the closure's environment—these values are moved into fields in the generated task structure, which allows them to be traced by the GC as part of a generated JSTraceable implementation. Since the closure itself is not a `move` closure, any attempts to reference values not explicitly captured will generate a borrow checker error since the closure requires the `'static` lifetime. Testing: Existing WPT tests exercise these code paths. I also attempted to write incorrect tasks that capture references or use values not explicitly captured, and the compiler correctly errors out. Fixes: part of #35517 Signed-off-by: Josh Matthews --- components/script/dom/document.rs | 6 +-- components/script/dom/htmlscriptelement.rs | 12 ++--- components/script/dom/node.rs | 22 ++++----- components/script/dom/userscripts.rs | 8 ++-- components/script/task.rs | 53 +++++++++++++++++++++- 5 files changed, 74 insertions(+), 27 deletions(-) diff --git a/components/script/dom/document.rs b/components/script/dom/document.rs index 9ac0e122ab4..30a67590507 100644 --- a/components/script/dom/document.rs +++ b/components/script/dom/document.rs @@ -212,7 +212,7 @@ use crate::realms::{AlreadyInRealm, InRealm, enter_realm}; use crate::script_runtime::{CanGc, ScriptThreadEventCategory}; use crate::script_thread::{ScriptThread, with_script_thread}; use crate::stylesheet_set::StylesheetSetRef; -use crate::task::TaskBox; +use crate::task::NonSendTaskBox; use crate::task_source::TaskSourceName; use crate::timers::OneshotTimerCallback; @@ -477,7 +477,7 @@ pub(crate) struct Document { script_and_layout_blockers: Cell, /// List of tasks to execute as soon as last script/layout blocker is removed. #[ignore_malloc_size_of = "Measuring trait objects is hard"] - delayed_tasks: DomRefCell>>, + delayed_tasks: DomRefCell>>, /// completely_loaded: Cell, /// Set of shadow roots connected to the document tree. @@ -4338,7 +4338,7 @@ impl Document { } /// Enqueue a task to run as soon as any JS and layout blockers are removed. - pub(crate) fn add_delayed_task(&self, task: T) { + pub(crate) fn add_delayed_task(&self, task: T) { self.delayed_tasks.borrow_mut().push(Box::new(task)); } diff --git a/components/script/dom/htmlscriptelement.rs b/components/script/dom/htmlscriptelement.rs index b35f35c4628..65d6c07ba52 100644 --- a/components/script/dom/htmlscriptelement.rs +++ b/components/script/dom/htmlscriptelement.rs @@ -1414,15 +1414,15 @@ impl VirtualMethods for HTMLScriptElement { } if self.upcast::().is_connected() && !self.parser_inserted.get() { - let script = Trusted::new(self); + let script = DomRoot::from_ref(self); // This method can be invoked while there are script/layout blockers present // as DOM mutations have not yet settled. We use a delayed task to avoid // running any scripts until the DOM tree is safe for interactions. - self.owner_document() - .add_delayed_task(task!(ScriptPrepare: move || { - let this = script.root(); - this.prepare(CanGc::note()); - })); + self.owner_document().add_delayed_task( + task!(ScriptPrepare: |script: DomRoot| { + script.prepare(CanGc::note()); + }), + ); } } diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs index b3d492a4ac5..f4e870b2e61 100644 --- a/components/script/dom/node.rs +++ b/components/script/dom/node.rs @@ -82,7 +82,6 @@ use crate::dom::bindings::inheritance::{ Castable, CharacterDataTypeId, ElementTypeId, EventTargetTypeId, HTMLElementTypeId, NodeTypeId, SVGElementTypeId, SVGGraphicsElementTypeId, TextTypeId, }; -use crate::dom::bindings::refcounted::Trusted; use crate::dom::bindings::reflector::{DomObject, DomObjectWrap, reflect_dom_object_with_proto}; use crate::dom::bindings::root::{Dom, DomRoot, DomSlice, LayoutDom, MutNullableDom, ToLayout}; use crate::dom::bindings::str::{DOMString, USVString}; @@ -2563,10 +2562,7 @@ impl Node { for node in new_nodes { // Step 11.1 For each shadow-including inclusive descendant inclusiveDescendant of node, // in shadow-including tree order, append inclusiveDescendant to staticNodeList. - static_node_list.extend( - node.traverse_preorder(ShadowIncluding::Yes) - .map(|n| Trusted::new(&*n)), - ); + static_node_list.extend(node.traverse_preorder(ShadowIncluding::Yes)); } // We use a delayed task for this step to work around an awkward interaction between @@ -2579,13 +2575,15 @@ impl Node { // 2) post_connection_steps from Node::insert, // we use a delayed task that will run as soon as Node::insert removes its // script/layout blocker. - parent_document.add_delayed_task(task!(PostConnectionSteps: move || { - // Step 12. For each node of staticNodeList, if node is connected, then run the - // post-connection steps with node. - for node in static_node_list.iter().map(Trusted::root).filter(|n| n.is_connected()) { - vtable_for(&node).post_connection_steps(); - } - })); + parent_document.add_delayed_task( + task!(PostConnectionSteps: |static_node_list: Vec>| { + // Step 12. For each node of staticNodeList, if node is connected, then run the + // post-connection steps with node. + for node in static_node_list.iter().filter(|n| n.is_connected()) { + vtable_for(node).post_connection_steps(); + } + }), + ); parent_document.remove_script_and_layout_blocker(); from_document.remove_script_and_layout_blocker(); diff --git a/components/script/dom/userscripts.rs b/components/script/dom/userscripts.rs index 79c2f58dbec..bf8febc8ad2 100644 --- a/components/script/dom/userscripts.rs +++ b/components/script/dom/userscripts.rs @@ -5,12 +5,13 @@ use std::rc::Rc; use js::jsval::UndefinedValue; +use script_bindings::root::DomRoot; -use crate::dom::bindings::refcounted::Trusted; use crate::dom::bindings::str::DOMString; use crate::dom::htmlheadelement::HTMLHeadElement; use crate::dom::htmlscriptelement::SourceCode; use crate::dom::node::NodeTraits; +use crate::dom::window::Window; use crate::script_module::ScriptFetchOptions; use crate::script_runtime::CanGc; @@ -20,9 +21,8 @@ pub(crate) fn load_script(head: &HTMLHeadElement) { if userscripts.is_empty() { return; } - let window = Trusted::new(doc.window()); - doc.add_delayed_task(task!(UserScriptExecute: move || { - let win = window.root(); + let win = DomRoot::from_ref(doc.window()); + doc.add_delayed_task(task!(UserScriptExecute: |win: DomRoot| { let cx = win.get_cx(); rooted!(in(*cx) let mut rval = UndefinedValue()); diff --git a/components/script/task.rs b/components/script/task.rs index 2a7e6b151bb..d149cadd8a6 100644 --- a/components/script/task.rs +++ b/components/script/task.rs @@ -9,6 +9,36 @@ use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; macro_rules! task { + ($name:ident: |$($field:ident: $field_type:ty$(,)*)*| $body:tt) => {{ + #[allow(non_camel_case_types)] + struct $name { + $($field: $field_type,)* + task: F, + } + #[allow(unsafe_code)] + unsafe impl crate::JSTraceable for $name { + #[allow(unsafe_code)] + unsafe fn trace(&self, tracer: *mut ::js::jsapi::JSTracer) { + $(self.$field.trace(tracer);)* + // We cannot trace the actual task closure. This is safe because + // all referenced values from within the closure are either borrowed + // or moved into fields in the struct (and therefore traced). + } + } + impl crate::task::NonSendTaskOnce for $name + where + F: ::std::ops::FnOnce($($field_type,)*), + { + fn run_once(self) { + (self.task)($(self.$field,)*); + } + } + $name { + $($field,)* + task: |$($field: $field_type,)*| $body, + } + }}; + ($name:ident: move || $body:tt) => {{ #[allow(non_camel_case_types)] struct $name(F); @@ -28,9 +58,9 @@ macro_rules! task { }}; } -/// A task that can be run. The name method is for profiling purposes. +/// A task that can be sent between threads and run. +/// The name method is for profiling purposes. pub(crate) trait TaskOnce: Send { - #[allow(unsafe_code)] fn name(&self) -> &'static str { ::std::any::type_name::() } @@ -38,6 +68,11 @@ pub(crate) trait TaskOnce: Send { fn run_once(self); } +/// A task that must be run on the same thread it originated in. +pub(crate) trait NonSendTaskOnce: crate::JSTraceable { + fn run_once(self); +} + /// A boxed version of `TaskOnce`. pub(crate) trait TaskBox: Send { fn name(&self) -> &'static str; @@ -45,6 +80,20 @@ pub(crate) trait TaskBox: Send { fn run_box(self: Box); } +/// A boxed version of `NonSendTaskOnce`. +pub(crate) trait NonSendTaskBox: crate::JSTraceable { + fn run_box(self: Box); +} + +impl NonSendTaskBox for T +where + T: NonSendTaskOnce, +{ + fn run_box(self: Box) { + self.run_once() + } +} + impl TaskBox for T where T: TaskOnce,