LibWeb: Address edge case on async module load

[crhaglun]

Attempting to address #6294

This is an edge case when the same module is loaded three times in a document, but all fail.

Failure scenario:

1. Module load 1 set the state to "Fetching"
2. Module load 2 registers a callback to on_complete since the current state is "Fetching"
3. Module load 1 finish with a failure, invoking the callback for load 2
4. Module load 3 cause a crash. The state is neither "Fetching" or "ModuleScript", so we'll reset the state to "Fetching". This invokes the callback for module load 2 again, now with an unexpected state which will cause an assert violation.

Proposed fix is to remove the condition that invokes on_complete callback immediately for successfully loaded modules only, the callback should be invoked regardless of whether the fetch succeeded or failed.

This reveals a separate bug in HTMLScriptElement, where mark_as_ready() can be invoked before m_steps_to_run_when_the_result_is_ready is assigned.
To mitigate that, add a new method set_steps_to_run_when_the_result_is_ready() instead of assigning m_steps_to_run_when_the_result_is_ready directly.

[shannonbooth]

Thanks! The change in fetch also fixes the WPT tests from crashing of:

• https://wpt.live/html/semantics/scripting-1/the-script-element/module/dynamic-import/dynamic-imports-fetch-error.sub.html
• https://wpt.live/html/semantics/scripting-1/the-script-element/json-module/load-error-events.html

However, the change to HTMLScriptElement here seems dubious - I don't quite understand the issue here. I believe we are matching the specification text as written currently. The crash also seems to be resolved without this change also.

What situation does the change to HTMLScriptElement fix?

[crhaglun]

Without this additional fix, the new test multiple-failed-imports.html fails with a timeout. This happens because one of the script elements never reach "executing" stage, i.e. we never call execute_script()

If a script has already been fetched, we invoke the on_complete callback immediately. If you follow the execution in HTMLScriptElement::prepare_script(), there is a path where we end up calling mark_as_ready before we set m_steps_to_run_when_the_result_is_ready

The specification on https://html.spec.whatwg.org/multipage/scripting.html does not explicitly cover how to handle the case of setting "steps to run when the result is ready" if the result has already been marked "ready" at that point in time.
I believe the intent is pretty clear, the steps should run "when the result is ready", so if the result is already "ready" they should run immediately.

---

An alternative way of resolving this inconsistency would be to queue a task to execute the completion callback in Fetching.cpp instead of calling it directly, something like

--- a/Libraries/LibWeb/HTML/Scripting/Fetching.cpp
+++ b/Libraries/LibWeb/HTML/Scripting/Fetching.cpp
@@ -655,7 +655,9 @@ void fetch_single_module_script(JS::Realm& realm,
     // 6. If moduleMap[(url, moduleType)] exists, run onComplete given moduleMap[(url, moduleType)], and return.
     auto entry = module_map.get(url, module_type.to_byte_string());
     if (entry.has_value() && entry->type == ModuleMap::EntryType::ModuleScript) {
-        on_complete->function()(entry->module_script);
+        HTML::queue_global_task(HTML::Task::Source::Networking, realm.global_object(), GC::create_function(realm.heap(), [on_complete, entry] {
+            on_complete->function()(entry->module_script);
+        }));
         return;
     }

That ensures the prepare_script() method executes fully before the callback is invoked, which means we never run mark_as_ready before setting m_steps_to_run_when_the_result_is_ready.
I feel this would clearly be a deviation from the spec though, since https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-single-module-script specifies what operations should be queued as tasks and what should be done immediately.