1279063 - Windows 7 unaccelerated skia content reftest crashes with skia\include\core\SkColorPriv.h:399: fatal error: ""r <= a""

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Mason Chang [Inactive] [:mchang]]

Attachment: crash stack

From - https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f0f578a1a47&selectedJob=22195256

Comment 1

[Mason Chang [Inactive] [:mchang]]

REFTEST TEST-START | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/bugs/1078262-1.html == about:blank
13:29:11     INFO -  REFTEST TEST-LOAD | file:///C:/slave/test/build/tests/reftest/tests/layout/reftests/bugs/1078262-1.html | 4791 / 13649 (35%)
13:29:11     INFO -  ++DOMWINDOW == 114 (18AD2C00) [pid = 4044] [serial = 15612] [outer = 1BA86C00]
13:29:11     INFO -  c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\gfx\skia\skia\include\core\SkColorPriv.h:399: fatal error: ""r <= a""
13:29:11     INFO -  Abort from sk_abort

Comment 2

[Mason Chang [Inactive] [:mchang]]

Attachment: prefs.patch

Prefs to create unaccelerated environment that's on try.

Skia content with d2d disabled but with accelerated compositor does not reproduce the issue.

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: Don't assume cairo is the default software backend

The crash was happening because we were given an RGBX Surface from gecko and converted it to an RGBA surface in Skia but didn't actually set the alpha value to opaque. This caused the assert to fail. Interestingly, the other blend operators don't seem to have a problem with this.

We normally set RGBX surfaces to 0xFF alpha values when we create the surface, but we didn't do that here because we only memset it for Skia backends. However, since we had basic layers, gfxWindowsPlatform just assumed that we'd use Cairo and thus we wouldn't memset the alpha value on the newly created surface.

This patch stops assuming cairo is our only software content backend.

Comment 4

[Lee Salzman [:lsalzman]]

(In reply to Mason Chang [:mchang] from comment #3)
> Created attachment 8762206 [details] [diff] [review]
> Don't assume cairo is the default software backend
> 
> The crash was happening because we were given an RGBX Surface from gecko and
> converted it to an RGBA surface in Skia but didn't actually set the alpha
> value to opaque. This caused the assert to fail. Interestingly, the other
> blend operators don't seem to have a problem with this.
> 
> We normally set RGBX surfaces to 0xFF alpha values when we create the
> surface, but we didn't do that here because we only memset it for Skia
> backends. However, since we had basic layers, gfxWindowsPlatform just
> assumed that we'd use Cairo and thus we wouldn't memset the alpha value on
> the newly created surface.
> 
> This patch stops assuming cairo is our only software content backend.

It seems a bit extreme to verify the alpha channel every time we hand an outside RGBX bitmap to Skia. Isn't this going to significantly derail the performance of debug builds?

Comment 5

[Mason Chang [Inactive] [:mchang]]

> It seems a bit extreme to verify the alpha channel every time we hand an
> outside RGBX bitmap to Skia. Isn't this going to significantly derail the
> performance of debug builds?

It shouldn't since most SurfacePatterns should already be a Skia surface [1]. This check is only for DataSourceSurfaces.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetSkia.cpp#115

Comment 6

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1: Don't assume cairo is the default software backend

Also updated to clear SkBitmaps and SkCanvas to SK_Black or SK_Transparent depending on the surface format.

Comment 7

[Mason Chang [Inactive] [:mchang]]

Attachment: Use RGBA instead of RGBX surface format for DXVA Video

Media tests were failing because we'd do a readback from the GPU for media mochitests as RGBX format and they wouldn't be in the format Skia wanted when DrawTargetSkia::OptimizeSourceSurface. Use RGBA surface format for video instead. I also double checked with Matt and Cpearce. Both think it should be fine.

Try finally looking ok I think - https://treeherder.mozilla.org/#/jobs?repo=try&revision=43cb27d175ef&selectedJob=24084865

Comment 8

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1: Don't assume cairo is the default software backend

Comment 9

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1: Don't assume cairo is the default software backend

Comment 10

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1: Don't assume cairo is the default software backend

Comment 11

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1: Don't assume cairo is the default software backend

Comment 12

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 1: Don't assume cairo is the default software backend

Comment 13

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 3: memset RGBX surfaces to 0xFF in Factory::CreateDataSourceSurface

Comment 14

[Mason Chang [Inactive] [:mchang]]

Attachment: Part 3: memset RGBX surfaces to 0xFF in Factory::CreateDataSourceSurface

Comment 15

[Mason Chang [Inactive] [:mchang]]

Woot try looks good finally :) https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ec9f6df1707&selectedJob=24484982

The dt2 OS X failures are bug 1288348.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1f81ccfb6d4
Part 1: Don't assume cairo is the default software backend. r=lsalzman
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bbf47e4f280
Part 2: Use RGBA instead of RGBX surface format for DXVA video. r=bas
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe0c6517b215
Part 3: Memset RGBX surfaces to 0xFF in Factory::CreateDataSourceSurface. r=lsalzman

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f1f81ccfb6d4
https://hg.mozilla.org/mozilla-central/rev/9bbf47e4f280
https://hg.mozilla.org/mozilla-central/rev/fe0c6517b215

Comment 18

[Jorg K (CEST = GMT+2)]

Hello, Thunderbird developer here:
In our Mozmill tests on debug compiles we get heaps of crashes due to this:
https://hg.mozilla.org/mozilla-central/rev/f1f81ccfb6d4#l1.69
MOZ_ASSERT(aData[topLeft] == 0xFF); etc.

See:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=adc56f0cd5b514b27a96cebb31afe02503971b0c
https://treeherder.mozilla.org/#/jobs?repo=comm-central&selectedJob=43674

Assertion failure: aData[middle] == 0xFF, at /builds/slave/tb-c-cen-m64-d-000000000000000/build/mozilla/gfx/2d/DrawTargetSkia.cpp:167
Assertion failure: aData[bottomRight] == 0xFF, at /builds/slave/tb-c-cen-m64-d-000000000000000/build/mozilla/gfx/2d/DrawTargetSkia.cpp:165
Assertion failure: aData[topLeft] == 0xFF, at /builds/slave/tb-c-cen-m64-d-000000000000000/build/mozilla/gfx/2d/DrawTargetSkia.cpp:163

Is this intentional/avoidable?

Comment 19

[Jorg K (CEST = GMT+2)]

I've just noticed: The bug summary says: Windows, but this fails on Mac.

Comment 20

[Mason Chang [Inactive] [:mchang]]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #18)
> Hello, Thunderbird developer here:
> In our Mozmill tests on debug compiles we get heaps of crashes due to this:
> https://hg.mozilla.org/mozilla-central/rev/f1f81ccfb6d4#l1.69
> MOZ_ASSERT(aData[topLeft] == 0xFF); etc.
> 
> See:
> https://treeherder.mozilla.org/#/jobs?repo=comm-
> central&revision=adc56f0cd5b514b27a96cebb31afe02503971b0c
> https://treeherder.mozilla.org/#/jobs?repo=comm-central&selectedJob=43674
> 
> Assertion failure: aData[middle] == 0xFF, at
> /builds/slave/tb-c-cen-m64-d-000000000000000/build/mozilla/gfx/2d/
> DrawTargetSkia.cpp:167
> Assertion failure: aData[bottomRight] == 0xFF, at
> /builds/slave/tb-c-cen-m64-d-000000000000000/build/mozilla/gfx/2d/
> DrawTargetSkia.cpp:165
> Assertion failure: aData[topLeft] == 0xFF, at
> /builds/slave/tb-c-cen-m64-d-000000000000000/build/mozilla/gfx/2d/
> DrawTargetSkia.cpp:163
> 
> Is this intentional/avoidable?

It is currently intentional. We use the Skia backend on Mac and plan to ship it on Linux / Windows as well. Do you currently work on gecko as well? The problem occurs because opaque surfaces (SurfaceFormat::B8G8R8X8) need to have their alpha values initialized to 0xFF. We did this all over gecko, at least wherever we saw the assertion failing in Firefox. If you can track where the surface is coming from and initialize the memory of that surface to 0xFF, the assertion should go away.

Comment 21

[Jorg K (CEST = GMT+2)]

Thanks for your reply. I'm not sure what information I should supply. As you know, Thunderbird is a giant Firefox "add-on", using Mozilla core technology. We're not working on Gecko (although I don't quite understand the question). We can of course get a call stack of the failure so you can see "where the surface is coming from". In fact, Alexander Surkov ran into this problem in bug 1246447 comment #23.

Alexander, could you provide a call stack here. I don't have a Mac. Or perhaps Aleth can.

Comment 22

[alexander :surkov (:asurkov)]

Attachment: log

log including assertion stack

Comment 23

[Jorg K (CEST = GMT+2)]

Thank you very much, Alexander.

Comment 24

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8772524 [details] [diff] [review]
Part 1: Don't assume cairo is the default software backend

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

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +540,5 @@
>    }
>  
> +  if (defaultBackend == BackendType::DIRECT2D1_1) {
> +    // We can't have D2D without D3D11 layers, so fallback to Cairo.
> +    return BackendType::CAIRO;

:mchang, is there a reason why it returns BackendType::CAIRO instead of skia?

Comment 25

[Mason Chang [Inactive] [:mchang]]

(In reply to Sotaro Ikeda [:sotaro] from comment #24)
> Comment on attachment 8772524 [details] [diff] [review]
> Part 1: Don't assume cairo is the default software backend
> 
> Review of attachment 8772524 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxWindowsPlatform.cpp
> @@ +540,5 @@
> >    }
> >  
> > +  if (defaultBackend == BackendType::DIRECT2D1_1) {
> > +    // We can't have D2D without D3D11 layers, so fallback to Cairo.
> > +    return BackendType::CAIRO;
> 
> :mchang, is there a reason why it returns BackendType::CAIRO instead of skia?

At the time of this patch, Skia wasn't enabled for Windows yet. We were still fixing bugs in Skia (which didn't land on Windows until 52. This is 50). I think now, it should become Skia instead of Cairo. We should probably fix the checks up to default to Skia now.