3d CSS cube demo is broken again

VERIFIED FIXED in Firefox 66

Status

()

defect
P2
normal
VERIFIED FIXED
7 months ago
2 months ago

People

(Reporter: mayankleoboy1, Assigned: emilio)

Tracking

(Blocks 1 bug, {nightly-community, regression})

Trunk
mozilla67
Unspecified
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox62 unaffected, firefox63 unaffected, firefox64 disabled, firefox65 disabled, firefox66 verified, firefox67 verified)

Details

()

Attachments

(6 attachments, 3 obsolete attachments)

Reporter

Description

7 months ago
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
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.
Flags: needinfo?(kats)
That fixed the problem.
Assignee: nobody → kats
Flags: needinfo?(kats)
Actually I'll leave this unassigned for the moment. If somebody else wants to look at this in the next few days feel free.
Assignee: kats → nobody
Assignee

Comment 7

6 months ago
I can take a look.
Assignee: nobody → emilio
Assignee

Comment 8

6 months ago
Posted file reduced.html (obsolete) —
Assignee

Updated

6 months ago
Flags: needinfo?(emilio)
Assignee

Comment 9

6 months ago
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 :)
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
Assignee

Comment 11

6 months ago
Posted file mScrollData.Dump() (obsolete) —
Sure, does this help?
Attachment #9025046 - Attachment is obsolete: true
Flags: needinfo?(kats)
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
Flags: needinfo?(kats)
Assignee

Comment 13

6 months ago
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 :)
Assignee

Comment 14

6 months ago
According to Miko the assertion might be gone with bug 1504233.
Depends on: 1504233
Assignee

Comment 15

6 months ago
Attachment #9026021 - Attachment is obsolete: true
Assignee

Comment 16

6 months ago
bug 1504233 did not fix it, so I'll take another look.
No longer depends on: 1504233
Assignee

Comment 17

6 months ago
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.
Attachment #9025889 - Attachment description: Bug 1498639 - Add a reftest. → Bug 1498639 - Add a reftest (failing in WR).

Comment 18

6 months ago
Pushed by ealvarez@mozilla.com:
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
Assignee

Updated

6 months ago
Keywords: leave-open

Comment 19

6 months ago
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7fa4a5ba4671
Add a reftest (failing in WR). r=kats

Comment 20

6 months ago
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/3b3c190f79e5
Mark the reftest random-if since it passes sometimes.
Assignee

Updated

6 months ago
Depends on: 1510780
Emilio, do you know what's left to do here?
Assignee

Comment 24

5 months ago
Yes, need to rebase and land the patch basically.
Assignee

Updated

4 months ago
Depends on: 1520664
Attachment #9029705 - Attachment is obsolete: true
Assignee

Updated

4 months ago
Blocks: 1523436

Comment 26

4 months ago
Pushed by ealvarez@mozilla.com:
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
Assignee

Updated

4 months ago
Flags: needinfo?(emilio)
Keywords: leave-open

Comment 27

4 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Assignee

Comment 28

4 months ago

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

Attachment #9039577 - Flags: approval-mozilla-beta?

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

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.

Attachment #9039577 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.