Bug 106890 – Delegated scrolling: Assertion on attempt to show a CSS sticky element

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mikhail Pozdnyakov]

Crash can be easily reproduced with opening css-sticky test cases (for example fast/css/sticky/inflow-sticky.html).

ASSERTION FAILED: enclosingIntRect(rendererMappedResult) == enclosingIntRect(FloatQuad(result).boundingBox())
/media/ssd/WebKit/Source/WebCore/rendering/RenderGeometryMap.cpp(142) : WebCore::FloatQuad WebCore::RenderGeometryMap::mapToContainer(const WebCore::FloatRect&, const WebCore::RenderLayerModelObject*) const
1   0x7f6765ecc2b2 WebCore::RenderGeometryMap::mapToContainer(WebCore::FloatRect const&, WebCore::RenderLayerModelObject const*) const
2   0x7f6765edd949 WebCore::RenderGeometryMap::absoluteRect(WebCore::FloatRect const&) const
3   0x7f6765f0edc5 WebCore::RenderLayerCompositor::addToOverlapMap(WebCore::RenderLayerCompositor::OverlapMap&, WebCore::RenderLayer*, WebCore::IntRect&, bool&)
4   0x7f6765f0f913 WebCore::RenderLayerCompositor::computeCompositingRequirements(WebCore::RenderLayer*, WebCore::RenderLayer*, WebCore::RenderLayerCompositor::OverlapMap*, WebCore::CompositingState&, bool&, bool&)
5   0x7f6765f0f7f1 WebCore::RenderLayerCompositor::computeCompositingRequirements(WebCore::RenderLayer*, WebCore::RenderLayer*, WebCore::RenderLayerCompositor::OverlapMap*, WebCore::CompositingState&, bool&, bool&)
6   0x7f6765f0f7f1 WebCore::RenderLayerCompositor::computeCompositingRequirements(WebCore::RenderLayer*, WebCore::RenderLayer*, WebCore::RenderLayerCompositor::OverlapMap*, WebCore::CompositingState&, bool&, bool&)
7   0x7f6765f0dc97 WebCore::RenderLayerCompositor::updateCompositingLayers(WebCore::CompositingUpdateType, WebCore::RenderLayer*)
8   0x7f6765bd83e1 WebCore::FrameView::updateFixedElementsAfterScrolling()
9   0x7f6765bd806d WebCore::FrameView::setFixedVisibleContentRect(WebCore::IntRect const&)
10  0x7f6769d14863 WebKit::WebPage::setFixedVisibleContentRect(WebCore::IntRect const&)

[Mikhail Pozdnyakov]

Created attachment 182754 [details]
patch

[Alexey Proskuryakov]

What is the reason why an automated test cannot be made? Manual tests are nearly useless.

How does this bug relate to bug 101609, bug 88128 and bug 89466?

[Mikhail Pozdnyakov]

(In reply to comment #2)
> What is the reason why an automated test cannot be made? Manual tests are nearly useless.

The EFL WTR is not using delegated scrolling so that the crash is not reproducible there. It only shows up in MiniBrowser.

> 
> How does this bug relate to bug 101609, bug 88128 and bug 89466?

Those are other issues even though assertions are the same, the issue that is solved here is delegated scrolling specific.

[Kenneth Rohde Christiansen]

(In reply to comment #3)
> (In reply to comment #2)
> > What is the reason why an automated test cannot be made? Manual tests are nearly useless.
> 
> The EFL WTR is not using delegated scrolling so that the crash is not reproducible there. It only shows up in MiniBrowser.

You can make it do so per test folder; just ask Thiago Santos how.

[Mikhail Pozdnyakov]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > What is the reason why an automated test cannot be made? Manual tests are nearly useless.
> > 
> > The EFL WTR is not using delegated scrolling so that the crash is not reproducible there. It only shows up in MiniBrowser.
> 
> You can make it do so per test folder; just ask Thiago Santos how.

yes I've tried it off course :) but than all css/sticky tests will fail because of raise condition caused by delegated scrolling..

[Mikhail Pozdnyakov]

Created attachment 183464 [details]
patch v2

Use layout tests rather than manual.

[Kenneth Rohde Christiansen]

Comment on attachment 183464 [details]
patch v2

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

LGTM

> LayoutTests/platform/efl-wk2/TestExpectations:413
> +# Most probably failures are result of delay in scrolling caused by 'delegated scrolling' usage.

shouldnt these be tracked in a bug?

[Kenneth Rohde Christiansen]

Comment on attachment 183464 [details]
patch v2

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

> LayoutTests/ChangeLog:4
> +        Avoid copying of ViewportArguments in computeViewportAttributes function
> +        https://bugs.webkit.org/show_bug.cgi?id=102354

what a bit ... wrong bug title!

[Mikhail Pozdnyakov]

Comment on attachment 183464 [details]
patch v2

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

>> LayoutTests/ChangeLog:4
>> +        https://bugs.webkit.org/show_bug.cgi?id=102354
> 
> what a bit ... wrong bug title!

Ah.. bash history let me down

>> LayoutTests/platform/efl-wk2/TestExpectations:413
>> +# Most probably failures are result of delay in scrolling caused by 'delegated scrolling' usage.
> 
> shouldnt these be tracked in a bug?

bug#107286 is created to track them.

[Mikhail Pozdnyakov]

Created attachment 183469 [details]
patch v3

Fixed change log.

[Simon Fraser (smfr)]

I think the real fix for this is to not make sticky composited when not using a ScrollingCoordinator.

[Mikhail Pozdnyakov]

(In reply to comment #11)
> I think the real fix for this is to not make sticky composited when not using a ScrollingCoordinator.

In delegated scrolling ScrollingCoordinator is not used at all. Cannot agree here.

[Alexey Proskuryakov]

Comment on attachment 183469 [details]
patch v3

Can we come to an agreement with respect to Simon's comment please? You said that you disagreed, but I think that you said the same thing actually.

[Kenneth Rohde Christiansen]

(In reply to comment #11)
> I think the real fix for this is to not make sticky composited when not using a ScrollingCoordinator.

EFL doesn't use the scrolling coordinator but uses composition for fixed position elements as well as sticky ones. This fits well into our scrolling and rendering architecture.

[Alexey Proskuryakov]

Comment on attachment 183469 [details]
patch v3

Given that Simon didn't override r+, I think that this answers the concern, at least for now.

[WebKit Review Bot]

Comment on attachment 183469 [details]
patch v3

Clearing flags on attachment: 183469

Committed r140262: <http://trac.webkit.org/changeset/140262>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Thiago Marcos P. Santos]

We are having lots of flaky tests after this patch landed.

[Mikhail Pozdnyakov]

(In reply to comment #18)
> We are having lots of flaky tests after this patch landed.

thanks for heads up. I'll take a look.

[Chris Dumez]

Major flakiness (~900 tests) started to occur on BOTH of our WK2 build bots after this patch landed. Can we roll out?

[Chris Dumez]

Comment on attachment 183469 [details]
patch v3

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

> Tools/WebKitTestRunner/TestInvocation.cpp:196
> +    if (!useFixedLayout)

Why are we returning early here? This seems like this could be the cause of the flakiness since the fixed layout would not get disabled after being enabled, right?

[Dominik Röttsches (drott)]

Comment on attachment 183469 [details]
patch v3

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

>> Tools/WebKitTestRunner/TestInvocation.cpp:196
>> +    if (!useFixedLayout)
> 
> Why are we returning early here? This seems like this could be the cause of the flakiness since the fixed layout would not get disabled after being enabled, right?

Agree - the layout mode doesn't get reset for the tests that are not in device-adapt/* or sticky/*. So everything that runs in the same shard after tests in these folders is potentially flaky.

[Chris Dumez]

(In reply to comment #22)
> (From update of attachment 183469 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183469&action=review
> 
> >> Tools/WebKitTestRunner/TestInvocation.cpp:196
> >> +    if (!useFixedLayout)
> > 
> > Why are we returning early here? This seems like this could be the cause of the flakiness since the fixed layout would not get disabled after being enabled, right?
> 
> Agree - the layout mode doesn't get reset for the tests that are not in device-adapt/* or sticky/*. So everything that runs in the same shard after tests in these folders is potentially flaky.

I'm going to confirm and fix this via Bug 107454.

[Mikhail Pozdnyakov]

(In reply to comment #23)
> (In reply to comment #22)
> > (From update of attachment 183469 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=183469&action=review
> > 
> > >> Tools/WebKitTestRunner/TestInvocation.cpp:196
> > >> +    if (!useFixedLayout)
> > > 
> > > Why are we returning early here? This seems like this could be the cause of the flakiness since the fixed layout would not get disabled after being enabled, right?
> > 
> > Agree - the layout mode doesn't get reset for the tests that are not in device-adapt/* or sticky/*. So everything that runs in the same shard after tests in these folders is potentially flaky.
> 
> I'm going to confirm and fix this via Bug 107454.

Yeah, this idea also came to my mind.. The problem for me however is that I cannot get rid of failures locally even if I completely revert the patch. Another thing is that I don't have flakiness: tests just fail. Maybe it is my environment problem :/

[Chris Dumez]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > (From update of attachment 183469 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=183469&action=review
> > > 
> > > >> Tools/WebKitTestRunner/TestInvocation.cpp:196
> > > >> +    if (!useFixedLayout)
> > > > 
> > > > Why are we returning early here? This seems like this could be the cause of the flakiness since the fixed layout would not get disabled after being enabled, right?
> > > 
> > > Agree - the layout mode doesn't get reset for the tests that are not in device-adapt/* or sticky/*. So everything that runs in the same shard after tests in these folders is potentially flaky.
> > 
> > I'm going to confirm and fix this via Bug 107454.
> 
> Yeah, this idea also came to my mind.. The problem for me however is that I cannot get rid of failures locally even if I completely revert the patch. Another thing is that I don't have flakiness: tests just fail. Maybe it is my environment problem :/

Patch at Bug 107454 fixes flakiness for me.