Closed Bug 1554502 Opened 5 months ago Closed 4 months ago

Webrender + Tree Style Tab: Visual artifact with text behind transparent div

Categories

(Core :: Graphics: WebRender, defect)

69 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- wontfix
firefox68 --- fixed
firefox69 --- fixed

People

(Reporter: valflaux+github, Assigned: kvark)

References

(Regression)

Details

(Keywords: regression)

Attachments

(6 files)

Attached file testcase.html

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

Steps to reproduce:

On Windows + Nvidia and Linux + Intel with nightly and Webrender enabled.

I can reproduce by following these steps:

Actual results:

The text of the "content" div is rendered over the "header" div.

Expected results:

The text from the "content" div should be rendered behind the "header" div. It should still be visible as the header is transparent (opacity: 0.9).

Attached image testcase.png
Attached image youtube.png

This also affects real websites like YouTube on video pages (https://www.youtube.com/watch?v=BCUwP2csmBM)

mozregression:

app_name: firefox
build_date: 2019-02-07 16:25:33.242000
build_type: inbound
build_url: https://queue.taskcluster.net/v1/task/CdAUon1lRhu-ruqgT837IQ/runs/0/artifacts/public%2Fbuild%2Ftarget.zip
changeset: 0258dc6318a821647ec61519590de275b6663ad6
pushlog_url: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0258dc6318a821647ec61519590de275b6663ad6&tochange=f526887aa3ae0087bbb9553627453eb9ecdec871
repo_name: autoland
repo_url: https://hg.mozilla.org/integration/autoland
task_id: CdAUon1lRhu-ruqgT837IQ

Differential Revision: https://phabricator.services.mozilla.com/D18917

Component: Untriaged → Graphics: WebRender
Product: Firefox → Core
Has Regression Range: --- → yes
Has STR: --- → yes
Keywords: regression
Regressed by: 1525740

I can reproduce the issue on Nightly69.0a1 Windows10.

Status: UNCONFIRMED → NEW
Ever confirmed: true

Setting flags based on the regressor's pushed changeset.

QA Whiteboard: [qa-regression-triage]

Thank you for the test cases!
I narrowed it down to our batching code by making the lookback size configurable. If we stop trying to merge with previous batches, the re-ordering stops occurring. The fact that it occurs tells me that our bounding rect is incorrect.

Assignee: nobody → dmalyshau
Status: NEW → ASSIGNED
Keywords: leave-open

We've had a constant of 10 hard-coded there since early days.
Turning it into a configurable number allows us to easier tune it and
debug related issues.

From a conversation with Dzmitry on irc, it sounded like the issue is that there were multiple display lists with cache_tiles=true. This was being caused by having multiple top-level content documents rendering to the same window.

If this is accurate, we should be able to use the 'primary' attribute that is set on <browser>'s in order to make a unique cache_tiles decision.

Here's a try run of a patch that implements this [1]. I tried to keep it as simple as possible so that this could be uplifted.

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

Hmm, a bit odd. I can still reproduce the issue with that test case with those patchers.

I added some logging and confirmed that only the main content area is setting cache_tiles=true. The tree-style tabs panel is setting cache_tiles=false.

Dzmitry, did I misunderstand the issue? And if not, would you be able to test out the patches from the try run or take a look at them?

Flags: needinfo?(dmalyshau)

Ryan, thank you for producing and testing the patch!
I observed the following:

  1. The main DL has 2 iframes, both of which contain a stacking context with "cache_tiles=true"
  2. Setting them to false leads to fixing the bug in this case

From that, and the fact we know that tile caching is only expected to be enabled on at most a single stacking context (today, at least), I concluded that (1) is triggering the issue. If it's still happening, I need to look once more into it, with your patch applied.

Flags: needinfo?(dmalyshau)

I figured out the problem, at last. Filed bug 1556053 separately for the cache flag changes.

Keywords: leave-open
QA Whiteboard: [qa-regression-triage]
Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05bbf91cf528
Track the reference frame of batched bounding boxes and enforce it in WR r=gw
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

Should this ride the trains or do we want it in 68?

Flags: needinfo?(dmalyshau)

I believe this is safe to uplift to 68, if there is still a room to do so. Both patches are needed.

Flags: needinfo?(dmalyshau)

Comment on attachment 9067880 [details]
Configurable lookback count

Beta/Release Uplift Approval Request

  • User impact if declined: Visual artifacts when WebRender is enabled on some page/extension combinations, including the Tree Style Tab
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Non-risky, since it just makes batching more conservative.
  • String changes made/needed:
Attachment #9067880 - Flags: approval-mozilla-beta?
Attachment #9069076 - Flags: approval-mozilla-beta?

Comment on attachment 9067880 [details]
Configurable lookback count

webrender fix for 68.0b9

Attachment #9067880 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9069076 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

I'll make a combined change that builds and works from these on Beta.

Flags: needinfo?(dmalyshau)

The automatic conflict merging got confused in this case because of the specific of other changes that came in before. Attaching here a combined patch that I resolved manually when cherry-picking.

Small try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a80f75fcec039b319d5ced675142a07fc841f2fd

Would you be able to uplift it to Beta? there is only a few character difference from the patch that failed :)

Flags: needinfo?(jcristau)
Flags: needinfo?(jcristau)
You need to log in before you can comment on or make changes to this bug.