Bug 226369 – REGRESSION(r276882): Style not invalidated correctly for media queries in shadow trees that share style

• Component: CSS
• Status: RESOLVED FIXED

[Antti Koivisto]

We now share style resolvers between shadow trees with identical styles. Media queries should be evaluated once per resolver, not once per shadow tree.

[Antti Koivisto]

Created attachment 429997 [details]
patch

[Radar WebKit Bug Importer]

<rdar://problem/78684562>

[Antti Koivisto]

Created attachment 430197 [details]
patch

[Sam Weinig]

Comment on attachment 430197 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=430197&action=review

> Source/WebCore/style/StyleScope.cpp:684
> +    auto resolverScopes = collectResolverScopes();
> +    for (auto& resolverAndScopes : resolverScopes) {
> +        auto& resolver = resolverAndScopes.key.get();
> +        auto& scopes = resolverAndScopes.value;

I think you can write this:

for (auto& [resolver, scopes] : collectResolverScopes()) {

(though you will then have to call .get() on the resolver below).

> Source/WebCore/style/StyleScope.cpp:691
> +        for (auto& scope : scopes) {

As these scopes are WeakPtrs, what is guaranteeing they are alive? Do they really have to be WeakPtrs? Given how short lived this Vector is, could they Ref<>s? Should you be null checking them?

[Antti Koivisto]

> As these scopes are WeakPtrs, what is guaranteeing they are alive? Do they
> really have to be WeakPtrs? Given how short lived this Vector is, could they
> Ref<>s? Should you be null checking them?

Logically they are guaranteed to be non-null. I'm just using WeakPtrs defensively here.

[Sam Weinig]

(In reply to Antti Koivisto from comment #5)
> > As these scopes are WeakPtrs, what is guaranteeing they are alive? Do they
> > really have to be WeakPtrs? Given how short lived this Vector is, could they
> > Ref<>s? Should you be null checking them?
> 
> Logically they are guaranteed to be non-null. I'm just using WeakPtrs
> defensively here.

I guess we can't use Refs as they are not reference counted. It's going to take me some time (if ever) to become comfortable with defensive use of WeakPtr :(.

[Ryosuke Niwa]

(In reply to Sam Weinig from comment #6)
> (In reply to Antti Koivisto from comment #5)
> > > As these scopes are WeakPtrs, what is guaranteeing they are alive? Do they
> > > really have to be WeakPtrs? Given how short lived this Vector is, could they
> > > Ref<>s? Should you be null checking them?
> > 
> > Logically they are guaranteed to be non-null. I'm just using WeakPtrs
> > defensively here.
> 
> I guess we can't use Refs as they are not reference counted. It's going to
> take me some time (if ever) to become comfortable with defensive use of
> WeakPtr :(.

I'm adding CheckedPtr for this exact use case: https://bugs.webkit.org/show_bug.cgi?id=226158

[Antti Koivisto]

Created attachment 430328 [details]
patch

[EWS]

Committed r278346 (238378@main): <https://commits.webkit.org/238378@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430328 [details].