1524385 - Set the current clip chain on the stacking context item

Status: RESOLVED FIXED

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

Description

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

Attachment: hack

The backstory here as I know it starts with bug 1514383 comment 8. In the testcase there is an element inside a stacking context and the clip chain for that element has a parent link to a clip chain outside the stacking context. This happens on the gecko side in ClipManager.cpp for historical reasons but is not in line with Gecko semantics.

Really what we want to be doing is dropping all those parent links in the clip chains, and instead ensuring that any enclosing stacking context has the clip chain that is relevant for that stacking context. This effectively means that every stacking context will have a clip chain. However doing so will trigger the isolation code path in WR which will produce perf regressions and possibly has correctness problems.

I'm attaching a patch Emilio tried yesterday where he saw correctness problems with picture caching enabled (about:home was rendering with a vertical mirror effect, among other things), although he said they mostly went away with picture caching disabled.

Comment 1

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

(Note that the hack patch applies without the patches from bug 1523776 - those patches are in the process of getting backed out from central)

Comment 2

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

With this patch bug 1523080 also seems to render correctly. I was able to reproduce severe artifacts with picture caching enabled that went away with picture caching disabled. I also did a try push with this patch and picture caching disabled, to see what else might need fixing up on the gecko side. That's at https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=c3aa2d67cb3513ed8651a81251bd7184414285f2

Comment 3

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

Also, some possibly relevant context/discussion: https://mozilla.logbot.info/gfx/20190131#c15907722

Comment 4

[Glenn Watson [:gw]]

Attachment: Bug 1524385 - Set the current clip chain on the stacking context item.

Comment 5

[Glenn Watson [:gw]]

I added a WIP patch to this bug of roughly what I expect the WR changes to be.

In normal browsing, I haven't noticed any problems. I think this also fixes the bug in https://bugzilla.mozilla.org/show_bug.cgi?id=1523080, but I didn't look really closely at it - I might be missing something?

I did a try run [1] with the Gecko patch above + the WR changes. It does fail a few tests (the same tests on each platform, so there's not as many failures as it looks).

I will start looking at the reftest failures in the morning. I figured I'd post the WIP patch here just in case you have time to look at this overnight (I'm not sure if the failures are an issue with the input DL from Gecko, or a WR bug in the patch above yet).

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9bc0c86812cebfd350dc75204ddf983b80518ce

Comment 6

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

Thanks! I'm looking into the failures.

So far, the mask-layer* ones are a problem on the gecko side. The nsDisplayOwnLayer::CreateWebRenderCommands should always create a SC instead of just for scrollthumb layers. I have a local patch to fix that.

The 906199-1.html failure I think is on the WR side. The SC in question has both preserve-3d and a clip, so maybe the BlitReason bitmask is dropping one of the flags somewhere, or maybe there's a semantics mismatch between WR and Gecko.

Looking into the others.

Comment 7

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

Attachment: Fix nsDisplayOwnLayer

This is the local patch I mentioned in the previous comment. It also fixes the fixed-pos-scrolled-clip-* failures. The only other UNEXPECTED-FAIL is 1081185-1.html which appears to be a change in fuzz numbers so it's not a real failure.

Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=87715ef08893ea6f40e02d7b56ff9956b9c0d32d

Comment 8

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

Argh, didn't mean to assign this to myself.

Comment 9

[Glenn Watson [:gw]]

Fixed the bug related to 906199-1.html and rebased the patch.

Kicked off a pending try with the rebased patch + the Gecko fix from kats above:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d157a2948cd9c15440a18d2865c06bd2fd9679a

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2853480ed90d
Set the current clip chain on the stacking context item. r=kvark

Comment 11

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/2853480ed90d

Comment 12

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

Comment on attachment 9040542 [details] [diff] [review]
hack

Obsoleting this WIP patch

Comment 13

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

Making this a P3 since it is a fix for bug 1523776 which is a P3. So this probably warrants an uplift to 66.

Comment 14

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

Would you like to request uplift to beta?

Comment 15

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

And if so would that also need the work in bug 1523776 and bug 1525740?

Comment 16

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

I'm hesitant now since it's pretty late in beta and the patch seems pretty big to me. I'm leaning towards not uplifting but Glenn probably has a better sense of how risky the patch is. And yes, if we do upload this we'd probably want to upload bug 1525740 and bug 1523776 as well.

Comment 17

[Glenn Watson [:gw]]

I agree it does introduce some risk, I'm also not sure there's a massive benefit to having this in beta. I'd lean towards letting it settle in nightly and go into the next beta, I think. Jeff may have an opinion also.

Comment 18

[Jeff Muizelaar [:jrmuizel]]

I'm fine with that strategy

Comment 19

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

Great thanks! That's what I like to hear about this time in the beta cycle :)