StyleResolver baseComputedStyle optimisation untested with asserts enabled |
|||||||||
Issue descriptionThe baseComputedStyle optimisation in StyleResolver (https://cs.chromium.org/search/?q=f:styleresolver+baseComputedStyle&sq=package:chromium&type=cs) is untested when assertions are enabled. This is due to ElementAnimations::baseComputedStyle() containing "#if DCHECK_IS_ON()". https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/animation/ElementAnimations.cpp?type=cs&q=f:elementanimations.cpp+dcheck_is_on&sq=package:chromium&l=94 This is so it can assert later that the baseComputedStyle does not change during an animation in ElementAnimations::updateBaseComputedStyle(). The problem here is that the code path for checking that the baseComputedStyle has not changed results in mutations to the StyleResolverState that would not have occured otherwise. We need a way to perform the check without behaviour modifying changes to StyleResolverState.
,
Jun 13 2017
,
Jun 30 2017
,
Jul 3 2017
This is probably not Squash-A-Bug worthy as it's non trivial to resolve. For extra context here's a patch that broke tests only when asserts were disabled due to this issue: https://codereview.chromium.org/2309963002
,
Jul 3 2017
This is the same patch but fixed to not break tests without assertions enabled: https://codereview.chromium.org/2532953008 The key difference is this part of the diff in CSSAnimations.cpp: @@ -324,8 +320,10 @@ void CSSAnimations::calculateAnimationUpdate(CSSAnimationUpdate& update, // If we're in an animation style change, no animations can have started, been // cancelled or changed play state. When DCHECK is enabled, we verify this // optimization. - if (isAnimationStyleChange) + if (isAnimationStyleChange) { + calculateAnimationActiveInterpolations(update, animatingElement); return; + } #endif
,
Sep 21 2017
,
Sep 21 2017
Any status update on this? This bug is surfacing when customizing CSS on chromecast. Would love to have the debug version working!
,
Sep 22 2017
#7: This isn't actually a user facing bug, it's an assertion coverage issue. Please file a separate bug for the issue you are seeing and link it here.
,
Dec 6 2017
,
Dec 27 2017
#8 I understand it's not a user facing bug. The assertion coverage issue is causing tests for our Chromecast team to crash and preventing us from developing a specific styling feature. Is there an ETA for a fix? Or if the path in #5 solve this issue we can apply that to the chromium build we're using.
,
Dec 27 2017
neosapien@ could you provide more info and preferably a repro of the issue you're seeing as a separate crbug issue?
,
Dec 28 2017
Hmm can't reproduce the issue anymore. Was there a fix applied on your end?
,
Dec 28 2017
For reference, The error was FATAL:ElementAnimations.cpp(112)] Check failed: *base_computed_style_ == *computed_style. But this is not an issue anymore.
,
Dec 28 2017
Yes, that was what I expected, but the problem is hard to track down without a repro. I don't know what would've fixed that.
,
Dec 28
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 1
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by nainar@chromium.org
, Feb 13 2017