Bug 226369

Summary: REGRESSION(r276882): Style not invalidated correctly for media queries in shadow trees that share style
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, ews-watchlist, glenn, jer.noble, philipj, rniwa, sam, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=226530
Attachments:
Description Flags
patch
ews-feeder: commit-queue-
patch
sam: review+
patch none

Antti Koivisto
Reported 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.
Attachments
patch (4.05 KB, patch)
2021-05-28 02:21 PDT, Antti Koivisto
ews-feeder: commit-queue-
patch (9.97 KB, patch)
2021-05-31 05:06 PDT, Antti Koivisto
sam: review+
patch (9.86 KB, patch)
2021-06-02 01:41 PDT, Antti Koivisto
no flags
Antti Koivisto
Comment 1 2021-05-28 02:21:09 PDT
Radar WebKit Bug Importer
Comment 2 2021-05-31 04:53:16 PDT
Antti Koivisto
Comment 3 2021-05-31 05:06:31 PDT
Sam Weinig
Comment 4 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?
Antti Koivisto
Comment 5 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.
Sam Weinig
Comment 6 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 :(.
Ryosuke Niwa
Comment 7 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
Antti Koivisto
Comment 8 2021-06-02 01:41:32 PDT
EWS
Comment 9 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].
Note You need to log in before you can comment on or make changes to this bug.