Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2618323004:
Don't force-update style and layout in Document::scrollingElement (Closed)
DescriptionDon't force-update style and layout in Document::scrollingElement
This method has callsites inside of compositing, in which it's illegal
to try to update layout and style because compositing comes after.
However, it's also pointless to do so from such callsites.
The other call sites are: FrameView::scrollBehaviorStyle
Various scrolling methods in Element. All of the Element callsites already
update style and layout; this CL adds it to the former (which has a very
complicated set of ways it can be called, making it very hard to determine
if style or layout could be dirty before calling it).
BUG=679099
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2618323004
Cr-Commit-Position: refs/heads/master@{#443049}
Committed: https://chromium.googlesource.com/chromium/src/+/e8c827b638b54a43e065cd6b3a431fefc5ae9768
Patch Set 1 #Patch Set 2 : none #Patch Set 3 : none #Patch Set 4 : none #
Total comments: 6
Patch Set 5 : none #Patch Set 6 : none #
Total comments: 1
Patch Set 7 : none #
Total comments: 1
Messages
Total messages: 45 (24 generated)
Description was changed from ========== none BUG= ========== to ========== Don't force-update style and layout in Document::scrollingElement This method has callsites inside of compositing, in which it's illegal to try to update layout and style because compositing comes after. However, it's also pointless to do so from such callsites. The other call sites are: FrameView::scrollBehaviorStyle Various scrolling methods in Element. All of the Element callsites already update style and layout; this CL adds it to the former (which has a very complicated set of ways it can be called, making it very hard to determine if style or layout could be dirty before calling it). BUG=679099 ==========
chrishtr@chromium.org changed reviewers: + wkorman@chromium.org
The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm Is there a test case we can reduce from the linked bug to prevent regression?
On 2017/01/10 at 00:33:10, wkorman wrote: > lgtm > > Is there a test case we can reduce from the linked bug to prevent regression? It's from a "renderer" fuzzer, which doesn't produce usable output AFAICS.
The CQ bit was unchecked by chrishtr@chromium.org
The CQ bit was checked by chrishtr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Description was changed from ========== Don't force-update style and layout in Document::scrollingElement This method has callsites inside of compositing, in which it's illegal to try to update layout and style because compositing comes after. However, it's also pointless to do so from such callsites. The other call sites are: FrameView::scrollBehaviorStyle Various scrolling methods in Element. All of the Element callsites already update style and layout; this CL adds it to the former (which has a very complicated set of ways it can be called, making it very hard to determine if style or layout could be dirty before calling it). BUG=679099 ========== to ========== Don't force-update style and layout in Document::scrollingElement This method has callsites inside of compositing, in which it's illegal to try to update layout and style because compositing comes after. However, it's also pointless to do so from such callsites. The other call sites are: FrameView::scrollBehaviorStyle Various scrolling methods in Element. All of the Element callsites already update style and layout; this CL adds it to the former (which has a very complicated set of ways it can be called, making it very hard to determine if style or layout could be dirty before calling it). BUG=679099 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Updated the CL to remove forced-style/layout update in all cases, and break out a different version of Document::scrollingElement that updates layout and is intended only for the Document.idl backing.
https://codereview.chromium.org/2618323004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2618323004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1354: if (RuntimeEnabledFeatures::scrollTopLeftInteropEnabled()) { Maybe add a DCHECK to ensure we're at LayoutClean? https://codereview.chromium.org/2618323004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2618323004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.h:349: Element* scrollingElementInternal(); Wondering if there is a better name, since isn't usually 'Internal' only used for calling from within the containing class? Maybe something that implies style/layout must already be clean. scrollingElementPostLayout? https://codereview.chromium.org/2618323004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2618323004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:895: LOG(ERROR) << "scrollto"; rm
https://codereview.chromium.org/2618323004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2618323004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1354: if (RuntimeEnabledFeatures::scrollTopLeftInteropEnabled()) { On 2017/01/11 at 03:56:39, wkorman wrote: > Maybe add a DCHECK to ensure we're at LayoutClean? Done. https://codereview.chromium.org/2618323004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2618323004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.h:349: Element* scrollingElementInternal(); On 2017/01/11 at 03:56:39, wkorman wrote: > Wondering if there is a better name, since isn't usually 'Internal' only used for calling from within the containing class? Maybe something that implies style/layout must already be clean. scrollingElementPostLayout? Done. https://codereview.chromium.org/2618323004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2618323004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:895: LOG(ERROR) << "scrollto"; On 2017/01/11 at 03:56:39, wkorman wrote: > rm argh. I thought I had removed that, sorry.
The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by chrishtr@chromium.org
The CQ bit was checked by chrishtr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chrishtr@chromium.org
https://codereview.chromium.org/2618323004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2618323004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Document.cpp:1356: DCHECK(m_lifecycle.state() >= DocumentLifecycle::StyleClean); StyleClean is the right one, since updateStyleAndLayoutTree() does not layout, just updates style and creates the layout tree.
The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org Link to the patchset: https://codereview.chromium.org/2618323004/#ps100001 (title: "none")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
How could we capture this with a test? Maybe a Sim test would help here?
On 2017/01/11 at 17:26:29, dglazkov wrote: > How could we capture this with a test? Maybe a Sim test would help here? Capture what exactly with a test?
On 2017/01/11 at 17:38:00, chrishtr wrote: > On 2017/01/11 at 17:26:29, dglazkov wrote: > > How could we capture this with a test? Maybe a Sim test would help here? > > Capture what exactly with a test? Good question. I should have read the CL properly. I see now that the problem is that the web-exposed function was used instead of what should have been a special, more gentle internal function.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2017/01/11 at 19:16:41, dglazkov wrote: > On 2017/01/11 at 17:38:00, chrishtr wrote: > > On 2017/01/11 at 17:26:29, dglazkov wrote: > > > How could we capture this with a test? Maybe a Sim test would help here? > > > > Capture what exactly with a test? > > Good question. I should have read the CL properly. I see now that the problem is that the web-exposed function was used instead of what should have been a special, more gentle internal function. Ok cool.
The CQ bit was checked by chrishtr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wkorman@chromium.org Link to the patchset: https://codereview.chromium.org/2618323004/#ps120001 (title: "none")
https://codereview.chromium.org/2618323004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2618323004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:777: if (!inActiveDocument()) These were needed because updateStyleAndLayoutIgnorePendingStylesheetsForNode early-outs if the element is not in the document, and hence the document's lifecycle remains dirty, triggering the assert. However, if the element is not in the document, the only possible return value is 0 anyway.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484170945718910,
"parent_rev": "b3cc3c0f613ff302f4833781a388e2ad8646f079", "commit_rev":
"e8c827b638b54a43e065cd6b3a431fefc5ae9768"}
Message was sent while issue was closed.
Description was changed from ========== Don't force-update style and layout in Document::scrollingElement This method has callsites inside of compositing, in which it's illegal to try to update layout and style because compositing comes after. However, it's also pointless to do so from such callsites. The other call sites are: FrameView::scrollBehaviorStyle Various scrolling methods in Element. All of the Element callsites already update style and layout; this CL adds it to the former (which has a very complicated set of ways it can be called, making it very hard to determine if style or layout could be dirty before calling it). BUG=679099 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Don't force-update style and layout in Document::scrollingElement This method has callsites inside of compositing, in which it's illegal to try to update layout and style because compositing comes after. However, it's also pointless to do so from such callsites. The other call sites are: FrameView::scrollBehaviorStyle Various scrolling methods in Element. All of the Element callsites already update style and layout; this CL adds it to the former (which has a very complicated set of ways it can be called, making it very hard to determine if style or layout could be dirty before calling it). BUG=679099 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2618323004 Cr-Commit-Position: refs/heads/master@{#443049} Committed: https://chromium.googlesource.com/chromium/src/+/e8c827b638b54a43e065cd6b3a43... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/e8c827b638b54a43e065cd6b3a43... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
