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 <Narfinger@users.noreply.github.com>
Signed-off-by: Josh Matthews <josh@joshmatthews.net>
Co-authored-by: Josh Matthews <josh@joshmatthews.net>

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) {

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 {
         }
     }
 
-    /// <https://dom.spec.whatwg.org/#queue-a-mutation-observer-compound-microtask>
-    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<Vec<DomRoot<MutationRecord>>> {
+        &self.record_queue
     }
 
-    /// <https://dom.spec.whatwg.org/#notify-mutation-observers>
-    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 &notify_list {
-            // Step 6.1 Let records be a clone of mo’s record queue.
-            let queue: Vec<DomRoot<MutationRecord>> = 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::<EventTarget>()
-                .fire_event(atom!("slotchange"), can_gc);
-        }
+    pub(crate) fn callback(&self) -> &Rc<MutationCallback> {
+        &self.callback
     }
 
     /// <https://dom.spec.whatwg.org/#queueing-a-mutation-record>
@@ -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<crate::DomTypeHolder> for MutationObserver {
     ) -> Fallible<DomRoot<MutationObserver>> {
         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)
     }
 

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::<EventTarget>().fire_bubbling_event(atom!("sessionavailable"), CanGc::note());
-                    ScriptThread::set_user_interacting(interacting);
+                        let _guard = ScriptThread::user_interacting_guard();
+                        xr.upcast::<EventTarget>().fire_bubbling_event(atom!("sessionavailable"), CanGc::note());
                 })
             );
     }

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<crate::DomTypeHolder> for XRTest {
 
     /// <https://github.com/immersive-web/webxr-test-api/blob/master/explainer.md>
     fn SimulateUserActivation(&self, f: Rc<Function>, 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<crate::DomTypeHolder> for XRTest {
             ExceptionHandling::Rethrow,
             can_gc,
         );
-        ScriptThread::set_user_interacting(false);
     }
 
     /// <https://github.com/immersive-web/webxr-test-api/blob/master/explainer.md>

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;

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);
                     },
                 }
             }

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<bool>,
+
+    /// The unit of related similar-origin browsing contexts' list of MutationObserver objects
+    mutation_observers: DomRefCell<Vec<Dom<MutationObserver>>>,
+}
+
+impl ScriptMutationObservers {
+    pub(crate) fn add_mutation_observer(&self, observer: &MutationObserver) {
+        self.mutation_observers
+            .borrow_mut()
+            .push(Dom::from_ref(observer));
+    }
+
+    /// <https://dom.spec.whatwg.org/#notify-mutation-observers>
+    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<DomRoot<HTMLSlotElement>> = 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<DomRoot<MutationRecord>> = 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::<EventTarget>()
+                .fire_event(atom!("slotchange"), can_gc);
+        }
+    }
+
+    /// <https://dom.spec.whatwg.org/#queue-a-mutation-observer-compound-microtask>
+    pub(crate) fn queue_mutation_observer_microtask(&self, microtask_queue: Rc<MicrotaskQueue>) {
+        // 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());
+        });
+    }
+}

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<MainThreadScriptMsg>);
 
 type NodeIdSet = HashSet<String>;
 
+/// 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<Cell<bool>>,
+}
+
+impl ScriptUserInteractingGuard {
+    fn new(user_interaction_cell: Rc<Cell<bool>>) -> 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 {
     /// <https://html.spec.whatwg.org/multipage/#microtask-queue>
     microtask_queue: Rc<MicrotaskQueue>,
 
-    /// Microtask Queue for adding support for mutation observer microtasks
-    mutation_observer_microtask_queued: Cell<bool>,
-
-    /// The unit of related similar-origin browsing contexts' list of MutationObserver objects
-    mutation_observers: DomRefCell<Vec<Dom<MutationObserver>>>,
+    mutation_observers: Rc<ScriptMutationObservers>,
 
     /// <https://dom.spec.whatwg.org/#signal-slot-list>
     signal_slots: DomRefCell<Vec<Dom<HTMLSlotElement>>>,
@@ -315,7 +335,7 @@ pub struct ScriptThread {
     pipeline_to_node_ids: DomRefCell<FxHashMap<PipelineId, NodeIdSet>>,
 
     /// Code is running as a consequence of a user interaction
-    is_user_interacting: Cell<bool>,
+    is_user_interacting: Rc<Cell<bool>>,
 
     /// 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<ScriptMutationObservers> {
+        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<DomRoot<MutationObserver>> {
-        with_script_thread(|script_thread| {
-            script_thread
-                .mutation_observers
-                .borrow()
-                .iter()
-                .map(|o| DomRoot::from_ref(&**o))
-                .collect()
-        })
+    pub(crate) fn microtask_queue() -> Rc<MicrotaskQueue> {
+        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) {

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);

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

tests/wpt/meta/webxr/xrDevice_requestSession_requiredFeatures_unknown.https.html.ini

@@ -1,3 +0,0 @@
-[xrDevice_requestSession_requiredFeatures_unknown.https.html]
-  [Tests requestSession rejects for unknown requiredFeatures]
-    expected: FAIL