Note current de-compositing patch https://codereview.chromium.org/2724083002/ has a set of crashing tests under animations and transitions that look like they could be variants of this issue.

Focusing currently on animations/timing-functions.html as an arbitrary selection among them to debug.

Now looking at transitions/opacity-transition-zindex.html as a simpler case.

Note issue exists at current ToT debug build w/ SPv2 enabled, without --enable-threaded-compositing. We fail DCHECK but test passes with release build. So the decompositing patch is not related.

% ./out/Debug/content_shell --run-layout-test --enable-slimming-paint-v2 third_party/WebKit/LayoutTests/transitions/opacity-transition-zindex.html

Attached is reduced test case that appears to be another example of this, reduced from virtual/threaded/animations/composited-animation-independent-transform-cancel.html

Deleted: differing-element-ids.html 792 bytes

differing-element-ids.html 792 bytes View Download

Simplified further.

Deleted: differing-element-ids.html 469 bytes

differing-element-ids.html 469 bytes View Download

Notes from debugging test case in c#5 above. The crash for is happening in TestRunner, here:

    // clean out the lifecycle if needed before capturing the layout tree
    // dump and pixels from the compositor.
    render_view()->GetWebView()->mainFrame()->toWebLocalFrame()
        ->frameWidget()->updateAllLifecyclePhases();

In this test we have a single parent element, and we create two child elements, one with a translate animation, the other with an opacity animation.

The DCHECK failure says that, for a single PropertyTreeState, we somehow have differing compositor element ids, which seems like it should be impossible.

The failure cites the two element ids as ((4, 0) vs. (2, 0)).

The relevant paint property tree state excerpts for these are below. I'm not yet sure how PropertyTreeState::compositorElementId() ends up validating element id (2, 0) which is tied to the FrameView ScrollTranslation transform node against the element id (4,0) which is tied to the opacity animation effect node.

(see ScrollTranslation below for (2, 0))

root 0x2dac9b269110 parent=(nil) transform=identity origin=0,0,0 flattensInheritedTransform=no renderingContextId=0 directCompositingReasons=none compositorElementId=(0, 0) scroll=parent=(nil) clip=0x0 bounds=0x0 userScrollable=none mainThreadReasons=none
  PreTranslation (FrameView) 0x2dac9b269010 parent=0x2dac9b269110 transform=identity origin=0,0,0 flattensInheritedTransform=no renderingContextId=0 directCompositingReasons=none compositorElementId=(0, 0)
    ScrollTranslation (FrameView) 0x2dac9b269410 parent=0x2dac9b269010 transform=identity origin=0,0,0 flattensInheritedTransform=no renderingContextId=0 directCompositingReasons=none compositorElementId=(2, 0) scroll=parent=0x2dac9b2bf6f0 clip=800x585 bounds=802x585 userScrollable=both mainThreadReasons=none scrollClient=0x136a9b4a2cd0
      PaintOffsetTranslation (LayoutBlockFlow DIV) 0x2dac9b269510 parent=0x2dac9b269410 transform=translation(8,8,0) origin=0,0,0 flattensInheritedTransform=yes renderingContextId=0 directCompositingReasons=none compositorElementId=(0, 0)
        Transform (LayoutBlockFlow DIV) 0x2dac9b269610 parent=0x2dac9b269510 transform=translation(10,10,10) origin=392,10,0 flattensInheritedTransform=yes renderingContextId=0 directCompositingReasons=transform3D,activeAnimation compositorElementId=(3, 0)

(see Effect below for (4, 0))

root 0x2dac9b294490 parent=(nil) localTransformSpace=0x2dac9b269110 outputClip=0x2dac9b258520 opacity=1.000000 filter={"FilterOperations":[]} blendMode=SrcOver directCompositingReasons=none compositorElementId=(0, 0) paintOffset=0,0
  Effect (LayoutBlockFlow DIV) 0x2dac9b2949d0 parent=0x2dac9b294490 localTransformSpace=0x2dac9b269410 outputClip=0x2dac9b258520 opacity=1.000000 filter={"FilterOperations":[]} blendMode=SrcOver directCompositingReasons=activeAnimation compositorElementId=(4, 0) paintOffset=0,0

Full paint property tree dump is:

root 0x265364e69110 parent=(nil) transform=identity origin=0,0,0 flattensInheritedTransform=no renderingContextId=0 directCompositingReasons=none compositorElementId=(0, 0) scroll=parent=(nil) clip=0x0 bounds=0x0 userScrollable=none mainThreadReasons=none
  PreTranslation (FrameView) 0x265364e69010 parent=0x265364e69110 transform=identity origin=0,0,0 flattensInheritedTransform=no renderingContextId=0 directCompositingReasons=none compositorElementId=(0, 0)
    ScrollTranslation (FrameView) 0x265364e69410 parent=0x265364e69010 transform=identity origin=0,0,0 flattensInheritedTransform=no renderingContextId=0 directCompositingReasons=none compositorElementId=(2, 0) scroll=parent=0x265364ebf6f0 clip=800x585 bounds=802x585 userScrollable=both mainThreadReasons=none scrollClient=0x16b7e5dc2cd0
      PaintOffsetTranslation (LayoutBlockFlow DIV) 0x265364e69510 parent=0x265364e69410 transform=translation(8,8,0) origin=0,0,0 flattensInheritedTransform=yes renderingContextId=0 directCompositingReasons=none compositorElementId=(0, 0)
        Transform (LayoutBlockFlow DIV) 0x265364e69610 parent=0x265364e69510 transform=translation(10,10,10) origin=392,10,0 flattensInheritedTransform=yes renderingContextId=0 directCompositingReasons=transform3D,activeAnimation compositorElementId=(3, 0)

root 0x265364e58520 parent=(nil) localTransformSpace=0x265364e69110 rect=-1.67772e+07,-1.67772e+07 3.35544e+07x3.35544e+07 radii:(tl:0x0; tr:0x0; bl:0x0; br:0x0) directCompositingReasons=none
  ContentClip (FrameView) 0x265364e587f0 parent=0x265364e58520 localTransformSpace=0x265364e69010 rect=0,0 800x585 radii:(tl:0x0; tr:0x0; bl:0x0; br:0x0) directCompositingReasons=none

root 0x265364e94490 parent=(nil) localTransformSpace=0x265364e69110 outputClip=0x265364e58520 opacity=1.000000 filter={"FilterOperations":[]} blendMode=SrcOver directCompositingReasons=none compositorElementId=(0, 0) paintOffset=0,0
  Effect (LayoutBlockFlow DIV) 0x265364e949d0 parent=0x265364e94490 localTransformSpace=0x265364e69410 outputClip=0x265364e58520 opacity=1.000000 filter={"FilterOperations":[]} blendMode=SrcOver directCompositingReasons=activeAnimation compositorElementId=(4, 0) paintOffset=0,0

root 0x265364ebf6f0 parent=(nil) clip=0x0 bounds=0x0 userScrollable=none mainThreadReasons=none
  Scroll (FrameView) 0x265364ebfa50 parent=0x265364ebf6f0 clip=800x585 bounds=802x585 userScrollable=both mainThreadReasons=none scrollClient=0x16b7e5dc2cd0

Through adding logging of property tree state at time of failing the DCHECK, we see that when we look up the transform node compositor element id for the opacity animated div's layer, we are incorrectly retrieving the compositor element id for its parent html element (which has a transform on it for scrolling purposes).

Essentially, we are propagating the html element's transform node down into the opacity animated div, and thus end up with conflicting compositor element ids in that property tree state.

This illustrates a fundamental design flaw with how we currently retrieve compositor element id in PAC, off of current PropertyTreeState, for purposes of setting on cc layers when we create them. This means we are susceptible to bugs with any scrolling/animation code that remains that is dependent on correct layer -> element id mapping.

Diagram for this test case roughly as 'dom element' ('compositor element id'):

     html (2)
       |
   parent div
   /        \
opacity     transform
  div (4)      div (3)

PropertyTreeState::compositorElementId() for opacity div is DCHECK-failing effect node (4) vs transform node (2), when it should only see (4) and no transform node compositor element id, since there is no layer/element-local transform animation.

Next step thoughts:

- look into whether we can rework cc side element animation to not rely on layer to element id map. Instead rely on layer to property tree node to element id.

- or, consider maintaining, in PAC, something like a map of first layer to ever be associated with a property tree node. Free form discussion note:

  Whichever layer is first gets the involved element id. We can use this to enforce that for example in this test case, the leaf opacity animated div only incorporates the one element id for its div, and not the one for its parent html element. Today the opacity div has two property nodes associated with it; transform and effect. But since we've already seen the html element, structure something so that it will have already claimed the transform node's element id, meaning that at opacity div, we can only use the effect node's element id. Once we do that, effect node's element id is also no longer available for children to use. Logically, like a bit on the property tree node re: whether we've claimed it, but we can't mutate the tree, so it has to be stored off the tree somehow.

On first next step thought above, re: reworking cc side element animation to not rely on layer to element id map, maybe we actually meant element id to layer map. I think work in  http://crbug.com/702774  may be much of what's needed.

From my previous notes on animation logic  http://crbug.com/674258#c3 :

- note some referenced methods have since moved from LayerTreeHost to LayerTree for example RegisterElement as part of http://crrev.com/2661523003

- revising LayerTreeHost::LayerByElementId seems primary work item:
  + can we go from element id to the layer with existing data, a.k.a., using element id -> property tree node -> layer somehow?
  + really, we want to switch layer tree host to managing animations via property tree nodes rather than layers
  + ElementAnimations::InitAffectedElementTypes -> LayerTreeHost::IsElementInList -> LayerTreeHost::LayerByElementId is key call flow for registering animations
    and may be the main remaining work item once http://crrev.com/2762123004 lands.

Presuming layer iteration order in PAC::update does not violate the design and there aren't any cases we've not thought, I think we actually get option 2 in c#7 for free with the set of composited element ids we build up in http://crrev.com/2724083002. We just need to pass that set into PropertyTreeState::compositorElementId() and check it therein.

I have an exploratory patch that does this but would like to pursue it separately from above patch, so will follow up shortly.

The second possible fix in comment 7 (first-come-first-serve) will probably
work for most/all of the tests, but may/probably will fail in some advance scenario.
Can't think of one specifically right now.

After discussion offline with Walter yesterday, I vote to just disable some tests
for SPv2 and re-enable them once the cc refactoring is done (and prioritize the
cc refactoring to do soon). But if you want to add some temporary PAC code in
advance of the cc refactoring that's cool too.

Exploratory patch to limit compositor element id consideration to when first seen while building layers:

https://codereview.chromium.org/2808463003

Comparing behavior at ToT to behavior with this patch, it looks like this could fix 14 crashes:

  animations/3d/matrix-transform-type-animation.html [ Crash ]
  animations/animation-offscreen-to-onscreen.html [ Crash ]
  animations/timing-functions.html [ Crash ]
  animations/viewport-unit-animation-responsive.html [ Crash ]
  transitions/opacity-transform-transitions-inside-iframe.html [ Crash ]
  transitions/opacity-transition-zindex.html [ Crash ]
  virtual/threaded/animations/3d/matrix-transform-type-animation.html [ Crash ]
  virtual/threaded/animations/3d/replace-filling-transform.html [ Crash ]
  virtual/threaded/animations/animation-offscreen-to-onscreen.html [ Crash ]
  virtual/threaded/animations/composited-animation-independent-transform-cancel.html [ Crash ]
  virtual/threaded/animations/timing-functions.html [ Crash ]
  virtual/threaded/animations/viewport-unit-animation-responsive.html [ Crash ]
  virtual/threaded/transitions/opacity-transform-transitions-inside-iframe.html [ Crash Crash Timeout Crash ]
  virtual/threaded/transitions/opacity-transition-zindex.html [ Crash ]

However, agree that best would be to finish cc refactoring to remove requirement for element id on layer.

https://codereview.chromium.org/2808463003 which is in CQ fixes the crash in:

transitions/opacity-transform-transitions-inside-iframe.html
virtual/threaded/transitions/opacity-transform-transitions-inside-iframe.html

but not the timeout, which must be due to an animation Ready promise and requires further investigation.

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

commit ce3c236fb74d87164e5de4a1f8f6fee2f0607f4a
Author: wkorman <wkorman@chromium.org>
Date: Mon Apr 17 22:01:19 2017

SPv2: Limit compositor element id application to a single layer.

PropertyTreeState acts as a context that accumulates state as we traverse the
tree building layers. This means that, with current code structure, we could see
a compositor element id 'A' for a parent layer in conjunction with a compositor
element id 'B' for a child layer.

This patch incorporates presence of an id in the set of previously seen
compositor element ids when determining whether the current property tree state
has a compositor element id.

BUG= 702350 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2808463003
Cr-Commit-Position: refs/heads/master@{#465037}

[modify] https://crrev.com/ce3c236fb74d87164e5de4a1f8f6fee2f0607f4a/third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
[modify] https://crrev.com/ce3c236fb74d87164e5de4a1f8f6fee2f0607f4a/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
[modify] https://crrev.com/ce3c236fb74d87164e5de4a1f8f6fee2f0607f4a/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp
[modify] https://crrev.com/ce3c236fb74d87164e5de4a1f8f6fee2f0607f4a/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h
[modify] https://crrev.com/ce3c236fb74d87164e5de4a1f8f6fee2f0607f4a/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeStateTest.cpp

Still in SPv2 expectations.

I believe with my fix in  issue 896549  that we no longer have this issue as animations are directly associated with property tree nodes and we inform the layertreehost of all propertytree nodes' element ids directly (rather than by assigning element id to layers).

We also now explicitly give the transform and effect nodes different element ids and directly target them with from the KeyframeModel when the composited animation is created.