Closed Bug 1310064 Opened 3 years ago Closed 3 years ago

Crash in VerifyRGBXFormat in DrawTargetSkia.

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: vliu, Assigned: vliu)

References

Details

Attachments

(2 files, 6 obsolete files)

Attached file 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/
See Also: → 1308619, 1309200
Assignee: nobody → vliu
Hi Nical,
Could you please have a review for me? Really thanks.
Attachment #8800941 - Flags: review?(nical.bugzilla)
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 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) ?
Attachment #8800941 - Flags: review?(nical.bugzilla) → review-
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.
Attachment #8801629 - Flags: review?(nical.bugzilla)
Attachment #8800941 - Attachment is obsolete: true
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.
Attachment #8801629 - Attachment is obsolete: true
Attachment #8801629 - Flags: review?(nical.bugzilla)
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
Flags: needinfo?(nical.bugzilla)
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.
Flags: needinfo?(nical.bugzilla)
Attachment #8803789 - Flags: review?(nical.bugzilla)
Hi :nical,
Thanks for your comment. I'd attached a patch to fix it. Could you please have a review? Thanks
Attachment #8803793 - Flags: review?(nical.bugzilla)
Attachment #8803789 - Attachment is obsolete: true
Attachment #8803789 - Flags: review?(nical.bugzilla)
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).
Attachment #8803793 - Flags: review?(nical.bugzilla) → review?(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.
Attachment #8803793 - Flags: review?(lsalzman) → review-
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.
Attachment #8804781 - Flags: review?(rjesup)
Attachment #8804781 - Flags: review?(lsalzman)
Attachment #8803793 - Attachment is obsolete: true
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.
Attachment #8804855 - Flags: review?(vliu)
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)
Attachment #8804781 - Flags: review?(rjesup) → review+
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.
Attachment #8804855 - Flags: review?(vliu) → review+
(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.
Hi Lee, jesup,
Thanks for the review and come out patch to correct the fix. I'd rearrange it to review.
Attachment #8805418 - Flags: review?(rjesup)
Attachment #8805418 - Flags: review?(lsalzman)
Attachment #8804781 - Attachment is obsolete: true
Attachment #8804781 - Flags: review?(lsalzman)
Attachment #8804855 - Attachment is obsolete: true
Attachment #8805418 - Flags: review?(lsalzman) → review+
Attachment #8805418 - Flags: review?(rjesup) → review+
Pushed by vliu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eef062272d19
mark MediaEngineTabVideoSource draw target as uninitialized to avoid triggering assertion with Skia. r=lsalzman, jesup
https://hg.mozilla.org/mozilla-central/rev/eef062272d19
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.