New issue
Advanced search Search tips

Issue 859520 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 4
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Under-invalidation with SVG foreign object

Project Member Reported by schenney@chromium.org, Jul 2

Issue description

Reported as part of https://bugs.chromium.org/p/chromium/issues/detail?id=859294

wangxianzhu@ says:

The root cause is mismatching hierarchies of PaintLayer and compositing. 
The DOM tree is like:
  <svg style="backface-visibility: hidden">
    <foreignObject>
      some inline boxes
    </foreignObject>
  </svg>

In the PaintLayer tree, the layers for the svg root and the foreign object are siblings:
 layer for HTML
  normal flow list
   layer for the svg
  positive z-order list
   layer for the foreignObject
while in the GraphicsLayer tree, the foreignObject is painted on the GraphicsLayer of the svg.

When the contents of the foreignObject changes, we mark the painting layer ancestors for NeedsRepaint, and miss the svg which is not in the painting layer ancestor path. In the next paint, the svg's GraphicsLayer is repainted. Then during FinishCycle() when we iterate the DisplayItemClients we access some deleted DisplayItemClients under the foreignObject.

I can change GraphicsLayer to call FinishCycle() for real repainted layers only to avoid the heap-use-after-free for the test case. That won't fix the under-invalidation though.

chrishtr@ can you look into the under-invalidation issue?
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 4

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

commit 5c301712a67bbe70026048a224b4608628199e12
Author: Chris Harrelson <chrishtr@chromium.org>
Date: Wed Jul 04 01:20:53 2018

Fix CompositingContainer for replaced normal-flow stacking contexts.

For such PaintLayers, the CompositingContainer is the parent, not the
containing stacking context. This is because such stacking contexts
paint during the normal-flow of the ancestor layout object.

This also allows us to enable subsequence caching; the previous attempt
last week was still suffering from the under-invalidation reported
in  issue 859520 .

The testcases that crash on ASAN from the referenced bugs are fixed;
one such example is included in this CL.

Bug:859294, 719835 ,723076, 859520 

Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I6ab5271d70bd834482c22b89f09b93907788ed0c
Reviewed-on: https://chromium-review.googlesource.com/1125269
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572428}
[add] https://crrev.com/5c301712a67bbe70026048a224b4608628199e12/third_party/WebKit/LayoutTests/svg/foreignObject/foreignObject-position-crash.html
[modify] https://crrev.com/5c301712a67bbe70026048a224b4608628199e12/third_party/blink/renderer/core/paint/compositing/graphics_layer_updater.cc
[modify] https://crrev.com/5c301712a67bbe70026048a224b4608628199e12/third_party/blink/renderer/core/paint/paint_invalidator.cc
[modify] https://crrev.com/5c301712a67bbe70026048a224b4608628199e12/third_party/blink/renderer/core/paint/paint_layer.cc
[modify] https://crrev.com/5c301712a67bbe70026048a224b4608628199e12/third_party/blink/renderer/core/paint/paint_layer.h
[modify] https://crrev.com/5c301712a67bbe70026048a224b4608628199e12/third_party/blink/renderer/core/paint/paint_layer_test.cc

Status: Fixed (was: Assigned)

Sign in to add a comment