944412 - Stride error in SourceSurfaceSkia::InitFromData

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Kevin Simons [:kevsim] (Telenor)]

In SourceSurfaceSkia::InitFromData the data is first copied into a temporary bitmap and then into the bitmap for the surface. However, an assumption is made that the target bitmap ends up with the same stride as the source bitmap. This isn't a valid assumption.

Comment 1

[Kevin Simons [:kevsim] (Telenor)]

Attachment: Fix an issue with the stride in SourceSurfaceSkia::InitFromData

After calling SkBitmap::copyTo, the InitFromData method assumed
that the stride of the destination SkBitmap was now the same as the
stride of the source bitmap. This was, however, not the case. Now
the stride is read back out of the destination bitmap.

This was causing a crash due to memory corruption for FORMAT_B8G8R8X8
surfaces.

Comment 2

[Kevin Simons [:kevsim] (Telenor)]

I also attempted to fix this by calling setConfig on the target bitmap and thus respect the stride that was passed in, but that didn't work (the stride was not set properly).

I've verified that before this patch reftest layout/reftests/backgrounds/background-size-cover-continuous.html doesn't work (and often crashes) and that afterwards the results match that of the cairo backend.

Comment 3

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

Comment on attachment 8339959 [details] [diff] [review]
Fix an issue with the stride in SourceSurfaceSkia::InitFromData

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

Drive by...

::: gfx/2d/SourceSurfaceSkia.cpp
@@ +78,4 @@
>    if (aFormat == FORMAT_B8G8R8X8) {
>      mBitmap.lockPixels();
>      // We have to manually set the A channel to be 255 as Skia doesn't understand BGRX
>      ConvertBGRXToBGRA(reinterpret_cast<unsigned char*>(mBitmap.getPixels()), aSize, aStride);

The intent is to call ConvertBGRXToBGRA with mBitmap.getPixels(), IntSize(mBitmap.width(),mBitmap.height()), mBitmap.rowBytes() as parameters, right?

No big deal, but I think I'd get the intent clearer with just using mBitmap.rowBytes() in the call, rather than setting aStride to the value.  aStride was the stride for the incoming aData, seems it may as well stay that, just not get reused.

Comment 4

[Kevin Simons [:kevsim] (Telenor)]

Attachment: Fix an issue with the stride in SourceSurfaceSkia::InitFromData

After calling SkBitmap::copyTo, the InitFromData method assumed
that the stride of the destination SkBitmap was now the same as the
stride of the source bitmap. This was, however, not the case. Now
the stride is read back out of the destination bitmap.

This was causing a crash due to memory corruption for FORMAT_B8G8R8X8
surfaces.

Comment 5

[Kevin Simons [:kevsim] (Telenor)]

(In reply to Milan Sreckovic [:milan] from comment #3)
> No big deal, but I think I'd get the intent clearer with just using
> mBitmap.rowBytes() in the call, rather than setting aStride to the value. 
> aStride was the stride for the incoming aData, seems it may as well stay
> that, just not get reused.

Yeah, you're right, that was pure laziness. Fixed now. Note, the intention was also that mStride be set correctly.

On that point, is there any reason that the SourceSurfaceSkia class even needs those mSize and mStride instance variables? Can't that all get delegated to the mBitmap?

Comment 6

[Andreas Gal :gal]

Comment on attachment 8339991 [details] [diff] [review]
Fix an issue with the stride in SourceSurfaceSkia::InitFromData

This looks trivial enough for me to r+. I do think it would make sense to not have local copies of mStride and mSize, fwiw.

Comment 7

[Kevin Simons [:kevsim] (Telenor)]

Attachment: Fix an issue with the stride in SourceSurfaceSkia::InitFromData. r=gal

After calling SkBitmap::copyTo, the InitFromData method assumed
that the stride of the destination SkBitmap was now the same as the
stride of the source bitmap. This was, however, not the case. Now
the stride is read back out of the destination bitmap.

This was causing a crash due to memory corruption for FORMAT_B8G8R8X8
surfaces.

Comment 8

[Kevin Simons [:kevsim] (Telenor)]

Is this another case of just marking checkin-needed rather than running through try since it's not hitting any live code?

Comment 9

[Andreas Gal :gal]

Yeah. If you just update a patch you don't have to request for review again. The previous review is fine.

For questions about a patch you can do feedback?.

Comment 10

[Kevin Simons [:kevsim] (Telenor)]

(In reply to Andreas Gal :gal from comment #9)
> Yeah. If you just update a patch you don't have to request for review again.
> The previous review is fine.

Ah ok. I wasn't sure if there it was somehow automated that only patches where the attachment is r+ could be landed.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/f6d8a9a56abb

Comment 12

[Kevin Simons [:kevsim] (Telenor)]

Attachment: Fix an issue with the stride in SourceSurfaceSkia::InitFromData. r=gal

After calling SkBitmap::copyTo, the InitFromData method assumed
that the stride of the destination SkBitmap was now the same as the
stride of the source bitmap. This was, however, not the case. Now
the stride is read back out of the destination bitmap.

This was causing a crash due to memory corruption for FORMAT_B8G8R8X8
surfaces.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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