| Summary: | CSS custom properties on pseudo elements background gradients causes infinite layout and high CPU load | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Vladimir <nolimits4web> | ||||||||||||||||||||||
| Component: | Animations | Assignee: | Antoine Quint <graouts> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||
| Severity: | Normal | CC: | ali.ghassemi, changseok, denton.don, dino, esprehn+autocc, ews-watchlist, glenn, graouts, graouts, gyuyoung.kim, jond, jonlee, koivisto, kondapallykalyan, macpherson, menard, pdr, phil+webkit, simon.fraser, webkit-bug-importer | ||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
| Version: | Safari 12 | ||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||
| OS: | macOS 10.14 | ||||||||||||||||||||||||
| URL: | https://jsfiddle.net/4gb9ynLz/ | ||||||||||||||||||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=226924 https://bugs.webkit.org/show_bug.cgi?id=227926 |
||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
This also occurs when loading background images. Replicated in a custom component where the background image of a button was repeatedly requested when a change to the button was expected. This could include a transform or simply a class change.
For example, the following snippet will cause the background image to reload constantly.
button {
background-color: transparent;
background-image: var(--indago-action-menu-icon);
transform: var(--indago-action-panel-transform);
transition: var(--indago-action-panel-transition);
}
This occurs on both iOS and MacOS. Lesser impacts include flickering of the images due to unnecessary reloading, but the CPU and the GET requests are significantly more severe, even if less obvious to the end-user.
Other examples currently experienced include a reload on every :hover.
It appears as though the alpha-channel is actually what is the final trigger for the problem. *Not* the custom property as the title of this bug indicates. Change the `rgba` functions to `rgb` in both locations and the problem vanishes. I do believe that the `transition` is also necessary to reproduce. Minimal repro: https://jsfiddle.net/0ph9xkrt/7/ In this example, when you hover over the paragraph, the rendering never ends. It only works when all the following are true: 1. a transition is defined that is not instantaneous 2. there is an alpha channel in the color that is *exclusively* between 0 and 1 3. there is at least one color channel which is *exclusively* between 0 and 255. The original example from this bug report pegs the CPU harder than mine, but I believe they are the same bug at its core. It appears as though the custom property is not what causes this bug as the title suggests. Here is a minimal repro, as far as I can tell: https://jsfiddle.net/0ph9xkrt/7/ You need the following: 1. a transition is defined that is not instantaneous 2. there is an alpha channel in the color that is *exclusively* between 0 and 1 3. there is at least one color channel which is *exclusively* between 0 and 255 The linked example has some interactions you may observe. Created attachment 369148 [details]
Standalone version of the JSFiddle
Animation logging shows us animating both box-shadow and -webkit-box-shadow. I wonder if this triggers the infinite animating: Created ImplicitAnimation 0x10b4ec720 on element 0x133100070 for property box-shadow duration 0.50 delay 0.00 Created ImplicitAnimation 0x10b4ec7b8 on element 0x133100070 for property -webkit-box-shadow duration 0.50 delay 0.00 0x10b4ec720 AnimationState New -> New 0x10b4ec720 AnimationState New -> StartWaitTimer blending ShadowData at 0 0x10b4ec720 AnimationState StartWaitTimer -> StartWaitStyleAvailable (time is 0.000000) 0x10b4ec7b8 AnimationState New -> New 0x10b4ec7b8 AnimationState New -> StartWaitTimer blending ShadowData at 0 0x10b4ec7b8 AnimationState StartWaitTimer -> StartWaitStyleAvailable (time is 0.000000) 0x10b4ec720 AnimationState StartWaitStyleAvailable -> StartWaitResponse (time is -1.000000) 0x10b4ec7b8 AnimationState StartWaitStyleAvailable -> StartWaitResponse (time is -1.000000) 0x10b4ec720 AnimationState StartWaitResponse -> StartTimeSet (time is 582830.660499) 0x10b4ec720 AnimationState StartWaitResponse -> Ending 0x10b4ec7b8 AnimationState StartWaitResponse -> StartTimeSet (time is 582830.660499) 0x10b4ec7b8 AnimationState StartWaitResponse -> Ending blending ShadowData at 0.00 blending ShadowData at 0.00 ... updateAnimationTimer: timeToNextService is 0.00 blending ShadowData at 1.00 blending ShadowData at 1.00 updateAnimationTimer: timeToNextService is 0.00 0x10b4ec720 AnimationState Ending -> Ending 0x10b4ec720 AnimationState Ending -> Done (time is 0.500000) CSSAnimationControllerPrivate 0x10b496140 animationWillBeRemoved: 0x10b4ec720 Removing ImplicitAnimation 0x10b4ec720 from element 0x133100070 for property box-shadow blending ShadowData at 1.00 0x10b4ec7b8 AnimationState Ending -> Ending 0x10b4ec7b8 AnimationState Ending -> Done (time is 0.500000) updateAnimationTimer: timeToNextService is 0.00 Created ImplicitAnimation 0x10b4ecd10 on element 0x133100070 for property box-shadow duration 0.50 delay 0.00 CSSAnimationControllerPrivate 0x10b496140 animationWillBeRemoved: 0x10b4ec7b8 Removing ImplicitAnimation 0x10b4ec7b8 from element 0x133100070 for property -webkit-box-shadow 0x10b4ecd10 AnimationState New -> New 0x10b4ecd10 AnimationState New -> StartWaitTimer blending ShadowData at 0 0x10b4ecd10 AnimationState StartWaitTimer -> StartWaitStyleAvailable (time is 0.000000) 0x10b4ecd10 AnimationState StartWaitStyleAvailable -> StartWaitResponse (time is -1.000000) 0x10b4ecd10 AnimationState StartWaitResponse -> StartTimeSet (time is 582831.176258) 0x10b4ecd10 AnimationState StartWaitResponse -> Ending Created ImplicitAnimation 0x10b4ecda8 on element 0x133100070 for property -webkit-box-shadow duration 0.50 delay 0.00 blending ShadowData at 0.00 0x10b4ecda8 AnimationState New -> New 0x10b4ecda8 AnimationState New -> StartWaitTimer blending ShadowData at 0 0x10b4ecda8 AnimationState StartWaitTimer -> StartWaitStyleAvailable (time is 0.000000) 0x10b4ecda8 AnimationState StartWaitStyleAvailable -> StartWaitResponse (time is -1.000000) 0x10b4ecda8 AnimationState StartWaitResponse -> StartTimeSet (time is 582831.177199) 0x10b4ecda8 AnimationState StartWaitResponse -> Ending blending ShadowData at 0.00 blending ShadowData at 0.00 ... updateAnimationTimer: timeToNextService is 0.00 blending ShadowData at 1.00 blending ShadowData at 1.00 updateAnimationTimer: timeToNextService is 0.00 0x10b4ecd10 AnimationState Ending -> Ending 0x10b4ecd10 AnimationState Ending -> Done (time is 0.500000) CSSAnimationControllerPrivate 0x10b496140 animationWillBeRemoved: 0x10b4ecd10 Removing ImplicitAnimation 0x10b4ecd10 from element 0x133100070 for property box-shadow blending ShadowData at 1.00 0x10b4ecda8 AnimationState Ending -> Ending 0x10b4ecda8 AnimationState Ending -> Done (time is 0.500000) updateAnimationTimer: timeToNextService is 0.00 Created ImplicitAnimation 0x10b4ec688 on element 0x133100070 for property box-shadow duration 0.50 delay 0.00 CSSAnimationControllerPrivate 0x10b496140 animationWillBeRemoved: 0x10b4ecda8 Removing ImplicitAnimation 0x10b4ecda8 from element 0x133100070 for property -webkit-box-shadow 0x10b4ec688 AnimationState New -> New 0x10b4ec688 AnimationState New -> StartWaitTimer blending ShadowData at 0 0x10b4ec688 AnimationState StartWaitTimer -> StartWaitStyleAvailable (time is 0.000000) 0x10b4ec688 AnimationState StartWaitStyleAvailable -> StartWaitResponse (time is -1.000000) 0x10b4ec688 AnimationState StartWaitResponse -> StartTimeSet (time is 582831.683405) 0x10b4ec688 AnimationState StartWaitResponse -> Ending Created ImplicitAnimation 0x10b4ec720 on element 0x133100070 for property -webkit-box-shadow duration 0.50 delay 0.00 blending ShadowData at 0.00 0x10b4ec720 AnimationState New -> New 0x10b4ec720 AnimationState New -> StartWaitTimer blending ShadowData at 0 0x10b4ec720 AnimationState StartWaitTimer -> StartWaitStyleAvailable (time is 0.000000) 0x10b4ec720 AnimationState StartWaitStyleAvailable -> StartWaitResponse (time is -1.000000) 0x10b4ec720 AnimationState StartWaitResponse -> StartTimeSet (time is 582831.684502) 0x10b4ec720 AnimationState StartWaitResponse -> Ending blending ShadowData at 0.00 blending ShadowData at 0.00 ... I'm not seeing the same CPU load with the latest JSFiddle or the standalone example. The original example still appears to show 100% CPU however. We also ran into this bug @ LinkedIn (In reply to Jon Lee from comment #7) > I'm not seeing the same CPU load with the latest JSFiddle or the standalone > example. The original example still appears to show 100% CPU however. Seeing the same thing with the standalone example but the original jsfiddle shows continuous CPU usage in Web Inspector, but nothing like 100% CPU for the WebProcess. Created attachment 427745 [details]
Reduction showing continuous updates in Web Inspector timeline
I have a reduction that shows the continuous updates in the Web Inspector timeline. It also logs any new transition and you'll see we create a background-image transition continuously, where we shouldn't event be generating one in the first place. I believe this is the bug here.
Created attachment 427747 [details]
Reduction
Created attachment 427754 [details]
Patch
The patch addresses both the reduced test case and the other original JSFiddle link. Created attachment 427756 [details]
Patch
Created attachment 427763 [details]
Patch
Comment on attachment 427763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427763&action=review > Source/WebCore/rendering/style/FillLayer.cpp:406 > for (; layer1 && layer2; layer1 = layer1->next(), layer2 = layer2->next()) { > - if (!arePointingToEqualData(layer1->image(), layer2->image())) > + if (layer1->image() != layer2->image()) > return false; You should also rename imagesIdentical to something that reflects its only call site, imageChangeRequiresClientReregistration or something. Comment on attachment 427763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427763&action=review > Source/WebCore/rendering/style/StyleGeneratedImage.cpp:51 > + if (is<StyleGeneratedImage>(other)) { > + auto& otherStyleGeneratedImage = downcast<StyleGeneratedImage>(other); > + if (is<CSSGradientValue>(m_imageGeneratorValue) && is<CSSGradientValue>(otherStyleGeneratedImage.m_imageGeneratorValue)) { > + auto& cssGradientValueA = downcast<CSSGradientValue>(m_imageGeneratorValue.get()); > + auto& cssGradientValueB = downcast<CSSGradientValue>(otherStyleGeneratedImage.m_imageGeneratorValue.get()); > + return cssGradientValueA.equals(cssGradientValueB); > + } > + } > + return data() == other.data(); data() == other.data() test would be better done first. Comment on attachment 427763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427763&action=review >> Source/WebCore/rendering/style/FillLayer.cpp:406 >> return false; > > You should also rename imagesIdentical to something that reflects its only call site, imageChangeRequiresClientReregistration or something. Why doesn't arePointingToEqualData() do the right thing? It tests pointer equality and then calls operator== > Source/WebCore/rendering/style/StyleGeneratedImage.cpp:48 > + return cssGradientValueA.equals(cssGradientValueB); Why doesn't CSSGradientValue have an operator==? Should we test pointer equality first? Do we need similar code for other kinds of StyleGeneratedImage? Why are gradients special? Can this just call arePointingToEqualData() on the m_imageGeneratorValues? (In reply to Simon Fraser (smfr) from comment #18) > Comment on attachment 427763 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427763&action=review > > >> Source/WebCore/rendering/style/FillLayer.cpp:406 > >> return false; > > > > You should also rename imagesIdentical to something that reflects its only call site, imageChangeRequiresClientReregistration or something. > > Why doesn't arePointingToEqualData() do the right thing? It tests pointer > equality and then calls operator== Because we specifically do not want to run operator== here since it would return true, and we're interested in the pointers being the same, which they are not. > > Source/WebCore/rendering/style/StyleGeneratedImage.cpp:48 > > + return cssGradientValueA.equals(cssGradientValueB); > > Why doesn't CSSGradientValue have an operator==? Sure could, will add that. > Should we test pointer equality first? I don't think that would be a problem. > Do we need similar code for other kinds of StyleGeneratedImage? Why are > gradients special? You mean other types of CSSImageGeneratorValue? All the subclasses actually have equals() methods so we should probably just add an operator== on CSSImageGeneratorValue which calls into equals(). > Can this just call arePointingToEqualData() on the m_imageGeneratorValues? Yes, I would think so, which would also take care of pointer equality. Comment on attachment 427763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427763&action=review >>>> Source/WebCore/rendering/style/FillLayer.cpp:406 >>>> return false; >>> >>> You should also rename imagesIdentical to something that reflects its only call site, imageChangeRequiresClientReregistration or something. >> >> Why doesn't arePointingToEqualData() do the right thing? It tests pointer equality and then calls operator== > > Because we specifically do not want to run operator== here since it would return true, and we're interested in the pointers being the same, which they are not. How is that possible, that the image pointers are the same, but internally they are different? Comment on attachment 427763 [details]
Patch
The repaint test failure shows that this isn't quite right, and it would be good to do the rename the Antti suggests.
(In reply to Simon Fraser (smfr) from comment #20) > Comment on attachment 427763 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427763&action=review > > >>>> Source/WebCore/rendering/style/FillLayer.cpp:406 > >>>> return false; > >>> > >>> You should also rename imagesIdentical to something that reflects its only call site, imageChangeRequiresClientReregistration or something. > >> > >> Why doesn't arePointingToEqualData() do the right thing? It tests pointer equality and then calls operator== > > > > Because we specifically do not want to run operator== here since it would return true, and we're interested in the pointers being the same, which they are not. > > How is that possible, that the image pointers are the same, but internally > they are different? It's the other way around, the images might be different instances with the same properties. We want imagesIdentical to only return true if they're the same object. Created attachment 427888 [details]
Patch
Comment on attachment 427888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427888&action=review > Source/WebCore/css/CSSCrossfadeValue.cpp:228 > + if (is<CSSCrossfadeValue>(other)) if ! Created attachment 427898 [details]
Patch
Comment on attachment 427898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427898&action=review > Source/WebCore/css/CSSImageGeneratorValue.h:44 > + virtual ~CSSImageGeneratorValue(); Lets not make any CSSValues virtual unless we decide to make all of them virtual. It will just create confusion. Created attachment 427904 [details]
Patch
Created attachment 427906 [details]
Patch
Committed r277112 (237410@main): <https://commits.webkit.org/237410@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 427906 [details]. |
CSS custom properties used in gradient images on pseudo elements causes infinite relayout and high CPU load. Reproduction scenario is simple: <body> <style> :root { --link-highlight-color: rgba(0,0,0,0.1); } a { position: relative; min-width: 44px; height: 44px; display: inline-flex; justify-content: center; align-items: center; text-decoration: none; } a:before { content: ''; width: 152%; height: 152%; position: absolute; left: -26%; top: -26%; background-image: radial-gradient(circle at center, var(--link-highlight-color) 66%, rgba(255,255,255,0) 66%); background-repeat: no-repeat; background-position: center; background-size: 100% 100%; opacity: 0; pointer-events: none; transition-duration: 600ms; } a:hover:before { opacity: 1; } </style> <p> <a href="#">Link 1</a> </p> <p> <a href="#">Another Link</a> </p> <p> <a href="#">Link 3</a> </p> </body> ``` Here is the live example on JSFiddle https://jsfiddle.net/4gb9ynLz/ Such simple layout causes 100% CPU load on my system: MacBook Pro 15 (Retina, Mid 2012) macOs 10.14.2 (18C54) Safari: 12.0.2 (14606.3.4) Same happens on latest iOS 12.1.3 (16D39) Safari, iPhone X