瀏覽代碼

LibWeb: Queue a task to proceed after module map entry finishes fetching

We were doing this synchronously, which was unsafe in that caused us to
re-enter the module map entry setting code while iterating over the
map's entries.

The fix is simply to do what the spec says and queue up a task. This way
the processing gets deferred to a later time.

To avoid stepping into this problem again, I've also added a reentrancy
check in ModuleMap.

This fixes a sporadic crash in HTML::ModuleMap::add() caught by ASAN.
In particular, this was happening regularly on https://shopify.com/
Andreas Kling 1 年之前
父節點
當前提交
e28ac74e0b

+ 6 - 6
Userland/Libraries/LibWeb/HTML/Scripting/Fetching.cpp

@@ -579,13 +579,13 @@ void fetch_single_module_script(JS::Realm& realm,
     // 5. If moduleMap[(url, moduleType)] is "fetching", wait in parallel until that entry's value changes,
     //    then queue a task on the networking task source to proceed with running the following steps.
     if (module_map.is_fetching(url, module_type)) {
-        module_map.wait_for_change(realm.heap(), url, module_type, [on_complete](auto entry) -> void {
-            // FIXME: This should queue a task.
+        module_map.wait_for_change(realm.heap(), url, module_type, [on_complete, &realm](auto entry) -> void {
+            HTML::queue_global_task(HTML::Task::Source::Networking, realm.global_object(), [on_complete, entry] {
+                // FIXME: This should run other steps, for now we just assume the script loaded.
+                VERIFY(entry.type == ModuleMap::EntryType::ModuleScript);
 
-            // FIXME: This should run other steps, for now we just assume the script loaded.
-            VERIFY(entry.type == ModuleMap::EntryType::ModuleScript);
-
-            on_complete->function()(entry.module_script);
+                on_complete->function()(entry.module_script);
+            });
         });
 
         return;

+ 7 - 1
Userland/Libraries/LibWeb/HTML/Scripting/ModuleMap.cpp

@@ -47,12 +47,18 @@ Optional<ModuleMap::Entry> ModuleMap::get(AK::URL const& url, DeprecatedString c
 
 AK::HashSetResult ModuleMap::set(AK::URL const& url, DeprecatedString const& type, Entry entry)
 {
+    // NOTE: Re-entering this function while firing wait_for_change callbacks is not allowed.
+    VERIFY(!m_firing_callbacks);
+
     auto value = m_values.set({ url, type }, entry);
 
     auto callbacks = m_callbacks.get({ url, type });
-    if (callbacks.has_value())
+    if (callbacks.has_value()) {
+        m_firing_callbacks = true;
         for (auto const& callback : *callbacks)
             callback->function()(entry);
+        m_firing_callbacks = false;
+    }
 
     return value;
 }

+ 2 - 0
Userland/Libraries/LibWeb/HTML/Scripting/ModuleMap.h

@@ -72,6 +72,8 @@ private:
 
     HashMap<ModuleLocationTuple, Entry> m_values;
     HashMap<ModuleLocationTuple, Vector<CallbackFunction>> m_callbacks;
+
+    bool m_firing_callbacks { false };
 };
 
 }