LibWeb: Add and remove MutationObservers at specified times

In the current spec, MutationObservers are explicitly added to the
pending mutation observers list, and they are removed when that list is
cleared in the "notify mutation observers" microtask.

This solves some issues with slotchange events.

As noted, we delay actually emptying the list of pending mutation
observers until after we're finished with the "clone", because we can't
actually copy or move the intrusive list. As far as I am aware, this
should not affect behaviour because only one microtask can run at once.

Libraries/LibWeb/Bindings/MainThreadVM.cpp

@@ -723,27 +723,36 @@ void queue_mutation_observer_microtask(DOM::Document const& document)
     surrounding_agent.mutation_observer_microtask_queued = true;
 
     // 3. Queue a microtask to notify mutation observers.
-    // NOTE: This uses the implied document concept. In the case of mutation observers, it is always done in a node context, so document should be that node's document.
+    // NOTE: This uses the implied document concept. In the case of mutation observers, it is always done in a node
+    //       context, so document should be that node's document.
     HTML::queue_a_microtask(&document, GC::create_function(vm.heap(), [&surrounding_agent, &heap = document.heap()]() {
+        // https://dom.spec.whatwg.org/#notify-mutation-observers
         // 1. Set the surrounding agent’s mutation observer microtask queued to false.
         surrounding_agent.mutation_observer_microtask_queued = false;
 
-        // 2. Let notifySet be a clone of the surrounding agent’s mutation observers.
+        // 2. Let notifySet be a clone of the surrounding agent’s pending mutation observers.
         GC::RootVector<DOM::MutationObserver*> notify_set(heap);
-        for (auto& observer : surrounding_agent.mutation_observers)
+        for (auto& observer : surrounding_agent.pending_mutation_observers)
             notify_set.append(&observer);
 
-        // 3. Let signalSet be a clone of the surrounding agent’s signal slots.
-        // 4. Empty the surrounding agent’s signal slots.
+        // 3. Empty the surrounding agent’s pending mutation observers.
+        // NB: We instead do this at the end of the microtask. Steps 2 and 3 are equivalent to moving
+        //     surrounding_agent.pending_mutation_observers, but it's unmovable. Actually copying the MutationObservers
+        //     causes issues, so for now, keep notify_set as pointers and defer this step until after we've finished
+        //     using the notify_set.
+
+        // 4. Let signalSet be a clone of the surrounding agent’s signal slots.
+        // 5. Empty the surrounding agent’s signal slots.
         auto signal_set = move(surrounding_agent.signal_slots);
 
-        // 5. For each mo of notifySet:
+        // 6. For each mo of notifySet:
         for (auto& mutation_observer : notify_set) {
             // 1. Let records be a clone of mo’s record queue.
             // 2. Empty mo’s record queue.
             auto records = mutation_observer->take_records();
 
-            // 3. For each node of mo’s node list, remove all transient registered observers whose observer is mo from node’s registered observer list.
+            // 3. For each node of mo’s node list, remove all transient registered observers whose observer is mo from
+            //    node’s registered observer list.
             for (auto& node : mutation_observer->node_list()) {
                 // FIXME: Is this correct?
                 if (!node)
@@ -756,7 +765,8 @@ void queue_mutation_observer_microtask(DOM::Document const& document)
                 }
             }
 
-            // 4. If records is not empty, then invoke mo’s callback with « records, mo » and "report", and with callback this value mo.
+            // 4. If records is not empty, then invoke mo’s callback with « records, mo » and "report", and with
+            //    callback this value mo.
             if (!records.is_empty()) {
                 auto& callback = mutation_observer->callback();
                 auto& realm = callback.callback_context;
@@ -772,12 +782,15 @@ void queue_mutation_observer_microtask(DOM::Document const& document)
             }
         }
 
-        // 6. For each slot of signalSet, fire an event named slotchange, with its bubbles attribute set to true, at slot.
+        // 7. For each slot of signalSet, fire an event named slotchange, with its bubbles attribute set to true, at slot.
         for (auto& slot : signal_set) {
             DOM::EventInit event_init;
             event_init.bubbles = true;
             slot->dispatch_event(DOM::Event::create(slot->realm(), HTML::EventNames::slotchange, event_init));
         }
+
+        // NB: Step 3, done later.
+        surrounding_agent.pending_mutation_observers.clear();
     }));
 }
 

Libraries/LibWeb/DOM/MutationObserver.cpp

@@ -26,21 +26,10 @@ MutationObserver::MutationObserver(JS::Realm& realm, GC::Ptr<WebIDL::CallbackTyp
     : PlatformObject(realm)
     , m_callback(move(callback))
 {
-
-    // 1. Set this’s callback to callback.
-
-    // 2. Append this to this’s relevant agent’s mutation observers.
-    HTML::relevant_similar_origin_window_agent(*this).mutation_observers.append(*this);
+    // The new MutationObserver(callback) constructor steps are to set this’s callback to callback.
 }
 
-MutationObserver::~MutationObserver()
-{
-}
-
-void MutationObserver::finalize()
-{
-    HTML::relevant_similar_origin_window_agent(*this).mutation_observers.remove(*this);
-}
+MutationObserver::~MutationObserver() = default;
 
 void MutationObserver::initialize(JS::Realm& realm)
 {
@@ -119,7 +108,7 @@ WebIDL::ExceptionOr<void> MutationObserver::observe(Node& target, MutationObserv
         auto new_registered_observer = RegisteredObserver::create(*this, options);
         target.add_registered_observer(new_registered_observer);
 
-        // 2. Append target to this’s node list.
+        // 2. Append a weak reference to target to this’s node list.
         m_node_list.append(target);
     }
 

Libraries/LibWeb/DOM/MutationObserver.h

@@ -53,7 +53,6 @@ private:
 
     virtual void initialize(JS::Realm&) override;
     virtual void visit_edges(Cell::Visitor&) override;
-    virtual void finalize() override;
 
     // https://dom.spec.whatwg.org/#concept-mo-callback
     GC::Ptr<WebIDL::CallbackType> m_callback;

Libraries/LibWeb/DOM/Node.cpp

@@ -53,6 +53,7 @@
 #include <LibWeb/HTML/Navigable.h>
 #include <LibWeb/HTML/NavigableContainer.h>
 #include <LibWeb/HTML/Parser/HTMLParser.h>
+#include <LibWeb/HTML/Scripting/SimilarOriginWindowAgent.h>
 #include <LibWeb/HTML/Scripting/TemporaryExecutionContext.h>
 #include <LibWeb/Infra/CharacterTypes.h>
 #include <LibWeb/Layout/Node.h>
@@ -2588,13 +2589,16 @@ void Node::queue_mutation_record(FlyString const& type, Optional<FlyString> cons
     auto removed_nodes_list = StaticNodeList::create(realm(), move(removed_nodes));
 
     // 4. For each observer → mappedOldValue of interestedObservers:
-    for (auto& interested_observer : interested_observers) {
+    for (auto& [observer, mapped_old_value] : interested_observers) {
         // 1. Let record be a new MutationRecord object with its type set to type, target set to target, attributeName set to name, attributeNamespace set to namespace, oldValue set to mappedOldValue,
         //    addedNodes set to addedNodes, removedNodes set to removedNodes, previousSibling set to previousSibling, and nextSibling set to nextSibling.
-        auto record = MutationRecord::create(realm(), type, *this, added_nodes_list, removed_nodes_list, previous_sibling, next_sibling, string_attribute_name, string_attribute_namespace, /* mappedOldValue */ interested_observer.value);
+        auto record = MutationRecord::create(realm(), type, *this, added_nodes_list, removed_nodes_list, previous_sibling, next_sibling, string_attribute_name, string_attribute_namespace, mapped_old_value);
 
         // 2. Enqueue record to observer’s record queue.
-        interested_observer.key->enqueue_record({}, move(record));
+        observer->enqueue_record({}, move(record));
+
+        // 3. Append observer to the surrounding agent’s pending mutation observers.
+        HTML::relevant_similar_origin_window_agent(*this).pending_mutation_observers.append(*observer);
     }
 
     // 5. Queue a mutation observer microtask.

Libraries/LibWeb/HTML/Scripting/SimilarOriginWindowAgent.h

@@ -29,7 +29,7 @@ struct SimilarOriginWindowAgent : public Agent {
 
     // https://dom.spec.whatwg.org/#mutation-observer-list
     // Each similar-origin window agent also has pending mutation observers (a set of zero or more MutationObserver objects), which is initially empty.
-    DOM::MutationObserver::List mutation_observers;
+    DOM::MutationObserver::List pending_mutation_observers;
 
     // https://html.spec.whatwg.org/multipage/custom-elements.html#custom-element-reactions-stack
     // Each similar-origin window agent has a custom element reactions stack, which is initially empty.

Tests/LibWeb/Text/expected/wpt-import/shadow-dom/slotchange-event.txt

@@ -2,8 +2,8 @@ Harness status: OK
 
 Found 32 tests
 
-12 Pass
-20 Fail
+16 Pass
+16 Fail
 Fail	slotchange event must fire on a default slot element inside an open shadow root  in a document
 Fail	slotchange event must fire on a default slot element inside a closed shadow root  in a document
 Fail	slotchange event must fire on a default slot element inside an open shadow root  not in a document
@@ -32,7 +32,7 @@ Pass	slotchange event must fire on a slot element inside an open shadow root  in
 Pass	slotchange event must fire on a slot element inside a closed shadow root  in a document when nested slots's contents change
 Pass	slotchange event must fire on a slot element inside an open shadow root  not in a document when nested slots's contents change
 Pass	slotchange event must fire on a slot element inside a closed shadow root  not in a document when nested slots's contents change
-Fail	slotchange event must fire at the end of current microtask after mutation observers are invoked inside an open shadow root  in a document when slots's contents change
-Fail	slotchange event must fire at the end of current microtask after mutation observers are invoked inside a closed shadow root  in a document when slots's contents change
-Fail	slotchange event must fire at the end of current microtask after mutation observers are invoked inside an open shadow root  not in a document when slots's contents change
-Fail	slotchange event must fire at the end of current microtask after mutation observers are invoked inside a closed shadow root  not in a document when slots's contents change
+Pass	slotchange event must fire at the end of current microtask after mutation observers are invoked inside an open shadow root  in a document when slots's contents change
+Pass	slotchange event must fire at the end of current microtask after mutation observers are invoked inside a closed shadow root  in a document when slots's contents change
+Pass	slotchange event must fire at the end of current microtask after mutation observers are invoked inside an open shadow root  not in a document when slots's contents change
+Pass	slotchange event must fire at the end of current microtask after mutation observers are invoked inside a closed shadow root  not in a document when slots's contents change