Bug 106271 – Sometimes RenderLayer::updateNeedsCompositedScrolling not called

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Xianzhu Wang]

Suppose a RenderLayer meets the requirement of composited scrolling, updateNeedsCompositedScrolling() is supposed to be called when m_hasOutOfFlowPositionedDescendant changes. However, because the initial value of m_hasOutOfFlowPositionedDescendant is false, it won't change for the RenderLayer never has out-of-flow-positioned descendant; if there is no other events triggering updateNeedsCompositedScrolling(), m_needsCompositedScrolling will always be false, causing slow scrolling of the layer.

[Xianzhu Wang]

Created attachment 181612 [details]
Wrong version, please ignore.

[Xianzhu Wang]

Created attachment 181614 [details]
Patch

[James Robinson]

Comment on attachment 181614 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181614&action=review

> LayoutTests/ChangeLog:8
> +        Actually the test didn't reproduce the issue. However the test can ensure the composited scrolling status is correct and not causing slow scrolling.

Do you know why this is?

[Xianzhu Wang]

(In reply to comment #3)
> (From update of attachment 181614 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181614&action=review
> 
> > LayoutTests/ChangeLog:8
> > +        Actually the test didn't reproduce the issue. However the test can ensure the composited scrolling status is correct and not causing slow scrolling.
> 
> Do you know why this is?

Not yet. I could not reproduce it with a manual test. Just did a debug with the test case and found that updateNeedsCompositedScrolling() is always called from styleChanged() during page loading. The issue happens with gmail.com. Will try if dynamically created elements cause it.

[Xianzhu Wang]

Found another case: if a container becomes scrollable dynamically, sometimes the composited scrolling status is not updated.

[Xianzhu Wang]

Created attachment 181746 [details]
Patch

[James Robinson]

Comment on attachment 181746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181746&action=review

> Source/WebCore/rendering/RenderLayer.cpp:150
> +    , m_hasOutOfFlowPositionedDescendant(true) // Initially true to be consistent with the initial value of m_needsCompositedScrolling.

why would the initial value of this matter if the bit is initially dirty?  No code should be looking at the value of m_hasOutOfFlowPositionedDescendant while m_hasOutOfFlowPositionedDescendantDirty is true, right?

[Xianzhu Wang]

Comment on attachment 181746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181746&action=review

>> Source/WebCore/rendering/RenderLayer.cpp:150
>> +    , m_hasOutOfFlowPositionedDescendant(true) // Initially true to be consistent with the initial value of m_needsCompositedScrolling.
> 
> why would the initial value of this matter if the bit is initially dirty?  No code should be looking at the value of m_hasOutOfFlowPositionedDescendant while m_hasOutOfFlowPositionedDescendantDirty is true, right?

In RenderLayer::updateDescendantDependentFlags(), the oldHasOutOfFlowPositionedDescendant is checked against the new value to determine if updateNeedsCompositedScrolling should be called:
        if (oldHasOutOfFlowPositionedDescendant != m_hasOutOfFlowPositionedDescendant)
            updateNeedsCompositedScrolling();

Here oldHasOutOfFlowPositionedDescendant may be the dirty value before the update.

[James Robinson]

(In reply to comment #8)
> (From update of attachment 181746 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181746&action=review
> 
> >> Source/WebCore/rendering/RenderLayer.cpp:150
> >> +    , m_hasOutOfFlowPositionedDescendant(true) // Initially true to be consistent with the initial value of m_needsCompositedScrolling.
> > 
> > why would the initial value of this matter if the bit is initially dirty?  No code should be looking at the value of m_hasOutOfFlowPositionedDescendant while m_hasOutOfFlowPositionedDescendantDirty is true, right?
> 
> In RenderLayer::updateDescendantDependentFlags(), the oldHasOutOfFlowPositionedDescendant is checked against the new value to determine if updateNeedsCompositedScrolling should be called:
>         if (oldHasOutOfFlowPositionedDescendant != m_hasOutOfFlowPositionedDescendant)
>             updateNeedsCompositedScrolling();
> 
> Here oldHasOutOfFlowPositionedDescendant may be the dirty value before the update.

Seems like the better fix would be to call updateNeedsCompositedScrolling() whenever m_hasOutOfFlowPositionedDescendantDirty transitions from true->false.

[Xianzhu Wang]

Comment on attachment 181746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181746&action=review

>>>> Source/WebCore/rendering/RenderLayer.cpp:150
>>>> +    , m_hasOutOfFlowPositionedDescendant(true) // Initially true to be consistent with the initial value of m_needsCompositedScrolling.
>>> 
>>> why would the initial value of this matter if the bit is initially dirty?  No code should be looking at the value of m_hasOutOfFlowPositionedDescendant while m_hasOutOfFlowPositionedDescendantDirty is true, right?
>> 
>> In RenderLayer::updateDescendantDependentFlags(), the oldHasOutOfFlowPositionedDescendant is checked against the new value to determine if updateNeedsCompositedScrolling should be called:
>>         if (oldHasOutOfFlowPositionedDescendant != m_hasOutOfFlowPositionedDescendant)
>>             updateNeedsCompositedScrolling();
>> 
>> Here oldHasOutOfFlowPositionedDescendant may be the dirty value before the update.
> 
> Seems like the better fix would be to call updateNeedsCompositedScrolling() whenever m_hasOutOfFlowPositionedDescendantDirty transitions from true->false.

Wouldn't that cause unnecessary updates? e.g. m_hasOutOfFlowPositionedDescendantDirty changes from false->true then ->false while m_hasOutOfFlowPositionedDescendant hasn't changed.

Actually I even consider unconditional updateNeedsCompositedScrolling() because it is not costly :)

Ian, what's your opinion?

(In reply to comment #10)
> (From update of attachment 181746 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181746&action=review
> 
> >>>> Source/WebCore/rendering/RenderLayer.cpp:150
> >>>> +    , m_hasOutOfFlowPositionedDescendant(true) // Initially true to be consistent with the initial value of m_needsCompositedScrolling.
> >>> 
> >>> why would the initial value of this matter if the bit is initially dirty?  No code should be looking at the value of m_hasOutOfFlowPositionedDescendant while m_hasOutOfFlowPositionedDescendantDirty is true, right?
> >> 
> >> In RenderLayer::updateDescendantDependentFlags(), the oldHasOutOfFlowPositionedDescendant is checked against the new value to determine if updateNeedsCompositedScrolling should be called:
> >>         if (oldHasOutOfFlowPositionedDescendant != m_hasOutOfFlowPositionedDescendant)
> >>             updateNeedsCompositedScrolling();
> >> 
> >> Here oldHasOutOfFlowPositionedDescendant may be the dirty value before the update.
> > 
> > Seems like the better fix would be to call updateNeedsCompositedScrolling() whenever m_hasOutOfFlowPositionedDescendantDirty transitions from true->false.
> 
> Wouldn't that cause unnecessary updates? e.g. m_hasOutOfFlowPositionedDescendantDirty changes from false->true then ->false while m_hasOutOfFlowPositionedDescendant hasn't changed.
> 
> Actually I even consider unconditional updateNeedsCompositedScrolling() because it is not costly :)
> 
> Ian, what's your opinion?

I'm not against calling updateNeedsCompositedScrolling more often here (as you mentioned, it's cheap), and I like James's suggestion of doing the update every time we 'undirty' m_hasOutOfFlowPositionedDescendant.

[Xianzhu Wang]

Created attachment 181951 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 181951 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181951&action=review

> Source/WebCore/ChangeLog:8
> +        Sometimes RenderLayer::updateNeedsCompositedScrolling is not called for a RenderLayer without out-of-flow-positioned descendant
> +        https://bugs.webkit.org/show_bug.cgi?id=106271
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: compositing/overflow/dynamic-composited-scrolling-status.html

Please describe the bug, and how you fixed it here.

[Xianzhu Wang]

Created attachment 181953 [details]
Patch

[Xianzhu Wang]

James, Simon and Ian, what do you think of the last patch?

Comment on attachment 181953 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181953&action=review

Just one nit.

> Source/WebCore/rendering/RenderLayer.h:477
> +    // FIXME: We should ASSERT(!m_hasOutOfFlowPositionedDescendant); here but we may hit the same bugs as visible content above.

m_hasOutOfFlowPositionedDescendantDirty?

[Xianzhu Wang]

Comment on attachment 181953 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181953&action=review

>> Source/WebCore/rendering/RenderLayer.h:477
>> +    // FIXME: We should ASSERT(!m_hasOutOfFlowPositionedDescendant); here but we may hit the same bugs as visible content above.
> 
> m_hasOutOfFlowPositionedDescendantDirty?

Done.

[Xianzhu Wang]

Created attachment 182161 [details]
Patch

[Xianzhu Wang]

Kindly ping reviewers...

[Xianzhu Wang]

Created attachment 182444 [details]
Slightly simplified

[Xianzhu Wang]

Kindly ping reviewers...

[Xianzhu Wang]

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

James and Simon?

[Simon Fraser (smfr)]

Comment on attachment 182444 [details]
Slightly simplified

View in context: https://bugs.webkit.org/attachment.cgi?id=182444&action=review

> Source/WebCore/rendering/RenderLayer.cpp:5547
> +    if (shouldBeScrollableArea != frameView->containsScrollableArea(this)) {
> +        if (shouldBeScrollableArea)
> +            frameView->addScrollableArea(this);
> +        else
> +            frameView->removeScrollableArea(this);

This is two hash lookups in either case. Would better if you could reduce it to one.

[Xianzhu Wang]

Created attachment 182638 [details]
Patch

[Xianzhu Wang]

Comment on attachment 182638 [details]
Patch

PTAL. I'm not sure about the style.

[Simon Fraser (smfr)]

Comment on attachment 182638 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182638&action=review

> Source/WebCore/page/FrameView.h:350
> +    bool addScrollableArea(ScrollableArea*);
> +    bool removeScrollableArea(ScrollableArea*);

Please add a comment saying what the return values mean.

[Xianzhu Wang]

Created attachment 182639 [details]
for landing

[WebKit Review Bot]

Comment on attachment 182639 [details]
for landing

Clearing flags on attachment: 182639

Committed r139691: <http://trac.webkit.org/changeset/139691>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Sami Kyöstilä]

*** Bug 106915 has been marked as a duplicate of this bug. ***