DCHECK on Transform property in ~FindObjectPropertiesNeedingUpdateScope |
|||||
Issue description
I have a problem with an (access restricted) application that has a sliding menu animating up from the bottom of the screen. The div in question has the following properties:
transition-property: transform, opacity;
transition-duration: 0.8s, 0.3s;
When I stress test this, triggering forward/retract in quick succession, I soon get:
[snip]
[1:1:1123/152004.802411:1475171395101:FATAL:FindPropertiesNeedingUpdate.h(147)] Check failed: *m_originalProperties->transform() == *objectProperties->transform(). Property was updated without the layout object ("LayoutBlockFlow DIV class='video-overlay__container'") needing a paint property update.
Original:
parent=0x1e72e51b05b0 transform=identity origin=640,140,0 flattensInheritedTransform=yes renderingContextId=0 directCompositingReasons=none compositorElementId=(0, 0)
Updated:
parent=0x1e72e51b05b0 transform=identity origin=640,140,0 flattensInheritedTransform=yes renderingContextId=0 directCompositingReasons=activeAnimation compositorElementId=(26, 0)
#0 0x7f53c622f4f7 base::debug::StackTrace::StackTrace()
#1 0x7f53c624f0f7 logging::LogMessage::~LogMessage()
#2 0x7f53c8f58e3e blink::FindObjectPropertiesNeedingUpdateScope::~FindObjectPropertiesNeedingUpdateScope()
#3 0x7f53c8f5cc55 blink::PaintPropertyTreeBuilder::updatePropertiesForSelf()
#4 0x7f53c8f6305e blink::PrePaintTreeWalk::walk()
#5 0x7f53c8f63162 blink::PrePaintTreeWalk::walk()
[snip]
I have traced it down to this commit: https://codereview.chromium.org/2695593005, more specifically the move of the following function: 'updatePaintOffsetTranslation(object, context)'
The issue reproduces still, despite heavy refactorings in the related code. I reproduced it on our integration of Chromium 62.0.3202.89
I don't think the code move in itself is the problem, but rather the small extra delay introduced before entering the 'FindObjectPropertiesNeedingUpdateScope' safeguard in 'PaintPropertyTreeBuilder::updatePropertiesForSelf'. Now my debugging tells me that we may enter updatePropertiesForSelf with a transform property set on the LayoutObject, but with no paint property invalidation!
Despite a clear hint on the DCHECK, I have not managed to pinpoint where the invalidation should have been made. I do have the following clues(?):
* If I disable 'transition-duration' in the application this state is not triggered.
* If I remove one of the transition-properties (and the matching duration) this state is not triggered.
I don't know if this is related, or if it just part-takes in the timing required for triggering the state...
Unfortunately, I have not managed to create a minimal test case to trigger this - maybe because it requires a heavy page to be triggered and I can't copy the whole application? Neither can I make the test page I'm running accessible to you.
Despite this, I am hoping you may help with some insights that could help me find the missing paint invalidation?
BR,
Åsa at Vewd (formerly Opera TV)
,
Nov 28 2017
Is this a debug only DCHECK, or a release CHECK?
,
Nov 28 2017
This is a DCHECK only, so not blocking. I'm not sure if it indicates an actual problem or is only an indication of in-efficient painting.
,
Nov 30 2017
We miss a SetNeedsPaintPropertyUpdate() when some style changes causing directCompositingReason changes from none to activeAnimation. The DCHECK failure is a regression but the underlying under-invalidation of paint properties is not new, but discovered by the under-invalidation checking mechanism. DCHECK failure should be priority 1 though.
,
Dec 1 2017
I couldn't create a test case for this. It seems that we always have NeedsPaintPropertyUpdate set when a transform or opacity becomes animation. asaka@vewd.com, which revision did you use? Based on the DCHECK failure "*m_originalProperties ..." it seems a very old revision before renaming. Can you try the latest revision?
,
Dec 1 2017
directCompositingReasons and compositorElementId are for slimming-paint-v2 only which is not shipped yet so they don't cause any real problems even if they are not updated.
,
Dec 4 2017
Thank you for looking at this @wangxianzhu! I also could not make a test case, but let me know if you need me to test anything on the restricted page where I can reproduce it!
When doing the debugging I was using the regression point, hence the "old code" references. I can also reproduce this on our itnegration of Chromium 63.0.3236.6:
[1:1:1204/091423.581652:2403630174310:FATAL:FindPropertiesNeedingUpdate.h(168)] Check failed: *original_properties_->Transform() == *object_properties->Transform(). Property was updated without the layout object ("LayoutBlockFlow DIV class='video-overlay__container'") needing a paint property update.
Original:
{"parent":"0x2a075e9ae010","compositorElementId":"(2800)"}
Updated:
{"parent":"0x2a075e9ae010","directCompositingReasons":"activeAnimation","compositorElementId":"(2800)"}
#0 0x7f3e55e24857 base::debug::StackTrace::StackTrace()
#1 0x7f3e55e44a8d logging::LogMessage::~LogMessage()
#2 0x7f3e58904d45 blink::FindObjectPropertiesNeedingUpdateScope::~FindObjectPropertiesNeedingUpdateScope()
#3 0x7f3e58901a1d blink::PaintPropertyTreeBuilder::UpdatePropertiesForSelf()
#4 0x7f3e5891649c blink::PrePaintTreeWalk::Walk()
#5 0x7f3e5891677e blink::PrePaintTreeWalk::Walk()
#6 0x7f3e5891677e blink::PrePaintTreeWalk::Walk()
,
Dec 4 2017
asaka@vewd.com can you try https://chromium-review.googlesource.com/c/chromium/src/+/802315 to see if it can eliminate the DCHECK failure?
,
Dec 5 2017
This patch work for me on both Chromium 63 and Chromium 59. No DCHECK hit. Thanks!!
,
Dec 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/555a1985c18cb6ca9053f7598adf8b1ba1a71f3f commit 555a1985c18cb6ca9053f7598adf8b1ba1a71f3f Author: Xianzhu Wang <wangxianzhu@chromium.org> Date: Sat Dec 09 14:18:05 2017 [PE] Distinguish different compositing reasons for animations Previously we had only 1 bit in compositing reasons for active animations. However, sometimes a new kind of animation starts before a previous animation ends, which needs a paint property update for compositing reasons of different property nodes. Now distinguish different animations so that PaintLayer::StyleDidChange() can see a compositing reason change in the case and trigger paint property update. Bug: 788718 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I2e4519e377ea507cb4856594a284750892f9ce30 Reviewed-on: https://chromium-review.googlesource.com/802315 Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org> Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Cr-Commit-Position: refs/heads/master@{#522987} [modify] https://crrev.com/555a1985c18cb6ca9053f7598adf8b1ba1a71f3f/third_party/WebKit/LayoutTests/compositing/animation/hidden-composited.html [modify] https://crrev.com/555a1985c18cb6ca9053f7598adf8b1ba1a71f3f/third_party/WebKit/LayoutTests/flag-specific/root-layer-scrolls/http/tests/devtools/layers/layer-compositing-reasons-expected.txt [modify] https://crrev.com/555a1985c18cb6ca9053f7598adf8b1ba1a71f3f/third_party/WebKit/LayoutTests/http/tests/devtools/layers/layer-compositing-reasons-expected.txt [modify] https://crrev.com/555a1985c18cb6ca9053f7598adf8b1ba1a71f3f/third_party/WebKit/Source/core/animation/Animation.cpp [modify] https://crrev.com/555a1985c18cb6ca9053f7598adf8b1ba1a71f3f/third_party/WebKit/Source/core/animation/CompositorAnimationsTest.cpp [modify] https://crrev.com/555a1985c18cb6ca9053f7598adf8b1ba1a71f3f/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp [modify] https://crrev.com/555a1985c18cb6ca9053f7598adf8b1ba1a71f3f/third_party/WebKit/Source/core/paint/PaintPropertyTreeUpdateTests.cpp [modify] https://crrev.com/555a1985c18cb6ca9053f7598adf8b1ba1a71f3f/third_party/WebKit/Source/core/paint/compositing/CompositingReasonFinder.cpp [modify] https://crrev.com/555a1985c18cb6ca9053f7598adf8b1ba1a71f3f/third_party/WebKit/Source/core/paint/compositing/CompositingReasonFinder.h [modify] https://crrev.com/555a1985c18cb6ca9053f7598adf8b1ba1a71f3f/third_party/WebKit/Source/core/paint/compositing/CompositingReasonFinderTest.cpp [modify] https://crrev.com/555a1985c18cb6ca9053f7598adf8b1ba1a71f3f/third_party/WebKit/Source/core/paint/compositing/CompositingRequirementsUpdater.cpp [modify] https://crrev.com/555a1985c18cb6ca9053f7598adf8b1ba1a71f3f/third_party/WebKit/Source/core/paint/compositing/PaintLayerCompositor.cpp [modify] https://crrev.com/555a1985c18cb6ca9053f7598adf8b1ba1a71f3f/third_party/WebKit/Source/platform/graphics/CompositingReasons.cpp [modify] https://crrev.com/555a1985c18cb6ca9053f7598adf8b1ba1a71f3f/third_party/WebKit/Source/platform/graphics/CompositingReasons.h [modify] https://crrev.com/555a1985c18cb6ca9053f7598adf8b1ba1a71f3f/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp [modify] https://crrev.com/555a1985c18cb6ca9053f7598adf8b1ba1a71f3f/third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h [modify] https://crrev.com/555a1985c18cb6ca9053f7598adf8b1ba1a71f3f/third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h
,
Dec 9 2017
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by f...@opera.com
, Nov 27 2017Labels: -Pri-3 Pri-2