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

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mchang, Assigned: mchang)

Tracking

unspecified
mozilla50
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(6 attachments, 7 obsolete attachments)

16.05 KB, text/plain
Details
1.31 KB, patch
Details | Diff | Splinter Review
5.18 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
8.54 KB, patch
mchang
: review+
Details | Diff | Splinter Review
5.96 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
log
13.71 KB, text/plain
Details
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
Posted patch prefs.patchSplinter Review
Prefs to create unaccelerated environment that's on try.

Skia content with d2d disabled but with accelerated compositor does not reproduce the issue.
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.
Attachment #8762206 - Flags: review?(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?
> 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
Attachment #8762206 - Flags: review?(lsalzman) → review+
Also updated to clear SkBitmaps and SkCanvas to SK_Black or SK_Transparent depending on the surface format.
Attachment #8772438 - Flags: review?(lsalzman)
Attachment #8762206 - Attachment is obsolete: true
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
Attachment #8772439 - Flags: review?(bas)
Attachment #8772438 - Attachment is obsolete: true
Attachment #8772438 - Flags: review?(lsalzman)
Attachment #8772497 - Flags: review?(lsalzman)
Attachment #8772497 - Attachment is obsolete: true
Attachment #8772497 - Flags: review?(lsalzman)
Attachment #8772502 - Flags: review?(lsalzman)
Attachment #8772502 - Attachment is obsolete: true
Attachment #8772502 - Flags: review?(lsalzman)
Attachment #8772508 - Flags: review?(lsalzman)
Attachment #8772508 - Attachment is obsolete: true
Attachment #8772508 - Flags: review?(lsalzman)
Attachment #8772516 - Flags: review?(lsalzman)
Attachment #8772516 - Flags: review?(lsalzman) → review+
Attachment #8772439 - Flags: review?(bas) → review+
Attachment #8774464 - Attachment is obsolete: true
Attachment #8774464 - Flags: review?(lsalzman)
Attachment #8774478 - Flags: review?(lsalzman)
Attachment #8774478 - Flags: review?(lsalzman) → review+
Pushed by mchang@mozilla.com:
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
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?
Flags: needinfo?(mchang)
Flags: needinfo?(lsalzman)
I've just noticed: The bug summary says: Windows, but this fails on Mac.
(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.
Flags: needinfo?(mchang)
Flags: needinfo?(lsalzman)
Flags: needinfo?(jorgk)
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.
Flags: needinfo?(surkov.alexander)
Flags: needinfo?(jorgk)
Flags: needinfo?(aleth)
Posted file log
log including assertion stack
Flags: needinfo?(surkov.alexander)
Thank you very much, Alexander.
Flags: needinfo?(aleth)
Blocks: 1293759
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?
Flags: needinfo?(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.
Flags: needinfo?(mchang)
You need to log in before you can comment on or make changes to this bug.