Style is dirtied during BoxPainter::PaintBoxDecorationBackgroundWithRect |
|||||||||||
Issue descriptionOriginal bug with repro instructions: https://bugs.chromium.org/p/chromium/issues/detail?id=712933 Viewing the repro link causes a style recalc during paint code. The reason the document was dirtied is the following: 1. BoxPainter::PaintBoxDecorationBackgroundWithRect uses paint code that calls into Element::clientHeight. 2. Element::clientHeight calls Document::UpdateStyleAndLayoutIgnorePendingStylesheetsForNode, which calls Document::UpdateStyleAndLayoutTreeIgnorePendingStylesheets 3. The document has pending scripts blocking sheets and dirties the entire document with kSubtreeStyleChange, which then in turn forces a style recalc over the whole document. The dirtying happens here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Document.cpp?type=cs&q=Document::UpdateStyleAndLayoutTreeIgnorePendingStylesheets&sq=package:chromium&l=2394 So either: a) The style code is performing as expected, and paint code should never call UpdateStyleAndLayoutTreeIgnorePendingStylesheets. b) The style code is wrong, and the document should not have been dirtied. From the code it looks like this is a style related issue. I'm marking this as available for someone more knowledgeable to comment on what's going on here. I've also put on a logging CL here: https://chromium-review.googlesource.com/c/571201/ You should get an output similar to: ------------------------------------------------------------------------------------------------------- [ERROR:BoxPainter.cpp(165)] [BoxPainter 0xe8806f9c] Painting. Original style: 0x4c436ee8 [ERROR:Element.cpp(843)] [Element 0x2df82b18] clientHeight [ERROR:Element.cpp(864)] [Element 0x2df82b18] UpdateStyleAndLayoutIgnorePendingStylesheetsForNode [ERROR:Document.cpp(2393)] [Document 0x2de620f0] dirtied document due to pending script blocking sheets [ERROR:Node.cpp(828)] [Node 0x2de620f0 was dirtied with change_type: 524288 .... [ERROR:BoxPainter.cpp(169)] [BoxPainter 0xe8806f9c] Finished painting. New style: 0xd3ec8280 ------------------------------------------------------------------------------------------------------- which shows clientHeight dirtying the document and BoxPainter ending up with a different style.
,
Jul 14 2017
+pdr who I believe originally wrote that style-specific bit of code in https://chromium.googlesource.com/chromium/src/+/0c31fb86e9388c56cf1f0ba5bb12dfcbdcb1f473 I'm honestly not sure. It's pretty old code (I've traced the blame for updateLayoutIgnorePendingStylesheets back as far to 2004, originally here by darin: https://chromium.googlesource.com/chromium/src/+/739947b25245246cd75715a44a30bff66184fabd)... From reading the comments, I'd guess this function was originally added to service JS that ran before all the stylesheets have loaded?
,
Jul 14 2017
UpdateStyleAndLayoutIgnorePendingStylesheetsForNode does ignore pending style sheets. The problem is that if Document::has_nodes_with_placeholder_style_ is true, style will be dirtied. There is also code which will dirty tree scopes, which is bad. UpdateStyleAndLayoutIgnorePendingStylesheetsForNode is known to be heuristical and hacky (one reason there is an intent to ship for removing it). On reading the contextual comments in Document::UpdateStyleAndLayoutIgnorePendingStylesheetsForNode, I think it is working "as intended". The core bug then is that paint code should not call Element::clientHeight (cause (a) in comment 0.
,
Jul 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/49b2e1c02da7073c6c86e7e591928fc58fd5de34 commit 49b2e1c02da7073c6c86e7e591928fc58fd5de34 Author: Chris Harrelson <chrishtr@chromium.org> Date: Mon Jul 17 18:02:24 2017 Don't call Element::clientHeight from paint code. This is a lifecycle violation, because clientHeight might invalidate style. Bug: 742704 Change-Id: Ifbe1d8df924e88074b5741dcd48f1d2171a6a04e Reviewed-on: https://chromium-review.googlesource.com/572213 Reviewed-by: Philip Rogers <pdr@chromium.org> Commit-Queue: Chris Harrelson <chrishtr@chromium.org> Cr-Commit-Position: refs/heads/master@{#487157} [modify] https://crrev.com/49b2e1c02da7073c6c86e7e591928fc58fd5de34/third_party/WebKit/Source/core/dom/Document.cpp [modify] https://crrev.com/49b2e1c02da7073c6c86e7e591928fc58fd5de34/third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp
,
Jul 17 2017
,
Jul 18 2017
Issue 712933 has been merged into this issue.
,
Jul 18 2017
See issue 712933 for context on crashes / rates. Merge approved for M60 branch 3112. Please merge ASAP.
,
Jul 18 2017
,
Jul 18 2017
Users experienced this crash on the following builds: Android Dev 61.0.3142.0 - 0.48 CPM, 73 reports, 45 clients (signature PaintInsetBoxShadow) Android Beta 60.0.3112.66 - 1.95 CPM, 158 reports, 85 clients (signature blink::BoxPainterBase::PaintInsetBoxShadow) If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates. - Go/Fracas
,
Jul 18 2017
Users experienced this crash on the following builds: Android Beta 60.0.3112.66 - 0.33 CPM, 27 reports, 16 clients (signature blink::ComputedStyle::BorderLeftWidth) If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates. - Go/Fracas
,
Jul 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/83868f62b06e0501051e0543c96125f670873fcf commit 83868f62b06e0501051e0543c96125f670873fcf Author: Chris Harrelson <chrishtr@chromium.org> Date: Tue Jul 18 16:55:38 2017 Don't call Element::clientHeight from paint code. This is a lifecycle violation, because clientHeight might invalidate style. TBR=pdr (cherry picked from commit 49b2e1c02da7073c6c86e7e591928fc58fd5de34) Bug: 742704 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: Ifbe1d8df924e88074b5741dcd48f1d2171a6a04e Reviewed-on: https://chromium-review.googlesource.com/572213 Reviewed-by: Philip Rogers <pdr@chromium.org> Commit-Queue: Chris Harrelson <chrishtr@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#487157} Reviewed-on: https://chromium-review.googlesource.com/576272 Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Cr-Commit-Position: refs/branch-heads/3112@{#634} Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897} [modify] https://crrev.com/83868f62b06e0501051e0543c96125f670873fcf/third_party/WebKit/Source/core/paint/MediaControlsPainter.cpp
,
Jul 18 2017
Users experienced this crash on the following builds: Android Dev 61.0.3142.0 - 0.48 CPM, 73 reports, 45 clients (signature PaintInsetBoxShadow) Android Beta 60.0.3112.66 - 1.95 CPM, 158 reports, 85 clients (signature blink::BoxPainterBase::PaintInsetBoxShadow) If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates. - Go/Fracas
,
Jul 18 2017
|
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by chrishtr@chromium.org
, Jul 14 2017