Skip to content

Commit

Permalink
LibWeb: Don't leak entire realm after parsing CSS @supports rule
Browse files Browse the repository at this point in the history
Before this change, every CSS @supports rule would keep the containing
JS realm alive via a JS::Handle. This led to a GC reference cycle and
the whole realm leaked.

Since we only need the realm at construction time, we can take it as a
parameter instead, and stop storing it.
  • Loading branch information
awesomekling committed Apr 4, 2024
1 parent ce78fcc commit 0cbc489
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 29 deletions.
6 changes: 3 additions & 3 deletions Userland/Libraries/LibWeb/CSS/Parser/Parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ RefPtr<Supports> Parser::parse_a_supports(TokenStream<T>& tokens)
auto maybe_condition = parse_supports_condition(token_stream);
token_stream.skip_whitespace();
if (maybe_condition && !token_stream.has_next_token())
return Supports::create(maybe_condition.release_nonnull());
return Supports::create(m_context.realm(), maybe_condition.release_nonnull());

return {};
}
Expand Down Expand Up @@ -303,7 +303,7 @@ Optional<Supports::Feature> Parser::parse_supports_feature(TokenStream<Component
if (auto declaration = consume_a_declaration(block_tokens); declaration.has_value()) {
transaction.commit();
return Supports::Feature {
Supports::Declaration { declaration->to_string(), JS::make_handle(m_context.realm()) }
Supports::Declaration { declaration->to_string() }
};
}
}
Expand All @@ -316,7 +316,7 @@ Optional<Supports::Feature> Parser::parse_supports_feature(TokenStream<Component
builder.append(item.to_string());
transaction.commit();
return Supports::Feature {
Supports::Selector { builder.to_string().release_value_but_fixme_should_propagate_errors(), JS::make_handle(m_context.realm()) }
Supports::Selector { builder.to_string().release_value_but_fixme_should_propagate_errors() }
};
}

Expand Down
32 changes: 16 additions & 16 deletions Userland/Libraries/LibWeb/CSS/Supports.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,67 +10,67 @@

namespace Web::CSS {

Supports::Supports(NonnullOwnPtr<Condition>&& condition)
Supports::Supports(JS::Realm& realm, NonnullOwnPtr<Condition>&& condition)
: m_condition(move(condition))
{
m_matches = m_condition->evaluate();
m_matches = m_condition->evaluate(realm);
}

bool Supports::Condition::evaluate() const
bool Supports::Condition::evaluate(JS::Realm& realm) const
{
switch (type) {
case Type::Not:
return !children.first().evaluate();
return !children.first().evaluate(realm);
case Type::And:
for (auto& child : children) {
if (!child.evaluate())
if (!child.evaluate(realm))
return false;
}
return true;
case Type::Or:
for (auto& child : children) {
if (child.evaluate())
if (child.evaluate(realm))
return true;
}
return false;
}
VERIFY_NOT_REACHED();
}

bool Supports::InParens::evaluate() const
bool Supports::InParens::evaluate(JS::Realm& realm) const
{
return value.visit(
[&](NonnullOwnPtr<Condition> const& condition) {
return condition->evaluate();
return condition->evaluate(realm);
},
[&](Feature const& feature) {
return feature.evaluate();
return feature.evaluate(realm);
},
[&](GeneralEnclosed const&) {
return false;
});
}

bool Supports::Declaration::evaluate() const
bool Supports::Declaration::evaluate(JS::Realm& realm) const
{
auto style_property = parse_css_supports_condition(Parser::ParsingContext { *realm }, declaration);
auto style_property = parse_css_supports_condition(Parser::ParsingContext { realm }, declaration);
return style_property.has_value();
}

bool Supports::Selector::evaluate() const
bool Supports::Selector::evaluate(JS::Realm& realm) const
{
auto style_property = parse_selector(Parser::ParsingContext { *realm }, selector);
auto style_property = parse_selector(Parser::ParsingContext { realm }, selector);
return style_property.has_value();
}

bool Supports::Feature::evaluate() const
bool Supports::Feature::evaluate(JS::Realm& realm) const
{
return value.visit(
[&](Declaration const& declaration) {
return declaration.evaluate();
return declaration.evaluate(realm);
},
[&](Selector const& selector) {
return selector.evaluate();
return selector.evaluate(realm);
});
}

Expand Down
18 changes: 8 additions & 10 deletions Userland/Libraries/LibWeb/CSS/Supports.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,29 +23,27 @@ class Supports final : public RefCounted<Supports> {
public:
struct Declaration {
String declaration;
JS::Handle<JS::Realm> realm;
bool evaluate() const;
[[nodiscard]] bool evaluate(JS::Realm&) const;
String to_string() const;
};

struct Selector {
String selector;
JS::Handle<JS::Realm> realm;
bool evaluate() const;
[[nodiscard]] bool evaluate(JS::Realm&) const;
String to_string() const;
};

struct Feature {
Variant<Declaration, Selector> value;
bool evaluate() const;
[[nodiscard]] bool evaluate(JS::Realm&) const;
String to_string() const;
};

struct Condition;
struct InParens {
Variant<NonnullOwnPtr<Condition>, Feature, GeneralEnclosed> value;

bool evaluate() const;
[[nodiscard]] bool evaluate(JS::Realm&) const;
String to_string() const;
};

Expand All @@ -58,20 +56,20 @@ class Supports final : public RefCounted<Supports> {
Type type;
Vector<InParens> children;

bool evaluate() const;
[[nodiscard]] bool evaluate(JS::Realm&) const;
String to_string() const;
};

static NonnullRefPtr<Supports> create(NonnullOwnPtr<Condition>&& condition)
static NonnullRefPtr<Supports> create(JS::Realm& realm, NonnullOwnPtr<Condition>&& condition)
{
return adopt_ref(*new Supports(move(condition)));
return adopt_ref(*new Supports(realm, move(condition)));
}

bool matches() const { return m_matches; }
String to_string() const;

private:
Supports(NonnullOwnPtr<Condition>&&);
Supports(JS::Realm&, NonnullOwnPtr<Condition>&&);

NonnullOwnPtr<Condition> m_condition;
bool m_matches { false };
Expand Down

0 comments on commit 0cbc489

Please sign in to comment.