Bug 194332

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:

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:

Description	Flags
Standalone version of the JSFiddle	none
Reduction showing continuous updates in Web Inspector timeline	none
Reduction	none
Patch	none
Patch	none
Patch	simon.fraser: review-, ews-feeder: commit-queue-
Patch	none
Patch	none
Patch	ews-feeder: commit-queue-
Patch	none

Vladimir

Reported 2019-02-06 01:06:16 PST

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

Attachments
Standalone version of the JSFiddle (1.77 KB, text/html) 2019-05-06 11:34 PDT, Simon Fraser (smfr)	no flags	Details
Reduction showing continuous updates in Web Inspector timeline (567 bytes, text/html) 2021-05-05 01:50 PDT, Antoine Quint	no flags	Details
Reduction (522 bytes, text/html) 2021-05-05 01:59 PDT, Antoine Quint	no flags	Details
Patch (7.53 KB, patch) 2021-05-05 04:53 PDT, Antoine Quint	no flags	Details Formatted Diff Diff
Patch (7.13 KB, patch) 2021-05-05 05:02 PDT, Antoine Quint	no flags	Details Formatted Diff Diff
Patch (9.22 KB, patch) 2021-05-05 06:57 PDT, Antoine Quint	simon.fraser: review- ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (28.67 KB, patch) 2021-05-06 08:46 PDT, Antoine Quint	no flags	Details Formatted Diff Diff
Patch (30.17 KB, patch) 2021-05-06 09:51 PDT, Antoine Quint	no flags	Details Formatted Diff Diff
Patch (17.82 KB, patch) 2021-05-06 10:36 PDT, Antoine Quint	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (17.84 KB, patch) 2021-05-06 10:38 PDT, Antoine Quint	no flags	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2019-02-06 19:25:27 PST

<rdar://problem/47873895>

P. J. Łaszkowicz

Comment 2 2019-02-26 22:35:37 PST

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.

Don Denton

Comment 3 2019-05-05 15:42:19 PDT

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.

Don Denton

Comment 4 2019-05-05 15:51:47 PDT

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.

Simon Fraser (smfr)

Comment 5 2019-05-06 11:34:30 PDT

Created attachment 369148 [details] Standalone version of the JSFiddle

Simon Fraser (smfr)

Comment 6 2019-05-06 11:50:14 PDT

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 ...

Jon Lee

Comment 7 2019-05-08 11:49:14 PDT

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.

Ali G

Comment 8 2020-11-03 13:53:02 PST

We also ran into this bug @ LinkedIn

Antoine Quint

Comment 9 2021-05-05 01:13:58 PDT

(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.

Antoine Quint

Comment 10 2021-05-05 01:50:54 PDT

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.

Antoine Quint

Comment 11 2021-05-05 01:59:27 PDT

Created attachment 427747 [details] Reduction

Antoine Quint

Comment 12 2021-05-05 04:53:56 PDT

Created attachment 427754 [details] Patch

Antoine Quint

Comment 13 2021-05-05 04:58:41 PDT

The patch addresses both the reduced test case and the other original JSFiddle link.

Antoine Quint

Comment 14 2021-05-05 05:02:09 PDT

Created attachment 427756 [details] Patch

Antoine Quint

Comment 15 2021-05-05 06:57:03 PDT

Created attachment 427763 [details] Patch

Antti Koivisto

Comment 16 2021-05-05 07:06:32 PDT

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.

Antti Koivisto

Comment 17 2021-05-05 07:07:52 PDT

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.

Simon Fraser (smfr)

Comment 18 2021-05-05 08:36:05 PDT

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?

Antoine Quint

Comment 19 2021-05-05 09:37:56 PDT

(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.

Simon Fraser (smfr)

Comment 20 2021-05-05 10:14:00 PDT

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?

Simon Fraser (smfr)

Comment 21 2021-05-05 10:24:16 PDT

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.

Antoine Quint

Comment 22 2021-05-05 10:52:21 PDT

(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.

Antoine Quint

Comment 23 2021-05-06 08:46:51 PDT

Created attachment 427888 [details] Patch

Simon Fraser (smfr)

Comment 24 2021-05-06 09:47:38 PDT

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 !

Antoine Quint

Comment 25 2021-05-06 09:51:53 PDT

Created attachment 427898 [details] Patch

Antti Koivisto

Comment 26 2021-05-06 10:07:43 PDT

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.

Antoine Quint

Comment 27 2021-05-06 10:36:48 PDT

Created attachment 427904 [details] Patch

Antoine Quint

Comment 28 2021-05-06 10:38:56 PDT

Created attachment 427906 [details] Patch

EWS

Comment 29 2021-05-06 13:11:26 PDT

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].

Note You need to log in before you can comment on or make changes to this bug.