1310064 - Crash in VerifyRGBXFormat in DrawTargetSkia.

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Vincent Liu[:vliu]]

Attachment: crash-backtrace.log

This is the following bug when I looked into bug 1308619. When the patch in bug 1309200 fixed bug 1308619, another crash happens. Please see the attached file for more detail.

I will tried to propose a patch to fix this problem.

STR (DEBUG build):
1. In about:config:
  - add ",fiddle.jshell.net" to the tail of media.getusermedia.screensharing.allowed_domains
  - set media.navigator.permission.disabled = true
2. Open URL and click [Start] button.
   URL: https://jsfiddle.net/uax4gom5/

Comment 1

[Vincent Liu[:vliu]]

Attachment: Skip VerifyRGBXFormat check if aData doesn't contain anything.

Hi Nical,
Could you please have a review for me? Really thanks.

Comment 2

[Vincent Liu[:vliu]]

Hi Nical,
Sorry to add comment for the patch. When we called Factory::CreateDrawTargetForData(), aData may contain nothing at this moment. In this case, crash happens in Skia check in [1].

[1]: https://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetSkia.cpp?q=VerifyRGBXFormat&redirect_type=direct#154

The patch in Comment 1 intends to this skip check if aData doesn't contains anything.

Comment 3

[Nicolas Silva [:nical]]

Comment on attachment 8800941 [details] [diff] [review]
Skip VerifyRGBXFormat check if aData doesn't contain anything.

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

::: gfx/2d/DrawTargetSkia.cpp
@@ +139,5 @@
>  #ifdef DEBUG
>  static bool
>  VerifyRGBXFormat(uint8_t* aData, const IntSize &aSize, const int32_t aStride, SurfaceFormat aFormat)
>  {
> +  if (aFormat != SurfaceFormat::B8G8R8X8 || aSize.IsEmpty() || (*aData == 0)) {

I belive you meant (aData == nullptr) ?

Comment 4

[Vincent Liu[:vliu]]

Attachment: Skip VerifyRGBXFormat or VerifyRGBXCorners check if aData is nullptr or doesn't contain anything.

Hi :nical,
Thanks for your review. I think there were two cases to skip the check in this patch.

1. aData is nullptr.
2. The pointer of aData is valid but it contains empty data in the initial stage. The current STR was an example for this case. 
   mData was created in [1] but it didn't contain any valid date when calling [2].

   [1]: https://dxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineTabVideoSource.cpp#279 
   [2]: https://dxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineTabVideoSource.cpp#300

In this patch, I also add the same code in VerifyRGBXCorners for the same purpose.

Comment 5

[Vincent Liu[:vliu]]

Comment on attachment 8801629 [details] [diff] [review]
Skip VerifyRGBXFormat or VerifyRGBXCorners check if aData is nullptr or doesn't contain anything.

Clear r? and obsoleted the patch since I think it is not a proper patch to fix this issue.

Comment 6

[Vincent Liu[:vliu]]

Hi :nical,
As I talked in Comment 4, the issue happens here just because mData was created in [1] but it didn't contain any valid date when calling [2].

   [1]: https://dxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineTabVideoSource.cpp#279 
   [2]: https://dxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineTabVideoSource.cpp#300

When I starting thinking of the possible patch to solve this, I got some qustions to ask.

1. About filling RGBAlphaOffset, does it applied for every backends or just skia only?
2. I think the possible patch to fix this issue is calling WriteRGBXFormat instead of VerifyRGBXFormat in DrawTargetSkia::Init(). But if I do like this,
   it would modify data created from caller. I am not sure if it is suitable for all use case. Do you have any idea about this?
3. If 2 is not a good way to go, we may do the thing like WriteRGBXFormat in MediaEngineTabVideoSource. But if so, MediaEengine becomes beckend dependent.
   An example for skia, it need to consider different offset value to fill RGBAlphaOffset because of [3]. I don't think it is a good way to do like this. 
   Would you please comment for this? Thanks

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

Comment 7

[Nicolas Silva [:nical]]

Skia needs RGBX surfaces to have their X channel containing 0xFF, while other backends just ignore the X channel. VerifyRGBXFormat checks that, so we should not return true if the X channel contains 0.

If we run int this assertion it means that we should have filled the X/A channel with 0xFF before attempting to draw. Usually this is done by finding the place where we allocate the surface and memsetting it with 0xFF before wrapping it with a skia target/surface.

Comment 8

[Vincent Liu[:vliu]]

Attachment: filling the X/A channel with 0xFF for Skia backend.

Comment 9

[Vincent Liu[:vliu]]

Attachment: filling the X/A channel with 0xFF for Skia backend.

Hi :nical,
Thanks for your comment. I'd attached a patch to fix it. Could you please have a review? Thanks

Comment 10

[Nicolas Silva [:nical]]

Comment on attachment 8803793 [details] [diff] [review]
filling the X/A channel with 0xFF for Skia backend.

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

Passing the review to Lee since he knows best skia's specifics.

That said I think that we should not clear in Factory::CreateDrawTargetForData because we also use this function to wrap DrawTargets around existing and valid data to draw on top of it. In other cases we may already know that the surface has its X/A channel correctly set to 0xFF so we should not pay the cost of clearing there.

You could add an optional parameter to clear the surface, maybe (or perhaps Lee has another suggestion).

Comment 11

[Lee Salzman [:lsalzman]]

Comment on attachment 8803793 [details] [diff] [review]
filling the X/A channel with 0xFF for Skia backend.

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

As Nical mentioned, this function is also used to deal with already initialized data so we don't want to just blindly modify the data here. As well, in cases where the data is already safe, we don't want to pay the redundant cost of overwriting that data, especially on large surfaces. So, I disagree with the general strategy of moving the initialization here.

I think a far simpler fix, based on the information you have mentioned, is to pass in true for the "aUninitialized" parameter to Factory::CreateDrawTargetForData. That parameter exists for cases such as these, so that the validation of uninitialized data will be skipped, on the promise that we will fill it in later during rendering.

Comment 12

[Vincent Liu[:vliu]]

Attachment: Fill in 0xFF into X channel if the format is BGRX for skia backend. r=lsalzman, jesup

Hi Lee, jesup,
This patch intends to fix the crash based on bug 1309200. This patch tried to fill in 0xFF in specific position in X channel if the format is BGRX for skia backend.
I also include jesup as reviewer because the patch was done in MediaEngineTabVideoSource. Could you please have a review for me? Really thanks.

Comment 13

[Lee Salzman [:lsalzman]]

Attachment: mark MediaEngineTabVideoSource draw target as uninitialized to avoid triggering assertion with Skia

This is the much simpler fix I was trying to explain. All we really have to do is pass true into CreateDrawTargetForData.

The only extra thing I do in the patch is take care to clear the draw target in case we didn't render anything to it, since the rendering will otherwise initialize the alpha in the other case.

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8804781 [details] [diff] [review]
Fill in 0xFF into X channel if the format is BGRX for skia backend. r=lsalzman, jesup

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

r+ assuming IsEmpty() works here

::: dom/media/webrtc/MediaEngineTabVideoSource.h
@@ +92,5 @@
> +      int width = aSize.width * 4;
> +
> +      for (int row = 0; row < height; ++row) {
> +        for (int column = 0; column < width; column += 4) {
> +          aData[column + kARGBAlphaOffset] = 0xFF;

Is there any chance of this being passed width or height == 0, or does IsEmpty() check for both (note: 4x0 and 0x4 should return true from IsEmpty() for this to be safe)

Comment 15

[Vincent Liu[:vliu]]

Comment on attachment 8804855 [details] [diff] [review]
mark MediaEngineTabVideoSource draw target as uninitialized to avoid triggering assertion with Skia

Thanks for the patch to correct the fix we need. I will rearrange a patch based on bug 1309200.

Comment 16

[Vincent Liu[:vliu]]

(In reply to Randell Jesup [:jesup] from comment #14)
> Comment on attachment 8804781 [details] [diff] [review]
> Fill in 0xFF into X channel if the format is BGRX for skia backend.
> r=lsalzman, jesup
> 
> Review of attachment 8804781 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ assuming IsEmpty() works here
> 
> ::: dom/media/webrtc/MediaEngineTabVideoSource.h
> @@ +92,5 @@
> > +      int width = aSize.width * 4;
> > +
> > +      for (int row = 0; row < height; ++row) {
> > +        for (int column = 0; column < width; column += 4) {
> > +          aData[column + kARGBAlphaOffset] = 0xFF;
> 
> Is there any chance of this being passed width or height == 0, or does
> IsEmpty() check for both (note: 4x0 and 0x4 should return true from
> IsEmpty() for this to be safe)

Thanks for the review for the patch. After discussed with Lee, it turns out that we don't need to pay extra cost to overwrite data for an uninitialized data. Based on this, we won't have WriteRGBXFormat() to do this for the proposed patch.

Comment 17

[Vincent Liu[:vliu]]

Attachment: mark MediaEngineTabVideoSource draw target as uninitialized to avoid triggering assertion with Skia. r=lsalzman, jesup

Hi Lee, jesup,
Thanks for the review and come out patch to correct the fix. I'd rearrange it to review.

Comment 18

[Vincent Liu[:vliu]]

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e0bd211cece8164022db597c6b3b41aca65e85a

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eef062272d19
mark MediaEngineTabVideoSource draw target as uninitialized to avoid triggering assertion with Skia. r=lsalzman, jesup

Comment 20

[Carsten Book [:Tomcat]]

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