Bug 95494

patch coming

Created attachment 161544 [details] patch (for EWS)

Comment on attachment 161544 [details] patch (for EWS) Attachment 161544 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13682856

Comment on attachment 161544 [details] patch (for EWS) Attachment 161544 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13687827

Comment on attachment 161544 [details] patch (for EWS) Attachment 161544 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13693872

Created attachment 161553 [details] patch

Getting back to this bug, finally...

Created attachment 179063 [details] patch 1

Comment on attachment 179063 [details] patch 1 Attachment 179063 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15278637 New failing tests: accessibility/disabled-controls-not-focusable.html accessibility/canvas-fallback-content-2.html accessibility/textarea-line-for-index.html accessibility/canvas-accessibilitynodeobject.html compositing/culling/unscrolled-within-boxshadow.html http/tests/appcache/cyrillic-uri.html accessibility/canvas-fallback-content.html accessibility/aria-labelledby-on-input.html accessibility/ellipsis-text.html accessibility/crash-determining-aria-role-when-label-present.html accessibility/aria-disabled.html accessibility/title-ui-element-correctness.html accessibility/aria-label.html compositing/layers-inside-overflow-scroll.html accessibility/aria-readonly.html compositing/geometry/flipped-blocks-inline-mapping.html compositing/self-painting-layers.html compositing/geometry/geometry-map-scroll-during-layout-assertion.html animations/animation-on-inline-crash.html accessibility/secure-textfield-title-ui.html compositing/geometry/limit-layer-bounds-overflow-root.html accessibility/textarea-insertion-point-line-number.html compositing/self-painting-layers2.html compositing/geometry/foreground-offset-change.html accessibility/aria-describedby-on-input.html accessibility/aria-roles.html accessibility/nested-layout-crash.html compositing/culling/scrolled-within-boxshadow.html compositing/masks/mask-of-clipped-layer.html accessibility/textarea-selected-text-range.html

Created attachment 179083 [details] patch 2 - fixed failing tests

Comment on attachment 179083 [details] patch 2 - fixed failing tests Attachment 179083 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15286394 New failing tests: css2.1/20110323/overflow-applies-to-007.htm css2.1/20110323/dynamic-top-change-005b.htm fast/block/positioning/absolute-in-inline-rtl.html fast/block/positioning/absolute-in-inline-ltr-3.html css2.1/20110323/inline-non-replaced-width-001.htm fast/block/positioning/absolute-in-inline-ltr.html http/tests/inspector/search/search-in-concatenated-script.html http/tests/inspector/network/network-initiator-from-console.html css2.1/20110323/inline-non-replaced-width-002.htm css2.1/20110323/inline-box-002.htm compositing/geometry/flipped-blocks-inline-mapping.html css3/filters/filtered-inline.html fast/block/positioning/absolute-in-inline-ltr-2.html http/tests/inspector/search/search-in-script.html compositing/overflow/get-transform-from-non-box-container.html http/tests/inspector/network/request-parameters-decoding.html fast/block/positioning/abs-inside-inline-rel.html http/tests/inspector/network/network-sidebar-width.html http/tests/inspector/compiler-source-mapping-debug.html fast/block/positioning/058.html fast/block/positioning/052.html css2.1/20110323/dynamic-top-change-005.htm compositing/transitions/opacity-on-inline.html animations/animation-on-inline-crash.html fast/block/positioning/absolute-in-inline-rtl-2.html compositing/repaint/inline-repaint-container.html fast/block/inline-children-root-linebox-crash.html http/tests/inspector/network/network-preview-json.html fast/block/positioning/absolute-in-inline-rtl-3.html

Comment on attachment 179083 [details] patch 2 - fixed failing tests View in context: https://bugs.webkit.org/attachment.cgi?id=179083&action=review > Source/WebCore/ChangeLog:37 > + (Pseudo-but-real-call-stack): > + RenderLayer::hasHorizontalOverflow() > + RenderLayer::hasScrollableHorizontalOverflow() > + RenderLayer::usesCompositedScrolling() > + RenderLayer::shouldBeNormalFlowOnly() > + RenderLayer::RenderLayer() > + RenderLayerModelObject::ensureLayer() > + RenderLayerModelObject::styleDidChange() > + RenderBox::styleDidChange() > + RenderBlock::styleDidChange() > + RenderObject::setStyle() > + RenderObject::setAnimatableStyle() > + NodeRenderingContext::createRendererForElementIfNeeded() > + Element::createRendererIfNeeded() > + Element::attach() But given this call stack, how can you compute whether you're scrollable, since layout hasn't resized renderers yet? Or does it not matter because you'll layout and update soon anyway?

(In reply to comment #12) > (From update of attachment 179083 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=179083&action=review > > > Source/WebCore/ChangeLog:37 > > + (Pseudo-but-real-call-stack): > > + RenderLayer::hasHorizontalOverflow() > > + RenderLayer::hasScrollableHorizontalOverflow() > > + RenderLayer::usesCompositedScrolling() > > + RenderLayer::shouldBeNormalFlowOnly() > > + RenderLayer::RenderLayer() > > + RenderLayerModelObject::ensureLayer() > > + RenderLayerModelObject::styleDidChange() > > + RenderBox::styleDidChange() > > + RenderBlock::styleDidChange() > > + RenderObject::setStyle() > > + RenderObject::setAnimatableStyle() > > + NodeRenderingContext::createRendererForElementIfNeeded() > > + Element::createRendererIfNeeded() > > + Element::attach() > > But given this call stack, how can you compute whether you're scrollable, since layout hasn't resized renderers yet? Or does it not matter because you'll layout and update soon anyway? I do not think it can be computed accurately indeed. I am trying to find a way to bail out when layer "is not ready", like above. From my understanding, later on, a layout would finish and the layer would properly get promoted or not.

(In reply to comment #11) > (From update of attachment 179083 [details]) > Attachment 179083 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/15286394 > > New failing tests: > css2.1/20110323/overflow-applies-to-007.htm > css2.1/20110323/dynamic-top-change-005b.htm > fast/block/positioning/absolute-in-inline-rtl.html > fast/block/positioning/absolute-in-inline-ltr-3.html > css2.1/20110323/inline-non-replaced-width-001.htm > fast/block/positioning/absolute-in-inline-ltr.html > http/tests/inspector/search/search-in-concatenated-script.html > http/tests/inspector/network/network-initiator-from-console.html > css2.1/20110323/inline-non-replaced-width-002.htm > css2.1/20110323/inline-box-002.htm > compositing/geometry/flipped-blocks-inline-mapping.html > css3/filters/filtered-inline.html > fast/block/positioning/absolute-in-inline-ltr-2.html > http/tests/inspector/search/search-in-script.html > compositing/overflow/get-transform-from-non-box-container.html > http/tests/inspector/network/request-parameters-decoding.html > fast/block/positioning/abs-inside-inline-rel.html > http/tests/inspector/network/network-sidebar-width.html > http/tests/inspector/compiler-source-mapping-debug.html > fast/block/positioning/058.html > fast/block/positioning/052.html > css2.1/20110323/dynamic-top-change-005.htm > compositing/transitions/opacity-on-inline.html > animations/animation-on-inline-crash.html > fast/block/positioning/absolute-in-inline-rtl-2.html > compositing/repaint/inline-repaint-container.html > fast/block/inline-children-root-linebox-crash.html > http/tests/inspector/network/network-preview-json.html > fast/block/positioning/absolute-in-inline-rtl-3.html (In reply to comment #11) > (From update of attachment 179083 [details]) > Attachment 179083 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/15286394 > > New failing tests: > css2.1/20110323/overflow-applies-to-007.htm > css2.1/20110323/dynamic-top-change-005b.htm > fast/block/positioning/absolute-in-inline-rtl.html > fast/block/positioning/absolute-in-inline-ltr-3.html > css2.1/20110323/inline-non-replaced-width-001.htm > fast/block/positioning/absolute-in-inline-ltr.html > http/tests/inspector/search/search-in-concatenated-script.html > http/tests/inspector/network/network-initiator-from-console.html > css2.1/20110323/inline-non-replaced-width-002.htm > css2.1/20110323/inline-box-002.htm > compositing/geometry/flipped-blocks-inline-mapping.html > css3/filters/filtered-inline.html > fast/block/positioning/absolute-in-inline-ltr-2.html > http/tests/inspector/search/search-in-script.html > compositing/overflow/get-transform-from-non-box-container.html > http/tests/inspector/network/request-parameters-decoding.html > fast/block/positioning/abs-inside-inline-rel.html > http/tests/inspector/network/network-sidebar-width.html > http/tests/inspector/compiler-source-mapping-debug.html > fast/block/positioning/058.html > fast/block/positioning/052.html > css2.1/20110323/dynamic-top-change-005.htm > compositing/transitions/opacity-on-inline.html > animations/animation-on-inline-crash.html > fast/block/positioning/absolute-in-inline-rtl-2.html > compositing/repaint/inline-repaint-container.html > fast/block/inline-children-root-linebox-crash.html > http/tests/inspector/network/network-preview-json.html > fast/block/positioning/absolute-in-inline-rtl-3.html These failures look real. I was able to get one of the stack traces (complete, and different from the previous one): ASSERTION FAILED: !m_scrollDimensionsDirty /home/agomes/OpenSource/webkit/Source/WebCore/rendering/RenderLayer.cpp(2648) : bool WebCore::RenderLayer::hasHorizontalOverflow() const 1 0xb2c2b6d9 WebCore::RenderLayer::hasHorizontalOverflow() const 2 0xb2c2b611 WebCore::RenderLayer::hasScrollableHorizontalOverflow() const 3 0xb2c26105 WebCore::RenderLayer::usesCompositedScrolling() const 4 0xb2c35cec WebCore::RenderLayer::shouldBeNormalFlowOnly() const 5 0xb2c219e0 WebCore::RenderLayer::RenderLayer(WebCore::RenderLayerModelObject*) 6 0xb2c40a69 WebCore::RenderLayerModelObject::ensureLayer() 7 0xb2c40fb9 WebCore::RenderLayerModelObject::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) 8 0xb2ba5577 WebCore::RenderBox::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) 9 0xb2b3e596 WebCore::RenderBlock::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) 10 0xb2ca2301 WebCore::RenderTableCell::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) 11 0xb2c6b40c WebCore::RenderObject::setStyle(WTF::PassRefPtr<WebCore::RenderStyle>) 12 0xb2c6adf9 WebCore::RenderObject::setAnimatableStyle(WTF::PassRefPtr<WebCore::RenderStyle>) 13 0xb24d61c8 WebCore::NodeRenderingContext::createRendererForElementIfNeeded() 14 0xb2473321 WebCore::Element::createRendererIfNeeded() 15 0xb247337e WebCore::Element::attach() 16 0xb26d1414 17 0xb26d1648 WebCore::HTMLConstructionSite::executeQueuedTasks() 18 0xb26f5436 WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken(WebCore::AtomicHTMLToken*) 19 0xb26f5267 WebCore::HTMLTreeBuilder::constructTreeFromToken(WebCore::HTMLToken&) 20 0xb26d7013 WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) 21 0xb26d6a7a WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode) 22 0xb26d75bd WebCore::HTMLDocumentParser::append(WebCore::SegmentedString const&) 23 0xb23ffab8 WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter*, char const*, unsigned int) 24 0xb28569fc WebCore::DocumentWriter::addData(char const*, unsigned int) 25 0xb28444c5 WebCore::DocumentLoader::commitData(char const*, unsigned int) 26 0xb7921035 WebKit::WebFrameLoaderClient::committedLoad(WebCore::DocumentLoader*, char const*, int) 27 0xb28442cb WebCore::DocumentLoader::commitLoad(char const*, int) 28 0xb28447b4 WebCore::DocumentLoader::receivedData(char const*, int) 29 0xb287db22 WebCore::MainResourceLoader::addData(char const*, int, bool) 30 0xb289142b WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool) 31 0xb287ee8c WebCore::MainResourceLoader::didReceiveData(char const*, int, long long, bool) Program received signal SIGSEGV, Segmentation fault. 0xb2c2b6e3 in WebCore::RenderLayer::hasHorizontalOverflow (this=0x816d484) at /home/agomes/OpenSource/webkit/Source/WebCore/rendering/RenderLayer.cpp:2648 2648 ASSERT(!m_scrollDimensionsDirty); Looking for a condition to best check within hasScrollable{Vertical,Horizontal}Overflow to bail out earlier when the layer is not ready like the above.

Created attachment 179187 [details] patch 3 - slightly more conservative approach to fix the tests

You could possible check needsLayout() on the renderer.

Comment on attachment 179187 [details] patch 3 - slightly more conservative approach to fix the tests Attachment 179187 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15323075 New failing tests: accessibility/disabled-controls-not-focusable.html compositing/overflow/overflow-overlay-with-touch-no-overflow.html accessibility/canvas-fallback-content-2.html accessibility/textarea-line-for-index.html compositing/overflow/overflow-clip-with-accelerated-scrolling-ancestor.html compositing/culling/unscrolled-within-boxshadow.html compositing/overflow/clipping-ancestor-with-accelerated-scrolling-ancestor.html compositing/overflow/overflow-auto-with-touch-toggle.html compositing/layer-creation/scroll-partial-update.html compositing/overflow/overflow-scroll-with-touch-no-overflow.html compositing/layers-inside-overflow-scroll.html accessibility/aria-readonly.html compositing/geometry/flipped-blocks-inline-mapping.html compositing/self-painting-layers.html compositing/overflow/scroll-ancestor-update.html compositing/geometry/geometry-map-scroll-during-layout-assertion.html animations/animation-on-inline-crash.html compositing/overflow/overflow-auto-with-touch-no-overflow.html compositing/overflow/remove-overflow-crash2.html accessibility/textarea-insertion-point-line-number.html compositing/overflow/remove-overflow-crash.html compositing/overflow/get-transform-from-non-box-container.html http/tests/inspector/compiler-source-mapping-debug.html compositing/overflow/overflow-scroll.html compositing/transitions/opacity-on-inline.html accessibility/nested-layout-crash.html compositing/culling/scrolled-within-boxshadow.html compositing/repaint/inline-repaint-container.html compositing/overflow/iframe-inside-overflow-clipping.html accessibility/textarea-selected-text-range.html

(In reply to comment #16) > You could possible check needsLayout() on the renderer. So, I've been trying a couple of conditions to catch when we should bail out before the layer is not ready to query the layer scrollability, including -needsLayout on the renderer -hasLayer on the renderer -if the renderer has been parented (i.e. renderer()->parent()) And in all cases, the assert we reach occasionally is ASSERT(!m_scrollDimensionsDirty) in RenderLayer::hasHorizontal/VerticalOverflow(). Given that, i could: 1) continue with this approach, and maybe call RenderLayer::computeScrollDimensions(). Problem here, could be a possible slightly performance loss. 2) return the layer as not scrollable if "scroll dimensions" is dirty. The problem here is that we can make a layer to toggle on/off needlessly its need for a graphics layer more often. 3) Change the approach: checking the containing frame view's scrollable area cache, if it contains this layer. No drawbacks I thought of. Simon?

You really have to delay this decision until layout is complete. We do that for some other things (see the requiresCompositingForPosition() code).

(In reply to comment #19) > You really have to delay this decision until layout is complete. We do that for some other things (see the requiresCompositingForPosition() code). Approach #3 fits here, since a RenderLayer is cached by FrameView from either RenderLayer::updateScrollbarsAfterLayout or RenderLayer::updateScrollbarsAfterStyleChange: .... #if USE(ACCELERATED_COMPOSITING) bool RenderLayer::usesCompositedScrolling() const { - if (!scrollsOverflow() || !allowsScrolling()) + FrameView* frameView = renderer()->view()->frameView(); + if (!frameView || !frameView->containsScrollableArea(const_cast<RenderLayer*>(this))) return false; #if ENABLE(ACCELERATED_OVERFLOW_SCROLLING) return renderer()->style()->useTouchOverflowScrolling(); #else return false; #endif } endif Makes sense?

Created attachment 179334 [details] patch 4 - materialization of comment #20

Comment on attachment 179334 [details] patch 4 - materialization of comment #20 Raising the r? flag, although Simon suggested a different path on IRC, for the sake of this bug, querying if FrameView's scrollable area set cache (via ::containsScrollableArea) contains itself is enough, since FrameView::m_scrollableAreaSet is updated after every layout from RenderLayer::updateScrollbarsAfterLayout. That way, we are sure that the information gotten from FrameView::containsScrollableArea is in accordance with the last complete layout. I can work on a follow up to convert this whole method to something similar to what other ::requiresCompositingForXXX method do with regards to "pending layout state".

Comment on attachment 179334 [details] patch 4 - materialization of comment #20 What are the implications of getting this wrong? For example, if in the previous layout the layer was scrollable (so it's in the scrollable area cache), but you're in the style change before a layout that will make it non-scrollable, is it OK to return true from usesCompositedScrolling()?

(In reply to comment #23) > (From update of attachment 179334 [details]) > What are the implications of getting this wrong? For example, if in the previous layout the layer was scrollable (so it's in the scrollable area cache), but you're in the style change before a layout that will make it non-scrollable, is it OK to return true from usesCompositedScrolling()? if a style changes, making a previously scrollable layer not non-scrollable (say by setting overflow:hidden), then the call stack to notify the layer would be something like: RenderLayer::styleChanged RenderLayer::updateScrollbarsAfterStyleChange RenderLayer::updateScrollableAreaSet <- FrameView gets up to date, and following calls to ::usesCompositedScrolling query the right value. If the a layout actually finishes, the call stack would be: RenderLayer::updateScrollInfoAfterLayout RenderLayer::updateScrollbarsAfterLayout RenderLayer::updateScrollableAreaSet <- FrameView gets up to date and following calls to ::usesCompositedScrolling query the right value. So, from my understanding, it is a safe situation. As least, it does not change the current behavior, since the methods that update the FrameView scrollable area set cache are the ones that also update the scrollbars (toggle them on/off).

(In reply to comment #24) > (In reply to comment #23) > > (From update of attachment 179334 [details] [details]) > > What are the implications of getting this wrong? For example, if in the previous layout the layer was scrollable (so it's in the scrollable area cache), but you're in the style change before a layout that will make it non-scrollable, is it OK to return true from usesCompositedScrolling()? > > if a style changes, making a previously scrollable layer not non-scrollable s/not non-scrollable/non-scrollable/g (sorry about the noise)

for the record: <tonikitoo> smfr, hi. does https://bugs.webkit.org/show_bug.cgi?id=95494#c24 address your concerns? <smfr> tonikitoo: i don't care about what the call stack looks like, but I care about whether the page is rendered correctly, or whether user interaction is broken somehow <smfr> tonikitoo: e.g. I imagine we could have layers when we don't need them, but is that benign? <tonikitoo> smfr, back. I understand your concern, and still I do not think that the patch changes the current behavior. As I said, I can work on a follow up (if needed) to address this. <tonikitoo> smfr, i.e. this patch should not fix this bug, if it exists, but instead enable a few other configurations/ports to make use of what is already available <tonikitoo> do you think this is also a valid way to get it going? <tonikitoo> smfr, answering your question (but is that benign?), as you said in the bug, if it triggers compositing on a layer right after a style change but still before the corresponding layout has finished (it will eventually), I do not see a real world problem <tonikitoo> but I would still stand, out of the scope of this bug

Kindly ping reviewers.

(In reply to comment #27) > Kindly ping reviewers. Added some potential reviewers.

Created attachment 180205 [details] patch 4.1 - rebased against ToT after bug 94743

Simon, filed bug 105551 (Investigate the implications of RenderLayer::updateNeedsCompositedScrolling when there is a pending layout) to address any possible issue of this situation.

Comment on attachment 180205 [details] patch 4.1 - rebased against ToT after bug 94743 View in context: https://bugs.webkit.org/attachment.cgi?id=180205&action=review > Source/WebCore/rendering/RenderLayer.cpp:1867 > + if (!frameView || !frameView->containsScrollableArea(const_cast<RenderLayer*>(this))) I still see a const_cast here

Created attachment 181341 [details] patch 4.2 - no const_cast > > Source/WebCore/rendering/RenderLayer.cpp:1867 > > + if (!frameView || !frameView->containsScrollableArea(const_cast<RenderLayer*>(this))) > > I still see a const_cast here Removed.

Committed https://trac.webkit.org/changeset/139054