From 05748ed607bfd14301a45bb34bb62e14e0b51d28 Mon Sep 17 00:00:00 2001 From: Luke Wilde Date: Sat, 7 May 2022 00:20:22 +0100 Subject: [PATCH] LibJS: Convert Console to use MarkedVector Using a Vector is unsafe as GC cannot see the stored values. This is then vended to outside users of ConsoleClient, e.g. LibWeb and WebContent, which is then outside of LibJS's control. An example issue is if the client stores it for later use and forgets to visit the stored values, meaning they can be destroyed at any time. We can save the client from this by vending a MarkedVector to them. --- Userland/Libraries/LibJS/Console.cpp | 53 ++++++++++++------- Userland/Libraries/LibJS/Console.h | 10 ++-- .../LibWeb/HTML/WorkerDebugConsoleClient.cpp | 3 +- .../WebContent/WebContentConsoleClient.cpp | 2 +- Userland/Utilities/js.cpp | 2 +- 5 files changed, 42 insertions(+), 28 deletions(-) diff --git a/Userland/Libraries/LibJS/Console.cpp b/Userland/Libraries/LibJS/Console.cpp index c398f09695e..956dc0c5c0c 100644 --- a/Userland/Libraries/LibJS/Console.cpp +++ b/Userland/Libraries/LibJS/Console.cpp @@ -138,7 +138,8 @@ ThrowCompletionOr Console::count() String concat = String::formatted("{}: {}", label, map.get(label).value()); // 5. Perform Logger("count", « concat »). - Vector concat_as_vector { js_string(vm(), concat) }; + MarkedVector concat_as_vector { global_object().heap() }; + concat_as_vector.append(js_string(vm(), concat)); if (m_client) TRY(m_client->logger(LogLevel::Count, concat_as_vector)); return js_undefined(); @@ -163,7 +164,8 @@ ThrowCompletionOr Console::count_reset() // that the given label does not have an associated count. auto message = String::formatted("\"{}\" doesn't have a count", label); // 2. Perform Logger("countReset", « message »); - Vector message_as_vector { js_string(vm(), message) }; + MarkedVector message_as_vector { global_object().heap() }; + message_as_vector.append(js_string(vm(), message)); if (m_client) TRY(m_client->logger(LogLevel::CountReset, message_as_vector)); } @@ -183,7 +185,7 @@ ThrowCompletionOr Console::assert_() auto message = js_string(vm(), "Assertion failed"); // NOTE: Assemble `data` from the function arguments. - Vector data; + MarkedVector data { global_object().heap() }; if (vm().argument_count() > 1) { data.ensure_capacity(vm().argument_count() - 1); for (size_t i = 1; i < vm().argument_count(); ++i) { @@ -309,8 +311,11 @@ ThrowCompletionOr Console::time() // 1. If the associated timer table contains an entry with key label, return, optionally reporting // a warning to the console indicating that a timer with label `label` has already been started. if (m_timer_table.contains(label)) { - if (m_client) - TRY(m_client->printer(LogLevel::Warn, { Vector { js_string(vm(), String::formatted("Timer '{}' already exists.", label)) } })); + if (m_client) { + MarkedVector timer_already_exists_warning_message_as_vector { global_object().heap() }; + timer_already_exists_warning_message_as_vector.append(js_string(vm(), String::formatted("Timer '{}' already exists.", label))); + TRY(m_client->printer(LogLevel::Warn, move(timer_already_exists_warning_message_as_vector))); + } return js_undefined(); } @@ -332,8 +337,11 @@ ThrowCompletionOr Console::time_log() // NOTE: Warn if the timer doesn't exist. Not part of the spec yet, but discussed here: https://github.com/whatwg/console/issues/134 if (maybe_start_time == m_timer_table.end()) { - if (m_client) - TRY(m_client->printer(LogLevel::Warn, { Vector { js_string(vm(), String::formatted("Timer '{}' does not exist.", label)) } })); + if (m_client) { + MarkedVector timer_does_not_exist_warning_message_as_vector { global_object().heap() }; + timer_does_not_exist_warning_message_as_vector.append(js_string(vm(), String::formatted("Timer '{}' does not exist.", label))); + TRY(m_client->printer(LogLevel::Warn, move(timer_does_not_exist_warning_message_as_vector))); + } return js_undefined(); } auto start_time = maybe_start_time->value; @@ -345,7 +353,7 @@ ThrowCompletionOr Console::time_log() auto concat = String::formatted("{}: {}", label, duration); // 5. Prepend concat to data. - Vector data; + MarkedVector data { global_object().heap() }; data.ensure_capacity(vm().argument_count()); data.append(js_string(vm(), concat)); for (size_t i = 1; i < vm().argument_count(); ++i) @@ -353,7 +361,7 @@ ThrowCompletionOr Console::time_log() // 6. Perform Printer("timeLog", data). if (m_client) - TRY(m_client->printer(LogLevel::TimeLog, data)); + TRY(m_client->printer(LogLevel::TimeLog, move(data))); return js_undefined(); } @@ -370,8 +378,11 @@ ThrowCompletionOr Console::time_end() // NOTE: Warn if the timer doesn't exist. Not part of the spec yet, but discussed here: https://github.com/whatwg/console/issues/134 if (maybe_start_time == m_timer_table.end()) { - if (m_client) - TRY(m_client->printer(LogLevel::Warn, { Vector { js_string(vm(), String::formatted("Timer '{}' does not exist.", label)) } })); + if (m_client) { + MarkedVector timer_does_not_exist_warning_message_as_vector { global_object().heap() }; + timer_does_not_exist_warning_message_as_vector.append(js_string(vm(), String::formatted("Timer '{}' does not exist.", label))); + TRY(m_client->printer(LogLevel::Warn, move(timer_does_not_exist_warning_message_as_vector))); + } return js_undefined(); } auto start_time = maybe_start_time->value; @@ -387,15 +398,16 @@ ThrowCompletionOr Console::time_end() // 6. Perform Printer("timeEnd", « concat »). if (m_client) { - Vector concat_as_vector { js_string(vm(), concat) }; - TRY(m_client->printer(LogLevel::TimeEnd, concat_as_vector)); + MarkedVector concat_as_vector { global_object().heap() }; + concat_as_vector.append(js_string(vm(), concat)); + TRY(m_client->printer(LogLevel::TimeEnd, move(concat_as_vector))); } return js_undefined(); } -Vector Console::vm_arguments() +MarkedVector Console::vm_arguments() { - Vector arguments; + MarkedVector arguments { global_object().heap() }; arguments.ensure_capacity(vm().argument_count()); for (size_t i = 0; i < vm().argument_count(); ++i) { arguments.append(vm().argument(i)); @@ -429,7 +441,7 @@ void Console::output_debug_message([[maybe_unused]] LogLevel log_level, [[maybe_ #endif } -ThrowCompletionOr Console::value_vector_to_string(Vector& values) +ThrowCompletionOr Console::value_vector_to_string(MarkedVector const& values) { StringBuilder builder; for (auto const& item : values) { @@ -471,7 +483,7 @@ VM& ConsoleClient::vm() } // 2.1. Logger(logLevel, args), https://console.spec.whatwg.org/#logger -ThrowCompletionOr ConsoleClient::logger(Console::LogLevel log_level, Vector& args) +ThrowCompletionOr ConsoleClient::logger(Console::LogLevel log_level, MarkedVector const& args) { auto& global_object = this->global_object(); @@ -487,8 +499,9 @@ ThrowCompletionOr ConsoleClient::logger(Console::LogLevel log_level, Vect // 4. If rest is empty, perform Printer(logLevel, « first ») and return. if (rest_size == 0) { - auto first_as_vector = Vector { first }; - return printer(log_level, first_as_vector); + MarkedVector first_as_vector { global_object.heap() }; + first_as_vector.append(first); + return printer(log_level, move(first_as_vector)); } // 5. If first does not contain any format specifiers, perform Printer(logLevel, args). @@ -505,7 +518,7 @@ ThrowCompletionOr ConsoleClient::logger(Console::LogLevel log_level, Vect } // 2.2. Formatter(args), https://console.spec.whatwg.org/#formatter -ThrowCompletionOr> ConsoleClient::formatter(Vector& args) +ThrowCompletionOr> ConsoleClient::formatter(MarkedVector const& args) { // TODO: Actually implement formatting return args; diff --git a/Userland/Libraries/LibJS/Console.h b/Userland/Libraries/LibJS/Console.h index 3928882a395..6d344adfaa9 100644 --- a/Userland/Libraries/LibJS/Console.h +++ b/Userland/Libraries/LibJS/Console.h @@ -61,7 +61,7 @@ public: GlobalObject const& global_object() const { return m_global_object; } VM& vm(); - Vector vm_arguments(); + MarkedVector vm_arguments(); HashMap& counters() { return m_counters; } HashMap const& counters() const { return m_counters; } @@ -86,7 +86,7 @@ public: void output_debug_message(LogLevel log_level, String output) const; private: - ThrowCompletionOr value_vector_to_string(Vector&); + ThrowCompletionOr value_vector_to_string(MarkedVector const&); ThrowCompletionOr format_time_since(Core::ElapsedTimer timer); GlobalObject& m_global_object; @@ -104,10 +104,10 @@ public: { } - using PrinterArguments = Variant>; + using PrinterArguments = Variant>; - ThrowCompletionOr logger(Console::LogLevel log_level, Vector& args); - ThrowCompletionOr> formatter(Vector& args); + ThrowCompletionOr logger(Console::LogLevel log_level, MarkedVector const& args); + ThrowCompletionOr> formatter(MarkedVector const& args); virtual ThrowCompletionOr printer(Console::LogLevel log_level, PrinterArguments) = 0; virtual void clear() = 0; diff --git a/Userland/Libraries/LibWeb/HTML/WorkerDebugConsoleClient.cpp b/Userland/Libraries/LibWeb/HTML/WorkerDebugConsoleClient.cpp index 86f4c5f143f..433f40c83a8 100644 --- a/Userland/Libraries/LibWeb/HTML/WorkerDebugConsoleClient.cpp +++ b/Userland/Libraries/LibWeb/HTML/WorkerDebugConsoleClient.cpp @@ -4,6 +4,7 @@ * SPDX-License-Identifier: BSD-2-Clause */ +#include #include #include @@ -52,7 +53,7 @@ JS::ThrowCompletionOr WorkerDebugConsoleClient::printer(JS::Console:: return JS::js_undefined(); } - auto output = String::join(" ", arguments.get>()); + auto output = String::join(" ", arguments.get>()); m_console.output_debug_message(log_level, output); switch (log_level) { diff --git a/Userland/Services/WebContent/WebContentConsoleClient.cpp b/Userland/Services/WebContent/WebContentConsoleClient.cpp index fb742e9a381..bcbe9ac0c38 100644 --- a/Userland/Services/WebContent/WebContentConsoleClient.cpp +++ b/Userland/Services/WebContent/WebContentConsoleClient.cpp @@ -162,7 +162,7 @@ JS::ThrowCompletionOr WebContentConsoleClient::printer(JS::Console::L return JS::js_undefined(); } - auto output = String::join(" ", arguments.get>()); + auto output = String::join(" ", arguments.get>()); m_console.output_debug_message(log_level, output); StringBuilder html; diff --git a/Userland/Utilities/js.cpp b/Userland/Utilities/js.cpp index bd840cbfcb8..37c10bde7a4 100644 --- a/Userland/Utilities/js.cpp +++ b/Userland/Utilities/js.cpp @@ -1374,7 +1374,7 @@ public: return JS::js_undefined(); } - auto output = String::join(" ", arguments.get>()); + auto output = String::join(" ", arguments.get>()); m_console.output_debug_message(log_level, output); switch (log_level) {