Bug 226369 - REGRESSION(r276882): Style not invalidated correctly for media queries in shadow trees that share style
Summary: REGRESSION(r276882): Style not invalidated correctly for media queries in sha...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-28 02:16 PDT by Antti Koivisto
Modified: 2021-06-02 05:05 PDT (History)
9 users (show)

See Also:


Attachments
patch (4.05 KB, patch)
2021-05-28 02:21 PDT, Antti Koivisto
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
patch (9.97 KB, patch)
2021-05-31 05:06 PDT, Antti Koivisto
sam: review+
Details | Formatted Diff | Diff
patch (9.86 KB, patch)
2021-06-02 01:41 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2021-05-28 02:16:01 PDT
We now share style resolvers between shadow trees with identical styles. Media queries should be evaluated once per resolver, not once per shadow tree.
Comment 1 Antti Koivisto 2021-05-28 02:21:09 PDT
Created attachment 429997 [details]
patch
Comment 2 Radar WebKit Bug Importer 2021-05-31 04:53:16 PDT
<rdar://problem/78684562>
Comment 3 Antti Koivisto 2021-05-31 05:06:31 PDT
Created attachment 430197 [details]
patch
Comment 4 Sam Weinig 2021-06-01 09:17:16 PDT
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?
Comment 5 Antti Koivisto 2021-06-01 09:21:35 PDT
> 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.
Comment 6 Sam Weinig 2021-06-01 09:38:02 PDT
(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 :(.
Comment 7 Ryosuke Niwa 2021-06-01 12:55:54 PDT
(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
Comment 8 Antti Koivisto 2021-06-02 01:41:32 PDT
Created attachment 430328 [details]
patch
Comment 9 EWS 2021-06-02 02:41:51 PDT
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].