1758975 - GIF without global color table allows seeing through Firefox window on Linux

Status: RESOLVED FIXED

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

Description

[Oscar Cunningham]

Attachment: Screenshot

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:97.0) Gecko/20100101 Firefox/97.0

Steps to reproduce:

Tested using Firefox 97.0.2 and Nightly 100.0a1, both on Linux
View this GIF: https://blog.darrien.dev/you-dont-know-gif/Sunflower_as_gif_89a-no-gct.gif
(GIF is from this blog post https://blog.darrien.dev/posts/you-dont-know-gif/, bug noticed on HackerNews https://news.ycombinator.com/item?id=30626502, people reported the same bug using both X and Wayland)

Actual results:

Where the GIF should appear instead is visible whatever is behind that part of the Firefox window, e.g. my desktop background.

Expected results:

Display the GIF as a solid black rectangle.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Widget: Gtk' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Testcase for posterity.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Mozregression says: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=404680ad41e4e7d08aa46081a70095486f14e15f&tochange=e704e3565db9260efaa854f2916ca023b717db3c

That's when WebRender was enabled by default for my hardware (confirmed that the good build had "Compositing: Basic", and the second one had WebRender enabled).

I wonder if we think the image is opaque (and thus avoid putting it in the alpha batches) but it's not, causing this funny effect.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Yeah, confirmed that this makes the artifact not happen (we just render a fully-transparent image). Note that Chrome and Firefox pre-webrender draw black:

diff --git a/gfx/webrender_bindings/src/bindings.rs b/gfx/webrender_bindings/src/bindings.rs
index ffcbe988c2ed3..40fc2b6881a12 100644
--- a/gfx/webrender_bindings/src/bindings.rs
+++ b/gfx/webrender_bindings/src/bindings.rs
@@ -338,7 +338,7 @@ impl<'a> From<&'a WrImageDescriptor> for ImageDescriptor {
         let mut flags = ImageDescriptorFlags::empty();
 
         if desc.opacity == OpacityType::Opaque {
-            flags |= ImageDescriptorFlags::IS_OPAQUE;
+            // flags |= ImageDescriptorFlags::IS_OPAQUE;
         }
 
         ImageDescriptor {

So probably an imagelib bug?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Is it this TODO?

Comment 6

[Darkspirit]

Gnome Xwayland, Debian Testing, Intel
good: black
bad: transparent
MOZ_DISABLE_CONTENT_SANDBOX=1 mozregression --good 2018-01-01 --bad 2019-01-01 --pref gfx.webrender.all:true layers.acceleration.force-enabled:true gfx.webrender.enabled:true gfx.webrendest.enabled:true -a https://bugzilla.mozilla.org/attachment.cgi?id=9267224

4:27.53 INFO: Last good revision: 5165e750ffabb930deb846410ab62dcc6d1f9e52 (2018-09-16)
4:27.53 INFO: First bad revision: 87a95e1b7ec691bef7b938e722fe1b01cce68664 (2018-09-17)
4:27.53 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5165e750ffabb930deb846410ab62dcc6d1f9e52&tochange=87a95e1b7ec691bef7b938e722fe1b01cce68664

Comment 7

[Darkspirit]

14c5eab0c6b359b7904a8d9807c8282352707ba0 Martin Stransky — Bug 1489097 - [Linux/Gtk] Enable default ARGB visual for toplevel windows on GNOME, r=jhorak

because it's reproducible with last good and mozilla.widget.use-argb-visuals:true
MOZ_DISABLE_CONTENT_SANDBOX=1 mozregression --launch 2018-09-16 --pref gfx.webrender.all:true layers.acceleration.force-enabled:true gfx.webrender.enabled:true gfx.webrendest.enabled:true mozilla.widget.use-argb-visuals:true -a https://bugzilla.mozilla.org/attachment.cgi?id=9267224

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

:stransky, since you are the author of the regressor, bug 1489097, could you take a look?
For more information, please visit auto_nag documentation.

Comment 9

[Darkspirit]

going back with mozilla.widget.use-argb-visuals:true:
2017-10-01 = grey (browser background)
2017-12-28 = transparent

MOZ_DISABLE_CONTENT_SANDBOX=1 mozregression --good 2017-10-01 --bad 2017-12-28 --pref gfx.webrender.all:true layers.acceleration.force-enabled:true gfx.webrender.enabled:true gfx.webrendest.enabled:true mozilla.widget.use-argb-visuals:true -a https://bugzilla.mozilla.org/attachment.cgi?id=9267224

4:00.74 INFO: Last good revision: a77c628829b389ed6ac608eadc88b09eb5115bef (2017-11-17)
4:00.74 INFO: First bad revision: 3a0aac55195fd50ca4b1b41be450bfbaaafc191c (2017-11-18)
4:00.74 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a77c628829b389ed6ac608eadc88b09eb5115bef&tochange=3a0aac55195fd50ca4b1b41be450bfbaaafc191c

contains WR image and gtk changes

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Pretty sure bug 1489097 only changed behavior here because it only effectively changes what happens when we don't draw to the frame buffer. The bug is in WR.

Comment 11

[Bob Hood [:bhood]]

Glenn, is this already on your radar? Seems fairly severe.

Comment 12

[Andrew Osmond [:aosmond] (he/him)]

We've seen this happen when an image is marked as opaque / BGRX, but actually has transparency. This is probably imagelib misreporting. I see my major refactoring for integrating WR with ImageLib from 2017 is in the push log.

Comment 13

[Timothy Nikkel (:tnikkel)]

Chrome and Safari think the image is opaque, and our gif decoder does too, so it tags it that way. But we write all 0 (transparent black) pixels. Is imagelib supposed to write FF for alpha in this situation?

Comment 14

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1758975 - Ensure GIF color tables are opaque by default.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0938f086f24
Ensure GIF color tables are opaque by default. r=tnikkel

Comment 16

[Andreea Pavel [:apavel]]

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

Comment 17

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9267973 [details]
Bug 1758975 - Ensure GIF color tables are opaque by default.

Beta/Release Uplift Approval Request

• User impact if declined: Carefully crafted GIFs may cause us to push transparent data marked as opaque to the compositor causing rendering glitches
• Is this code covered by automated tests?: Yes
• 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): All we do is clear the global/local color tables. Most GIFs will populate these tables with their actual values and there is no change. The patch is very simple and easily understood.
• String changes made/needed:

Comment 18

[Donal Meehan [:dmeehan]]

Comment on attachment 9267973 [details]
Bug 1758975 - Ensure GIF color tables are opaque by default.

Approved for 99.0b7. Thanks.

Comment 19

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d13e60572d13

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Seems like we should probably take this on ESR also? The patch grafts cleanly there as well.

Comment 21

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9267973 [details]
Bug 1758975 - Ensure GIF color tables are opaque by default.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Carefully crafted GIFs may cause us to push transparent data marked as opaque to the compositor causing rendering glitches
• Fix Landed on Version: 100
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): All we do is clear the global/local color tables. Most GIFs will populate these tables with their actual values and there is no change. The patch is very simple and easily understood.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9267973 [details]
Bug 1758975 - Ensure GIF color tables are opaque by default.

Approved for 91.8esr.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/00a6ebc0c7e0

Comment 24

[Daniel Bodea [:danibodea]]

I have managed to reproduce this issue on Ubuntu 22 (wayland) and Nightly v100.0a1 from 2022-03-17. The GIF shows what's behind the browser window, along with some weird light stuttering.

On the latest version of Nightly v100.0a1 from 2022-03-24, it no longer shows what's behind the browser window and neither does it show any stuttering, but the rectangle appears as white, not black, as mentioned in comment 0.

Is this an acceptable result to verify this fix?

Comment 25

[Andrew Osmond [:aosmond] (he/him)]

I think the colour doesn't matter too much given it is a corrupted GIF situation, just as long as we do something sensible.

Comment 26

[Andrew Osmond [:aosmond] (he/him)]

(To be clear, I expect white.)

Comment 27

[Daniel Bodea [:danibodea]]

This fix was also verified in Beta v99.0b7. There is no build for 91.8esr, yet.
Not even one downloadable "target" file from treeherder:
https://treeherder.mozilla.org/jobs?repo=mozilla-esr91&revision=160f7156fea4465a99bbd3ec66d50bcc9cec4a91&selectedTaskRun=HqlMlbuFSn2H-51-H392gQ.0

Verification for ESR channel is postponed; leaving qe-verify+ flag.

Comment 29

[Daniel Bodea [:danibodea]]

This fix was also verified on ESR v91.8.0esr on Ubuntu 21.10.