LibGC+ClangPlugins: Forbid non-trivial destructors in Cell subclasses

Add a clang plugin check that flags GC::Cell subclasses (and their
base classes within the Cell hierarchy) that have destructors with
non-trivial bodies. Such logic should use Cell::finalize() instead.

Add GC_ALLOW_CELL_DESTRUCTOR annotation macro for opting out in
exceptional cases (currently only JS::Object).

This prevents us from accidentally adding code in destructors that
runs after something we're pointing to may have been destroyed.
(This could become a problem when the garbage collector sweeps
objects in an unfortunate order.)

This new check uncovered a handful of bugs which are then also fixed
in this commit. :^)

Libraries/LibGC/Cell.h

@@ -25,8 +25,10 @@ namespace GC {
 // It should only be used when the lifetime of the GC-allocated member is always longer than the object
 #if defined(AK_COMPILER_CLANG)
 #    define IGNORE_GC [[clang::annotate("serenity::ignore_gc")]]
+#    define GC_ALLOW_CELL_DESTRUCTOR [[clang::annotate("ladybird::allow_cell_destructor")]]
 #else
 #    define IGNORE_GC
+#    define GC_ALLOW_CELL_DESTRUCTOR
 #endif
 
 #define GC_CELL(class_, base_class)                \

Libraries/LibJS/Runtime/Object.h

@@ -79,7 +79,7 @@ public:
     static GC::Ref<Object> create_with_premade_shape(Shape&);
 
     virtual void initialize(Realm&) override;
-    virtual ~Object();
+    GC_ALLOW_CELL_DESTRUCTOR virtual ~Object();
 
     enum class PropertyKind {
         Key,

Libraries/LibJS/Runtime/PrimitiveString.cpp

@@ -140,8 +140,11 @@ PrimitiveString::PrimitiveString(String string)
 {
 }
 
-PrimitiveString::~PrimitiveString()
+PrimitiveString::~PrimitiveString() = default;
+
+void PrimitiveString::finalize()
 {
+    Base::finalize();
     if (has_utf16_string()) {
         auto const& string = *m_utf16_string;
         if (string.length_in_code_units() <= MAX_LENGTH_FOR_STRING_CACHE)

Libraries/LibJS/Runtime/PrimitiveString.h

@@ -25,6 +25,8 @@ class JS_API PrimitiveString : public Cell {
     GC_DECLARE_ALLOCATOR(PrimitiveString);
 
 public:
+    static constexpr bool OVERRIDES_FINALIZE = true;
+
     [[nodiscard]] static GC::Ref<PrimitiveString> create(VM&, Utf16String const&);
     [[nodiscard]] static GC::Ref<PrimitiveString> create(VM&, Utf16View const&);
     [[nodiscard]] static GC::Ref<PrimitiveString> create(VM&, Utf16FlyString const&);
@@ -37,7 +39,7 @@ public:
 
     [[nodiscard]] static GC::Ref<PrimitiveString> create_from_unsigned_integer(VM&, u64);
 
-    virtual ~PrimitiveString();
+    virtual ~PrimitiveString() override;
 
     PrimitiveString(PrimitiveString const&) = delete;
     PrimitiveString& operator=(PrimitiveString const&) = delete;
@@ -78,6 +80,8 @@ protected:
 private:
     friend class RopeString;
 
+    virtual void finalize() override;
+
     explicit PrimitiveString(Utf16String);
     explicit PrimitiveString(String);
 

Libraries/LibJS/Runtime/Shape.cpp

@@ -16,8 +16,11 @@ GC_DEFINE_ALLOCATOR(PrototypeChainValidity);
 
 static HashTable<GC::Ptr<Shape>> s_all_prototype_shapes;
 
-Shape::~Shape()
+Shape::~Shape() = default;
+
+void Shape::finalize()
 {
+    Base::finalize();
     if (m_is_prototype_shape)
         s_all_prototype_shapes.remove(this);
 }

Libraries/LibJS/Runtime/Shape.h

@@ -59,6 +59,8 @@ class JS_API Shape final : public Cell {
     GC_DECLARE_ALLOCATOR(Shape);
 
 public:
+    static constexpr bool OVERRIDES_FINALIZE = true;
+
     virtual ~Shape() override;
 
     enum class TransitionType : u8 {
@@ -113,6 +115,7 @@ private:
     void invalidate_prototype_if_needed_for_change_without_transition();
     void invalidate_all_prototype_chains_leading_to_this();
 
+    virtual void finalize() override;
     virtual void visit_edges(Visitor&) override;
 
     [[nodiscard]] GC::Ptr<Shape> get_or_prune_cached_forward_transition(TransitionKey const&);

Libraries/LibWeb/Crypto/CryptoKey.cpp

@@ -44,8 +44,9 @@ CryptoKey::CryptoKey(JS::Realm& realm)
 {
 }
 
-CryptoKey::~CryptoKey()
+void CryptoKey::finalize()
 {
+    Base::finalize();
     m_key_data.visit(
         [](ByteBuffer& data) { secure_zero(data.data(), data.size()); },
         [](auto& data) { secure_zero(reinterpret_cast<u8*>(&data), sizeof(data)); });

Libraries/LibWeb/Crypto/CryptoKey.h

@@ -29,11 +29,11 @@ class CryptoKey final
 public:
     using InternalKeyData = Variant<ByteBuffer, Bindings::JsonWebKey, ::Crypto::PK::RSAPublicKey, ::Crypto::PK::RSAPrivateKey, ::Crypto::PK::ECPublicKey, ::Crypto::PK::ECPrivateKey, ::Crypto::PK::MLDSAPublicKey, ::Crypto::PK::MLDSAPrivateKey, ::Crypto::PK::MLKEMPublicKey, ::Crypto::PK::MLKEMPrivateKey>;
 
+    static constexpr bool OVERRIDES_FINALIZE = true;
+
     [[nodiscard]] static GC::Ref<CryptoKey> create(JS::Realm&, InternalKeyData);
     [[nodiscard]] static GC::Ref<CryptoKey> create(JS::Realm&);
 
-    virtual ~CryptoKey() override;
-
     bool extractable() const { return m_extractable; }
     Bindings::KeyType type() const { return m_type; }
     JS::Object const* algorithm() const { return m_algorithm; }
@@ -59,6 +59,7 @@ private:
 
     virtual void initialize(JS::Realm&) override;
     virtual void visit_edges(Visitor&) override;
+    virtual void finalize() override;
 
     Bindings::KeyType m_type;
     bool m_extractable { false };

Libraries/LibWeb/DOM/Range.cpp

@@ -77,8 +77,11 @@ Range::Range(GC::Ref<Node> start_container, WebIDL::UnsignedLong start_offset, G
     live_ranges().set(this);
 }
 
-Range::~Range()
+Range::~Range() = default;
+
+void Range::finalize()
 {
+    Base::finalize();
     live_ranges().remove(this);
 }
 

Libraries/LibWeb/DOM/Range.h

@@ -32,6 +32,8 @@ class WEB_API Range final : public AbstractRange {
     GC_DECLARE_ALLOCATOR(Range);
 
 public:
+    static constexpr bool OVERRIDES_FINALIZE = true;
+
     [[nodiscard]] static GC::Ref<Range> create(Document&);
     [[nodiscard]] static GC::Ref<Range> create(HTML::Window&);
     [[nodiscard]] static GC::Ref<Range> create(GC::Ref<Node> start_container, WebIDL::UnsignedLong start_offset, GC::Ref<Node> end_container, WebIDL::UnsignedLong end_offset);
@@ -127,6 +129,7 @@ private:
 
     virtual void initialize(JS::Realm&) override;
     virtual void visit_edges(Cell::Visitor&) override;
+    virtual void finalize() override;
 
     GC::Ref<Node> root() const;
 

Libraries/LibWeb/HTML/BrowsingContextGroup.cpp

@@ -26,8 +26,9 @@ BrowsingContextGroup::BrowsingContextGroup(GC::Ref<Web::Page> page)
     user_agent_browsing_context_group_set().set(*this);
 }
 
-BrowsingContextGroup::~BrowsingContextGroup()
+void BrowsingContextGroup::finalize()
 {
+    Base::finalize();
     user_agent_browsing_context_group_set().remove(*this);
 }
 

Libraries/LibWeb/HTML/BrowsingContextGroup.h

@@ -22,9 +22,9 @@ public:
         GC::Ref<HTML::BrowsingContextGroup> browsing_context;
         GC::Ref<DOM::Document> document;
     };
-    static WebIDL::ExceptionOr<BrowsingContextGroupAndDocument> create_a_new_browsing_context_group_and_document(GC::Ref<Page>);
+    static constexpr bool OVERRIDES_FINALIZE = true;
 
-    ~BrowsingContextGroup();
+    static WebIDL::ExceptionOr<BrowsingContextGroupAndDocument> create_a_new_browsing_context_group_and_document(GC::Ref<Page>);
 
     Page& page() { return m_page; }
     Page const& page() const { return m_page; }
@@ -38,6 +38,7 @@ private:
     explicit BrowsingContextGroup(GC::Ref<Web::Page>);
 
     virtual void visit_edges(Cell::Visitor&) override;
+    virtual void finalize() override;
 
     // https://html.spec.whatwg.org/multipage/browsers.html#browsing-context-group-set
     OrderedHashTable<GC::Ref<BrowsingContext>> m_browsing_context_set;

Libraries/LibWeb/HTML/NavigableContainer.cpp

@@ -38,8 +38,11 @@ NavigableContainer::NavigableContainer(DOM::Document& document, DOM::QualifiedNa
     all_instances().set(this);
 }
 
-NavigableContainer::~NavigableContainer()
+NavigableContainer::~NavigableContainer() = default;
+
+void NavigableContainer::finalize()
 {
+    Base::finalize();
     all_instances().remove(this);
 }
 

Libraries/LibWeb/HTML/NavigableContainer.h

@@ -16,6 +16,8 @@ class WEB_API NavigableContainer : public HTMLElement {
     WEB_PLATFORM_OBJECT(NavigableContainer, HTMLElement);
 
 public:
+    static constexpr bool OVERRIDES_FINALIZE = true;
+
     static GC::Ptr<NavigableContainer> navigable_container_with_content_navigable(GC::Ref<Navigable> navigable);
 
     virtual ~NavigableContainer() override;
@@ -63,6 +65,9 @@ protected:
 
 private:
     virtual bool is_navigable_container() const override { return true; }
+
+    virtual void finalize() override;
+
     bool m_potentially_delays_the_load_event { true };
 };
 

Libraries/LibWeb/HTML/Timer.cpp

@@ -35,8 +35,9 @@ void Timer::visit_edges(Cell::Visitor& visitor)
     visitor.visit_possible_values(m_timer->on_timeout.raw_capture_range());
 }
 
-Timer::~Timer()
+void Timer::finalize()
 {
+    Base::finalize();
     VERIFY(!m_timer->is_active());
 }
 

Libraries/LibWeb/HTML/Timer.h

@@ -27,8 +27,9 @@ public:
         Yes,
     };
 
+    static constexpr bool OVERRIDES_FINALIZE = true;
+
     static GC::Ref<Timer> create(JS::Object&, i32 milliseconds, Function<void()> callback, i32 id, Repeating);
-    virtual ~Timer() override;
 
     void start();
     void stop();
@@ -39,6 +40,7 @@ private:
     Timer(JS::Object& window, i32 milliseconds, Function<void()> callback, i32 id, Repeating);
 
     virtual void visit_edges(Cell::Visitor&) override;
+    virtual void finalize() override;
 
     RefPtr<Core::Timer> m_timer;
     GC::Ref<JS::Object> m_window_or_worker_global_scope;

Meta/Lagom/ClangPlugins/LibJSGCPluginAction.cpp

@@ -496,6 +496,45 @@ bool LibJSGCVisitor::VisitCXXRecordDecl(clang::CXXRecordDecl* record)
     check_override_requires_flag("must_survive_garbage_collection", "OVERRIDES_MUST_SURVIVE_GARBAGE_COLLECTION");
     check_override_requires_flag("finalize", "OVERRIDES_FINALIZE");
 
+    // Check that Cell subclasses (and all their base classes) don't have non-trivial destructors.
+    // They should override Cell::finalize() instead.
+    auto check_no_nontrivial_destructor = [&](clang::CXXRecordDecl const* check_record) {
+        if (!check_record || !check_record->isCompleteDefinition())
+            return;
+        if (check_record->getQualifiedNameAsString() == "GC::Cell")
+            return;
+        auto const* destructor = check_record->getDestructor();
+        if (!destructor || !destructor->isUserProvided())
+            return;
+        // Only flag destructors whose body we can see, that aren't defaulted,
+        // and that have a non-empty body. This way, out-of-line `= default` destructors
+        // and empty-body destructors `~Foo() {}` are fine.
+        if (!destructor->getBody() || destructor->isDefaulted())
+            return;
+        if (auto const* body = llvm::dyn_cast<clang::CompoundStmt>(destructor->getBody())) {
+            if (body->body_empty())
+                return;
+        }
+        if (decl_has_annotation(destructor, "ladybird::allow_cell_destructor"))
+            return;
+        auto diag_id = diag_engine.getCustomDiagID(clang::DiagnosticsEngine::Error,
+            "GC::Cell-inheriting class %0 has a non-trivial destructor; override Cell::finalize() instead (and set OVERRIDES_FINALIZE)");
+        auto builder = diag_engine.Report(destructor->getBeginLoc(), diag_id);
+        builder << check_record->getName();
+    };
+
+    check_no_nontrivial_destructor(record);
+    record->forallBases([&](clang::CXXRecordDecl const* base) -> bool {
+        if (base->getQualifiedNameAsString() == "GC::Cell")
+            return false;
+        // Only check bases that are themselves part of the Cell hierarchy.
+        // Non-Cell mixins (e.g. Weakable) are not our concern here.
+        if (!record_inherits_from_cell(*base))
+            return true;
+        check_no_nontrivial_destructor(base);
+        return true;
+    });
+
     clang::DeclarationName name = &m_context.Idents.get("visit_edges");
     auto const* visit_edges_method = record->lookup(name).find_first<clang::CXXMethodDecl>();
     if (!visit_edges_method && !fields_that_need_visiting.empty()) {