Believe we skip layers with element_id = (0, 0), due to operator bool():
https://cs.chromium.org/chromium/src/cc/trees/element_id.cc?sq=package:chromium&dr=CSs&l=29

and this bit:
https://cs.chromium.org/chromium/src/cc/trees/layer_tree_impl.cc?dr=CSs&q=layer_tree_impl.cc&sq=package:chromium&l=548

However I do see the check fail for element_id=(16, 0) at ToT with above test case. I will look into it.

Actually, logging from DCHECK directly:

Check failed: !element_id_collision_detected. element_id=(3, 0)

This makes more sense as that's the id I would expect for the test case, though still unsure why we see a duplicate.

(my comment that this was due to duplicate (0, 0) ids was wrong; this is due to duplicate ids that are non-zero).

Ali and Steve:
Walter and I walked through the example above and we think it is okay for the SPV2 design to violate the DCHECK that was added for element id collisions. Because SPV2 squashing is not yet fully evolved, we have the following scenario:
layer1(scrolled background: xform b, clip b, elementid a), layer2(non-scrolled foreground: xform a, clip a, elementid b), layer3(scrolled foreground: xform b, clip b, elementid a).

In this case, layer1 and layer3 would both share the same property tree state, so we end up setting the layer's element id twice: once for the scrolled background, and a second time for the scrolled foreground. We would like for this not to be an issue because the element_id <-> property tree node mapping is still unique even though it passes through different layers. Do you think we could make the DCHECK not fire for SPV2?

Scrolling looks up the layer by the element ID in LayerTreeHostImpl::SetTreeLayerScrollOffsetMutated.  This uses LayerTreeImpl::element_layers_map_, which is a map of element ID -> layer ID.

If two layers have the same element ID, how do you guarantee that the correct one is scrolled?

Re: how does this work in the current SPv1 world, I think this could be it -- PaintLayerCompositor::ensureRootLayer() sets element id on the scrolling contents layer. Note that it uses a secondary id as CompositorSubElementId::Scroll. This would result in a unique compositor element id for the layer.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp?q=file:paintlayercompositor+elementid&sq=package:chromium&dr=C&l=1142

We could do similar somehow? Currently we are naively using CompositorSubElementId::Primary:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp?dr&q=propertytreebuilder+file:webkit&sq=package:chromium&l=92

There is add'l SPv1 element id logic that I think is unrelated in CompositedLayerMapping::updateElementIdAndCompositorMutableProperties() but including for reference:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp?q=file:compositedlayermapping+elementid&sq=package:chromium&l=2082&dr=CSs

For SPv2 we'll be scrolling scroll nodes, not layers, so the equivalent DCHECK would be making sure no two scroll nodes have the same element id. Once we've finished untangling property tree nodes from owning layers I don't think layers will have element ids at all.

How much of the current logic depends on layers? I thought we moved off layers so all the real logic was on scroll nodes today.

Very little of the real logic depends on layers, but there's still some plumbing-type stuff that either directly goes through LayerImpl or uses LayerImpl::id, e.g.:
-LayerImpl::DistributeScroll
-LayerImpl::ScrollBy (still called during pinch zoom and for the elastic overflow effect)
-LayerTreeImpl::DidUpdateScrollOffset and DidUpdateScrollState (which take a layer id, not an ElementId)
-the visual viewport layers (inner/outer viewport scroll layers) 

Thanks for the info Ali. It sounds like we will need to maintain the current behavior with layers, and this DCHECK is correct after all. I think we have at least two distinct issues for spv2:
1) A bug in squashing creates too many layers in the presence of scroll nodes:  https://crbug.com/684138 
2) The current approach of using compositor element ids in a fixed order (PropertyTreeState::compositorElementId) will not work. For example, if we have a 3d transformed element inside a scroll node, the 3d transformed layer will still be associated with a scroll node and will incorrectly use the scroll node's compositor element id instead of the transform node's.

For #2, I think we need to add some concept of the compositor element id that is responsible for creating the pending layer.

Another option is to rework the plumbing stuff mentioned in c#8 to actually remove dependence on layers. That's surely more work but we know we want to do it at some point. Worth considering.

I need to better understand #2, I roughly get what you're saying but are we confident it will work in all circumstances? The current approach in PropertyTreeState::compositorElementId is actually arbitrary, not a fixed order, since they are all supposed to have the exact same element id. If we break that precondition we just need to think it through to be sure we don't violate something w.r.t. the intended design melding compositor state into property trees.

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

commit 231ba29e221aa7ff9e7839c5bae0a05afbff28dc
Author: pdr <pdr@chromium.org>
Date: Fri Jan 27 02:36:52 2017

Make scroll translation transform nodes reference scroll nodes

This is the first of two patches to move scroll tree ownership to
the transform nodes that are used for scroll translation. This will
let us track just the transform, effect, and clip trees for geometry
mapping purposes without worrying about scroll nodes which do not
affect compositing decisions on their own (only through their
associated scroll translation transform nodes).

This patch removes the scroll node pointer to a transform node and
adds a pointer from transform nodes to scroll nodes. The compositing
element id has also been moved from the scroll node to the associated
transform node to remove redundancy.

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

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

[modify] https://crrev.com/231ba29e221aa7ff9e7839c5bae0a05afbff28dc/third_party/WebKit/Source/core/animation/CompositorAnimationsTest.cpp
[modify] https://crrev.com/231ba29e221aa7ff9e7839c5bae0a05afbff28dc/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
[modify] https://crrev.com/231ba29e221aa7ff9e7839c5bae0a05afbff28dc/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
[modify] https://crrev.com/231ba29e221aa7ff9e7839c5bae0a05afbff28dc/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinterTest.cpp
[modify] https://crrev.com/231ba29e221aa7ff9e7839c5bae0a05afbff28dc/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
[modify] https://crrev.com/231ba29e221aa7ff9e7839c5bae0a05afbff28dc/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp
[modify] https://crrev.com/231ba29e221aa7ff9e7839c5bae0a05afbff28dc/third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp
[modify] https://crrev.com/231ba29e221aa7ff9e7839c5bae0a05afbff28dc/third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.h
[modify] https://crrev.com/231ba29e221aa7ff9e7839c5bae0a05afbff28dc/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp
[modify] https://crrev.com/231ba29e221aa7ff9e7839c5bae0a05afbff28dc/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h
[modify] https://crrev.com/231ba29e221aa7ff9e7839c5bae0a05afbff28dc/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeStateTest.cpp
[modify] https://crrev.com/231ba29e221aa7ff9e7839c5bae0a05afbff28dc/third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.cpp
[modify] https://crrev.com/231ba29e221aa7ff9e7839c5bae0a05afbff28dc/third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.h
[modify] https://crrev.com/231ba29e221aa7ff9e7839c5bae0a05afbff28dc/third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.cpp
[modify] https://crrev.com/231ba29e221aa7ff9e7839c5bae0a05afbff28dc/third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h

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

commit 46cf3df5c1b750d2bade33909005304d5a4234a9
Author: pdr <pdr@chromium.org>
Date: Sun Jan 29 03:02:16 2017

Make ScrollPaintPropertyNode::toString() 26% more concise

Before:
root 0x119cf8954118 parent=0x0 clip=0x0 bounds=0x0 userScrollableHorizontal=no userScrollableVertical=no mainThreadScrollingReasons=
  Scroll (FrameView) 0x119cf89540c0 parent=0x119cf8954118 clip=785x276 bounds=785x699 userScrollableHorizontal=yes userScrollableVertical=yes mainThreadScrollingReasons=

After:
root 0x36350c954118 parent=0x0 clip=0x0 bounds=0x0 userScrollable=none mainThreadReasons=none
  Scroll (FrameView) 0x36350c9540c0 parent=0x36350c954118 clip=785x276 bounds=785x699 userScrollable=both mainThreadReasons=none

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

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

[modify] https://crrev.com/46cf3df5c1b750d2bade33909005304d5a4234a9/third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.cpp

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

commit 3eee970eb6757c6ea3997e0722d7bab727b9c11c
Author: pdr <pdr@chromium.org>
Date: Mon Jan 30 21:50:52 2017

Move scroll paint property nodes to be owned by the transform tree

This patch moves the ownership of scroll paint property tree nodes
to their associated scroll translation nodes. This has two benefits:
1) The paint property nodes directly stored on LayoutObject's are
only related to geometry so special-cases are not needed to exclude
non-geometry property nodes in PaintArtifactCompositor.
2) There is a memory savings because scroll nodes are effectively
rare data in the transform tree. A TODO has been added to make this
more explicit.

The major changes in this patch are:
* m_scroll has been removed from ObjectPaintProperties, as ownership is
now on the transform tree.
* m_scroll has been removed from FrameView, as ownership is now on the
transform tree.
* PaintLayer::sticksToViewport required some involved refactoring
because the cached scroll node is no longer available.

This fixes a squashing bug where PaintArtifactCompositor would stop
merging paint chunks when their scroll nodes differed. This is tested
in: PaintArtifactCompositorTestWithPropertyTrees:NestedScrollNodes.
This patch also passes 30 new layout tests with spv2 enabled.

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

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

[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/core/animation/CompositorAnimationsTest.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/core/frame/FrameView.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/core/frame/FrameView.h
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/core/paint/BlockPainter.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/core/paint/FindPropertiesNeedingUpdate.h
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/core/paint/FramePainter.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/core/paint/ObjectPaintProperties.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/core/paint/ObjectPaintProperties.h
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/core/paint/PaintControllerPaintTest.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/core/paint/PaintInvalidator.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/core/paint/PaintLayer.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinterTest.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/core/paint/PaintPropertyTreeUpdateTests.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/platform/graphics/paint/GeometryMapperTest.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/platform/graphics/paint/PaintChunkProperties.h
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/platform/graphics/paint/PropertyTreeStateTest.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/platform/graphics/paint/SkPictureBuilder.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/platform/testing/PaintPrinters.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/platform/testing/PaintPropertyTestHelpers.h
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/platform/testing/TestPaintArtifact.cpp
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/platform/testing/TestPaintArtifact.h
[modify] https://crrev.com/3eee970eb6757c6ea3997e0722d7bab727b9c11c/third_party/WebKit/Source/web/tests/WebPluginContainerTest.cpp

http://crbug.com/691154 is possibly related.

Also see  http://crbug.com/702350 

I think the work item left for this task is to remove animation system dependency on layers. Marking as dupe of that but if there's more work let's discuss.