wangxianzhu@, could you please look into this change (https://chromium.googlesource.com/chromium/src/+/8895ae9799f7de65a747c97b6f2442d83717463d%5E%21/third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp) if possible?

Thank you!

Reduced test case:
<div id="columns" style="-webkit-columns:9;">
  <div id="span" style="-webkit-column-span:all;">
    <div id="layer" style="height: 100px; overflow: hidden"></div>
  </div>
</div>

When checking paint invalidation for "layer", LayoutObject::paintingLayer() and the paintingLayer tracked by PaintInvalidationState mismatch:
- LayoutObject::paintingLayer() returns the anonymous LayoutmultiColumnFlowThread
- PaintInvalidationState::m_paintingLayer is <div id="columns">   

In layer tree:

 layer 0x5f2e614010 at (0,0) size 800x600 (composited, bounds=at (0,0) size 800x600, drawsContent=1)
  LayoutView 0x5f2e604010 at (0,0) size 800x600
 positive z-order list(1)
   layer 0x5f2e614100 at (0,0) size 800x600
    LayoutBlockFlow 0x5f2e624010 {HTML} at (0,0) size 800x600
      LayoutBlockFlow 0x5f2e624128 {BODY} at (8,8) size 784x584
   normal flow list(1)
     layer 0x5f2e6141f0 at (8,8) size 784x100
      ^^^ PaintInvalidationState::m_paintingLayer for <div id="layer">
      LayoutBlockFlow 0x5f2e624240 {DIV} at (0,0) size 784x100 id="columns"
        LayoutMultiColumnSpannerPlaceholder (anonymous) 0x5f2e63c010 at (0,0) size 0x100
     normal flow list(1)
       layer 0x5f2e6142e0 at (8,8) size 73x0
       ^^^ LayoutObject::paintingLayer() for <div id="layer">
        LayoutMultiColumnFlowThread (anonymous) 0x5f2e62c010 at (0,0) size 72.88x0
          LayoutBlockFlow (column spanner) 0x5f2e624358 {DIV} at (0,0) size 784x100 id="span"
       normal flow list(1)
         layer 0x5f2e6143d0 at (8,8) size 784x100
          LayoutBlockFlow 0x5f2e624470 {DIV} at (0,0) size 784x100 id="layer"

It seems that the PaintInvalidationState one is correct. However, based on the layer tree, the <div id="layer"> layer is in the normal flow list of the LayoutMultiColumnFlowThread which seems to mean that we'll paint <div id="layer"> layer as a sublayer of the LayoutMultiColumnFlowThread layer. Is this correct? If not, should we put <div id="layer"> layer in the normal flow list <div id="columns"> layer?

#layer is inside a spanner, so it should not be inside the flow thread layer, since it's not to be fragmented into columns, but rather use the whole width of the multicol container. It should be in the normal flow list of #columns.

In many places (e.g. LayoutObject::willBeRemovedFromTree() and PaintLayer::insertOnlyThisLayerAfterStyleChange()) we assume that parent()->enclosingLayer() is the parent layer of the current object's layer if the object has a layer, or the enclosingLayer of the current object. If we let the multicol container layer be the parent layer of the layers of the descendant objects of the spanner, but keep the spanner object (LayoutBlockFlow id="span") as a child of the flow thread object, we'll break these assumptions.

Can we let the spanner object be a child of the multicol container (in the place of the placeholder) or the spanner placeholder instead of the flow thread object? I think in this way we'll avoid a lot of logics handling the specialties caused by the spanner.

This was actually my initial approach when implementing support for column-span:all - i.e. don't place its layout object where it should be according to the DOM, but rather as a child of the multicol container (exactly where the LayoutMultiColumnSpannerPlaceholder is, in the current implementation).

See https://codereview.chromium.org/296413007/diff/200001/Source/core/rendering/RenderMultiColumnFlowThread.cpp#newcode176 - which is what made us end up with LayoutMultiColumnSpannerPlaceholder as the multicol child.

Note that WebKit actually has the RenderObject of the spanner as a direct child of the multicol container. I submitted patches for column-span:all to both Blink and WebKit. While WebKit accepted my initial approach, we ended up with a quite different solution in Blink.

Can we revisit the reasons that we took the LayoutMultiColumnSpannerPlaceholder way in blink? Your initial approach looks more reasonable to me, and LayoutMultiColumnSpannerPlaceholder seems to cause more complexities and troubles.

I probably won't have time before my leave, though. I'll be gone from next week until 2016-08-08.

eae@ what's layout team's current viewpoint about layout tree ordering and anonymous wrappers?

If our current approach is causing problems for the paint team we should reconsider.

I think we should be careful about this because layout tree reordering may still be a big concern.

Just found a method to fix the problem without needing reordering of PaintLayers or LayoutObjects. Will upload a patch soon.

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

commit db39a0e1882876a8c833821caf0544e8155eaa72
Author: wangxianzhu <wangxianzhu@chromium.org>
Date: Thu May 26 19:45:36 2016

Fix LayoutObject::paintingLayer() about multicol spanner

This fixes LayoutObject::paintLayer() for the following case:
<div style="columns: 3">
  <div style="column-span: all">
    <div style="overflow: hidden"></div>
  </div>
</div>

The inner div has a non-self-painting layer whose parent layer is the
multicol flow thread layer because layout tree and layer tree structure
follows the dom structure. However, the multicol flow thread layer is
not the actual painting layer but the column container layer is.

When looking for the painting layer, we need to follow the layout tree
instead of the layer tree and skip to the multicol container when we see
a spanner.

BUG= 611609 
TEST=LayoutObjectTest.PaintingLayerOfOverflowClipLayerUnderColumnSpanAll

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

[modify] https://crrev.com/db39a0e1882876a8c833821caf0544e8155eaa72/third_party/WebKit/Source/core/layout/LayoutObject.cpp
[modify] https://crrev.com/db39a0e1882876a8c833821caf0544e8155eaa72/third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp

ClusterFuzz has detected this testcase as flaky and is unable to reproduce it in the original crash revision. Skipping fixed testing check and marking it as potentially fixed.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=4877060354342912

Fuzzer: mbarbella_js_mutation_layout
Job Type: linux_debug_content_shell_drt
Platform Id: linux

Crash Type: ASSERT
Crash Address: 
Crash State:
  ASSERTION FAILED: &m_paintingLayer == currentObject.paintingLayer()
  blink::PaintInvalidationState::PaintInvalidationState
  blink::LayoutBoxModelObject::invalidateTreeIfNeeded
  

Minimized Testcase (0.12 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv94Qr3wDZV8TJo9Lu-lP3wYgOH1u3hI2v-PJ-yeZQf-V_SyIG_UioISZ9qkvS81PGFc9BIuvaI9Ucn4txYSY5FprvMJdc_8k3MUrFlZfNImpwKnLEBe60AnNBeVx4E30pHYrJx5dL9bBLF63NUQB6E9jMyjB7Q
<div style="-webkit-columns:9;">
   <img src="../resources/blimp.png" style="display:block; -webkit-column-span:all;"/>


See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

Removing EditIssue view restrictions from ClusterFuzz filed bugs. If you believe that this issue should still be restricted, please reapply the label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot