1498639 - 3d CSS cube demo is broken again

Status: VERIFIED FIXED

(Core :: Graphics: WebRender, defect, P2)

Description

[Mayank Bansal]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0

Steps to reproduce:

enable WR
go to https://3dtransforms.desandro.com/cube
scroll down till the actual cubes


Actual results:

cubes sort of rotate in place


Expected results:

not so

Comment 1

[Darkspirit]

mozregression --good 2018-10-05 --bad 2018-10-12 --pref gfx.webrender.all:true -a https://3dtransforms.desandro.com/cube
> 8:09.05 INFO: Last good revision: 4f55976a9e9115c9f41075843bc48955684364d9
> 8:09.05 INFO: First bad revision: 2cee53dd577363866c3cc6ed7baf679a9936abbf
> 8:09.05 INFO: Pushlog:
> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4f55976a9e9115c9f41075843bc48955684364d9&tochange=2cee53dd577363866c3cc6ed7baf679a9936abbf

> 2cee53dd5773	Jeff Muizelaar — Bug 1496670. Update layout/reftests/async-scrolling/reftest.list for #3173
> bc87d3ddef61	Sotaro Ikeda — Bug 1461239 - Use InvalidateRenderedFrame() when necessary r=nical
> 335625773207	Jeff Muizelaar — Bug 1495902. Create a shared Shaders object for all windows to use. r=jrmuizel
> 7801a4fb37db	Jeff Muizelaar — Bug 1496670. Update webrender to 923ee495bd9b0fda8a4a94c5a6cf42e2f0548731

https://github.com/servo/webrender/compare/3c3f9a4e919b81639f078d7bd101012de61b9396...923ee495bd9b0fda8a4a94c5a6cf42e2f0548731

https://github.com/servo/webrender/pull/3167#issuecomment-427201295
mozregression --repo try --launch c94d12d7017379f652427b658247b959a57fa59c --pref gfx.webrender.all:true -a https://3dtransforms.desandro.com/cube
good

https://github.com/servo/webrender/pull/3158
no try

https://github.com/servo/webrender/pull/3170
no try

https://github.com/servo/webrender/pull/3142
irrelevant

https://github.com/servo/webrender/pull/3173#issuecomment-427453143
mozregression --repo try --launch b5b03f9334ff26c20354a0f967c04a232c6fd30c --pref gfx.webrender.all:true -a https://3dtransforms.desandro.com/cube
bad

https://github.com/servo/webrender/pull/3172
no try

https://github.com/servo/webrender/pull/3174#issuecomment-427700798
mozregression --repo try --launch 7ce002f81fad9f2c150c54fa9a90659f72a49eff --pref gfx.webrender.all:true -a https://3dtransforms.desandro.com/cube
bad

https://github.com/servo/webrender/pull/3176
no try

https://github.com/servo/webrender/pull/3177
irrelevant

Caused by servo/webrender#3173 or servo/webrender#3174.

Comment 2

[Darkspirit]

Attachment: 2018-10-12_19-53-16.mp4

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Here's a try push with #3173 backed out to check: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=0398eb92bf59d330c8701a5c1e7689ecd61be9a5

Comment 4

[Darkspirit]

That fixed the problem.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Actually I'll leave this unassigned for the moment. If somebody else wants to look at this in the next few days feel free.

Comment 6

[Darkspirit]

Attachment: superhuman.mp4

https://superhuman.com/

Comment 7

[Emilio Cobos Álvarez (:emilio)]

I can take a look.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: reduced.html

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1498639 - Add a reftest (failing in WR).

I still haven't figured out the root cause for this bug.

The code looks right to me, and I figured that adding a reftest would help me
figure out.

This reftest consistently crashes with WR enabled hitting this assertion:

  https://searchfox.org/mozilla-central/rev/e67e6f648b1d0977f7610154da731ddf0e4f31f0/gfx/layers/apz/src/APZCTreeManager.cpp#1122

Non-WR doesn't hit this though... In any case the fact that this reftest works
just fine without e10s / with APZ disabled hints me that this may be some kind
of bug in the WR / APZ integration.

Still worth getting this reviewed. I'll keep looking at this but hints on where
to keep looking at, put uploading this test should be helpful :)

Comment 10

[Kartikaya Gupta (email:[e-mail])]

I don't know exactly what the problem is but at a high level the structure of WebRenderScrollData that we're sending from content to APZ is violating APZ assumptions. So we will need to adjust what happens in the UpdateScrollData calls on the display items. If you can get a dump from mScrollData.Dump around [1] that would be helpful as I can take a look and hopefully identify what is wrong.

[1] https://searchfox.org/mozilla-central/rev/5117a4c4e29fcf80a627fecf899a62f117368abf/gfx/layers/wr/WebRenderLayerManager.cpp#336

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: mScrollData.Dump()

Sure, does this help?

Comment 12

[Kartikaya Gupta (email:[e-mail])]

That helps somewhat. In the second and third LayerScrollData items in that dump (which are siblings and children of the first one), the scroll metadata entries are the same (for scrollId 3 and 4), but the ancestor transforms are different. So when APZ gets that data it sees that a given scrollframe has two different ancestor transforms, which is the thing violating the assumption. The next step is to get the display list dump to see why the metadata is turning out the way it is. I'm happy to take this bug (at least to track down the assertion problem) if you want to offload it.

Otherwise the comment at [1] should give you some context on what this is all about. The code at [2] is what is populating these LayerScrollData items, and presumably it's somehow getting a different deferred transform for the two LayerScrollData items in question, even though I think they should be the same. Maybe the deferred transform propagation stuff at [3] is buggy somehow.

[1] https://searchfox.org/mozilla-central/rev/b03a62c3c82316e733a3b09622c1cb7e59f64cc3/gfx/layers/wr/StackingContextHelper.h#89
[2] https://searchfox.org/mozilla-central/rev/b03a62c3c82316e733a3b09622c1cb7e59f64cc3/gfx/layers/wr/WebRenderCommandBuilder.cpp#1579
[3] https://searchfox.org/mozilla-central/rev/b03a62c3c82316e733a3b09622c1cb7e59f64cc3/gfx/layers/wr/StackingContextHelper.cpp#89

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Np, I can look into it.

It's a bit annoying because I had to update the kernel and mesa to get rr to work with WR in Gecko, and when I got it working I happen to hit https://github.com/mozilla/rr/issues/2288 a lot, so it's taking a bit to get a working trace :)

Comment 14

[Emilio Cobos Álvarez (:emilio)]

According to Miko the assertion might be gone with bug 1504233.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Attachment: debug log (for the record)

Comment 16

[Emilio Cobos Álvarez (:emilio)]

bug 1504233 did not fix it, so I'll take another look.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1498639 - Force the creation of APZ scroll data for the frames with perspective property, not just their children. r=kats

This fixes the assertion, though not the test-case, but now I can debug the
test-case and also land it as failing for now.

What was happening is that we had two ASRs (for the canvas and for the <body>),
and we were creating scroll data for the CompositorHitTestInfo of the <body>
(which obviously didn't have any ancestor transform) and for the
nsDisplayTransform in the backface for which HasPerspective returns true for,
but which already had an ancestor transform for the frame for which
ChildrenHavePerspective returns true.

It may be less risky to force it for both, but this doesn't make anything fail.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f53223742744
Force the creation of APZ scroll data for the frames with perspective property, not just their children. r=kats

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7fa4a5ba4671
Add a reftest (failing in WR). r=kats

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b3c190f79e5
Mark the reftest random-if since it passes sometimes.

Comment 21

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/f53223742744
https://hg.mozilla.org/mozilla-central/rev/7fa4a5ba4671
https://hg.mozilla.org/mozilla-central/rev/3b3c190f79e5

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1498639 - Make WebRender know how many scroll frames a perspective item has been moved into.

Comment 23

[Jeff Muizelaar [:jrmuizel]]

Emilio, do you know what's left to do here?

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Yes, need to rebase and land the patch basically.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1498639 - Give WR the id of the scroll frame perspective scrolls relative to, and compute the right transform based on that.

I think this is as clean as it can get.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7a369cd3b4a
Give WR the id of the scroll frame perspective scrolls relative to, and compute the right transform based on that. r=kats,kvark

Comment 27

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d7a369cd3b4a

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9039577 [details]
Bug 1498639 - Give WR the id of the scroll frame perspective scrolls relative to, and compute the right transform based on that.

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1496670

User impact if declined

Wrong scrolling.

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

Yes

If yes, steps to reproduce

See STR

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

Well understood and not too risky fix that makes us match the non-WR path.

String changes made/needed

none

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

WR is still off by default in beta but this is good to uplift b/c of experiments running in beta.

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9039577 [details]
Bug 1498639 - Give WR the id of the scroll frame perspective scrolls relative to, and compute the right transform based on that.

OK to uplift for beta 5.

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/551c4945683f

Comment 32

[Stefan [:StefanG_QA]]

I was able to reproduce the mentioned behavior on Windows 10 using the old Beta 66.0b4 (20190131191227)

The issue is verified as fixed on the latest Beta 67.0b7 (20190331141835) and Fx release 66.0.2 (20190326175229). With "gfx.webrender.all" set to "true" the provided page at comment 0 and all elements are properly scrollable.