Scope::collectResolverScopes should recreate resolver if on doesn't exist #47119
Conversation
…xist rdar://142851714 https://bugs.webkit.org/show_bug.cgi?id=294905 Reviewed by NOBODY (OOPS!). In specific cases, it's possible to get to Scope::collectResolverScopes without an existing resolver in place. One way is by changing the "invert colors" accessibility settings in macOS. Changing the settings sends two notifications to the web process: WebProcess::setScreenProperties and WebCore::Page::accessibilitySettingsDidChange. WebProcess::setScreenProperties wipes out the existing resolver (by indirectly calling Scope::scheduleUpdate). Then WebCore::Page::accessibilitySettingsDidChange indirectly calls document.styleScope().evaluateMediaQueriesForAccessibilitySettingsChange(), which uses Scope::evaluateMediaQueries to re-evaluate media queries in all resolvers. Scope::evaluateMediaQueries sees that the Scope doesn't have a resolver and bails. Fixes this by using Scope::resolver() to get the scope resolver in Scope::collectResolverScopes instead of Scope::resolverIfExists(). Scope::resolver() creates a resolver if one doesn't exists, while Scope::resolverIfExists() doesn't and just returns nullptr. * Source/WebCore/style/StyleScope.cpp: (WebCore::Style::Scope::collectResolverScopes):
|
EWS run on current version of this PR (hash ea1b23c) |
|
Drive-by nit: typo in the commit message "if on" -> "if one". |
|
Having a layout or API test here would be very valuable for this kind of bug. |
The original bug repro is pretty much impossible to replicate in a layout/API test case (I think), but I'll see if there's another way. |
| if (!resolverIfExists()) | ||
| return { }; | ||
|
|
||
| ResolverScopes resolverScopes; | ||
|
|
||
| resolverScopes.add(*resolverIfExists(), Vector<WeakPtr<Scope>> { this }); | ||
| resolverScopes.add(resolver(), Vector<WeakPtr<Scope>> { this }); |
There was a problem hiding this comment.
I think the actual issue here is that we bail out if the top level resolver has been cleared but that does not mean there can't be live resolvers in shadow trees. Doing
if (auto* resolver = resolverIfExists())
resolverScopes.add(*resolver, Vector<WeakPtr<Scope>> { this });
seems to fix this.
There was a problem hiding this comment.
You should make a test case. The original bug says you need dual screens but I can repro just fine with a single screen. See fast/media/mq-inverted-colors-live-update.html, fast/media/mq-inverted-colors-live-update-in-subframes.htm, LayoutTests/accessibility/smart-invert.html for examples.
There was a problem hiding this comment.
To make a test for this you'll need to figure out how to delete the document style resolver without deleting the shadow tree ones. I think you can achieve this by using any non-dynamic media query (one without MediaQueryDynamicDependency in MediaQueryFeature.cpp) on a document stylesheet.
It would be better if the bug title described the problem rather than a solution. |
ea1b23c
ea1b23c