From 19f70dccf6954b046a76d81a810f0cf22084e923 Mon Sep 17 00:00:00 2001 From: Narfinger Date: Thu, 11 Sep 2025 11:40:32 +0200 Subject: [PATCH] Combine some access to the thread local variable for script thread. (#38752) This combines some access to the thread local variable for script thread. - We introduce a new UserInteractingScriptGuard which on drop handles the resetting of was_interacting to the previous value. Sometimes throughout the code `ScriptThread::is_user_interacting` was reset to the previous value while sometimes just set to false. This should remove this footgun. - This also reduces the amount of thread local access for MutationObservers and task queue. Testing: WPT tests should cover this. Fixes: This addresses part of https://github.com/servo/servo/issues/37969 but there is probably still stuff to be done. --------- Signed-off-by: Narfinger Signed-off-by: Josh Matthews Co-authored-by: Josh Matthews --- components/script/dom/html/htmlslotelement.rs | 4 +- components/script/dom/mutationobserver.rs | 64 ++----------- components/script/dom/webxr/xrsystem.rs | 6 +- components/script/dom/webxr/xrtest.rs | 5 +- components/script/lib.rs | 1 + components/script/microtask.rs | 7 +- .../script/script_mutation_observers.rs | 96 +++++++++++++++++++ components/script/script_thread.rs | 80 ++++++++-------- components/script/timers.rs | 4 +- ...ession_immersive_no_gesture.https.html.ini | 3 + ...on_requiredFeatures_unknown.https.html.ini | 3 - 11 files changed, 156 insertions(+), 117 deletions(-) create mode 100644 components/script/script_mutation_observers.rs create mode 100644 tests/wpt/meta/webxr/xrDevice_requestSession_immersive_no_gesture.https.html.ini delete mode 100644 tests/wpt/meta/webxr/xrDevice_requestSession_requiredFeatures_unknown.https.html.ini diff --git a/components/script/dom/html/htmlslotelement.rs b/components/script/dom/html/htmlslotelement.rs index ae1d7fed1e1..e77cf89c30b 100644 --- a/components/script/dom/html/htmlslotelement.rs +++ b/components/script/dom/html/htmlslotelement.rs @@ -28,7 +28,6 @@ use crate::dom::document::Document; use crate::dom::element::{AttributeMutation, Element}; use crate::dom::globalscope::GlobalScope; use crate::dom::html::htmlelement::HTMLElement; -use crate::dom::mutationobserver::MutationObserver; use crate::dom::node::{BindContext, Node, NodeDamage, NodeTraits, ShadowIncluding, UnbindContext}; use crate::dom::virtualmethods::VirtualMethods; use crate::script_runtime::CanGc; @@ -357,7 +356,8 @@ impl HTMLSlotElement { ScriptThread::add_signal_slot(self); // Step 2. Queue a mutation observer microtask. - MutationObserver::queue_mutation_observer_microtask(); + let mutation_observers = ScriptThread::mutation_observers(); + mutation_observers.queue_mutation_observer_microtask(ScriptThread::microtask_queue()); } pub(crate) fn remove_from_signal_slots(&self) { diff --git a/components/script/dom/mutationobserver.rs b/components/script/dom/mutationobserver.rs index 2501f50e0b7..fe70048d22f 100644 --- a/components/script/dom/mutationobserver.rs +++ b/components/script/dom/mutationobserver.rs @@ -9,22 +9,18 @@ use dom_struct::dom_struct; use html5ever::{LocalName, Namespace, ns}; use js::rust::HandleObject; -use crate::dom::bindings::callback::ExceptionHandling; use crate::dom::bindings::cell::DomRefCell; use crate::dom::bindings::codegen::Bindings::MutationObserverBinding::MutationObserver_Binding::MutationObserverMethods; use crate::dom::bindings::codegen::Bindings::MutationObserverBinding::{ MutationCallback, MutationObserverInit, }; use crate::dom::bindings::error::{Error, Fallible}; -use crate::dom::bindings::inheritance::Castable; use crate::dom::bindings::reflector::{DomGlobal, Reflector, reflect_dom_object_with_proto}; use crate::dom::bindings::root::DomRoot; use crate::dom::bindings::str::DOMString; -use crate::dom::eventtarget::EventTarget; use crate::dom::mutationrecord::MutationRecord; use crate::dom::node::{Node, ShadowIncluding}; use crate::dom::window::Window; -use crate::microtask::Microtask; use crate::script_runtime::CanGc; use crate::script_thread::ScriptThread; @@ -91,59 +87,12 @@ impl MutationObserver { } } - /// - pub(crate) fn queue_mutation_observer_microtask() { - // Step 1. If the surrounding agent’s mutation observer microtask queued is true, then return. - if ScriptThread::is_mutation_observer_microtask_queued() { - return; - } - - // Step 2. Set the surrounding agent’s mutation observer microtask queued to true. - ScriptThread::set_mutation_observer_microtask_queued(true); - - // Step 3. Queue a microtask to notify mutation observers. - ScriptThread::enqueue_microtask(Microtask::NotifyMutationObservers); + pub(crate) fn record_queue(&self) -> &DomRefCell>> { + &self.record_queue } - /// - pub(crate) fn notify_mutation_observers(can_gc: CanGc) { - // Step 1. Set the surrounding agent’s mutation observer microtask queued to false. - ScriptThread::set_mutation_observer_microtask_queued(false); - - // Step 2. Let notifySet be a clone of the surrounding agent’s pending mutation observers. - // TODO Step 3. Empty the surrounding agent’s pending mutation observers. - let notify_list = ScriptThread::get_mutation_observers(); - - // Step 4. Let signalSet be a clone of the surrounding agent’s signal slots. - // Step 5. Empty the surrounding agent’s signal slots. - let signal_set = ScriptThread::take_signal_slots(); - - // Step 6. For each mo of notifySet: - for mo in ¬ify_list { - // Step 6.1 Let records be a clone of mo’s record queue. - let queue: Vec> = mo.record_queue.borrow().clone(); - - // Step 6.2 Empty mo’s record queue. - mo.record_queue.borrow_mut().clear(); - - // TODO Step 6.3 For each node of mo’s node list, remove all transient registered observers - // whose observer is mo from node’s registered observer list. - - // Step 6.4 If records is not empty, then invoke mo’s callback with « records, - // mo » and "report", and with callback this value mo. - if !queue.is_empty() { - let _ = mo - .callback - .Call_(&**mo, queue, mo, ExceptionHandling::Report, can_gc); - } - } - - // Step 6. For each slot of signalSet, fire an event named slotchange, - // with its bubbles attribute set to true, at slot. - for slot in signal_set { - slot.upcast::() - .fire_event(atom!("slotchange"), can_gc); - } + pub(crate) fn callback(&self) -> &Rc { + &self.callback } /// @@ -286,7 +235,8 @@ impl MutationObserver { } // Step 5 - MutationObserver::queue_mutation_observer_microtask(); + let mutation_observers = ScriptThread::mutation_observers(); + mutation_observers.queue_mutation_observer_microtask(ScriptThread::microtask_queue()); } } @@ -300,7 +250,7 @@ impl MutationObserverMethods for MutationObserver { ) -> Fallible> { global.set_exists_mut_observer(); let observer = MutationObserver::new_with_proto(global, proto, callback, can_gc); - ScriptThread::add_mutation_observer(&observer); + ScriptThread::mutation_observers().add_mutation_observer(&observer); Ok(observer) } diff --git a/components/script/dom/webxr/xrsystem.rs b/components/script/dom/webxr/xrsystem.rs index f905982e88e..2db13aa2a20 100644 --- a/components/script/dom/webxr/xrsystem.rs +++ b/components/script/dom/webxr/xrsystem.rs @@ -323,10 +323,8 @@ impl XRSystem { task!(fire_sessionavailable_event: move || { // The sessionavailable event indicates user intent to enter an XR session let xr = xr.root(); - let interacting = ScriptThread::is_user_interacting(); - ScriptThread::set_user_interacting(true); - xr.upcast::().fire_bubbling_event(atom!("sessionavailable"), CanGc::note()); - ScriptThread::set_user_interacting(interacting); + let _guard = ScriptThread::user_interacting_guard(); + xr.upcast::().fire_bubbling_event(atom!("sessionavailable"), CanGc::note()); }) ); } diff --git a/components/script/dom/webxr/xrtest.rs b/components/script/dom/webxr/xrtest.rs index 8d80c5df69e..cdf45c30fd2 100644 --- a/components/script/dom/webxr/xrtest.rs +++ b/components/script/dom/webxr/xrtest.rs @@ -15,6 +15,7 @@ use js::jsval::JSVal; use profile_traits::ipc; use webxr_api::{self, Error as XRError, MockDeviceInit, MockDeviceMsg}; +use crate::ScriptThread; use crate::dom::bindings::callback::ExceptionHandling; use crate::dom::bindings::cell::DomRefCell; use crate::dom::bindings::codegen::Bindings::FunctionBinding::Function; @@ -27,7 +28,6 @@ use crate::dom::fakexrdevice::{FakeXRDevice, get_origin, get_views, get_world}; use crate::dom::globalscope::GlobalScope; use crate::dom::promise::Promise; use crate::script_runtime::CanGc; -use crate::script_thread::ScriptThread; #[dom_struct] pub(crate) struct XRTest { @@ -181,7 +181,7 @@ impl XRTestMethods for XRTest { /// fn SimulateUserActivation(&self, f: Rc, can_gc: CanGc) { - ScriptThread::set_user_interacting(true); + let _guard = ScriptThread::user_interacting_guard(); rooted!(in(*GlobalScope::get_cx()) let mut value: JSVal); let _ = f.Call__( vec![], @@ -189,7 +189,6 @@ impl XRTestMethods for XRTest { ExceptionHandling::Rethrow, can_gc, ); - ScriptThread::set_user_interacting(false); } /// diff --git a/components/script/lib.rs b/components/script/lib.rs index 56896bcde8e..28dc3d087eb 100644 --- a/components/script/lib.rs +++ b/components/script/lib.rs @@ -51,6 +51,7 @@ mod realms; mod routed_promise; #[allow(dead_code)] mod script_module; +mod script_mutation_observers; pub(crate) mod script_runtime; #[allow(unsafe_code)] pub(crate) mod script_thread; diff --git a/components/script/microtask.rs b/components/script/microtask.rs index d939bbd7284..7979c943769 100644 --- a/components/script/microtask.rs +++ b/components/script/microtask.rs @@ -22,7 +22,6 @@ use crate::dom::defaultteereadrequest::DefaultTeeReadRequestMicrotask; use crate::dom::globalscope::GlobalScope; use crate::dom::html::htmlimageelement::ImageElementMicrotask; use crate::dom::html::htmlmediaelement::MediaElementMicrotask; -use crate::dom::mutationobserver::MutationObserver; use crate::dom::promise::WaitForAllSuccessStepsMicrotask; use crate::realms::enter_realm; use crate::script_runtime::{CanGc, JSContext, notify_about_rejected_promises}; @@ -118,13 +117,11 @@ impl MicrotaskQueue { match *job { Microtask::Promise(ref job) => { if let Some(target) = target_provider(job.pipeline) { - let was_interacting = ScriptThread::is_user_interacting(); - ScriptThread::set_user_interacting(job.is_user_interacting); + let _guard = ScriptThread::user_interacting_guard(); let _realm = enter_realm(&*target); let _ = job .callback .Call_(&*target, ExceptionHandling::Report, can_gc); - ScriptThread::set_user_interacting(was_interacting); } }, Microtask::User(ref job) => { @@ -155,7 +152,7 @@ impl MicrotaskQueue { ScriptThread::invoke_backup_element_queue(can_gc); }, Microtask::NotifyMutationObservers => { - MutationObserver::notify_mutation_observers(can_gc); + ScriptThread::mutation_observers().notify_mutation_observers(can_gc); }, } } diff --git a/components/script/script_mutation_observers.rs b/components/script/script_mutation_observers.rs new file mode 100644 index 00000000000..670e45b5a52 --- /dev/null +++ b/components/script/script_mutation_observers.rs @@ -0,0 +1,96 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ + +use std::cell::Cell; +use std::rc::Rc; + +use script_bindings::callback::ExceptionHandling; +use script_bindings::inheritance::Castable; +use script_bindings::root::{Dom, DomRoot}; +use script_bindings::script_runtime::CanGc; + +use crate::ScriptThread; +use crate::dom::bindings::cell::DomRefCell; +use crate::dom::types::{EventTarget, HTMLSlotElement, MutationObserver, MutationRecord}; +use crate::microtask::{Microtask, MicrotaskQueue}; + +/// A helper struct for mutation observers used in `ScriptThread` +/// Since the Rc is always stored in ScriptThread, it's always reachable by the GC. +#[derive(JSTraceable, Default)] +#[cfg_attr(crown, crown::unrooted_must_root_lint::allow_unrooted_in_rc)] +#[cfg_attr(crown, crown::unrooted_must_root_lint::must_root)] +pub(crate) struct ScriptMutationObservers { + /// Microtask Queue for adding support for mutation observer microtasks + mutation_observer_microtask_queued: Cell, + + /// The unit of related similar-origin browsing contexts' list of MutationObserver objects + mutation_observers: DomRefCell>>, +} + +impl ScriptMutationObservers { + pub(crate) fn add_mutation_observer(&self, observer: &MutationObserver) { + self.mutation_observers + .borrow_mut() + .push(Dom::from_ref(observer)); + } + + /// + pub(crate) fn notify_mutation_observers(&self, can_gc: CanGc) { + // Step 1. Set the surrounding agent’s mutation observer microtask queued to false. + self.mutation_observer_microtask_queued.set(false); + + // Step 2. Let notifySet be a clone of the surrounding agent’s pending mutation observers. + // TODO Step 3. Empty the surrounding agent’s pending mutation observers. + let notify_list = self.mutation_observers.borrow(); + + // Step 4. Let signalSet be a clone of the surrounding agent’s signal slots. + // Step 5. Empty the surrounding agent’s signal slots. + let signal_set: Vec> = ScriptThread::take_signal_slots(); + + // Step 6. For each mo of notifySet: + for mo in notify_list.iter() { + let record_queue = mo.record_queue(); + + // Step 6.1 Let records be a clone of mo’s record queue. + let queue: Vec> = record_queue.borrow().clone(); + + // Step 6.2 Empty mo’s record queue. + record_queue.borrow_mut().clear(); + + // TODO Step 6.3 For each node of mo’s node list, remove all transient registered observers + // whose observer is mo from node’s registered observer list. + + // Step 6.4 If records is not empty, then invoke mo’s callback with « records, + // mo » and "report", and with callback this value mo. + if !queue.is_empty() { + let _ = mo + .callback() + .Call_(&**mo, queue, mo, ExceptionHandling::Report, can_gc); + } + } + + // Step 6. For each slot of signalSet, fire an event named slotchange, + // with its bubbles attribute set to true, at slot. + for slot in signal_set { + slot.upcast::() + .fire_event(atom!("slotchange"), can_gc); + } + } + + /// + pub(crate) fn queue_mutation_observer_microtask(&self, microtask_queue: Rc) { + // Step 1. If the surrounding agent’s mutation observer microtask queued is true, then return. + if self.mutation_observer_microtask_queued.get() { + return; + } + + // Step 2. Set the surrounding agent’s mutation observer microtask queued to true. + self.mutation_observer_microtask_queued.set(true); + + // Step 3. Queue a microtask to notify mutation observers. + crate::script_thread::with_script_thread(|script_thread| { + microtask_queue.enqueue(Microtask::NotifyMutationObservers, script_thread.get_cx()); + }); + } +} diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index 527e340035b..b221ae807c3 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -127,7 +127,6 @@ use crate::dom::element::Element; use crate::dom::globalscope::GlobalScope; use crate::dom::html::htmliframeelement::HTMLIFrameElement; use crate::dom::html::htmlslotelement::HTMLSlotElement; -use crate::dom::mutationobserver::MutationObserver; use crate::dom::node::NodeTraits; use crate::dom::servoparser::{ParserContext, ServoParser}; use crate::dom::types::DebuggerGlobalScope; @@ -147,6 +146,7 @@ use crate::mime::{APPLICATION, MimeExt, TEXT, XML}; use crate::navigation::{InProgressLoad, NavigationListener}; use crate::realms::enter_realm; use crate::script_module::ScriptFetchOptions; +use crate::script_mutation_observers::ScriptMutationObservers; use crate::script_runtime::{ CanGc, IntroductionType, JSContext, JSContextHelper, Runtime, ScriptThreadEventCategory, ThreadSafeJSContext, @@ -193,6 +193,30 @@ unsafe_no_jsmanaged_fields!(TaskQueue); type NodeIdSet = HashSet; +/// A simple guard structure that restore the user interacting state when dropped +#[derive(Default)] +pub(crate) struct ScriptUserInteractingGuard { + was_interacting: bool, + user_interaction_cell: Rc>, +} + +impl ScriptUserInteractingGuard { + fn new(user_interaction_cell: Rc>) -> Self { + let was_interacting = user_interaction_cell.get(); + user_interaction_cell.set(true); + Self { + was_interacting, + user_interaction_cell, + } + } +} + +impl Drop for ScriptUserInteractingGuard { + fn drop(&mut self) { + self.user_interaction_cell.set(self.was_interacting) + } +} + #[derive(JSTraceable)] // ScriptThread instances are rooted on creation, so this is okay #[cfg_attr(crown, allow(crown::unrooted_must_root))] @@ -255,11 +279,7 @@ pub struct ScriptThread { /// microtask_queue: Rc, - /// Microtask Queue for adding support for mutation observer microtasks - mutation_observer_microtask_queued: Cell, - - /// The unit of related similar-origin browsing contexts' list of MutationObserver objects - mutation_observers: DomRefCell>>, + mutation_observers: Rc, /// signal_slots: DomRefCell>>, @@ -315,7 +335,7 @@ pub struct ScriptThread { pipeline_to_node_ids: DomRefCell>, /// Code is running as a consequence of a user interaction - is_user_interacting: Cell, + is_user_interacting: Rc>, /// Identity manager for WebGPU resources #[no_trace] @@ -479,34 +499,12 @@ impl ScriptThread { }) } - pub(crate) fn set_mutation_observer_microtask_queued(value: bool) { - with_script_thread(|script_thread| { - script_thread.mutation_observer_microtask_queued.set(value); - }) + pub(crate) fn mutation_observers() -> Rc { + with_script_thread(|script_thread| script_thread.mutation_observers.clone()) } - pub(crate) fn is_mutation_observer_microtask_queued() -> bool { - with_script_thread(|script_thread| script_thread.mutation_observer_microtask_queued.get()) - } - - pub(crate) fn add_mutation_observer(observer: &MutationObserver) { - with_script_thread(|script_thread| { - script_thread - .mutation_observers - .borrow_mut() - .push(Dom::from_ref(observer)); - }) - } - - pub(crate) fn get_mutation_observers() -> Vec> { - with_script_thread(|script_thread| { - script_thread - .mutation_observers - .borrow() - .iter() - .map(|o| DomRoot::from_ref(&**o)) - .collect() - }) + pub(crate) fn microtask_queue() -> Rc { + with_script_thread(|script_thread| script_thread.microtask_queue.clone()) } pub(crate) fn add_signal_slot(observer: &HTMLSlotElement) { @@ -717,10 +715,14 @@ impl ScriptThread { with_script_thread(|script_thread| script_thread.documents.borrow().find_document(id)) } - pub(crate) fn set_user_interacting(interacting: bool) { + /// Creates a guard that sets user_is_interacting to true and returns the + /// state of user_is_interacting on drop of the guard. + /// Notice that you need to use `let _guard = ...` as `let _ = ...` is not enough + #[must_use] + pub(crate) fn user_interacting_guard() -> ScriptUserInteractingGuard { with_script_thread(|script_thread| { - script_thread.is_user_interacting.set(interacting); - }); + ScriptUserInteractingGuard::new(script_thread.is_user_interacting.clone()) + }) } pub(crate) fn is_user_interacting() -> bool { @@ -998,7 +1000,6 @@ impl ScriptThread { microtask_queue, js_runtime, closed_pipelines: DomRefCell::new(FxHashSet::default()), - mutation_observer_microtask_queued: Default::default(), mutation_observers: Default::default(), signal_slots: Default::default(), system_font_service, @@ -1017,7 +1018,7 @@ impl ScriptThread { user_content_manager: state.user_content_manager, player_context: state.player_context, pipeline_to_node_ids: Default::default(), - is_user_interacting: Cell::new(false), + is_user_interacting: Rc::new(Cell::new(false)), #[cfg(feature = "webgpu")] gpu_id_hub, inherited_secure_context: state.inherited_secure_context, @@ -1076,10 +1077,9 @@ impl ScriptThread { warn!("Compositor event sent to a pipeline with a closed window {pipeline_id}."); return; } - ScriptThread::set_user_interacting(true); + let _guard = ScriptUserInteractingGuard::new(self.is_user_interacting.clone()); document.event_handler().handle_pending_input_events(can_gc); - ScriptThread::set_user_interacting(false); } fn cancel_scheduled_update_the_rendering(&self) { diff --git a/components/script/timers.rs b/components/script/timers.rs index 681ef183336..f0662e8572d 100644 --- a/components/script/timers.rs +++ b/components/script/timers.rs @@ -589,8 +589,7 @@ impl JsTimerTask { // prep for step ? in nested set_timeout_or_interval calls timers.nesting_level.set(self.nesting_level); - let was_user_interacting = ScriptThread::is_user_interacting(); - ScriptThread::set_user_interacting(self.is_user_interacting); + let _guard = ScriptThread::user_interacting_guard(); match self.callback { InternalTimerCallback::StringTimerCallback(ref code_str) => { // Step 6.4. Let settings object be global's relevant settings object. @@ -630,7 +629,6 @@ impl JsTimerTask { let _ = function.Call_(this, arguments, value.handle_mut(), Report, can_gc); }, }; - ScriptThread::set_user_interacting(was_user_interacting); // reset nesting level (see above) timers.nesting_level.set(0); diff --git a/tests/wpt/meta/webxr/xrDevice_requestSession_immersive_no_gesture.https.html.ini b/tests/wpt/meta/webxr/xrDevice_requestSession_immersive_no_gesture.https.html.ini new file mode 100644 index 00000000000..3149f2a5dd3 --- /dev/null +++ b/tests/wpt/meta/webxr/xrDevice_requestSession_immersive_no_gesture.https.html.ini @@ -0,0 +1,3 @@ +[xrDevice_requestSession_immersive_no_gesture.https.html] + [Requesting immersive session outside of a user gesture rejects] + expected: FAIL diff --git a/tests/wpt/meta/webxr/xrDevice_requestSession_requiredFeatures_unknown.https.html.ini b/tests/wpt/meta/webxr/xrDevice_requestSession_requiredFeatures_unknown.https.html.ini deleted file mode 100644 index 121ca2e72d4..00000000000 --- a/tests/wpt/meta/webxr/xrDevice_requestSession_requiredFeatures_unknown.https.html.ini +++ /dev/null @@ -1,3 +0,0 @@ -[xrDevice_requestSession_requiredFeatures_unknown.https.html] - [Tests requestSession rejects for unknown requiredFeatures] - expected: FAIL