dom: Track "removed" event listener status (#36163)

The DOM event listener "removed" status should be supported to track
the following situations (with immediate effect of listener removal):
- Removing a later event listener while an earlier listener
  for the same event is running
- Nested usage (recursively dispatch another event) of "once" listeners
https://dom.spec.whatwg.org/#event-listener-removed

During event dispatching requires to clone event listeners list
on "invoke" step https://dom.spec.whatwg.org/#concept-event-listener-invoke
and the lowercase "event listener" concept in Servo is EventListenerEntry
https://dom.spec.whatwg.org/#concept-event-listener

Bug: #25479, #25090

Signed-off-by: Andrei Volykhin <andrei.volykhin@gmail.com>

components/script/dom/event.rs

@@ -29,7 +29,7 @@ use crate::dom::bindings::reflector::{DomGlobal, Reflector, reflect_dom_object_w
 use crate::dom::bindings::root::{Dom, DomRoot, MutNullableDom};
 use crate::dom::bindings::str::DOMString;
 use crate::dom::element::Element;
-use crate::dom::eventtarget::{CompiledEventListener, EventTarget, ListenerPhase};
+use crate::dom::eventtarget::{EventListeners, EventTarget, ListenerPhase};
 use crate::dom::globalscope::GlobalScope;
 use crate::dom::htmlinputelement::InputActivationState;
 use crate::dom::htmlslotelement::HTMLSlotElement;
@@ -1151,10 +1151,7 @@ fn invoke(
     event.current_target.set(Some(&segment.invocation_target));
 
     // Step 6. Let listeners be a clone of event’s currentTarget attribute value’s event listener list.
-    let listeners =
-        segment
-            .invocation_target
-            .get_listeners_for(&event.type_(), Some(phase), can_gc);
+    let listeners = segment.invocation_target.get_listeners_for(&event.type_());
 
     // Step 7. Let invocationTargetInShadowTree be struct’s invocation-target-in-shadow-tree.
     let invocation_target_in_shadow_tree = segment.invocation_target_in_shadow_tree;
@@ -1207,8 +1204,8 @@ fn invoke(
 /// <https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke>
 fn inner_invoke(
     event: &Event,
-    listeners: &[CompiledEventListener],
-    _phase: ListenerPhase,
+    listeners: &EventListeners,
+    phase: ListenerPhase,
     invocation_target_in_shadow_tree: bool,
     timeline_window: Option<&Window>,
     can_gc: CanGc,
@@ -1217,25 +1214,21 @@ fn inner_invoke(
     let mut found = false;
 
     // Step 2. For each listener in listeners, whose removed is false:
-    for listener in listeners {
-        // FIXME(#25479): We need an "if !listener.removed()" here,
-        // but there's a subtlety. Where Servo is currently using the
-        // CompiledEventListener, we really need something that maps to
-        // https://dom.spec.whatwg.org/#concept-event-listener
-        // which is not the same thing as the EventListener interface.
-        // script::dom::eventtarget::EventListenerEntry is the closest
-        // match we have, and is already holding the "once" flag,
-        // but it's not a drop-in replacement.
+    for listener in listeners.iter() {
+        if listener.borrow().removed() {
+            continue;
+        }
 
-        // Steps 2.1 and 2.3-2.4 are not done because `listeners` contain only the
-        // relevant ones for this invoke call during the dispatch algorithm.
-        // TODO: Step 2.1 If event’s type attribute value is not listener’s type, then continue.
+        // Step 2.1 If event’s type attribute value is not listener’s type, then continue.
 
         // Step 2.2. Set found to true.
         found = true;
 
-        // TODO Step 2.3 If phase is "capturing" and listener’s capture is false, then continue.
-        // TODO Step 2.4 If phase is "bubbling" and listener’s capture is true, then continue.
+        // Step 2.3 If phase is "capturing" and listener’s capture is false, then continue.
+        // Step 2.4 If phase is "bubbling" and listener’s capture is true, then continue.
+        if listener.borrow().phase() != phase {
+            continue;
+        }
 
         let event_target = event
             .GetCurrentTarget()
@@ -1243,12 +1236,20 @@ fn inner_invoke(
 
         // Step 2.5 If listener’s once is true, then remove an event listener given event’s currentTarget
         // attribute value and listener.
-        if let CompiledEventListener::Listener(event_listener) = listener {
-            event_target.remove_listener_if_once(&event.type_(), event_listener);
+        if listener.borrow().once() {
+            event_target.remove_listener(&event.type_(), listener);
         }
 
+        let Some(compiled_listener) =
+            listener
+                .borrow()
+                .get_compiled_listener(&event_target, &event.type_(), can_gc)
+        else {
+            continue;
+        };
+
         // Step 2.6 Let global be listener callback’s associated realm’s global object.
-        let global = listener.associated_global();
+        let global = compiled_listener.associated_global();
 
         // Step 2.7 Let currentEvent be undefined.
         let mut current_event = None;
@@ -1264,18 +1265,22 @@ fn inner_invoke(
         }
 
         // Step 2.9 If listener’s passive is true, then set event's in passive listener flag.
-        if let CompiledEventListener::Listener(event_listener) = listener {
-            event.set_in_passive_listener(event_target.is_passive(&event.type_(), event_listener));
-        }
+        event.set_in_passive_listener(event_target.is_passive(&event.type_(), listener));
 
-        // Step 2.10 If global is a Window object, then record timing info for event listener given event and listener.
+        // Step 2.10 If global is a Window object, then record timing info for event listener
+        // given event and listener.
         // Step 2.11 Call a user object’s operation with listener’s callback, "handleEvent", « event »,
         // and event’s currentTarget attribute value. If this throws an exception exception:
         //     Step 2.10.1 Report exception for listener’s callback’s corresponding JavaScript object’s
         //     associated realm’s global object.
         //     TODO Step 2.10.2 Set legacyOutputDidListenersThrowFlag if given.
         let marker = TimelineMarker::start("DOMEvent".to_owned());
-        listener.call_or_handle_event(&event_target, event, ExceptionHandling::Report, can_gc);
+        compiled_listener.call_or_handle_event(
+            &event_target,
+            event,
+            ExceptionHandling::Report,
+            can_gc,
+        );
         if let Some(window) = timeline_window {
             window.emit_timeline_marker(marker.end());
         }

components/script/dom/eventtarget.rs

@@ -307,11 +307,36 @@ impl CompiledEventListener {
 // (as distinct from https://dom.spec.whatwg.org/#callbackdef-eventlistener)
 #[derive(Clone, DenyPublicFields, JSTraceable, MallocSizeOf)]
 /// A listener in a collection of event listeners.
-struct EventListenerEntry {
+pub(crate) struct EventListenerEntry {
     phase: ListenerPhase,
     listener: EventListenerType,
     once: bool,
     passive: Option<bool>,
+    removed: bool,
+}
+
+impl EventListenerEntry {
+    pub(crate) fn phase(&self) -> ListenerPhase {
+        self.phase
+    }
+
+    pub(crate) fn once(&self) -> bool {
+        self.once
+    }
+
+    pub(crate) fn removed(&self) -> bool {
+        self.removed
+    }
+
+    /// <https://html.spec.whatwg.org/multipage/#getting-the-current-value-of-the-event-handler>
+    pub(crate) fn get_compiled_listener(
+        &self,
+        owner: &EventTarget,
+        ty: &Atom,
+        can_gc: CanGc,
+    ) -> Option<CompiledEventListener> {
+        self.listener.get_compiled_listener(owner, ty, can_gc)
+    }
 }
 
 impl std::cmp::PartialEq for EventListenerEntry {
@@ -320,19 +345,21 @@ impl std::cmp::PartialEq for EventListenerEntry {
     }
 }
 
-#[derive(JSTraceable, MallocSizeOf)]
+#[derive(Clone, JSTraceable, MallocSizeOf)]
 /// A mix of potentially uncompiled and compiled event listeners of the same type.
-struct EventListeners(Vec<EventListenerEntry>);
+pub(crate) struct EventListeners(
+    #[ignore_malloc_size_of = "Rc"] Vec<Rc<RefCell<EventListenerEntry>>>,
+);
 
 impl Deref for EventListeners {
-    type Target = Vec<EventListenerEntry>;
-    fn deref(&self) -> &Vec<EventListenerEntry> {
+    type Target = Vec<Rc<RefCell<EventListenerEntry>>>;
+    fn deref(&self) -> &Vec<Rc<RefCell<EventListenerEntry>>> {
         &self.0
     }
 }
 
 impl DerefMut for EventListeners {
-    fn deref_mut(&mut self) -> &mut Vec<EventListenerEntry> {
+    fn deref_mut(&mut self) -> &mut Vec<Rc<RefCell<EventListenerEntry>>> {
         &mut self.0
     }
 }
@@ -346,7 +373,7 @@ impl EventListeners {
         can_gc: CanGc,
     ) -> Option<CommonEventHandler> {
         for entry in &self.0 {
-            if let EventListenerType::Inline(ref inline) = entry.listener {
+            if let EventListenerType::Inline(ref inline) = entry.borrow().listener {
                 // Step 1.1-1.8 and Step 2
                 return get_compiled_handler(inline, owner, ty, can_gc);
             }
@@ -356,30 +383,7 @@ impl EventListeners {
         None
     }
 
-    // https://html.spec.whatwg.org/multipage/#getting-the-current-value-of-the-event-handler
-    fn get_listeners(
-        &self,
-        phase: Option<ListenerPhase>,
-        owner: &EventTarget,
-        ty: &Atom,
-        can_gc: CanGc,
-    ) -> Vec<CompiledEventListener> {
-        self.0
-            .iter()
-            .filter_map(|entry| {
-                if phase.is_none() || Some(entry.phase) == phase {
-                    // Step 1.1-1.8, 2
-                    entry.listener.get_compiled_listener(owner, ty, can_gc)
-                } else {
-                    None
-                }
-            })
-            .collect()
-    }
-
     fn has_listeners(&self) -> bool {
-        // TODO: add, and take into account, a 'removed' field?
-        // https://dom.spec.whatwg.org/#event-listener-removed
         !self.0.is_empty()
     }
 }
@@ -420,18 +424,11 @@ impl EventTarget {
         }
     }
 
-    pub(crate) fn get_listeners_for(
-        &self,
-        type_: &Atom,
-        specific_phase: Option<ListenerPhase>,
-        can_gc: CanGc,
-    ) -> Vec<CompiledEventListener> {
+    pub(crate) fn get_listeners_for(&self, type_: &Atom) -> EventListeners {
         self.handlers
             .borrow()
             .get(type_)
-            .map_or(vec![], |listeners| {
-                listeners.get_listeners(specific_phase, self, type_, can_gc)
-            })
+            .map_or(EventListeners(vec![]), |listeners| listeners.clone())
     }
 
     pub(crate) fn dispatch_event(&self, event: &Event, can_gc: CanGc) -> EventStatus {
@@ -439,7 +436,14 @@ impl EventTarget {
     }
 
     pub(crate) fn remove_all_listeners(&self) {
-        *self.handlers.borrow_mut() = Default::default();
+        let mut handlers = self.handlers.borrow_mut();
+        for (_, entries) in handlers.iter() {
+            entries
+                .iter()
+                .for_each(|entry| entry.borrow_mut().removed = true);
+        }
+
+        *handlers = Default::default();
     }
 
     /// <https://dom.spec.whatwg.org/#default-passive-value>
@@ -488,50 +492,49 @@ impl EventTarget {
 
         let idx = entries
             .iter()
-            .position(|entry| matches!(entry.listener, EventListenerType::Inline(_)));
+            .position(|entry| matches!(entry.borrow().listener, EventListenerType::Inline(_)));
 
         match idx {
             Some(idx) => match listener {
                 // Replace if there's something to replace with,
                 // but remove entirely if there isn't.
                 Some(listener) => {
-                    entries[idx].listener = EventListenerType::Inline(listener.into());
+                    entries[idx].borrow_mut().listener = EventListenerType::Inline(listener.into());
                 },
                 None => {
-                    entries.remove(idx);
+                    entries.remove(idx).borrow_mut().removed = true;
                 },
             },
             None => {
                 if let Some(listener) = listener {
-                    entries.push(EventListenerEntry {
+                    entries.push(Rc::new(RefCell::new(EventListenerEntry {
                         phase: ListenerPhase::Bubbling,
                         listener: EventListenerType::Inline(listener.into()),
                         once: false,
                         passive: None,
-                    });
+                        removed: false,
+                    })));
                 }
             },
         }
     }
 
-    pub(crate) fn remove_listener_if_once(&self, ty: &Atom, listener: &Rc<EventListener>) {
+    pub(crate) fn remove_listener(&self, ty: &Atom, entry: &Rc<RefCell<EventListenerEntry>>) {
         let mut handlers = self.handlers.borrow_mut();
 
-        let listener = EventListenerType::Additive(listener.clone());
         if let Some(entries) = handlers.get_mut(ty) {
-            entries.retain(|e| e.listener != listener || !e.once)
+            if let Some(position) = entries.iter().position(|e| *e == *entry) {
+                entries.remove(position).borrow_mut().removed = true;
+            }
         }
     }
 
     /// Determines the `passive` attribute of an associated event listener
-    pub(crate) fn is_passive(&self, ty: &Atom, listener: &Rc<EventListener>) -> bool {
-        let handlers = self.handlers.borrow();
-        let listener_instance = EventListenerType::Additive(listener.clone());
-
-        handlers
-            .get(ty)
-            .and_then(|entries| entries.iter().find(|e| e.listener == listener_instance))
-            .is_some_and(|entry| entry.passive.unwrap_or(self.default_passive_value(ty)))
+    pub(crate) fn is_passive(&self, ty: &Atom, listener: &Rc<RefCell<EventListenerEntry>>) -> bool {
+        listener
+            .borrow()
+            .passive
+            .unwrap_or(self.default_passive_value(ty))
     }
 
     fn get_inline_event_listener(&self, ty: &Atom, can_gc: CanGc) -> Option<CommonEventHandler> {
@@ -810,7 +813,7 @@ impl EventTarget {
             None => return,
         };
         let mut handlers = self.handlers.borrow_mut();
-        let entry = match handlers.entry(Atom::from(ty)) {
+        let entries = match handlers.entry(Atom::from(ty)) {
             Occupied(entry) => entry.into_mut(),
             Vacant(entry) => entry.insert(EventListeners(vec![])),
         };
@@ -820,14 +823,16 @@ impl EventTarget {
         } else {
             ListenerPhase::Bubbling
         };
-        let new_entry = EventListenerEntry {
+        let new_entry = Rc::new(RefCell::new(EventListenerEntry {
             phase,
             listener: EventListenerType::Additive(listener),
             once: options.once,
             passive: options.passive,
-        };
-        if !entry.contains(&new_entry) {
-            entry.push(new_entry);
+            removed: false,
+        }));
+
+        if !entries.contains(&new_entry) {
+            entries.push(new_entry);
         }
     }
 
@@ -842,21 +847,21 @@ impl EventTarget {
             return;
         };
         let mut handlers = self.handlers.borrow_mut();
-        let entry = handlers.get_mut(&Atom::from(ty));
-        if let Some(entry) = entry {
+        if let Some(entries) = handlers.get_mut(&Atom::from(ty)) {
             let phase = if options.capture {
                 ListenerPhase::Capturing
             } else {
                 ListenerPhase::Bubbling
             };
-            let old_entry = EventListenerEntry {
+            let old_entry = Rc::new(RefCell::new(EventListenerEntry {
                 phase,
                 listener: EventListenerType::Additive(listener.clone()),
                 once: false,
                 passive: None,
-            };
-            if let Some(position) = entry.iter().position(|e| *e == old_entry) {
-                entry.remove(position);
+                removed: false,
+            }));
+            if let Some(position) = entries.iter().position(|e| *e == old_entry) {
+                entries.remove(position).borrow_mut().removed = true;
             }
         }
     }

tests/wpt/meta/dom/events/remove-all-listeners.html.ini

@@ -1,6 +0,0 @@
-[remove-all-listeners.html]
-  [Removing all listeners and then adding a new one should work.]
-    expected: FAIL
-
-  [Nested usage of once listeners should work.]
-    expected: FAIL

tests/wpt/meta/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/event-listeners.window.js.ini

@@ -10,6 +10,3 @@
 
   [Custom event listeners are to be removed from Window for an active but not fully active document]
     expected: FAIL
-
-  [Event listeners are to be removed with immediate effect]
-    expected: FAIL