From e28ac74e0b65334c92e0ba569dfa3a6ff9ece0cf Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Sat, 16 Dec 2023 18:32:01 +0100 Subject: [PATCH] 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/ --- .../Libraries/LibWeb/HTML/Scripting/Fetching.cpp | 12 ++++++------ .../Libraries/LibWeb/HTML/Scripting/ModuleMap.cpp | 8 +++++++- Userland/Libraries/LibWeb/HTML/Scripting/ModuleMap.h | 2 ++ 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/Userland/Libraries/LibWeb/HTML/Scripting/Fetching.cpp b/Userland/Libraries/LibWeb/HTML/Scripting/Fetching.cpp index 09cc4fa4ba1..8a65efd4d3e 100644 --- a/Userland/Libraries/LibWeb/HTML/Scripting/Fetching.cpp +++ b/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; diff --git a/Userland/Libraries/LibWeb/HTML/Scripting/ModuleMap.cpp b/Userland/Libraries/LibWeb/HTML/Scripting/ModuleMap.cpp index ccf28f998d5..655094f32e9 100644 --- a/Userland/Libraries/LibWeb/HTML/Scripting/ModuleMap.cpp +++ b/Userland/Libraries/LibWeb/HTML/Scripting/ModuleMap.cpp @@ -47,12 +47,18 @@ Optional 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; } diff --git a/Userland/Libraries/LibWeb/HTML/Scripting/ModuleMap.h b/Userland/Libraries/LibWeb/HTML/Scripting/ModuleMap.h index 7cf5e53dd56..3a699978543 100644 --- a/Userland/Libraries/LibWeb/HTML/Scripting/ModuleMap.h +++ b/Userland/Libraries/LibWeb/HTML/Scripting/ModuleMap.h @@ -72,6 +72,8 @@ private: HashMap m_values; HashMap> m_callbacks; + + bool m_firing_callbacks { false }; }; }