LibWeb: Remove tasks for destroyed documents instead of running them

Previously, destroyed-document tasks were forced to be runnable to
prevent them from leaking in the task queue. Instead, discard them
during task selection so their callbacks never run with stale state.

This used to cause issues with a couple of `spin_until()`s in the past,
but since we've removed some of them that had to do with the document
lifecycle, let's see if we can stick closer to the spec now.

Libraries/LibWeb/DOM/Document.cpp

@@ -4532,9 +4532,7 @@ void Document::destroy()
         return task.document() == this;
     });
 
-    // AD-HOC: Mark this document as destroyed. This makes any tasks scheduled for this document in the
-    //         future immediately runnable instead of blocking on the document becoming fully active.
-    //         This is important because otherwise those tasks will get stuck in the task queue forever.
+    // AD-HOC: Mark this document as destroyed so we can remove tasks from the queue that will never be able to run.
     m_has_been_destroyed = true;
 
     // 8. Set document's browsing context to null.
@@ -4744,8 +4742,6 @@ GC::Ptr<HTML::HTMLParser> Document::active_parser()
 
 void Document::set_browsing_context(GC::Ptr<HTML::BrowsingContext> browsing_context)
 {
-    if (browsing_context)
-        m_has_been_browsing_context_associated = true;
     m_browsing_context = browsing_context;
 }
 

Libraries/LibWeb/DOM/Document.h

@@ -683,8 +683,6 @@ public:
 
     [[nodiscard]] bool has_been_destroyed() const { return m_has_been_destroyed; }
 
-    [[nodiscard]] bool has_been_browsing_context_associated() const { return m_has_been_browsing_context_associated; }
-
     // https://html.spec.whatwg.org/multipage/document-lifecycle.html#destroy-a-document
     void destroy();
     // https://html.spec.whatwg.org/multipage/document-lifecycle.html#destroy-a-document-and-its-descendants
@@ -1126,8 +1124,6 @@ private:
     bool m_has_been_destroyed { false };
     bool m_has_fired_document_became_inactive { false };
 
-    bool m_has_been_browsing_context_associated { false };
-
     String m_source;
 
     GC::Ptr<HTML::HTMLScriptElement> m_pending_parsing_blocking_script;

Libraries/LibWeb/HTML/EventLoop/Task.cpp

@@ -51,15 +51,7 @@ void Task::execute()
 bool Task::is_runnable() const
 {
     // A task is runnable if its document is either null or fully active.
-    if (!m_document)
-        return true;
-
-    // AD-HOC: If the document has been destroyed, we'll consider the task runnable.
-    //         Otherwise it would get stuck here forever, since a destroyed document never becomes fully active again.
-    if (m_document->has_been_destroyed())
-        return true;
-
-    return m_document->is_fully_active();
+    return !m_document || m_document->is_fully_active();
 }
 
 DOM::Document const* Task::document() const

Libraries/LibWeb/HTML/EventLoop/TaskQueue.cpp

@@ -34,10 +34,6 @@ void TaskQueue::add(GC::Ref<Task> task)
     if (task->document() && task->document()->is_temporary_document_for_fragment_parsing())
         return;
 
-    // AD-HOC: Don't enqueue tasks for documents that haven't been browsing context associated.
-    if (task->document() && !task->document()->has_been_browsing_context_associated())
-        return;
-
     m_tasks.append(task);
     m_event_loop->schedule();
 }
@@ -47,11 +43,22 @@ GC::Ptr<Task> TaskQueue::take_first_runnable()
     if (m_event_loop->execution_paused())
         return nullptr;
 
-    for (size_t i = 0; i < m_tasks.size(); ++i) {
-        if (m_event_loop->running_rendering_task() && m_tasks[i]->source() == Task::Source::Rendering)
+    for (size_t i = 0; i < m_tasks.size();) {
+        if (m_event_loop->running_rendering_task() && m_tasks[i]->source() == Task::Source::Rendering) {
+            ++i;
             continue;
+        }
+
         if (m_tasks[i]->is_runnable())
             return m_tasks.take(i);
+
+        // A non-runnable task with a destroyed document will never become runnable again; remove it.
+        if (m_tasks[i]->document() && m_tasks[i]->document()->has_been_destroyed()) {
+            m_tasks.remove(i);
+            continue;
+        }
+
+        ++i;
     }
     return nullptr;
 }

Libraries/LibWeb/HTML/Navigable.cpp

@@ -1391,10 +1391,6 @@ void Navigable::populate_session_history_entry_document(
 
         // 5. Queue a global task on the navigation and traversal task source, given navigable's active window, to run these steps:
         queue_global_task(Task::Source::NavigationAndTraversal, *active_window(), GC::create_function(heap(), [this, entry, received_navigation_params = move(received_navigation_params), navigation_id, user_involvement, completion_steps, csp_navigation_type, signal_to_continue_session_history_processing]() mutable {
-            // NOTE: This check is not in the spec but we should not continue navigation if navigable has been destroyed.
-            if (has_been_destroyed())
-                return;
-
             // 1. If navigable's ongoing navigation no longer equals navigationId, then run completionSteps and abort these steps.
             if (navigation_id.has_value() && ongoing_navigation() != navigation_id) {
                 if (completion_steps) {
@@ -1871,7 +1867,6 @@ void Navigable::begin_navigation(NavigateParams params)
 
         // 3. Queue a global task on the navigation and traversal task source given navigable's active window to abort a document and its descendants given navigable's active document.
         queue_global_task(Task::Source::NavigationAndTraversal, *active_window(), GC::create_function(heap(), [this] {
-            VERIFY(this->active_document());
             this->active_document()->abort_a_document_and_its_descendants();
         }));
 

Libraries/LibWeb/HTML/Scripting/Environments.cpp

@@ -126,13 +126,10 @@ EventLoop& EnvironmentSettingsObject::responsible_event_loop()
 // https://whatpr.org/html/9893/webappapis.html#check-if-we-can-run-script
 RunScriptDecision can_run_script(JS::Realm const& realm)
 {
-    // 1. If the global object specified by realm is a Window object whose Document object is not fully active, then return "do not run".
-    if (auto const* window = as_if<HTML::Window>(realm.global_object())) {
-        auto const& document = window->associated_document();
-        // AD-HOC: We allow tasks for destroyed documents to run so that microtasks queued during the fetch of a new
-        //         document in a navigation can still be processed, even after the previous document, the one that
-        //         initiated the fetch, has been destroyed.
-        if (!document.has_been_destroyed() && !document.is_fully_active())
+    // 1. If the global object specified by realm is a Window object whose Document object is not fully active, then
+    //    return "do not run".
+    if (auto const* window = as_if<Window>(realm.global_object())) {
+        if (!window->associated_document().is_fully_active())
             return RunScriptDecision::DoNotRun;
     }