I have no idea what's going on here, but it seems to be something related to animations, so cc'ing flackr.

Labelling as blocked as per triage process, but this one shouldn't be too hard as it's reproducible.

@shend: Did you mean to add 706281 as a label? Or should this be blocking  issue 706281  instead?

Also, I am concerned that marking this as Blocked will discourage Blink devs from investigating, even though is reproducible as you mentioned. Isn't  issue 706281  relevant for bugs that have been automatically filed as opposed from bugs that have been filed from developers directly?

@shend: See question at #6 above.

Yes you are right, it shouldn't really be blocked. My mistake.

FYI, here is a 2nd repro with the same DCHECK failure (provided by @rbpotter again). This time the code uses -webkit-linear-gradient with CSS variables, and still crashes.

Deleted: test_html_2.html 539 bytes

test_html_2.html 539 bytes View Download

I attached a smaller test case. The problem is using a CSS variable in a gradient when there's also a transition occurring. The timeline is something like:

1. We call ApplyProperty to do the transition.
2. We resolve the variable reference in the gradient.
3. This (re)parses the gradient value.
4. Which creates a brand new CSSLinearGradientValue
5. ComputedStyle is applied with the new CSSLinearGradientValue
6. The DCHECK will compare base ComputedStyle with the other ComputedStyle, which compares their background, which compares their gradient values *by address*. Even though both gradients have structurally identical values, they have different addresses, so the two computed styles are considered not equal. This causes the DCHECK failure.

You might be able to fix this by:
a) disabling base computed style optimisation when there's variables inside a gradient value (like in [7])
b) compare gradients structurally when pointers are different (might have perf implications)
c) don't create new gradient values when parsing to resolve variables

I'm going to redirect this to Blink>Animation as it seems to be more of an animations bug. To get this off our triage queue, I'm just going to assign it to someone on the animations team :P Thanks!

[1] https://cs.chromium.org/chromium/src/out/Debug/gen/blink/core/StyleBuilder.cpp?q=StyleBuilder::ApplyProperty&sq=package:chromium&l=25
[2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/resolver/CSSVariableResolver.cpp?q=CSSVariableResolver::ResolveVariableReferences&sq=package:chromium&dr=CSs&l=225
[3] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp?q=CSSPropertyParserHelpers::ConsumeLinearGradient&sq=package:chromium&l=1253
[4] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/parser/CSSPropertyParserHelpers.cpp?q=CSSPropertyParserHelpers::ConsumeLinearGradient&sq=package:chromium&ssfr=1&l=1283
[6] (StyleImage comparing pointers) https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/StyleImage.h?type=cs&q=StyleImage::operator%3D%3D&sq=package:chromium&l=48
[7] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/animation/ElementAnimations.cpp?type=cs&sq=package:chromium&l=122

Happy to take a look. shend@ - you mentioned attaching a smaller test case in #11, but I don't see an attachment. Do you still have that smaller test case?

Oops, yes. Here it is.

Deleted: animation.html 301 bytes

animation.html 301 bytes View Download

It would help issue 788331 if we fixed this. At least so that we could tell if these are duplicates or not.

Sorry, I have not taken an indepth look at this yet. My initial investigations confused the heck out of me (we have a DCHECK that checks that the thing you're setting is equivalent to the thing you're already storing? Why?), so I backed off of it.

I could definitely implement suggestion (a) from comment #11 above, but I honestly wouldn't be sure of the implications.

I'll try to start looking at this this week.

What's worse than the DCHECK failure and the animations optimization here is the fact that gradients will always look different for paint invalidation and cause a repaint every time some other computed style property changes. Adding/removing --bar below should ideally not have any other effects than a style recalc. Running the example below causes a repaint and GPU work on every frame (run it in the devtools performance inspector):

<!DOCTYPE html>
<style>
  div {
    height: 500px;
    --foo: rgb(0, 0, 0);
    background-image: linear-gradient(to bottom, var(--foo), red);
  }

  div.custom {
    --bar: 2px;
  }
</style>
<div id="d"></div>
<script>
  function rAF() {
    if (d.className == "")
      d.className = "custom";
    else
      d.className = "";
    requestAnimationFrame(rAF);
  }

  rAF();
</script>

Any updates? Is this likely to be fixed or should the DCHECK just be removed or made smarter, like comparing for equivalence, instead of comparing by reference?

Same crash here:
https://paste.googleplex.com/4774673338335232

> b) compare gradients structurally when pointers are different (might have perf implications)

This is absolutely the correct solution here, the common image case is not gradients so I don't think perf is worth worrying about here.

Haha, fixing the equivalency bug (https://chromium-review.googlesource.com/#/c/chromium/src/+/1080366) reveals another bug. Now it crashes on:

css_image_generator_value.cc(105)] Security DCHECK failed: it != clients_.end().        
#0 0x7f43d0146aec base::debug::StackTrace::StackTrace()                                                               
#1 0x7f43d008e6db logging::LogMessage::~LogMessage()                                                                  
#2 0x7f43c7de40bc blink::CSSImageGeneratorValue::RemoveClient()                                                       
#3 0x7f43c8773edf blink::LayoutObject::UpdateFillImages()                                                             
#4 0x7f43c8772e72 blink::LayoutObject::SetStyle()                                                                     
#5 0x7f43c805ea04 blink::Element::RecalcOwnStyle()                                                                    
#6 0x7f43c805df06 blink::Element::RecalcStyle()                                                                       
#7 0x7f43c7ff7de1 blink::ContainerNode::RecalcDescendantStyles()                                                      
#8 0x7f43c805e0c1 blink::Element::RecalcStyle()                                                                       
#9 0x7f43c7ff7de1 blink::ContainerNode::RecalcDescendantStyles()                                                      
#10 0x7f43c805e0c1 blink::Element::RecalcStyle()                                                                      
#11 0x7f43c8012d0d blink::Document::UpdateStyle()                                                                     
#12 0x7f43c800e279 blink::Document::UpdateStyleAndLayoutTree()                                                        
#13 0x7f43c8369142 blink::LocalFrameView::UpdateStyleAndLayoutIfNeededRecursiveInternal()                             
#14 0x7f43c8366676 blink::LocalFrameView::UpdateStyleAndLayoutIfNeededRecursive()                                     
#15 0x7f43c83649bb blink::LocalFrameView::UpdateLifecyclePhasesInternal()                                             
#16 0x7f43c8364797 blink::LocalFrameView::UpdateAllLifecyclePhases()                                                  
#17 0x7f43c8977d5e blink::PageAnimator::UpdateAllLifecyclePhases()                                                    
#18 0x7f43c82aa8f8 blink::WebViewImpl::UpdateLifecycle()                                                              
#19 0x7f43c83cb978 blink::WebViewFrameWidget::UpdateLifecycle()
#20 0x7f43cde6e8e2 content::RenderWidget::UpdateVisualState()
#21 0x7f43c1768ec8 cc::ProxyMain::BeginMainFrame()

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bce1afaaf6eabd354a252b056d2eb3abfe68f541

commit bce1afaaf6eabd354a252b056d2eb3abfe68f541
Author: Alan Cutter <alancutter@chromium.org>
Date: Thu May 31 05:40:52 2018

Remove unused FillLayer::ContainsImage()

Bug:  781461 
Change-Id: I464ad57e04ad5047dc034f42dc9befd30696f940
Reviewed-on: https://chromium-review.googlesource.com/1080363
Reviewed-by: Darren Shen <shend@chromium.org>
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563157}
[modify] https://crrev.com/bce1afaaf6eabd354a252b056d2eb3abfe68f541/third_party/blink/renderer/core/style/fill_layer.cc
[modify] https://crrev.com/bce1afaaf6eabd354a252b056d2eb3abfe68f541/third_party/blink/renderer/core/style/fill_layer.h

It looks like Alan is currently looking into this (thanks Alan! Much, much appreciated), so passing bug off to him for now. Alan, if you end up not being able to chase this down, please pass the bug back to me - it is something I want to see fixed, I've just been procrastinating on it.

The notion of equality is getting messy here. The DCHECK(*base_computed_style_ == *computed_style) assertion just wants to check value equality but for StyleImages CSSValue instance equality is more important due to the presence of ImageResourceObservers attached to their instances.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e49eb5d429524b7294839bb33c57e3fffaf7b21e

commit e49eb5d429524b7294839bb33c57e3fffaf7b21e
Author: Alan Cutter <alancutter@chromium.org>
Date: Tue Jun 19 12:21:16 2018

Skip ComputedStyle equivalency assertion under false positive scenarios

Differences between ComputedStyle for background-image and
-webkit-mask-image properties may be due to the StyleImages having
different CSSValue instances rather than being different by semantic
value.
This CL disables the base ComputedStyle equivalency check if
either of these properties detect a difference as they can produce
false positive assertions.

As part of this the FontFaceCache bypass of the base ComputedStyle
optimisation was reworked into a bypass of the ComputedStyle
check instead for consistency and to allow the optimisation to
proceed where it is technically valid to do so.


Bug:  781461 
Change-Id: I4c28c500819fb75da27130c147f2fbfde2ac93da
Reviewed-on: https://chromium-review.googlesource.com/1080366
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568411}
[add] https://crrev.com/e49eb5d429524b7294839bb33c57e3fffaf7b21e/third_party/WebKit/LayoutTests/animations/stability/transition-var-gradient-crash.html
[modify] https://crrev.com/e49eb5d429524b7294839bb33c57e3fffaf7b21e/third_party/blink/renderer/core/animation/element_animations.cc
[modify] https://crrev.com/e49eb5d429524b7294839bb33c57e3fffaf7b21e/third_party/blink/renderer/core/animation/element_animations.h