Bug 95494 – Make RenderLayer::updateNeedsCompositedScrolling scrollbars agnostic

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Antonio Gomes]

This is always false for the Blackberry port, for example:

bool RenderLayer::allowsScrolling() const
{
    return (m_hBar && m_hBar->enabled()) || (m_vBar && m_vBar->enabled());
}

.....
We can query FrameView::ScrollableAreaSet instead

[Antonio Gomes]

patch coming

[Antonio Gomes]

Created attachment 161544 [details]
patch (for EWS)

[WebKit Review Bot]

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

[Build Bot]

Comment on attachment 161544 [details]
patch (for EWS)

Attachment 161544 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13687827

[Early Warning System Bot]

Comment on attachment 161544 [details]
patch (for EWS)

Attachment 161544 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13693872

[Antonio Gomes]

Created attachment 161553 [details]
patch

[Antonio Gomes]

Getting back to this bug, finally...

[Antonio Gomes]

Created attachment 179063 [details]
patch 1

[WebKit Review Bot]

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

[Antonio Gomes]

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

[WebKit Review Bot]

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

[Simon Fraser (smfr)]

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?

[Antonio Gomes]

(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.

[Antonio Gomes]

(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.

[Antonio Gomes]

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

[Simon Fraser (smfr)]

You could possible check needsLayout() on the renderer.

[WebKit Review Bot]

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

[Antonio Gomes]

(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?

[Simon Fraser (smfr)]

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

[Antonio Gomes]

(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?

[Antonio Gomes]

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

[Antonio Gomes]

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".

[Simon Fraser (smfr)]

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()?

[Antonio Gomes]

(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).

[Antonio Gomes]

(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)

[Antonio Gomes]

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

[Antonio Gomes]

Kindly ping reviewers.

[Antonio Gomes]

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

Added some potential reviewers.

[Antonio Gomes]

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

[Antonio Gomes]

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

[James Robinson]

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

[Antonio Gomes]

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.

[Antonio Gomes]

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