Bug 106890 - Delegated scrolling: Assertion on attempt to show a CSS sticky element
Summary: Delegated scrolling: Assertion on attempt to show a CSS sticky element
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mikhail Pozdnyakov
URL:
Keywords:
Depends on: 107454
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-15 04:47 PST by Mikhail Pozdnyakov
Modified: 2013-01-21 10:36 PST (History)
9 users (show)

See Also:


Attachments
patch (4.12 KB, patch)
2013-01-15 06:36 PST, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v2 (7.64 KB, patch)
2013-01-18 08:22 PST, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch v3 (7.63 KB, patch)
2013-01-18 08:48 PST, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Pozdnyakov 2013-01-15 04:47:13 PST
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&)
Comment 1 Mikhail Pozdnyakov 2013-01-15 06:36:12 PST
Created attachment 182754 [details]
patch
Comment 2 Alexey Proskuryakov 2013-01-15 09:40:57 PST
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?
Comment 3 Mikhail Pozdnyakov 2013-01-16 01:15:54 PST
(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.
Comment 4 Kenneth Rohde Christiansen 2013-01-18 04:17:29 PST
(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.
Comment 5 Mikhail Pozdnyakov 2013-01-18 05:49:10 PST
(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..
Comment 6 Mikhail Pozdnyakov 2013-01-18 08:22:40 PST
Created attachment 183464 [details]
patch v2

Use layout tests rather than manual.
Comment 7 Kenneth Rohde Christiansen 2013-01-18 08:34:54 PST
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?
Comment 8 Kenneth Rohde Christiansen 2013-01-18 08:35:17 PST
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!
Comment 9 Mikhail Pozdnyakov 2013-01-18 08:44:44 PST
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.
Comment 10 Mikhail Pozdnyakov 2013-01-18 08:48:41 PST
Created attachment 183469 [details]
patch v3

Fixed change log.
Comment 11 Simon Fraser (smfr) 2013-01-18 08:57:25 PST
I think the real fix for this is to not make sticky composited when not using a ScrollingCoordinator.
Comment 12 Mikhail Pozdnyakov 2013-01-18 09:11:28 PST
(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.
Comment 13 Alexey Proskuryakov 2013-01-19 10:49:45 PST
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.
Comment 14 Kenneth Rohde Christiansen 2013-01-19 12:24:44 PST
(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.
Comment 15 Alexey Proskuryakov 2013-01-19 16:07:51 PST
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.
Comment 16 WebKit Review Bot 2013-01-19 16:12:57 PST
Comment on attachment 183469 [details]
patch v3

Clearing flags on attachment: 183469

Committed r140262: <http://trac.webkit.org/changeset/140262>
Comment 17 WebKit Review Bot 2013-01-19 16:13:02 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 Thiago Marcos P. Santos 2013-01-21 00:50:08 PST
We are having lots of flaky tests after this patch landed.
Comment 19 Mikhail Pozdnyakov 2013-01-21 01:08:09 PST
(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.
Comment 20 Chris Dumez 2013-01-21 07:50:54 PST
Major flakiness (~900 tests) started to occur on BOTH of our WK2 build bots after this patch landed. Can we roll out?
Comment 21 Chris Dumez 2013-01-21 07:57:08 PST
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?
Comment 22 Dominik Röttsches (drott) 2013-01-21 09:00:37 PST
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.
Comment 23 Chris Dumez 2013-01-21 09:03:39 PST
(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.
Comment 24 Mikhail Pozdnyakov 2013-01-21 10:20:44 PST
(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 :/
Comment 25 Chris Dumez 2013-01-21 10:36:32 PST
(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.