From cfe663435ed0780440e52bc66d69d787f6c0af2f Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Sun, 21 May 2023 12:42:22 +0200 Subject: [PATCH] LibWeb+LibJS: Don't lazily construct web prototypes in cell constructors It's not safe to allocate from the GC heap while in the constructor of a GC heap cell. (Because if this ends up triggering a collection, we may end up trying to call through an uninitialized vtable). This was already done safely in the initialize() virtual in much of LibJS and LibWeb. This patch moves the logic for prototypes, mixins, and CSSStyleDeclaration as well. Fixes a long-standing GC crash that was pretty easy to reproduce by refreshing https://vercel.com/ --- .../BindingsGenerator/IDLGenerators.cpp | 40 ++++++++++++++++++- Userland/Libraries/LibJS/Runtime/Object.h | 4 +- .../LibWeb/CSS/CSSStyleDeclaration.cpp | 9 ++++- .../LibWeb/CSS/CSSStyleDeclaration.h | 1 + 4 files changed, 50 insertions(+), 4 deletions(-) diff --git a/Meta/Lagom/Tools/CodeGenerators/LibWeb/BindingsGenerator/IDLGenerators.cpp b/Meta/Lagom/Tools/CodeGenerators/LibWeb/BindingsGenerator/IDLGenerators.cpp index 251575c70a0..6cd38439048 100644 --- a/Meta/Lagom/Tools/CodeGenerators/LibWeb/BindingsGenerator/IDLGenerators.cpp +++ b/Meta/Lagom/Tools/CodeGenerators/LibWeb/BindingsGenerator/IDLGenerators.cpp @@ -2504,6 +2504,8 @@ static void generate_prototype_or_global_mixin_definitions(IDL::Interface const& generator.set("namespaced_name", interface.namespaced_name); generator.set("class_name", class_name); generator.set("fully_qualified_name", interface.fully_qualified_name); + generator.set("parent_name", interface.parent_name); + generator.set("prototype_base_class", interface.prototype_base_class); if (interface.pair_iterator_types.has_value()) { generator.set("iterator_name", DeprecatedString::formatted("{}Iterator", interface.name)); @@ -2515,6 +2517,7 @@ static void generate_prototype_or_global_mixin_definitions(IDL::Interface const& #define define_direct_property (object.define_direct_property) #define define_native_accessor (object.define_native_accessor) #define define_native_function (object.define_native_function) +#define set_prototype (object.set_prototype) JS::ThrowCompletionOr @class_name@::initialize(JS::Realm& realm, JS::Object& object) { @@ -2535,6 +2538,20 @@ JS::ThrowCompletionOr @class_name@::initialize(JS::Realm& realm) )~~~"); + if (interface.prototype_base_class == "ObjectPrototype") { + generator.append(R"~~~( + + set_prototype(realm.intrinsics().object_prototype()); + +)~~~"); + } else { + generator.append(R"~~~( + + set_prototype(&ensure_web_prototype<@prototype_base_class@>(realm, "@parent_name@")); + +)~~~"); + } + if (interface.has_unscopable_member) { generator.append(R"~~~( auto unscopable_object = JS::Object::create(realm, nullptr); @@ -3710,7 +3727,7 @@ namespace Web::Bindings { )~~~"); } else if (!interface.parent_name.is_empty()) { generator.append(R"~~~( - : Object(ConstructWithPrototypeTag::Tag, ensure_web_prototype<@prototype_base_class@>(realm, "@parent_name@")) + : Object(realm, nullptr) )~~~"); } else { generator.append(R"~~~( @@ -3781,7 +3798,9 @@ void generate_iterator_prototype_implementation(IDL::Interface const& interface, SourceGenerator generator { builder }; generator.set("name", DeprecatedString::formatted("{}Iterator", interface.name)); + generator.set("parent_name", interface.parent_name); generator.set("prototype_class", DeprecatedString::formatted("{}IteratorPrototype", interface.name)); + generator.set("prototype_base_class", interface.prototype_base_class); generator.set("fully_qualified_name", DeprecatedString::formatted("{}Iterator", interface.fully_qualified_name)); generator.set("possible_include_path", DeprecatedString::formatted("{}Iterator", interface.name.replace("::"sv, "/"sv, ReplaceMode::All))); @@ -3795,6 +3814,7 @@ void generate_iterator_prototype_implementation(IDL::Interface const& interface, #include #include #include +#include #if __has_include() # include @@ -3842,6 +3862,24 @@ JS::ThrowCompletionOr @prototype_class@::initialize(JS::Realm& realm) auto& vm = this->vm(); MUST_OR_THROW_OOM(Base::initialize(realm)); +)~~~"); + + if (interface.prototype_base_class == "ObjectPrototype") { + generator.append(R"~~~( + + set_prototype(realm.intrinsics().object_prototype()); + +)~~~"); + } else { + generator.append(R"~~~( + + set_prototype(&ensure_web_prototype<@prototype_base_class@>(realm, "@parent_name@")); + +)~~~"); + } + + generator.append(R"~~~( + define_native_function(realm, vm.names.next, next, 0, JS::Attribute::Writable | JS::Attribute::Enumerable | JS::Attribute::Configurable); define_direct_property(vm.well_known_symbol_to_string_tag(), MUST_OR_THROW_OOM(JS::PrimitiveString::create(vm, "Iterator"sv)), JS::Attribute::Configurable); diff --git a/Userland/Libraries/LibJS/Runtime/Object.h b/Userland/Libraries/LibJS/Runtime/Object.h index cea4538d10f..2d01081d45b 100644 --- a/Userland/Libraries/LibJS/Runtime/Object.h +++ b/Userland/Libraries/LibJS/Runtime/Object.h @@ -187,6 +187,8 @@ public: template bool fast_is() const = delete; + void set_prototype(Object*); + protected: enum class GlobalObjectTag { Tag }; enum class ConstructWithoutPrototypeTag { Tag }; @@ -198,8 +200,6 @@ protected: Object(ConstructWithPrototypeTag, Object& prototype); explicit Object(Shape&); - void set_prototype(Object*); - // [[Extensible]] bool m_is_extensible { true }; diff --git a/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.cpp b/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.cpp index 8dcbd800938..b734a2304f8 100644 --- a/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.cpp +++ b/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.cpp @@ -16,10 +16,17 @@ namespace Web::CSS { CSSStyleDeclaration::CSSStyleDeclaration(JS::Realm& realm) - : PlatformObject(Bindings::ensure_web_prototype(realm, "CSSStyleDeclaration")) + : PlatformObject(realm) { } +JS::ThrowCompletionOr CSSStyleDeclaration::initialize(JS::Realm& realm) +{ + MUST_OR_THROW_OOM(Base::initialize(realm)); + set_prototype(&Bindings::ensure_web_prototype(realm, "CSSStyleDeclaration")); + return {}; +} + WebIDL::ExceptionOr> PropertyOwningCSSStyleDeclaration::create(JS::Realm& realm, Vector properties, HashMap custom_properties) { return MUST_OR_THROW_OOM(realm.heap().allocate(realm, realm, move(properties), move(custom_properties))); diff --git a/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.h b/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.h index 6bb10890abe..c349dc4a202 100644 --- a/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.h +++ b/Userland/Libraries/LibWeb/CSS/CSSStyleDeclaration.h @@ -19,6 +19,7 @@ class CSSStyleDeclaration : public Bindings::PlatformObject { public: virtual ~CSSStyleDeclaration() = default; + virtual JS::ThrowCompletionOr initialize(JS::Realm&) override; virtual size_t length() const = 0; virtual DeprecatedString item(size_t index) const = 0;