RESOLVED FIXED226369
REGRESSION(r276882): Style not invalidated correctly for media queries in shadow trees that share style 
https://bugs.webkit.org/show_bug.cgi?id=226369
Summary REGRESSION(r276882): Style not invalidated correctly for media queries in sha...
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-
DetailsFormatted DiffDiff
patch (9.97 KB, patch)
2021-05-31 05:06 PDT, Antti Koivisto 		sam: review+
DetailsFormatted DiffDiff
patch (9.86 KB, patch)
2021-06-02 01:41 PDT, Antti Koivisto 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Antti Koivisto
Comment 1 2021-05-28 02:21:09 PDT
Created attachment 429997 [details] patch
Radar WebKit Bug Importer
Comment 2 2021-05-31 04:53:16 PDT
<rdar://problem/78684562>
Antti Koivisto
Comment 3 2021-05-31 05:06:31 PDT
Created attachment 430197 [details] patch
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
Created attachment 430328 [details] patch
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.