1222171 - "GraphicsCriticalError: |[0][GFX1]: Unknown cairo format 3" on startup on Linux machine with VNC "screen"

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Markus Stange [:mstange]]

I'm seeing this startup crash when running a build on Jeff's "hawkman" machine in the Toronto office (which I use for rr debugging). The DISPLAY env variable is set to something that I get by running "vnc4server".

Stack:
> #0  0x00007fffe45d3ff6 in mozilla::gfx::CriticalLogger::OutputMessage(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int, bool) (aString="[GFX1]: Unknown cairo format 3", aLevel=1, aNoNewline=false) at /home/mstange/code/mozilla-central/gfx/2d/Factory.cpp:900
> #1  0x00007fffe4612329 in mozilla::gfx::Log<1, mozilla::gfx::CriticalLogger>::WriteLog(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (this=0x7fffd2e966b0, aString="[GFX1]: Unknown cairo format 3") at /home/mstange/code/mozilla-central/gfx/2d/Logging.h:502
> #2  0x00007fffe460a902 in mozilla::gfx::Log<1, mozilla::gfx::CriticalLogger>::Flush() (this=0x7fffd2e966b0) at /home/mstange/code/mozilla-central/gfx/2d/Logging.h:276
> #3  0x00007fffe45ff9fe in mozilla::gfx::Log<1, mozilla::gfx::CriticalLogger>::~Log() (this=0x7fffd2e966b0, __in_chrg=<optimized out>)
>     at /home/mstange/code/mozilla-central/gfx/2d/Logging.h:268
> #4  0x00007fffe45bfef8 in mozilla::gfx::CairoFormatToGfxFormat(cairo_format_t) (format=CAIRO_FORMAT_A1) at /home/mstange/code/mozilla-central/gfx/2d/HelpersCairo.h:246
> #5  0x00007fffe45c1c98 in mozilla::gfx::GfxFormatForCairoSurface(_cairo_surface*) (surface=0x7fffc28db200) at /home/mstange/code/mozilla-central/gfx/2d/DrawTargetCairo.cpp:668
> #6  0x00007fffe4863122 in gfxASurface::GetSurfaceFormat() const (this=0x7fffc28ff920) at /home/mstange/code/mozilla-central/gfx/thebes/gfxASurface.cpp:715
> #7  0x00007fffe4883d8c in gfxPlatform::CreateDrawTargetForSurface(gfxASurface*, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&) (this=0x7fffd49cea40, aSurface=0x7fffc28ff920, aSize=...) at /home/mstange/code/mozilla-central/gfx/thebes/gfxPlatform.cpp:786
> #8  0x00007fffe46f4aec in mozilla::layers::X11DataTextureSourceBasic::Update(mozilla::gfx::DataSourceSurface*, nsIntRegion*, mozilla::gfx::IntPointTyped<mozilla::gfx::UnknownUnits>*) (this=0x7fffc2b07ee0, aSurface=0x7fffc28ff8d0, aDestRegion=0x0, aSrcOffset=0x0) at /home/mstange/code/mozilla-central/gfx/layers/basic/X11BasicCompositor.cpp:47
> #9  0x00007fffe47c2f44 in mozilla::layers::BufferTextureHost::Upload(nsIntRegion*) (this=0x7fffc28d2d80, aRegion=0x0)
>     at /home/mstange/code/mozilla-central/gfx/layers/composite/TextureHost.cpp:621
> #10 0x00007fffe47c232d in mozilla::layers::BufferTextureHost::MaybeUpload(nsIntRegion*) (this=0x7fffc28d2d80, aRegion=0x0)
>     at /home/mstange/code/mozilla-central/gfx/layers/composite/TextureHost.cpp:504
> #11 0x00007fffe47c20db in mozilla::layers::BufferTextureHost::Lock() (this=0x7fffc28d2d80) at /home/mstange/code/mozilla-central/gfx/layers/composite/TextureHost.cpp:459
> #12 0x00007fffe47cdd0e in mozilla::layers::ContentHostTexture::Lock() (this=0x7fffc28b5df0) at /home/mstange/code/mozilla-central/gfx/layers/composite/ContentHost.h:145
> #13 0x00007fffe4707b77 in mozilla::layers::AutoLockCompositableHost::AutoLockCompositableHost(mozilla::layers::CompositableHost*) (this=0x7fffd2e96e00, aHost=0x7fffc28b5df0)
>     at ../../dist/include/CompositableHost.h:252
[...]

CairoFormatToGfxFormat complains that it doesn't know the format CAIRO_FORMAT_A1.
The image surface that we're looking at was created with CAIRO_FORMAT_A1 in gfxImageSurface::AllocateAndInit, where mFormat is gfxImageFormat::RGB16_565.
So gfxImageFormatToCairoFormat, which just reinterprets the integer value, wrongly casts gfxImageFormat::RGB16 to CAIRO_FORMAT_A1. This seems bad.

The gfxImageSurface is created with gfxImageFormat::RGB16_565 because that's what gfxPlatform::GetPlatform()->Optimal2DFormatForContent(gfxContentType::COLOR) returns when called from ContentClientRemoteBuffer::CreateBuffer - this value is coming from gfxPlatformGtk::GetOffscreenFormat() in the case that gdk_visual_get_depth(gdk_visual_get_system()) == 16. So it looks like this VNC setup simulates a 16 bit screen.

Comment 1

[Markus Stange [:mstange]]

And this is a regression from bug 1209812, which removed gfxImageFormat::A1, which causes gfxImageFormatToCairoFormat to no longer work because gfxImageFormat and _cairo_format no longer match up.

Comment 2

[Nicholas Nethercote [inactive]]

This one's my fault.

In part 1 of bug 1209812 I made the conversions between gfxImageFormat and cairo_format_t more explicit -- they now use a macro instead of just casts, which is good.

But the macros still assume that the values in gfxImageFormat line up with the equivalent values in cairo_format_t. And then in part 2 I removed gfxImageFormat::A1, which breaks the assumption.

Should be an easy fix -- just make the values line up again by making them explicit. Patch coming up soon.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Re-establish equivalence between gfxImageFormat and cairo_format_t

Markus, can you test this? Thank you.

Comment 4

[Markus Stange [:mstange]]

Comment on attachment 8683873 [details] [diff] [review]
Re-establish equivalence between gfxImageFormat and cairo_format_t

Review of attachment 8683873 [details] [diff] [review]:
-----------------------------------------------------------------

Sure.

Comment 5

[Markus Stange [:mstange]]

It works!

Comment 6

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2da18c113b3722ea61e51077ba7dd31e547a0fee
Bug 1222171 - Re-establish equivalence between gfxImageFormat and cairo_format_t. r=mstange.

Comment 7

[Nicholas Nethercote [inactive]]

Thank you for the fast testing and review.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2da18c113b37

Comment 9

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79382e397d1d

Comment 10

[Bryan Quigley]

I have what seems to be the same issue on a released version of Firefox 44 - https://bugzilla.mozilla.org/show_bug.cgi?id=1222171

Comment 11

[Jeff Muizelaar [:jrmuizel]]

It seems like this should've been uplifted to FF44?

Comment 12

[Bryan Quigley]

Oops, pasted wrong bug #1243491 is the new report.

Comment 14

[u279076]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #11)
> It seems like this should've been uplifted to FF44?

Supplemental, Bryan confirmed over on bug 1243491 that the patch here fixes that bug. Unfortunately this missed the 44 release so we'd need to take this as a ride-along if we do a 44.0.1. Alternatively these folks will need to wait six weeks for 45.0 and switch to Beta in the meantime.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8683873 [details] [diff] [review]
Re-establish equivalence between gfxImageFormat and cairo_format_t

Approval Request Comment
[Feature/regressing bug #]: Bug 209812
[User impact if declined]: Crashes on remote desktop on linux
[Describe test coverage new/current, TreeHerder]: Fixed in 45 and comfirmed
[Risks and why]: Should be pretty low. This was just a logic error.

Comment 16

[Nicholas Nethercote [inactive]]

I agree with Anthony in comment 14. Apologies for the failure to uplift.

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

See bug 916762 comment 4 which came back up in 44.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Anthony, I might consider taking this fix in a 44.0.1. Before I can do that, can you confirm whether the fix here was verified or not? I know you said so in comment 14 but it doesn't hurt to check again. Thanks!

Comment 19

[Bryan Quigley]

We've verified it in Ubuntu's bug tracker here:
https://bugs.launchpad.net/ubuntu/+source/firefox/+bug/1538724

PPA direct link 
https://launchpad.net/~dgadomski/+archive/ubuntu/lp1538724

We (I work for Canonical Support, btw) are currently pursuing an SRU (Stable Release Update) to get it to our users but doing 44.0.1 would be welcome!

Comment 20

[Robert Kaiser]

We are pondering a 44.0.1 dot release, can we get an uplift request here?

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Robert Kaiser (:[e-mail]) from comment #20)
> We are pondering a 44.0.1 dot release, can we get an uplift request here?

KaiRo, the patch is already nominated for uplift to m-r in comment 15.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8683873 [details] [diff] [review]
Re-establish equivalence between gfxImageFormat and cairo_format_t

Given the verification mentioned in comment 19 and from the beta channel, this is a good & safe fix to take as a ride-along in 44.0.1

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-release/rev/3de3ef8b8b3d

Comment 24

[Petruta Horea [:phorea], Desktop QA (back on May 31)]

Bryan, could you please check that it works with 44.0.1 too? Thank you!

Comment 25

[Bryan Quigley]

I wasn't able to test (lost access to environment), but another user tested in the LP bug and confirmed it [1] worked.  Thanks!
[1] https://ftp.mozilla.org/pub/firefox/candidates/44.0.1-candidates/build1/linux-x86_64/en-US/

Comment 26

[Petruta Horea [:phorea], Desktop QA (back on May 31)]

Thanks a lot, Bryan!

Marking as verified based on above comments.

Comment 27

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

Adding a release note for 44.0.1 as
Fix for graphics startup crash (Linux) with a link to the bug.