From 2ab8d474c6709b07eaad0a58ed9625f3dbf613a6 Mon Sep 17 00:00:00 2001 From: DexesTTP Date: Sat, 30 Apr 2022 19:27:50 +0200 Subject: [PATCH] Lagom: Fix leaks in the IDL Wrapper generator By using RefPtrs to handle interfaces, the IDL parser could store cyclic references to interfaces that import each other. One main example is the "EventTarget.idl" and the "AbortSignal.idl" files, which both reference each other. This caused huge amounts of memory not to be freed on exit. To fix this, the parsed IDL interfaces are now stored in a HashTable of NonnullOwnPtr, which serves as the sole reference for every parsed interface. All other usages of the Interface are changed to use references instead of RefPtrs, or occasionally as raw pointers where references don't fit inside the data structures. This new HashTable is static, and as such will automatically be freed prior to exiting the generator. This ensures that the code generator properly cleans up after itself. With this change, The IDL code generators can properly run on Lagom when compiled with the -DENABLE_ADDRESS_SANITIZER=ON flag, and gets compiled properly on the CI :^) --- .../LibWeb/WrapperGenerator/IDLParser.cpp | 164 +++++++++--------- .../LibWeb/WrapperGenerator/IDLParser.h | 9 +- .../LibWeb/WrapperGenerator/IDLTypes.h | 14 +- .../LibWeb/WrapperGenerator/main.cpp | 34 ++-- 4 files changed, 117 insertions(+), 104 deletions(-) diff --git a/Meta/Lagom/Tools/CodeGenerators/LibWeb/WrapperGenerator/IDLParser.cpp b/Meta/Lagom/Tools/CodeGenerators/LibWeb/WrapperGenerator/IDLParser.cpp index f009e16c6f..8171319598 100644 --- a/Meta/Lagom/Tools/CodeGenerators/LibWeb/WrapperGenerator/IDLParser.cpp +++ b/Meta/Lagom/Tools/CodeGenerators/LibWeb/WrapperGenerator/IDLParser.cpp @@ -76,7 +76,8 @@ static String convert_enumeration_value_to_cpp_enum_member(String const& value, namespace IDL { -HashMap> Parser::s_resolved_imports {}; +HashTable> Parser::s_interfaces {}; +HashMap Parser::s_resolved_imports {}; void Parser::assert_specific(char ch) { @@ -125,7 +126,7 @@ HashMap Parser::parse_extended_attributes() } static HashTable import_stack; -Optional> Parser::resolve_import(auto path) +Optional Parser::resolve_import(auto path) { auto include_path = LexicalPath::join(import_base_path, path).string(); if (!Core::File::exists(include_path)) @@ -133,7 +134,7 @@ Optional> Parser::resolve_import(auto path) auto real_path = Core::File::real_path_for(include_path); if (s_resolved_imports.contains(real_path)) - return s_resolved_imports.find(real_path)->value; + return *s_resolved_imports.find(real_path)->value; if (import_stack.contains(real_path)) report_parsing_error(String::formatted("Circular import detected: {}", include_path), filename, input, lexer.tell()); @@ -144,10 +145,10 @@ Optional> Parser::resolve_import(auto path) report_parsing_error(String::formatted("Failed to open {}: {}", real_path, file_or_error.error()), filename, input, lexer.tell()); auto data = file_or_error.value()->read_all(); - auto result = Parser(real_path, data, import_base_path).parse(); + auto& result = Parser(real_path, data, import_base_path).parse(); import_stack.remove(real_path); - s_resolved_imports.set(real_path, result); + s_resolved_imports.set(real_path, &result); return result; } @@ -709,21 +710,23 @@ void Parser::parse_dictionary(Interface& interface) void Parser::parse_interface_mixin(Interface& interface) { - auto mixin_interface = make_ref_counted(); - mixin_interface->module_own_path = interface.module_own_path; - mixin_interface->is_mixin = true; + auto mixin_interface_ptr = make(); + auto& mixin_interface = *mixin_interface_ptr; + VERIFY(s_interfaces.set(move(mixin_interface_ptr)) == AK::HashSetResult::InsertedNewEntry); + mixin_interface.module_own_path = interface.module_own_path; + mixin_interface.is_mixin = true; assert_string("interface"); consume_whitespace(); assert_string("mixin"); auto offset = lexer.tell(); - parse_interface(*mixin_interface); - if (!mixin_interface->parent_name.is_empty()) + parse_interface(mixin_interface); + if (!mixin_interface.parent_name.is_empty()) report_parsing_error("Mixin interfaces are not allowed to have inherited parents", filename, input, offset); - auto name = mixin_interface->name; - interface.mixins.set(move(name), move(mixin_interface)); + auto name = mixin_interface.name; + interface.mixins.set(move(name), &mixin_interface); } void Parser::parse_callback_function(HashMap& extended_attributes, Interface& interface) @@ -817,15 +820,18 @@ void resolve_function_typedefs(Interface& interface, FunctionType& function) resolve_parameters_typedefs(interface, function.parameters); } -NonnullRefPtr Parser::parse() +Interface& Parser::parse() { auto this_module = Core::File::real_path_for(filename); - auto interface = make_ref_counted(); - interface->module_own_path = this_module; - s_resolved_imports.set(this_module, interface); + auto interface_ptr = make(); + auto& interface = *interface_ptr; + VERIFY(s_interfaces.set(move(interface_ptr)) == AK::HashSetResult::InsertedNewEntry); + interface.module_own_path = this_module; + s_resolved_imports.set(this_module, &interface); - NonnullRefPtrVector imports; + Vector imports; + HashTable required_imported_paths; while (lexer.consume_specific("#import")) { consume_whitespace(); assert_specific('<'); @@ -833,121 +839,121 @@ NonnullRefPtr Parser::parse() lexer.ignore(); auto maybe_interface = resolve_import(path); if (maybe_interface.has_value()) { - for (auto& entry : maybe_interface.value()->required_imported_paths) + for (auto& entry : maybe_interface.value().required_imported_paths) required_imported_paths.set(entry); imports.append(maybe_interface.release_value()); } consume_whitespace(); } - interface->required_imported_paths = required_imported_paths; + interface.required_imported_paths = required_imported_paths; - parse_non_interface_entities(true, *interface); + parse_non_interface_entities(true, interface); if (lexer.consume_specific("interface")) - parse_interface(*interface); + parse_interface(interface); - parse_non_interface_entities(false, *interface); + parse_non_interface_entities(false, interface); for (auto& import : imports) { // FIXME: Instead of copying every imported entity into the current interface, query imports directly for (auto& dictionary : import.dictionaries) - interface->dictionaries.set(dictionary.key, dictionary.value); + interface.dictionaries.set(dictionary.key, dictionary.value); for (auto& enumeration : import.enumerations) { auto enumeration_copy = enumeration.value; enumeration_copy.is_original_definition = false; - interface->enumerations.set(enumeration.key, move(enumeration_copy)); + interface.enumerations.set(enumeration.key, move(enumeration_copy)); } for (auto& typedef_ : import.typedefs) - interface->typedefs.set(typedef_.key, typedef_.value); + interface.typedefs.set(typedef_.key, typedef_.value); for (auto& mixin : import.mixins) { - if (auto it = interface->mixins.find(mixin.key); it != interface->mixins.end() && it->value.ptr() != mixin.value.ptr()) + if (auto it = interface.mixins.find(mixin.key); it != interface.mixins.end() && it->value != mixin.value) report_parsing_error(String::formatted("Mixin '{}' was already defined in {}", mixin.key, mixin.value->module_own_path), filename, input, lexer.tell()); - interface->mixins.set(mixin.key, mixin.value); + interface.mixins.set(mixin.key, mixin.value); } for (auto& callback_function : import.callback_functions) - interface->callback_functions.set(callback_function.key, callback_function.value); + interface.callback_functions.set(callback_function.key, callback_function.value); } // Resolve mixins - if (auto it = interface->included_mixins.find(interface->name); it != interface->included_mixins.end()) { + if (auto it = interface.included_mixins.find(interface.name); it != interface.included_mixins.end()) { for (auto& entry : it->value) { - auto mixin_it = interface->mixins.find(entry); - if (mixin_it == interface->mixins.end()) + auto mixin_it = interface.mixins.find(entry); + if (mixin_it == interface.mixins.end()) report_parsing_error(String::formatted("Mixin '{}' was never defined", entry), filename, input, lexer.tell()); auto& mixin = mixin_it->value; - interface->attributes.extend(mixin->attributes); - interface->constants.extend(mixin->constants); - interface->functions.extend(mixin->functions); - interface->static_functions.extend(mixin->static_functions); - if (interface->has_stringifier && mixin->has_stringifier) - report_parsing_error(String::formatted("Both interface '{}' and mixin '{}' have defined stringifier attributes", interface->name, mixin->name), filename, input, lexer.tell()); + interface.attributes.extend(mixin->attributes); + interface.constants.extend(mixin->constants); + interface.functions.extend(mixin->functions); + interface.static_functions.extend(mixin->static_functions); + if (interface.has_stringifier && mixin->has_stringifier) + report_parsing_error(String::formatted("Both interface '{}' and mixin '{}' have defined stringifier attributes", interface.name, mixin->name), filename, input, lexer.tell()); if (mixin->has_stringifier) { - interface->stringifier_attribute = mixin->stringifier_attribute; - interface->has_stringifier = true; + interface.stringifier_attribute = mixin->stringifier_attribute; + interface.has_stringifier = true; } } } // Resolve typedefs - for (auto& attribute : interface->attributes) - resolve_typedef(*interface, attribute.type, &attribute.extended_attributes); - for (auto& constant : interface->constants) - resolve_typedef(*interface, constant.type); - for (auto& constructor : interface->constructors) - resolve_parameters_typedefs(*interface, constructor.parameters); - for (auto& function : interface->functions) - resolve_function_typedefs(*interface, function); - for (auto& static_function : interface->static_functions) - resolve_function_typedefs(*interface, static_function); - if (interface->value_iterator_type.has_value()) - resolve_typedef(*interface, *interface->value_iterator_type); - if (interface->pair_iterator_types.has_value()) { - resolve_typedef(*interface, interface->pair_iterator_types->get<0>()); - resolve_typedef(*interface, interface->pair_iterator_types->get<1>()); + for (auto& attribute : interface.attributes) + resolve_typedef(interface, attribute.type, &attribute.extended_attributes); + for (auto& constant : interface.constants) + resolve_typedef(interface, constant.type); + for (auto& constructor : interface.constructors) + resolve_parameters_typedefs(interface, constructor.parameters); + for (auto& function : interface.functions) + resolve_function_typedefs(interface, function); + for (auto& static_function : interface.static_functions) + resolve_function_typedefs(interface, static_function); + if (interface.value_iterator_type.has_value()) + resolve_typedef(interface, *interface.value_iterator_type); + if (interface.pair_iterator_types.has_value()) { + resolve_typedef(interface, interface.pair_iterator_types->get<0>()); + resolve_typedef(interface, interface.pair_iterator_types->get<1>()); } - if (interface->named_property_getter.has_value()) - resolve_function_typedefs(*interface, *interface->named_property_getter); - if (interface->named_property_setter.has_value()) - resolve_function_typedefs(*interface, *interface->named_property_setter); - if (interface->indexed_property_getter.has_value()) - resolve_function_typedefs(*interface, *interface->indexed_property_getter); - if (interface->indexed_property_setter.has_value()) - resolve_function_typedefs(*interface, *interface->indexed_property_setter); - if (interface->named_property_deleter.has_value()) - resolve_function_typedefs(*interface, *interface->named_property_deleter); - if (interface->named_property_getter.has_value()) - resolve_function_typedefs(*interface, *interface->named_property_getter); - for (auto& dictionary : interface->dictionaries) { + if (interface.named_property_getter.has_value()) + resolve_function_typedefs(interface, *interface.named_property_getter); + if (interface.named_property_setter.has_value()) + resolve_function_typedefs(interface, *interface.named_property_setter); + if (interface.indexed_property_getter.has_value()) + resolve_function_typedefs(interface, *interface.indexed_property_getter); + if (interface.indexed_property_setter.has_value()) + resolve_function_typedefs(interface, *interface.indexed_property_setter); + if (interface.named_property_deleter.has_value()) + resolve_function_typedefs(interface, *interface.named_property_deleter); + if (interface.named_property_getter.has_value()) + resolve_function_typedefs(interface, *interface.named_property_getter); + for (auto& dictionary : interface.dictionaries) { for (auto& dictionary_member : dictionary.value.members) - resolve_typedef(*interface, dictionary_member.type, &dictionary_member.extended_attributes); + resolve_typedef(interface, dictionary_member.type, &dictionary_member.extended_attributes); } - for (auto& callback_function : interface->callback_functions) - resolve_function_typedefs(*interface, callback_function.value); + for (auto& callback_function : interface.callback_functions) + resolve_function_typedefs(interface, callback_function.value); // Create overload sets - for (auto& function : interface->functions) { - auto& overload_set = interface->overload_sets.ensure(function.name); + for (auto& function : interface.functions) { + auto& overload_set = interface.overload_sets.ensure(function.name); function.overload_index = overload_set.size(); overload_set.append(function); } - for (auto& overload_set : interface->overload_sets) { + for (auto& overload_set : interface.overload_sets) { if (overload_set.value.size() == 1) continue; for (auto& overloaded_function : overload_set.value) overloaded_function.is_overloaded = true; } - for (auto& function : interface->static_functions) { - auto& overload_set = interface->static_overload_sets.ensure(function.name); + for (auto& function : interface.static_functions) { + auto& overload_set = interface.static_overload_sets.ensure(function.name); function.overload_index = overload_set.size(); overload_set.append(function); } - for (auto& overload_set : interface->static_overload_sets) { + for (auto& overload_set : interface.static_overload_sets) { if (overload_set.value.size() == 1) continue; for (auto& overloaded_function : overload_set.value) @@ -955,9 +961,9 @@ NonnullRefPtr Parser::parse() } // FIXME: Add support for overloading constructors - if (interface->will_generate_code()) - interface->required_imported_paths.set(this_module); - interface->imported_modules = move(imports); + if (interface.will_generate_code()) + interface.required_imported_paths.set(this_module); + interface.imported_modules = move(imports); return interface; } diff --git a/Meta/Lagom/Tools/CodeGenerators/LibWeb/WrapperGenerator/IDLParser.h b/Meta/Lagom/Tools/CodeGenerators/LibWeb/WrapperGenerator/IDLParser.h index de0cd00aaf..a735537f82 100644 --- a/Meta/Lagom/Tools/CodeGenerators/LibWeb/WrapperGenerator/IDLParser.h +++ b/Meta/Lagom/Tools/CodeGenerators/LibWeb/WrapperGenerator/IDLParser.h @@ -18,7 +18,7 @@ namespace IDL { class Parser { public: Parser(String filename, StringView contents, String import_base_path); - NonnullRefPtr parse(); + Interface& parse(); private: // https://webidl.spec.whatwg.org/#dfn-special-operation @@ -31,7 +31,7 @@ private: void assert_specific(char ch); void assert_string(StringView expected); void consume_whitespace(); - Optional> resolve_import(auto path); + Optional resolve_import(auto path); HashMap parse_extended_attributes(); void parse_attribute(HashMap& extended_attributes, Interface&); @@ -53,8 +53,9 @@ private: NonnullRefPtr parse_type(); void parse_constant(Interface&); - static HashMap> s_resolved_imports; - HashTable required_imported_paths; + static HashTable> s_interfaces; + static HashMap s_resolved_imports; + String import_base_path; String filename; StringView input; diff --git a/Meta/Lagom/Tools/CodeGenerators/LibWeb/WrapperGenerator/IDLTypes.h b/Meta/Lagom/Tools/CodeGenerators/LibWeb/WrapperGenerator/IDLTypes.h index 6874d0190c..88de8e3374 100644 --- a/Meta/Lagom/Tools/CodeGenerators/LibWeb/WrapperGenerator/IDLTypes.h +++ b/Meta/Lagom/Tools/CodeGenerators/LibWeb/WrapperGenerator/IDLTypes.h @@ -141,7 +141,7 @@ struct CallbackFunction { bool is_legacy_treat_non_object_as_null { false }; }; -struct Interface; +class Interface; struct ParameterizedType : public Type { ParameterizedType() = default; @@ -167,7 +167,13 @@ static inline size_t get_shortest_function_length(Vector const& overl return longest_length; } -struct Interface : public RefCounted { +class Interface { + AK_MAKE_NONCOPYABLE(Interface); + AK_MAKE_NONMOVABLE(Interface); + +public: + explicit Interface() = default; + String name; String parent_name; @@ -198,7 +204,7 @@ struct Interface : public RefCounted { HashMap dictionaries; HashMap enumerations; HashMap typedefs; - HashMap> mixins; + HashMap mixins; HashMap callback_functions; // Added for convenience after parsing @@ -212,7 +218,7 @@ struct Interface : public RefCounted { String module_own_path; HashTable required_imported_paths; - NonnullRefPtrVector imported_modules; + Vector imported_modules; HashMap> overload_sets; HashMap> static_overload_sets; diff --git a/Meta/Lagom/Tools/CodeGenerators/LibWeb/WrapperGenerator/main.cpp b/Meta/Lagom/Tools/CodeGenerators/LibWeb/WrapperGenerator/main.cpp index 5bec84bedf..8f501a9709 100644 --- a/Meta/Lagom/Tools/CodeGenerators/LibWeb/WrapperGenerator/main.cpp +++ b/Meta/Lagom/Tools/CodeGenerators/LibWeb/WrapperGenerator/main.cpp @@ -83,21 +83,21 @@ int main(int argc, char** argv) if (import_base_path.is_null()) import_base_path = lexical_path.dirname(); - auto interface = IDL::Parser(path, data, import_base_path).parse(); + auto& interface = IDL::Parser(path, data, import_base_path).parse(); if (namespace_.is_one_of("Crypto", "CSS", "DOM", "Encoding", "HTML", "UIEvents", "Geometry", "HighResolutionTime", "IntersectionObserver", "NavigationTiming", "RequestIdleCallback", "ResizeObserver", "SVG", "Selection", "URL", "WebSockets", "XHR")) { StringBuilder builder; builder.append(namespace_); builder.append("::"); - builder.append(interface->name); - interface->fully_qualified_name = builder.to_string(); + builder.append(interface.name); + interface.fully_qualified_name = builder.to_string(); } else { - interface->fully_qualified_name = interface->name; + interface.fully_qualified_name = interface.name; } if constexpr (WRAPPER_GENERATOR_DEBUG) { dbgln("Attributes:"); - for (auto& attribute : interface->attributes) { + for (auto& attribute : interface.attributes) { dbgln(" {}{}{} {}", attribute.readonly ? "readonly " : "", attribute.type->name, @@ -106,7 +106,7 @@ int main(int argc, char** argv) } dbgln("Functions:"); - for (auto& function : interface->functions) { + for (auto& function : interface.functions) { dbgln(" {}{} {}", function.return_type->name, function.return_type->nullable ? "?" : "", @@ -120,7 +120,7 @@ int main(int argc, char** argv) } dbgln("Static Functions:"); - for (auto& function : interface->static_functions) { + for (auto& function : interface.static_functions) { dbgln(" static {}{} {}", function.return_type->name, function.return_type->nullable ? "?" : "", @@ -135,34 +135,34 @@ int main(int argc, char** argv) } if (header_mode) - IDL::generate_header(*interface); + IDL::generate_header(interface); if (implementation_mode) - IDL::generate_implementation(*interface); + IDL::generate_implementation(interface); if (constructor_header_mode) - IDL::generate_constructor_header(*interface); + IDL::generate_constructor_header(interface); if (constructor_implementation_mode) - IDL::generate_constructor_implementation(*interface); + IDL::generate_constructor_implementation(interface); if (prototype_header_mode) - IDL::generate_prototype_header(*interface); + IDL::generate_prototype_header(interface); if (prototype_implementation_mode) - IDL::generate_prototype_implementation(*interface); + IDL::generate_prototype_implementation(interface); if (iterator_header_mode) - IDL::generate_iterator_header(*interface); + IDL::generate_iterator_header(interface); if (iterator_implementation_mode) - IDL::generate_iterator_implementation(*interface); + IDL::generate_iterator_implementation(interface); if (iterator_prototype_header_mode) - IDL::generate_iterator_prototype_header(*interface); + IDL::generate_iterator_prototype_header(interface); if (iterator_prototype_implementation_mode) - IDL::generate_iterator_prototype_implementation(*interface); + IDL::generate_iterator_prototype_implementation(interface); return 0; }